On Tue, Feb 18, 2020 at 10:28:23AM -0800, Linus Torvalds wrote:
> On Tue, Feb 18, 2020 at 10:20 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> >
> > You don't want to move wake_up_partner() up and call it from pipe_release()?
>
> I was actually thinking of going the other way - two of three users of
> wake_up_partner() are redundantly waking up the wrong side, and the
> third user is pointlessly written too.
>
> So I was _thinking_ of a patch like the appended (which is on top of
> the previous patch), but ended up not doing it. Until you brought it
> up.
>
> But I won't bother committing this, since it shouldn't really matter.
I run CRIU tests on the kernel with both these patches. Everything work
as expected. Thank you for the fix.
>
> Linus
> fs/pipe.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 2144507447c5..79ba61430f9c 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1025,12 +1025,6 @@ static int wait_for_partner(struct pipe_inode_info *pipe, unsigned int *cnt)
> return cur == *cnt ? -ERESTARTSYS : 0;
> }
>
> -static void wake_up_partner(struct pipe_inode_info *pipe)
> -{
> - wake_up_interruptible_all(&pipe->rd_wait);
> - wake_up_interruptible_all(&pipe->wr_wait);
> -}
> -
> static int fifo_open(struct inode *inode, struct file *filp)
> {
> struct pipe_inode_info *pipe;
> @@ -1078,7 +1072,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
> */
> pipe->r_counter++;
> if (pipe->readers++ == 0)
> - wake_up_partner(pipe);
> + wake_up_interruptible_all(&pipe->wr_wait);
>
> if (!is_pipe && !pipe->writers) {
> if ((filp->f_flags & O_NONBLOCK)) {
> @@ -1104,7 +1098,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
>
> pipe->w_counter++;
> if (!pipe->writers++)
> - wake_up_partner(pipe);
> + wake_up_interruptible_all(&pipe->rd_wait);
>
> if (!is_pipe && !pipe->readers) {
> if (wait_for_partner(pipe, &pipe->r_counter))
> @@ -1120,12 +1114,12 @@ static int fifo_open(struct inode *inode, struct file *filp)
> * the process can at least talk to itself.
> */
>
> - pipe->readers++;
> - pipe->writers++;
> + if (pipe->readers++ == 0)
> + wake_up_interruptible_all(&pipe->wr_wait);
> + if (pipe->writers++ == 0)
> + wake_up_interruptible_all(&pipe->rd_wait);
> pipe->r_counter++;
> pipe->w_counter++;
> - if (pipe->readers == 1 || pipe->writers == 1)
> - wake_up_partner(pipe);
> break;
>
> default:
