On 2020/10/06 3:26, Linus Torvalds wrote:
> On Mon, Oct 5, 2020 at 5:14 AM Tetsuo Handa
> <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>>
>> splice() from pipe should return 0 when there is no pipe writer. However,
>> since commit a194dfe6e6f6f720 ("pipe: Rearrange sequence in pipe_write()
>> to preallocate slot") started inserting empty pages, splice() from pipe
>> also returns 0 when all ready buffers are empty pages.
>
> Well... Only if you had writers that intentionally did that whole "no
> valid data write" thing.
>
> Which could be seen as a feature.
pipe_read() is aware of such writers.
/*
* 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.
*/
We also need to care about handle_userfault()
( https://lkml.kernel.org/r/29dd8637-5e44-db4a-9aea-305b079941fb@xxxxxxxxxxxxxxxxxxx )
which we might change it to return an error when pagefault from
pipe_write() takes too long.
>
> That said, if this actually broke some code, then we need to fix it -
> but I really hate how you have that whole !pipe_empty() loop around
> the empty buffers.
>
> That case is very unlikely, and you have a loop with !pipe_empty()
> *anyway* with the whole "goto refill". So the loop is completely
> pointless.
This loop just removes all leading empty pages at once.
"goto refill" is a result of all pages were empty.
>
> Also, what if we have a packet pipe? Do we perhaps want to return at
> packet boundaries? I don't think splice() has cared, so probably not,
> but it's worth perhaps thinking about.
Since manpage says that "Zero-length packets are not supported.", I think that
skipping leading empty pages (either my version or your version) will not break
packet boundaries.
This check is there to avoid returning 0 when all available buffers are empty.
This check should remain no-op if some buffer is not empty.
>
> Anyway, I'd be a lot happier with the patch being structured something
> like this instead.. UNTESTED
I'm OK with your version.
