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.
