Message ID | 20090105064728.GE16185@xi.wantstofly.org |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Lennert Buytenhek <buytenh@wantstofly.org> Date: Mon, 5 Jan 2009 07:47:29 +0100 > tcp: don't mask EOF and socket errors on nonblocking splice receive > > Currently, setting SPLICE_F_NONBLOCK on splice from a TCP socket > results in masking of EOF (RDHUP) and error conditions on the socket > by an -EAGAIN return. Move the NONBLOCK check in tcp_splice_read() > to be after the EOF and error checks to fix this. > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> This change looks like the perfect fix for this problem. I'll apply this, thanks Lennert! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 04 2009, David Miller wrote: > From: Lennert Buytenhek <buytenh@wantstofly.org> > Date: Mon, 5 Jan 2009 07:47:29 +0100 > > > tcp: don't mask EOF and socket errors on nonblocking splice receive > > > > Currently, setting SPLICE_F_NONBLOCK on splice from a TCP socket > > results in masking of EOF (RDHUP) and error conditions on the socket > > by an -EAGAIN return. Move the NONBLOCK check in tcp_splice_read() > > to be after the EOF and error checks to fix this. > > > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > > This change looks like the perfect fix for this problem. > > I'll apply this, thanks Lennert! Indeed, you can add my acked-by as well if you want.
On Sun, Jan 04, 2009 at 11:59:46PM -0800, David Miller wrote: > From: Lennert Buytenhek <buytenh@wantstofly.org> > Date: Mon, 5 Jan 2009 07:47:29 +0100 > > > tcp: don't mask EOF and socket errors on nonblocking splice receive > > > > Currently, setting SPLICE_F_NONBLOCK on splice from a TCP socket > > results in masking of EOF (RDHUP) and error conditions on the socket > > by an -EAGAIN return. Move the NONBLOCK check in tcp_splice_read() > > to be after the EOF and error checks to fix this. > > > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > > This change looks like the perfect fix for this problem. > Actually, I wonder why this "if (flags & SPLICE_F_NONBLOCK)" can't be skipped at all. Isn't "if (!timeo)" enough now? Jarek P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jarek Poplawski <jarkao2@gmail.com> Date: Mon, 5 Jan 2009 08:11:37 +0000 > On Sun, Jan 04, 2009 at 11:59:46PM -0800, David Miller wrote: > > From: Lennert Buytenhek <buytenh@wantstofly.org> > > Date: Mon, 5 Jan 2009 07:47:29 +0100 > > > > > tcp: don't mask EOF and socket errors on nonblocking splice receive > > > > > > Currently, setting SPLICE_F_NONBLOCK on splice from a TCP socket > > > results in masking of EOF (RDHUP) and error conditions on the socket > > > by an -EAGAIN return. Move the NONBLOCK check in tcp_splice_read() > > > to be after the EOF and error checks to fix this. > > > > > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > > > > This change looks like the perfect fix for this problem. > > > > Actually, I wonder why this "if (flags & SPLICE_F_NONBLOCK)" can't > be skipped at all. Isn't "if (!timeo)" enough now? Is it really the same condition in the end? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 05, 2009 at 12:14:01AM -0800, David Miller wrote: > From: Jarek Poplawski <jarkao2@gmail.com> > Date: Mon, 5 Jan 2009 08:11:37 +0000 > > > On Sun, Jan 04, 2009 at 11:59:46PM -0800, David Miller wrote: > > > From: Lennert Buytenhek <buytenh@wantstofly.org> > > > Date: Mon, 5 Jan 2009 07:47:29 +0100 > > > > > > > tcp: don't mask EOF and socket errors on nonblocking splice receive > > > > > > > > Currently, setting SPLICE_F_NONBLOCK on splice from a TCP socket > > > > results in masking of EOF (RDHUP) and error conditions on the socket > > > > by an -EAGAIN return. Move the NONBLOCK check in tcp_splice_read() > > > > to be after the EOF and error checks to fix this. > > > > > > > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > > > > > > This change looks like the perfect fix for this problem. > > > > > > > Actually, I wonder why this "if (flags & SPLICE_F_NONBLOCK)" can't > > be skipped at all. Isn't "if (!timeo)" enough now? > > Is it really the same condition in the end? Yes!? Jarek P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 05, 2009 at 12:14:01AM -0800, David Miller wrote: > > > > tcp: don't mask EOF and socket errors on nonblocking splice receive > > > > > > > > Currently, setting SPLICE_F_NONBLOCK on splice from a TCP socket > > > > results in masking of EOF (RDHUP) and error conditions on the socket > > > > by an -EAGAIN return. Move the NONBLOCK check in tcp_splice_read() > > > > to be after the EOF and error checks to fix this. > > > > > > > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > > > > > > This change looks like the perfect fix for this problem. > > > > > > > Actually, I wonder why this "if (flags & SPLICE_F_NONBLOCK)" can't > > be skipped at all. Isn't "if (!timeo)" enough now? > > Is it really the same condition in the end? It _seems_ to be? This: timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK); sets 'timeo' to zero if SPLICE_F_NONBLOCK is set. And when we get to the timeo / SPLICE_F_NONBLOCK checks, it must be in the first iteration of the loop, because if 'spliced' was nonzero, it would have broken out of the loop earlier, and we wouldn't have reached the check at all. So it would seem that the SPLICE_F_NONBLOCK check can be deleted entirely. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jarek Poplawski <jarkao2@gmail.com> Date: Mon, 5 Jan 2009 08:22:52 +0000 > On Mon, Jan 05, 2009 at 12:14:01AM -0800, David Miller wrote: > > Is it really the same condition in the end? > > Yes!? Indeed, as you explained in your follow-up, this splice non-block flag is used to compute the socket timeout value and thus it is in fact totally unnecessary. I'll toss these checks in the net-2.6 tree, thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Mon, 05 Jan 2009 00:52:49 -0800 (PST) > From: Jarek Poplawski <jarkao2@gmail.com> > Date: Mon, 5 Jan 2009 08:22:52 +0000 > > > On Mon, Jan 05, 2009 at 12:14:01AM -0800, David Miller wrote: > > > Is it really the same condition in the end? > > > > Yes!? > > Indeed, as you explained in your follow-up, this splice ^^^ I meant Lennert here, of course. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index c5aca0b..9e31f91 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -571,41 +571,41 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, lock_sock(sk); timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK); while (tss.len) { ret = __tcp_splice_read(sk, &tss); if (ret < 0) break; else if (!ret) { if (spliced) break; - if (flags & SPLICE_F_NONBLOCK) { - ret = -EAGAIN; - break; - } if (sock_flag(sk, SOCK_DONE)) break; if (sk->sk_err) { ret = sock_error(sk); break; } if (sk->sk_shutdown & RCV_SHUTDOWN) break; if (sk->sk_state == TCP_CLOSE) { /* * This occurs when user tries to read * from never connected socket. */ if (!sock_flag(sk, SOCK_DONE)) ret = -ENOTCONN; break; } + if (flags & SPLICE_F_NONBLOCK) { + ret = -EAGAIN; + break; + } if (!timeo) { ret = -EAGAIN; break; } sk_wait_data(sk, &timeo); if (signal_pending(current)) { ret = sock_intr_errno(timeo); break; } continue;