Re: Pending splice(file -> FIFO) excludes all other FIFO operations forever (was: ... always blocks read(FIFO), regardless of O_NONBLOCK on read side?)

[Linus Torvalds]

On Fri, 7 Jul 2023 at 15:41, Ahelenia Ziemiańska
<nabijaczleweli@xxxxxxxxxxxxxxxxxx> wrote:
>
> (inlined by) eat_empty_buffer at fs/splice.c:594

Ahh, eat_empty_buffer() ends up releasing the buffer without waiting for it.

And the reason for that is actually somewhat interesting: we do have that

        while (!pipe_readable(pipe)) {
             ..

above it, but the logic for this all is that pipes with pipe buffers
are by *default* considered readable until they try to actually
confirm the buffer, and at that point they might say "oh, I have to
return -EAGAIN and set 'not_ready'".

And that splice_from_pipe_next() doesn't do that.

End result: it will happily free that pipe buffer that is still in the
process of being filled.

The good news is that I think the fix is probably trivial. Something
like the attached?

Again - NOT TESTED.

> Besides that, this doesn't solve the original issue, inasmuch as
>   ./v > fifo &
>   head fifo &
>   echo zupa > fifo
> (where ./v splices from an empty pty to stdout; v.c attached)
> echo still sleeps until ./v dies, though it also succumbs to ^C now.

Yeah, I concentrated on just making everything interruptible,

But the fact that the echo has to wait for the previous write to
finish is kind of fundamental. We can't just magically do writes out
of order. 'v' is busy writing to the fifo, we can't let some other
write just come in.

(We *could* make the splice in ./v not fill the whole pipe buffer, and
allow some other writes to fill in buffers afterwards, but at _some_
point you'll hit the "pipe buffers are full and busy, can't add any
more without waiting for them to empty").

One thing we could possibly do is to say that we just don't accept any
new writes if there are old busy splices in process. So we could make
new writes return -EBUSY or something, I guess.

             Linus
 fs/splice.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/splice.c b/fs/splice.c
index 49139413457d..df6d34dbf116 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -590,6 +590,13 @@ static inline bool eat_empty_buffer(struct pipe_inode_info *pipe)
 	unsigned int mask = pipe->ring_size - 1;
 	struct pipe_buffer *buf = &pipe->bufs[tail & mask];
 
+	/*
+	 * Do a non-blocking buffer confirm. We may need
+	 * to go back to waiting for the pipe to be readable.
+	 */
+	if (pipe_buf_confirm(pipe, buf, true) == -EAGAIN)
+		return true;
+
 	if (unlikely(!buf->len)) {
 		pipe_buf_release(pipe, buf);
 		pipe->tail = tail+1;