diff mbox

[04/12] splice: lift pipe_lock out of splice_to_pipe()

Message ID 20161218203003.GZ1555@ZenIV.linux.org.uk (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Al Viro Dec. 18, 2016, 8:30 p.m. UTC 
On Sun, Dec 18, 2016 at 08:12:07PM +0000, Al Viro wrote:
> On Sun, Dec 18, 2016 at 11:28:44AM -0800, Linus Torvalds wrote:
> > On Sat, Dec 17, 2016 at 11:54 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> > > This break EPIPE handling inside splice when SIGPIPE is ignored:
> > >
> > > Before:
> > > $ { sleep 1; strace -e splice pv -q /dev/zero; } | :
> > 
> > Where is that "splice" program from? Google isn't helpful, and fedora
> > doesn't seem to have it. I'm assuming it was posted in one of the
> > threads, but if so I've long since lost sight of it..
> 
> It's pv(1), actually.  I'm looking into that - debian-packaged pv reproduced
> that crap.

OK, I see what's going on - it's wait_for_space() lifted past the checks
for lack of readers.  The fix, AFAICS, is simply

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Torvalds Dec. 18, 2016, 10:10 p.m. UTC | #1 
On Sun, Dec 18, 2016 at 12:30 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> OK, I see what's going on - it's wait_for_space() lifted past the checks
> for lack of readers.  The fix, AFAICS, is simply

Ugh. Does it have to be duplicated?

How about just making the wait_for_space() loop be a for-loop, and writing it as

   for (;;) {
        if (unlikely(!pipe->readers)) {
                send_sig(SIGPIPE, current, 0);
                return -EPIPE;
        }
        if (pipe->nrbufs == pipe->buffers)
                return 0;
        if (flags & SPLICE_F_NONBLOCK)
                return -EAGAIN;
        if (signal_pending(current))
                return -ERESTARTSYS;
        pipe->waiting_writers++;
        pipe_wait(pipe);
        pipe->waiting_writers--;
   }

and just having it once?

Regardless - Andreas, can you verify that that fixes your issues? I'm
assuming you had some real load that made you notice this, not just he
dummy example..

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Dec. 18, 2016, 10:18 p.m. UTC | #2 
On Sun, Dec 18, 2016 at 02:10:54PM -0800, Linus Torvalds wrote:
> On Sun, Dec 18, 2016 at 12:30 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > OK, I see what's going on - it's wait_for_space() lifted past the checks
> > for lack of readers.  The fix, AFAICS, is simply
> 
> Ugh. Does it have to be duplicated?
> 
> How about just making the wait_for_space() loop be a for-loop, and writing it as
> 
>    for (;;) {
>         if (unlikely(!pipe->readers)) {
>                 send_sig(SIGPIPE, current, 0);
>                 return -EPIPE;
>         }
>         if (pipe->nrbufs == pipe->buffers)

ITYM "!="...
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds Dec. 18, 2016, 10:22 p.m. UTC | #3 
On Sun, Dec 18, 2016 at 2:18 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> ITYM "!="...

Right. A bit too much cut-and-pasting going on in my email ;)

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Schwab Dec. 18, 2016, 10:49 p.m. UTC | #4 
On Dez 18 2016, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Regardless - Andreas, can you verify that that fixes your issues? I'm
> assuming you had some real load that made you notice this, not just he
> dummy example..

This is from the testsuite of pv, I only noticed because it was hanging.

Andreas.
diff mbox

Patch

diff --git a/fs/splice.c b/fs/splice.c
index 6a2b0db5..aeba2b7 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1082,6 +1082,10 @@  EXPORT_SYMBOL(do_splice_direct);
 
 static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 {
+	if (unlikely(!pipe->readers)) {
+		send_sig(SIGPIPE, current, 0);
+		return -EPIPE;
+	}
 	while (pipe->nrbufs == pipe->buffers) {
 		if (flags & SPLICE_F_NONBLOCK)
 			return -EAGAIN;
@@ -1090,6 +1094,10 @@  static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 		pipe->waiting_writers++;
 		pipe_wait(pipe);
 		pipe->waiting_writers--;
+		if (unlikely(!pipe->readers)) {
+			send_sig(SIGPIPE, current, 0);
+			return -EPIPE;
+		}
 	}
 	return 0;
 }