Re: [PATCH] Revert "SUNRPC: xs_sock_mark_closed() does not need to trigger socket autoclose"

From: NeilBrown <neilb@suse.com>
To: Trond Myklebust <trondmy@primarydata.com>
Cc: List Linux NFS Mailing <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] Revert "SUNRPC: xs_sock_mark_closed() does not need to trigger socket autoclose"
Date: Mon, 21 Nov 2016 14:24:17 +1100	[thread overview]
Message-ID: <87lgwdecri.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <E2DB8B0D-1A5D-49DD-A710-ED26FE328187@primarydata.com>

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

On Sat, Nov 19 2016, Trond Myklebust wrote:

>> On Aug 9, 2016, at 22:05, NeilBrown <neilb@suse.com> wrote:
....
>> 
>> Another approach which works is:
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 7f79fb7dc6a0..f80b135b4830 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -1936,8 +1936,10 @@ call_connect_status(struct rpc_task *task)
>> 	case -EADDRINUSE:
>> 	case -ENOBUFS:
>> 	case -EPIPE:
>> -		if (RPC_IS_SOFTCONN(task))
>> +		if (RPC_IS_SOFTCONN(task)) {
>> +			xprt_force_disconnect(task->tk_rqstp->rq_xprt);
>> 			break;
>> +		}
>> 		/* retry with existing socket, after a delay */
>> 		rpc_delay(task, 3*HZ);
>> 	case -EAGAIN:
>> 
>> so that we force a disconnect precisely when a connection attempt fails
>> on a SOFTCONN task.
>> 
>> Which of these solutions do you prefer?
>
>
> Instead of the above, could we rather look at adding a call to xprt_conditional_disconnect() to ensure that we don’t keep disconnecting multiple times? It means we’d have to set req->rq_connect_cookie in xprt_connect() before sleeping on xprt->pending, but I don’t think that is a major imposition.
>
> I also think we might not want to make that call conditional on RPC_IS_SOFTCONN.

Something like this maybe?

Just using xprt_conditional_disconnect() doesn't work as it does
nothing when !xprt_connected(xprt).
I commented out that test and then the change works.  I don't know if
that is what you want though.

Thanks,
NeilBrown

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b31ca97e754e..53acd4e3a317 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1926,6 +1926,8 @@ call_connect_status(struct rpc_task *task)
 	case -EADDRINUSE:
 	case -ENOBUFS:
 	case -EPIPE:
+		xprt_conditional_disconnect(task->tk_rqstp->rq_xprt,
+					    task->tk_rqstp->rq_connect_cookie);
 		if (RPC_IS_SOFTCONN(task))
 			break;
 		/* retry with existing socket, after a delay */
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 685e6d225414..52007d018135 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -669,7 +669,7 @@ void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie)
 	spin_lock_bh(&xprt->transport_lock);
 	if (cookie != xprt->connect_cookie)
 		goto out;
-	if (test_bit(XPRT_CLOSING, &xprt->state) || !xprt_connected(xprt))
+	if (test_bit(XPRT_CLOSING, &xprt->state) /*|| !xprt_connected(xprt)*/)
 		goto out;
 	set_bit(XPRT_CLOSE_WAIT, &xprt->state);
 	/* Try to schedule an autoclose RPC call */
@@ -772,6 +772,7 @@ void xprt_connect(struct rpc_task *task)
 	if (!xprt_connected(xprt)) {
 		task->tk_rqstp->rq_bytes_sent = 0;
 		task->tk_timeout = task->tk_rqstp->rq_timeout;
+		task->tk_rqstp->rq_connect_cookie = xprt->connect_cookie;
 		rpc_sleep_on(&xprt->pending, task, xprt_connect_status);
 
 		if (test_bit(XPRT_CLOSING, &xprt->state))

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
