Skip to contentSkip to conversation

2.6.5-rc3 xprt_connect changes?

PATCH Olaf Kirch 2 participants 4 messages Apr 1, 2004 – Apr 2, 2004

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

Olaf Kirch

author
#0 lore raw

Hi,

I have a problem with the xprt_connect changes introduced in 2.6.5-rc1. They break pmap_register, it always returns with EIO now.

It seems the problem is this:

- xprt_connect puts the task on the pending queue
- xprt_socket_connect calls xprt_close, which in turn
calls xprt_disconnect, which wakes up all tasks on the pending list with ENOTCONN

For transports with "soft" semantics (this includes the portmap client), this will always cause it to fail with EIO.

For "hard" transports, it seems that after some delay (set in xprt_connect_status) we come back to call_connect_status, which notices the ENOTCONN status. It sets the function pointer to call_connect. If xprt_connected() evaluates to true when we get there (which is always the case for UDP, and which is true for TCP igf the SYNACK has arrived in the meanwhile), the call to xprt_connect() succeeds, and we can continue.

The attached, untested patch should hopefully fix this. I still need to test this.

Olaf

Signature · 3 lines
Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+
net/sunrpc/xprt.c renamed from net/sunrpc/xprt.c.connfix +14 -11
@@ -82,7 +82,7 @@
  */
 static void	xprt_request_init(struct rpc_task *, struct rpc_xprt *);
 static inline void	do_xprt_reserve(struct rpc_task *);
-static void	xprt_disconnect(struct rpc_xprt *);
+static void	xprt_disconnect(struct rpc_xprt *, int);
 static void	xprt_connect_status(struct rpc_task *task);
 static struct rpc_xprt * xprt_setup(int proto, struct sockaddr_in *ap,
 						struct rpc_timeout *to);

@@ -387,7 +387,7 @@
  * Close down a transport socket
  */
 static void
-xprt_close(struct rpc_xprt *xprt)
+xprt_close(struct rpc_xprt *xprt, int reconnecting)
 {
 	struct socket	*sock;
 	struct sock	*sk;

@@ -407,7 +407,7 @@
 	}
 	write_unlock_bh(&sk->sk_callback_lock);
 
-	xprt_disconnect(xprt);
+	xprt_disconnect(xprt, reconnecting);
 
 	if (sock)
 		sock_release(sock);

@@ -418,7 +418,7 @@
 {
 	struct rpc_xprt *xprt = (struct rpc_xprt *)args;
 
-	xprt_close(xprt);
+	xprt_close(xprt, 0);
 	xprt_release_write(xprt, NULL);
 }
 

@@ -426,12 +426,13 @@
  * Mark a transport as disconnected
  */
 static void
-xprt_disconnect(struct rpc_xprt *xprt)
+xprt_disconnect(struct rpc_xprt *xprt, int reconnecting)
 {
 	dprintk("RPC:      disconnected transport %p\n", xprt);
 	spin_lock_bh(&xprt->sock_lock);
 	xprt_clear_connected(xprt);
-	rpc_wake_up_status(&xprt->pending, -ENOTCONN);
+	if (!reconnecting)
+		rpc_wake_up_status(&xprt->pending, -ENOTCONN);
 	spin_unlock_bh(&xprt->sock_lock);
 }
 

@@ -471,8 +472,10 @@
 
 	/*
 	 * Start by resetting any existing state
+	 * Close any sockets, but sshhh: don't wake up the tasks
+	 * on the pending queue.
 	 */
-	xprt_close(xprt);
+	xprt_close(xprt, 1);
 	sock = xprt_create_socket(xprt, xprt->prot, xprt->resvport);
 	if (sock == NULL) {
 		/* couldn't create socket or bind to reserved port;

@@ -830,7 +833,7 @@
 	/* Sanity check of the record length */
 	if (xprt->tcp_reclen < 4) {
 		printk(KERN_ERR "RPC: Invalid TCP record fragment length\n");
-		xprt_disconnect(xprt);
+		xprt_disconnect(xprt, 0);
 	}
 	dprintk("RPC:      reading TCP record fragment of length %d\n",
 			xprt->tcp_reclen);

@@ -1046,7 +1049,7 @@
 					NIPQUAD(xprt->addr.sin_addr.s_addr),
 					xprt_connected(xprt)? "closed" : "refused");
 		}
-		xprt_disconnect(xprt);
+		xprt_disconnect(xprt, 0);
 		break;
 	}
  out:

@@ -1258,7 +1261,7 @@
 		return;
 	default:
 		if (xprt->stream)
-			xprt_disconnect(xprt);
+			xprt_disconnect(xprt, 0);
 	}
 	xprt_release_write(xprt, task);
 	return;

@@ -1657,7 +1660,7 @@
 {
 	dprintk("RPC:      destroying transport %p\n", xprt);
 	xprt_shutdown(xprt);
-	xprt_close(xprt);
+	xprt_close(xprt, 0);
 	kfree(xprt->slot);
 	kfree(xprt);
#1 lore raw
Quoted 24 lines · Olaf Kirch

Hi,

I have a problem with the xprt_connect changes introduced in 2.6.5-rc1. They break pmap_register, it always returns with EIO now.

It seems the problem is this:

- xprt_connect puts the task on the pending queue
- xprt_socket_connect calls xprt_close, which in turn
calls xprt_disconnect, which wakes up all tasks on the pending list with ENOTCONN

For transports with "soft" semantics (this includes the portmap client), this will always cause it to fail with EIO.

For "hard" transports, it seems that after some delay (set in xprt_connect_status) we come back to call_connect_status, which notices the ENOTCONN status. It sets the function pointer to call_connect. If xprt_connected() evaluates to true when we get there (which is always the case for UDP, and which is true for TCP igf the SYNACK has arrived in the meanwhile), the call to xprt_connect() succeeds, and we can continue.

Quoted 2 lines

The attached, untested patch should hopefully fix this. I still need to test this.

How about if we simply move the call for xprt_disconnect() out of xprt_close()?

I fully agree that xprt_connect() doesn't actually need to tell the world that the socket has disconnected: we should already have an RPC task that is taking responsibility for monitoring the reconnection of the socket.

Cheers,
Trond

-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials Free Linux tutorial presented by Daniel Robbins, President and CEO of GenToo technologies. Learn everything from fundamentals to system administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

#2 lore raw
Quoted 2 lines · Trond Myklebust

How about if we simply move the call for xprt_disconnect() out of xprt_close()?

IOW, something like the appended patch. (Note the change to ensure that we *do* wake up all pending tasks if the current attempt at reconnecting actually fails.)

Cheers,
Trond

 xprt.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
net/sunrpc/xprt.c +4 -2
diff -u --recursive --show-c-function linux-2.6.5-33-rpc_queue_lock/net/sunrpc/xprt.c linux-2.6.5-34-rpc_connect/net/sunrpc/xprt.c
@@ -405,7 +405,6 @@ xprt_close(struct rpc_xprt *xprt)
 	sk->sk_write_space  = xprt->old_write_space;
 	write_unlock_bh(&sk->sk_callback_lock);
 
-	xprt_disconnect(xprt);
 	sk->sk_no_check	 = 0;
 
 	sock_release(sock);

@@ -416,6 +415,7 @@ xprt_socket_autoclose(void *args)
 {
 	struct rpc_xprt *xprt = (struct rpc_xprt *)args;
 
+	xprt_disconnect(xprt);
 	xprt_close(xprt);
 	xprt_release_write(xprt, NULL);
 }

@@ -511,7 +511,8 @@ out_err:
 	if (xprt->snd_task) {
 		xprt->snd_task->tk_status = status;
 		rpc_wake_up_task(xprt->snd_task);
-	}
+	} else
+		rpc_wake_up_status(&xprt->pending, -ENOTCONN);
 	spin_unlock_bh(&xprt->sock_lock);
 }
 

@@ -1642,6 +1643,7 @@ xprt_destroy(struct rpc_xprt *xprt)
 {
 	dprintk("RPC:      destroying transport %p\n", xprt);
 	xprt_shutdown(xprt);
+	xprt_disconnect(xprt);
 	xprt_close(xprt);
 	kfree(xprt->slot);
 	kfree(xprt);
author
#3 lore raw
Quoted 8 lines · Trond Myklebust
Quoted 2 lines · Trond Myklebust

How about if we simply move the call for xprt_disconnect() out of xprt_close()?

IOW, something like the appended patch. (Note the change to ensure that we *do* wake up all pending tasks if the current attempt at reconnecting actually fails.)

Yes, this patch should do the same, and it's nicer.

Olaf

Signature · 13 lines
Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs