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

[Hillf Danton]

On Fri, 7 Mar 2025 13:34:43 +0100 Oleg Nesterov <oleg@xxxxxxxxxx>
> On 03/07, Oleg Nesterov wrote:
> > On 03/07, Hillf Danton wrote:
> > > On Fri, 7 Mar 2025 11:54:56 +0530 K Prateek Nayak <kprateek.nayak@xxxxxxx>
> > > >> step-03
> > > >> 	task-118766 new reader
> > > >> 	makes pipe empty
> > > >
> > > >Reader seeing a pipe full should wake up a writer allowing 118768 to
> > > >wakeup again and fill the pipe. Am I missing something?
> > > >
> > > Good catch, but that wakeup was cut off [2,3]
> 
> Please note that "that wakeup" was _not_ removed by the patch below.
> 
After another look, you did cut it.

Link: https://lore.kernel.org/all/20250209150718.GA17013@xxxxxxxxxx/
Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
Tested-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>
---
 fs/pipe.c | 45 +++++++++------------------------------------
 1 file changed, 9 insertions(+), 36 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 2ae75adfba64..b0641f75b1ba 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -360,29 +360,9 @@ anon_pipe_read(struct kiocb *iocb, struct iov_iter *to)
 			break;
 		}
 		mutex_unlock(&pipe->mutex);
-
 		/*
 		 * We only get here if we didn't actually read anything.
 		 *
-		 * However, we could have seen (and removed) a zero-sized
-		 * pipe buffer, and might have made space in the buffers
-		 * that way.
-		 *
-		 * You can't make zero-sized pipe buffers by doing an empty
-		 * write (not even in packet mode), but they can happen if
-		 * the writer gets an EFAULT when trying to fill a buffer
-		 * that already got allocated and inserted in the buffer
-		 * array.
-		 *
-		 * So we still need to wake up any pending writers in the
-		 * _very_ unlikely case that the pipe was full, but we got
-		 * no data.
-		 */
-		if (unlikely(wake_writer))
-			wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM);
-		kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
-
-		/*
 		 * But because we didn't read anything, at this point we can
 		 * just return directly with -ERESTARTSYS if we're interrupted,
 		 * since we've done any required wakeups and there's no need
@@ -391,7 +371,6 @@ anon_pipe_read(struct kiocb *iocb, struct iov_iter *to)
 		if (wait_event_interruptible_exclusive(pipe->rd_wait, pipe_readable(pipe)) < 0)
 			return -ERESTARTSYS;
 
-		wake_writer = false;
 		wake_next_reader = true;
 		mutex_lock(&pipe->mutex);
 	}

> "That wakeup" is another wakeup pipe_read() does before return:
> 
> 	if (wake_writer)
> 		wake_up_interruptible_sync_poll(&pipe->wr_wait, ...);
> 
> And wake_writer must be true if this reader changed the pipe_full()
> condition from T to F.
> 
Could you read Prateek's comment again, then try to work out why he
did so?

> Note also that pipe_read() won't sleep if it has read even one byte.
> 
> > > [2] https://lore.kernel.org/lkml/20250304123457.GA25281@xxxxxxxxxx/
> > > [3] https://lore.kernel.org/all/20250210114039.GA3588@xxxxxxxxxx/
> >
> > Why do you think
> >
> > 	[PATCH v2 1/1] pipe: change pipe_write() to never add a zero-sized buffer
> > 	https://lore.kernel.org/all/20250210114039.GA3588@xxxxxxxxxx/
> >
> > can make any difference ???
> >
> > Where do you think a zero-sized buffer with ->len == 0 can come from?
> 
> Oleg.