Skip to contentSkip to conversation

NFS client over TCP hangs due to packet loss

PATCH In review Andy Chittenden 5 participants 12 messages Aug 3, 2010 – Jun 20, 2018

Subsystems (4)
Networking Network drivers NFS client NFS server
lore
Share & tools
lore

Andy Chittenden

author
#0 lore raw

PATCH 1/1 NFS client over TCP hangs due to packet loss

I don't know whether this patch is the correct fix or not but it enables the NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes <https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears down any previous shutdown attempts so that reconnects on a socket that's been shutdown leave the socket in a usable state (otherwise tcp_sendmsg() returns -EPIPE).

# diff -up /home/company/software/src/linux-2.6.34.1/net/ipv4/tcp_output.c
net/ipv4
--- /home/company/software/src/linux-2.6.34.1/net/ipv4/tcp_output.c
2010-07-27 08:46:46.917000000 +0100
+++ net/ipv4/tcp_output.c       2010-07-27 09:19:16.000000000 +0100
@@ -2522,6 +2522,13 @@ static void tcp_connect_init(struct sock
        struct tcp_sock *tp = tcp_sk(sk);
        __u8 rcv_wscale;

+ /* clear down any previous shutdown attempts so that

+        * reconnects on a socket that's been shutdown leave the
+        * socket in a usable state (otherwise tcp_sendmsg() returns
+        * -EPIPE).
+        */
+       sk->sk_shutdown = 0;
+
        /* We'll fix this up when we get a response from the other end.
         * See tcp_input.c:tcp_rcv_state_process case TCP_SYN_SENT.
         */
Signed-off-by Andy Chittenden <[email protected]>
maintainer
#1 lore raw

From: "Andy Chittenden" <[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 9 lines

I don't know whether this patch is the correct fix or not but it enables the NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes <https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears down any previous shutdown attempts so that reconnects on a socket that's been shutdown leave the socket in a usable state (otherwise tcp_sendmsg() returns -EPIPE).

If the SunRPC code wants to close a TCP socket then use it again, it should disconnect by doing a connect() with sa_family == AF_UNSPEC

#2 lore raw

(cc linux-nfs)

Quoted 8 lines · On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David Miller…

From: "Andy Chittenden" <[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 5 lines

I don't know whether this patch is the correct fix or not but it enables the NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Quoted 7 lines
Quoted 4 lines

Fixes <https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears down any previous shutdown attempts so that reconnects on a socket that's been shutdown leave the socket in a usable state (otherwise tcp_sendmsg() returns -EPIPE).

If the SunRPC code wants to close a TCP socket then use it again, it should disconnect by doing a connect() with sa_family == AF_UNSPEC

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

author
#3 lore raw
Quoted 19 lines · Andrew Morton

(cc linux-nfs)

Quoted 15 lines · On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David…

From: "Andy Chittenden"<[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 9 lines

I don't know whether this patch is the correct fix or not but it enables the NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes<https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears down any previous shutdown attempts so that reconnects on a socket that's been shutdown leave the socket in a usable state (otherwise tcp_sendmsg() returns -EPIPE).

If the SunRPC code wants to close a TCP socket then use it again, it should disconnect by doing a connect() with sa_family == AF_UNSPEC

There is code to do that in the SunRPC code in xs_abort_connection() but that's conditionally called from xs_tcp_reuse_connection():

static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)

{
	unsigned int state = transport->inet->sk_state;

	if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
		return;
	if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
		return;
	xs_abort_connection(xprt, transport);
}

That's changed since 2.6.26 where it unconditionally did the connect() with sa_family == AF_UNSPEC. FWIW we cannot reproduce this problem with 2.6.26.

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

author
#4 lore raw
Quoted 44 lines · Andrew Morton
Quoted 3 lines

(cc linux-nfs)

On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David

Quoted 5 lines · Miller
Quoted 5 lines

From: "Andy Chittenden"<[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 1 line

I don't know whether this patch is the correct fix or not but it

enables the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes<https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears

down

Quoted 1 line
Quoted 1 line
Quoted 1 line

any previous shutdown attempts so that reconnects on a socket

that's been

Quoted 1 line
Quoted 1 line
Quoted 1 line

shutdown leave the socket in a usable state (otherwise

tcp_sendmsg() returns

Quoted 4 lines
Quoted 4 lines
Quoted 1 line

-EPIPE).

If the SunRPC code wants to close a TCP socket then use it again, it should disconnect by doing a connect() with sa_family ==

AF_UNSPEC

There is code to do that in the SunRPC code in xs_abort_connection() but
that's conditionally called from xs_tcp_reuse_connection():

static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)

{
	unsigned int state = transport->inet->sk_state;

	if (state == TCP_CLOSE && transport->sock->state ==
SS_UNCONNECTED)
		return;
	if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
		return;
	xs_abort_connection(xprt, transport);
}
Quoted 3 lines

That's changed since 2.6.26 where it unconditionally did the connect() with sa_family == AF_UNSPEC. FWIW we cannot reproduce this problem with 2.6.26.

The problem is fixed with this patch which also prints out that sk_shutdown can be non-zero on entry to xs_tcp_reuse_connection:

# diff -up /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
net/sunrpc/xprtsock.c 
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
2010-08-02 18:30:51.000000000 +0100
+++ net/sunrpc/xprtsock.c       2010-08-05 12:21:11.000000000 +0100
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n", xprt);
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d sk_shutdown
%d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
        switch (sk->sk_state) {
        case TCP_ESTABLISHED:
@@ -1796,10 +1797,18 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;

- if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
- return;
- if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
- return;
+ if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
{
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ printk("%s: TCP_CLOSEd and sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }
+ if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ printk("%s: sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }

        xs_abort_connection(xprt, transport);

}

Signed-off-by Andy Chittenden <[email protected]>

dmesg displays:

[ 2840.896043] xs_tcp_reuse_connection: TCP_CLOSEd and sk_shutdown set to 2

so previously the code was attempting to reuse the connection but wasn't aborting it and thus didn't clear down sk_shutdown.

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

#5 lore raw
Quoted 95 lines · Andy Chittenden
Quoted 44 lines · Andrew Morton
Quoted 3 lines

(cc linux-nfs)

On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David

Quoted 5 lines · Miller
Quoted 5 lines

From: "Andy Chittenden"<[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 1 line

I don't know whether this patch is the correct fix or not but it

enables the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes<https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears

down

Quoted 1 line
Quoted 1 line
Quoted 1 line

any previous shutdown attempts so that reconnects on a socket

that's been

Quoted 1 line
Quoted 1 line
Quoted 1 line

shutdown leave the socket in a usable state (otherwise

tcp_sendmsg() returns

Quoted 4 lines
Quoted 4 lines
Quoted 1 line

-EPIPE).

If the SunRPC code wants to close a TCP socket then use it again, it should disconnect by doing a connect() with sa_family ==

AF_UNSPEC

There is code to do that in the SunRPC code in xs_abort_connection() but
that's conditionally called from xs_tcp_reuse_connection():

static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)

{
	unsigned int state = transport->inet->sk_state;

	if (state == TCP_CLOSE && transport->sock->state ==
SS_UNCONNECTED)
		return;
	if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
		return;
	xs_abort_connection(xprt, transport);
}
Quoted 3 lines

That's changed since 2.6.26 where it unconditionally did the connect() with sa_family == AF_UNSPEC. FWIW we cannot reproduce this problem with 2.6.26.

The problem is fixed with this patch which also prints out that sk_shutdown can be non-zero on entry to xs_tcp_reuse_connection:

# diff -up /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
net/sunrpc/xprtsock.c 
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
2010-08-02 18:30:51.000000000 +0100
+++ net/sunrpc/xprtsock.c       2010-08-05 12:21:11.000000000 +0100
(quoted diff) +3 -2
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n", xprt);
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d sk_shutdown
 %d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
 
        switch (sk->sk_state) {

case TCP_ESTABLISHED:

(quoted diff) +12 -4
@@ -1796,10 +1797,18 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;
 
-       if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
-               return;
-       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
-               return;
+       if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
 {
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               printk("%s: TCP_CLOSEd and sk_shutdown set to %d\n",
+                       __func__, transport->inet->sk_shutdown);
+       }
+       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               printk("%s: sk_shutdown set to %d\n",
+                       __func__, transport->inet->sk_shutdown);
+       }
        xs_abort_connection(xprt, transport);
 }
Quoted 8 lines
Signed-off-by Andy Chittenden <[email protected]>

dmesg displays:

[ 2840.896043] xs_tcp_reuse_connection: TCP_CLOSEd and sk_shutdown set to 2

so previously the code was attempting to reuse the connection but wasn't aborting it and thus didn't clear down sk_shutdown.

Hi Andy,

I note that you are adding in two new printk()s. Why should they be printk(), and not dprintk()? Are you trying to report an exception that the user needs to be aware of, or is this only debugging info that we'll want to turn off under normal operation?

Also, it might be useful to add a comment to the code here to remind us what the 'sk_shutdown == 0' case corresponds to as far as the socket state is concerned, so that the casual reader can see why we shouldn't reset the connection.

Cheers
Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#6 lore raw
Quoted 119 lines · Andy Chittenden
Quoted 10 lines
Quoted 10 lines · Andrew Morton
Quoted 3 lines

(cc linux-nfs)

On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David

Quoted 5 lines · Miller
Quoted 5 lines

From: "Andy Chittenden"<[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 1 line

I don't know whether this patch is the correct fix or not but

it

Quoted 6 lines
Quoted 6 lines

enables the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes<https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It

clears

Quoted 8 lines
Quoted 8 lines

down

Quoted 1 line
Quoted 1 line
Quoted 1 line

any previous shutdown attempts so that reconnects on a socket

that's been

Quoted 1 line
Quoted 1 line
Quoted 1 line

shutdown leave the socket in a usable state (otherwise

tcp_sendmsg() returns

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

-EPIPE).

If the SunRPC code wants to close a TCP socket then use it

again,

Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line

it should disconnect by doing a connect() with sa_family ==

AF_UNSPEC

There is code to do that in the SunRPC code in

xs_abort_connection()

Quoted 16 lines
Quoted 16 lines

but
that's conditionally called from xs_tcp_reuse_connection():

static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)

{
	unsigned int state = transport->inet->sk_state;

	if (state == TCP_CLOSE && transport->sock->state ==
SS_UNCONNECTED)
		return;
	if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
		return;
	xs_abort_connection(xprt, transport);
}
Quoted 1 line
Quoted 1 line

That's changed since 2.6.26 where it unconditionally did the

connect()

Quoted 1 line
Quoted 1 line

with sa_family == AF_UNSPEC. FWIW we cannot reproduce this problem

with

Quoted 3 lines
Quoted 1 line

2.6.26.

The problem is fixed with this patch which also prints out that

sk_shutdown

Quoted 3 lines

can be non-zero on entry to xs_tcp_reuse_connection:

# diff -up /home/company/software/src/linux-

2.6.34.2/net/sunrpc/xprtsock.c

Review static void xs_tcp_state_change(struct s · line 1324

Quoted 8 lines

net/sunrpc/xprtsock.c
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c

2010-08-02 18:30:51.000000000 +0100

+++ net/sunrpc/xprtsock.c 2010-08-05 12:21:11.000000000 +0100

(quoted diff) +0 -0
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n",

xprt);

Review static void xs_tcp_state_change(struct s · line 1325

Quoted 2 lines
(quoted diff) +1 -1
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d

sk_shutdown

Quoted 10 lines
(quoted diff) +2 -1
 %d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
 
        switch (sk->sk_state) {

case TCP_ESTABLISHED:

Review static void xs_tcp_reuse_connection(stru · line 1799

Quoted 4 lines
(quoted diff) +0 -1
@@ -1796,10 +1797,18 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;
 
-       if (state == TCP_CLOSE && transport->sock->state ==

SS_UNCONNECTED)

Review static void xs_tcp_reuse_connection(stru · line 1800

Quoted 4 lines
(quoted diff) +1 -3
-               return;
-       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
-               return;
+       if (state == TCP_CLOSE && transport->sock->state ==

SS_UNCONNECTED)

Quoted 15 lines
(quoted diff) +11 -0
 {
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               printk("%s: TCP_CLOSEd and sk_shutdown set to %d\n",
+                       __func__, transport->inet->sk_shutdown);
+       }
+       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               printk("%s: sk_shutdown set to %d\n",
+                       __func__, transport->inet->sk_shutdown);
+       }
        xs_abort_connection(xprt, transport);
 }
Quoted 5 lines
Signed-off-by Andy Chittenden <[email protected]>

dmesg displays:

[ 2840.896043] xs_tcp_reuse_connection: TCP_CLOSEd and sk_shutdown

set to 2

Quoted 2 lines

so previously the code was attempting to reuse the connection but

wasn't

Quoted 1 line

aborting it and thus didn't clear down sk_shutdown.

Quoted 7 lines

Hi Andy,

I note that you are adding in two new printk()s. Why should they be printk(), and not dprintk()? Are you trying to report an exception that the user needs to be aware of, or is this only debugging info that we'll
want to turn off under normal operation?

Hi Trond

Thanks for replying. It was debugging info: the printk()s show what the problem is. I was half expecting someone to pipe up "that isn't the correct way to fix this" and suggest another avenue to look at, or even, hopefully, come up with an alternative appropriate patch as I'm not an expert in this code: I don't know whether the sk_shutdown field being left set has implications elsewhere in the sunrpc code. FWIW I left the test case running overnight and have had only 50 such messages logged so it's not a heavy printk() load.

Quoted 5 lines

Also, it might be useful to add a comment to the code here to remind us what the 'sk_shutdown == 0' case corresponds to as far as the socket state is concerned, so that the casual reader can see why we shouldn't reset the connection.

If I knew what sk_shutdown == 0 really corresponded to, I could well add a comment! :-). I just knew that in 2.6.26 we didn't see this problem and that in later kernels the connection abort sequence was being done conditionally and that the sk_shutdown flag being left set was making tcp_sendmsg return an error. So, putting two and two together, I've effectively just added another condition in which to abort the connection.

As nobody has objected to the essence of my patch, I'll attempt a new patch that changes those printk()s into dprintk() and drop in what I think are appropriate comments. So here's a revised patch:

# diff -up /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c net/sunrpc/xprtsock.c 
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c     2010-08-02 18:30:51.000000000 +0100
+++ net/sunrpc/xprtsock.c       2010-08-06 08:09:08.000000000 +0100
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n", xprt);
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d sk_shutdown %d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
        switch (sk->sk_state) {
        case TCP_ESTABLISHED:
@@ -1796,10 +1797,25 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;

- if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
- return;
- if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
- return;
+ if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED) {
+ /* we don't need to abort the connection if the socket
+ * hasn't undergone a shutdown
+ */
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ dprintk("RPC: %s: TCP_CLOSEd and sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }
+ if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+ /* we don't need to abort the connection if the socket
+ * hasn't undergone a shutdown
+ */
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ dprintk("RPC: %s: ESTABLISHED/SYN_SENT "
+ "sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }

        xs_abort_connection(xprt, transport);

}

Signed-off-by Andy Chittenden <[email protected]>

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

author
#7 lore raw
Quoted 211 lines
Quoted 49 lines · Andy Chittenden
Quoted 10 lines
Quoted 10 lines · Andrew Morton
Quoted 3 lines

(cc linux-nfs)

On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David

Quoted 5 lines · Miller
Quoted 5 lines

From: "Andy Chittenden"<[email protected]>
Date: Tue, 3 Aug 2010 09:14:31 +0100

Quoted 1 line

I don't know whether this patch is the correct fix or not but

it

Quoted 6 lines
Quoted 6 lines

enables the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

NFS client to recover.

Kernel version: 2.6.34.1 and 2.6.32.

Fixes<https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It

clears

Quoted 8 lines
Quoted 8 lines

down

Quoted 1 line
Quoted 1 line
Quoted 1 line

any previous shutdown attempts so that reconnects on a socket

that's been

Quoted 1 line
Quoted 1 line
Quoted 1 line

shutdown leave the socket in a usable state (otherwise

tcp_sendmsg() returns

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

-EPIPE).

If the SunRPC code wants to close a TCP socket then use it

again,

Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line

it should disconnect by doing a connect() with sa_family ==

AF_UNSPEC

There is code to do that in the SunRPC code in

xs_abort_connection()

Quoted 16 lines
Quoted 16 lines

but
that's conditionally called from xs_tcp_reuse_connection():

static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct sock_xprt *transport)

{
	unsigned int state = transport->inet->sk_state;

	if (state == TCP_CLOSE && transport->sock->state ==
SS_UNCONNECTED)
		return;
	if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
		return;
	xs_abort_connection(xprt, transport);
}
Quoted 3 lines
Quoted 1 line
Quoted 1 line

That's changed since 2.6.26 where it unconditionally did the

connect()

Quoted 1 line
Quoted 1 line

with sa_family == AF_UNSPEC. FWIW we cannot reproduce this

problem

Quoted 34 lines

with

Quoted 3 lines
Quoted 1 line

2.6.26.

The problem is fixed with this patch which also prints out that

sk_shutdown

Quoted 3 lines

can be non-zero on entry to xs_tcp_reuse_connection:

# diff -up /home/company/software/src/linux-

2.6.34.2/net/sunrpc/xprtsock.c

Review static void xs_tcp_state_change(struct s · line 1324

Quoted 8 lines

net/sunrpc/xprtsock.c
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c

2010-08-02 18:30:51.000000000 +0100

+++ net/sunrpc/xprtsock.c 2010-08-05 12:21:11.000000000 +0100

(quoted diff) +0 -0
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n",

xprt);

Review static void xs_tcp_state_change(struct s · line 1325

Quoted 2 lines
(quoted diff) +1 -1
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d

sk_shutdown

Quoted 13 lines
(quoted diff) +2 -1
 %d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
 
        switch (sk->sk_state) {

case TCP_ESTABLISHED:

(quoted diff) +0 -0
@@ -1796,10 +1797,18 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;
Quoted 11 lines

Review static void xs_tcp_reuse_connection(stru · line 1799

Quoted 1 line
(quoted diff) +0 -1
-       if (state == TCP_CLOSE && transport->sock->state ==

SS_UNCONNECTED)

Review static void xs_tcp_reuse_connection(stru · line 1800

Quoted 4 lines
(quoted diff) +1 -3
-               return;
-       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
-               return;
+       if (state == TCP_CLOSE && transport->sock->state ==

SS_UNCONNECTED)

Quoted 4 lines
(quoted diff) +3 -0
 {
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               printk("%s: TCP_CLOSEd and sk_shutdown set to

%d\n",

Quoted 18 lines
Quoted 11 lines
(quoted diff) +8 -0
+                       __func__, transport->inet->sk_shutdown);
+       }
+       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               printk("%s: sk_shutdown set to %d\n",
+                       __func__, transport->inet->sk_shutdown);
+       }
        xs_abort_connection(xprt, transport);
 }
Quoted 5 lines
Signed-off-by Andy Chittenden <[email protected]>

dmesg displays:

[ 2840.896043] xs_tcp_reuse_connection: TCP_CLOSEd and sk_shutdown

set to 2

Quoted 1 line
Quoted 8 lines
Quoted 1 line

so previously the code was attempting to reuse the connection but

wasn't

Quoted 1 line

aborting it and thus didn't clear down sk_shutdown.

Hi Andy,

I note that you are adding in two new printk()s. Why should they be printk(), and not dprintk()? Are you trying to report an exception

that

Quoted 3 lines

the user needs to be aware of, or is this only debugging info that we'll
want to turn off under normal operation?

Hi Trond

Thanks for replying. It was debugging info: the printk()s show what the problem is. I was half expecting someone to pipe up "that isn't the correct way to fix this" and suggest another avenue to look at, or even, hopefully, come up with an alternative appropriate patch as I'm not an expert in this code: I don't know whether the sk_shutdown field being left set has implications elsewhere in the sunrpc code. FWIW I left the test case running overnight and have had only 50 such messages logged so it's not a heavy printk() load.

Quoted 2 lines

Also, it might be useful to add a comment to the code here to remind

us

Quoted 2 lines

what the 'sk_shutdown == 0' case corresponds to as far as the socket state is concerned, so that the casual reader can see why we

shouldn't

Quoted 1 line

reset the connection.

If I knew what sk_shutdown == 0 really corresponded to, I could well add a comment! :-). I just knew that in 2.6.26 we didn't see this problem and that in later kernels the connection abort sequence was being done conditionally and that the sk_shutdown flag being left set was making tcp_sendmsg return an error. So, putting two and two together, I've effectively just added another condition in which to abort the connection.

As nobody has objected to the essence of my patch, I'll attempt a new patch that changes those printk()s into dprintk() and drop in what I think are appropriate comments. So here's a revised patch:

# diff -up /home/company/software/src/linux-
2.6.34.2/net/sunrpc/xprtsock.c net/sunrpc/xprtsock.c
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
2010-08-02 18:30:51.000000000 +0100
+++ net/sunrpc/xprtsock.c       2010-08-06 08:09:08.000000000 +0100
(quoted diff) +3 -2
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n", xprt);
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d
 sk_shutdown %d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
 
        switch (sk->sk_state) {

case TCP_ESTABLISHED:

(quoted diff) +18 -4
@@ -1796,10 +1797,25 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;
 
-       if (state == TCP_CLOSE && transport->sock->state ==
 SS_UNCONNECTED)
-               return;
-       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
-               return;
+       if (state == TCP_CLOSE && transport->sock->state ==
 SS_UNCONNECTED) {
+               /* we don't need to abort the connection if the socket
+                * hasn't undergone a shutdown
+                */
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               dprintk("RPC:       %s: TCP_CLOSEd and sk_shutdown set
 to %d\n",
+                       __func__, transport->inet->sk_shutdown);
+       }
+       if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+               /* we don't need to abort the connection if the socket
+                * hasn't undergone a shutdown
+                */
+               if (transport->inet->sk_shutdown == 0)
+                       return;
+               dprintk("RPC:       %s: ESTABLISHED/SYN_SENT "
+                               "sk_shutdown set to %d\n",
+                               __func__, transport->inet-
Quoted 1 line

sk_shutdown);

Quoted 6 lines

+ }

        xs_abort_connection(xprt, transport);

}

Signed-off-by Andy Chittenden <[email protected]>

A weekend run with that patch applied to 2.6.34.2 was successful. As nobody has objected, what's the next step to getting it applied to the official source trees?

#8 lore raw
Quoted 1 line · Andy Chittenden

A weekend run with that patch applied to 2.6.34.2 was successful. As nobody has objected, what's the next step to getting it applied to the official source trees?

Please resend me a version with a cleaned up changelog entry. I can then push it as a bugfix.

Cheers
Trond

author
#9 lore raw
Quoted 11 lines · Andy Chittenden
Quoted 1 line

A weekend run with that patch applied to 2.6.34.2 was successful. As

nobody has objected, what's the next step to getting it applied to the official source trees?

Please resend me a version with a cleaned up changelog entry. I can then
push it as a bugfix.

Cheers
Trond

Thanks. I think this sums it up:

SUNRPC: fix NFS client over TCP hangs due to packet loss (Bug 16494)

When reusing a TCP connection, ensure that it's aborted if a previous shutdown attempt has been made on that connection so that the RPC over TCP recovery mechanism succeeds.

# diff -up /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c net/sunrpc/xprtsock.c 
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c     2010-08-02 18:30:51.000000000 +0100
+++ net/sunrpc/xprtsock.c       2010-08-06 08:09:08.000000000 +0100
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
        if (!(xprt = xprt_from_sock(sk)))
                goto out;
        dprintk("RPC:       xs_tcp_state_change client %p...\n", xprt);
-       dprintk("RPC:       state %x conn %d dead %d zapped %d\n",
+       dprintk("RPC:       state %x conn %d dead %d zapped %d sk_shutdown %d\n",
                        sk->sk_state, xprt_connected(xprt),
                        sock_flag(sk, SOCK_DEAD),
-                       sock_flag(sk, SOCK_ZAPPED));
+                       sock_flag(sk, SOCK_ZAPPED),
+                       sk->sk_shutdown);
        switch (sk->sk_state) {
        case TCP_ESTABLISHED:
@@ -1796,10 +1797,25 @@ static void xs_tcp_reuse_connection(stru
 {
        unsigned int state = transport->inet->sk_state;

- if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
- return;
- if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
- return;
+ if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED) {
+ /* we don't need to abort the connection if the socket
+ * hasn't undergone a shutdown
+ */
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ dprintk("RPC: %s: TCP_CLOSEd and sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }
+ if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+ /* we don't need to abort the connection if the socket
+ * hasn't undergone a shutdown
+ */
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ dprintk("RPC: %s: ESTABLISHED/SYN_SENT "
+ "sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }

        xs_abort_connection(xprt, transport);

}

Signed-off-by Andy Chittenden <[email protected]>

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

author
#11 lore raw
Quoted 5 lines · Joe Perches

On Mon, 2010-08-09 at 10:27 +0100, Andy Chittenden wrote:

You really need to check your clock.
Mail sent in the year 2010?

I didn't send it just now. I sent it on 2010-08-09 at 10:27. And I did receive a copy at that time. If I'm reading the headers correctly, something happened here:

Received: from mchehab by bombadil.infradead.org with local (Exim 4.90_1 #2 (Red Hat Linux))

	id 1fVNCq-0005da-3g; Tue, 19 Jun 2018 20:26:24 +0000
Received: from vger.kernel.org ([209.132.180.67])
	by bombadil.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux))
	id 1OiOdm-0004za-Ks; Mon, 09 Aug 2010 09:27:30 +0000
Received: ([email protected]) by vger.kernel.org via listexpand
	id S1755981Ab0HIJ1U (ORCPT <rfc822;[email protected]> + 8 others);
	Mon, 9 Aug 2010 05:27:20 -0400
Signature · 1 line
Andy