Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

[K Prateek Nayak]

Hello Mateusz,

On 3/3/2025 11:24 PM, Mateusz Guzik wrote:
> Can you guys try out the patch below?
>
> It changes things up so that there is no need to read 2 different vars.
>
> It is not the final version and I don't claim to be able to fully
> justify the thing at the moment either, but I would like to know if it
> fixes the problem.

Happy to help! We've queued the below patch for an overnight run, will
report back once it is done.

Full disclaimer: We're testing on top of commit aaec5a95d596
("pipe_read: don't wake up the writer if the pipe is still full") where the
issue is more reproducible. I've replaced the VFS_BUG_ON() with a plain
BUG_ON() based on [1] since v6.14-rc1 did not include the CONFIG_DEBUG_VFS
bits. Hope that is alright.

[1] https://lore.kernel.org/lkml/20250209185523.745956-2-mjguzik@xxxxxxxxx/

/off to get some shut eyes/

--
Thanks and Regards,
Prateek

> If you don't have time that's fine, this is a quick jab. While I can't
> reproduce the bug myself even after inserting a delay by hand with
> msleep between the loads, I verified it does not outright break either.
> :P
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 19a7948ab234..e61ad589fc2c 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -210,11 +210,21 @@ static const struct pipe_buf_operations anon_pipe_buf_ops = {
>   /* Done while waiting without holding the pipe lock - thus the READ_ONCE() */
>   static inline bool pipe_readable(const struct pipe_inode_info *pipe)
>   {
> -	unsigned int head = READ_ONCE(pipe->head);
> -	unsigned int tail = READ_ONCE(pipe->tail);
> -	unsigned int writers = READ_ONCE(pipe->writers);
> +	return !READ_ONCE(pipe->isempty) || !READ_ONCE(pipe->writers);
> +}
> +
> +static inline void pipe_recalc_state(struct pipe_inode_info *pipe)
> +{
> +	pipe->isempty = pipe_empty(pipe->head, pipe->tail);
> +	pipe->isfull = pipe_full(pipe->head, pipe->tail, pipe->max_usage);
> +	VFS_BUG_ON(pipe->isempty && pipe->isfull);
> +}
>   
> -	return !pipe_empty(head, tail) || !writers;
> +static inline void pipe_update_head(struct pipe_inode_info *pipe,
> +				    unsigned int head)
> +{
> +	pipe->head = ++head;
> +	pipe_recalc_state(pipe);
>   }
>   
>   static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
> @@ -244,6 +254,7 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
>   	 * without the spinlock - the mutex is enough.
>   	 */
>   	pipe->tail = ++tail;
> +	pipe_recalc_state(pipe);
>   	return tail;
>   }
>   
> @@ -403,12 +414,7 @@ static inline int is_packetized(struct file *file)
>   /* Done while waiting without holding the pipe lock - thus the READ_ONCE() */
>   static inline bool pipe_writable(const struct pipe_inode_info *pipe)
>   {
> -	unsigned int head = READ_ONCE(pipe->head);
> -	unsigned int tail = READ_ONCE(pipe->tail);
> -	unsigned int max_usage = READ_ONCE(pipe->max_usage);
> -
> -	return !pipe_full(head, tail, max_usage) ||
> -		!READ_ONCE(pipe->readers);
> +	return !READ_ONCE(pipe->isfull) || !READ_ONCE(pipe->readers);
>   }
>   
>   static ssize_t
> @@ -512,7 +518,7 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>   				break;
>   			}
>   
> -			pipe->head = head + 1;
> +			pipe_update_head(pipe, head);
>   			pipe->tmp_page = NULL;
>   			/* Insert it into the buffer array */
>   			buf = &pipe->bufs[head & mask];
> @@ -529,10 +535,9 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>   
>   			if (!iov_iter_count(from))
>   				break;
> -		}
>   
> -		if (!pipe_full(head, pipe->tail, pipe->max_usage))
>   			continue;
> +		}
>   
>   		/* Wait for buffer space to become available. */
>   		if ((filp->f_flags & O_NONBLOCK) ||
> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 8ff23bf5a819..d4b7539399b5 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -69,6 +69,8 @@ struct pipe_inode_info {
>   	unsigned int r_counter;
>   	unsigned int w_counter;
>   	bool poll_usage;
> +	bool isempty;
> +	bool isfull;
>   #ifdef CONFIG_WATCH_QUEUE
>   	bool note_loss;
>   #endif