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

[Oleg Nesterov <oleg@redhat.com>]

Forgot to mention...

Even if this patch is likely "offtopic", it probably makes sense.
However, it is "incomplete" in that there are other scenarious.

Again, the pipe is full. A writer W1 tries to write 4096 bytes and
sleeps. A writer W2 tries to write 1 byte and sleeps too.

A reader reads 4096 bytes, updates pipe->tail and wakes W1.

Another writer comes, writes 1 byte and "steals" the buffer released
by the reader.

W1 sleeps again, this is correct. But nobody will wake W2 which could
succeed.

This (and the previous more simple scenario) means that

	if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))
		wake_next_writer = false;

before return in anon_pipe_write() is not really right in this sense.

Oleg.

On 02/27, Oleg Nesterov wrote:
>
> Hmm...
> 
> Suppose that pipe is full, a writer W tries to write a single byte
> and sleeps on pipe->wr_wait.
> 
> A reader reads PAGE_SIZE bytes, updates pipe->tail, and wakes W up.
> 
> But, before the woken W takes pipe->mutex, another writer comes and
> writes 1 byte. This updates ->head and makes pipe_full() true again.
> 
> Now, W could happily merge its "small" write into the last buffer,
> but it will sleep again, despite the fact the last buffer has room
> for 4095 bytes.
> 
> Sapkal, I don't think this can explain the hang, receiver()->read()
> should wake this writer later anyway. But could you please retest
> with the patch below?
> 
> Thanks,
> 
> Oleg.
> ---
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index b0641f75b1ba..222881559c30 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -455,6 +455,7 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>  	 * page-aligns the rest of the writes for large writes
>  	 * spanning multiple pages.
>  	 */
> +again:
>  	head = pipe->head;
>  	was_empty = pipe_empty(head, pipe->tail);
>  	chars = total_len & (PAGE_SIZE-1);
> @@ -559,8 +560,8 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>  		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>  		wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe));
>  		mutex_lock(&pipe->mutex);
> -		was_empty = pipe_empty(pipe->head, pipe->tail);
>  		wake_next_writer = true;
> +		goto again;
>  	}
>  out:
>  	if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))