Re: [PATCH AUTOSEL 5.5 542/542] pipe: use exclusive waits when reading or writing

[Andrei Vagin <avagin@gmail.com>]

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@infradead.org> 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: