2.6.5-rc3 xprt_connect changes?

2.6.5-rc3 xprt_connect changes?

[Olaf Kirch]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

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
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 

[-- Attachment #2: sunrpc-xprt-connfix --]
[-- Type: text/plain, Size: 2757 bytes --]

--- linux-2.6.4/net/sunrpc/xprt.c.connfix	2004-04-01 15:57:50.000000000 +0200
+++ linux-2.6.4/net/sunrpc/xprt.c	2004-04-01 16:46:19.000000000 +0200
@@ -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);
 

Re: 2.6.5-rc3 xprt_connect changes?

[Trond Myklebust]

On Thu, 2004-04-01 at 09:47, Olaf Kirch wrote:
> 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.

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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: 2.6.5-rc3 xprt_connect changes?

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

On Thu, 2004-04-01 at 15:57, Trond Myklebust wrote:

> 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



[-- Attachment #2: Type: text/plain, Size: 1281 bytes --]

 xprt.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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
--- linux-2.6.5-33-rpc_queue_lock/net/sunrpc/xprt.c	2004-03-31 21:48:44.000000000 -0500
+++ linux-2.6.5-34-rpc_connect/net/sunrpc/xprt.c	2004-04-01 16:03:33.000000000 -0500
@@ -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);

Re: 2.6.5-rc3 xprt_connect changes?

[Olaf Kirch]

On Thu, Apr 01, 2004 at 04:18:49PM -0500, Trond Myklebust wrote:
> On Thu, 2004-04-01 at 15:57, Trond Myklebust wrote:
> 
> > 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
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 


-------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs