DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 681F52082F
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH] SUNRPC: Fix a potential race in xprt_connect()
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20181202144223.42562-1-trond.myklebust@hammerspace.com>
Date:   Sun, 2 Dec 2018 12:07:06 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: 7bit
Message-Id: <4BAC007E-35BE-423B-BA11-5413D0EEDDBD@oracle.com>
References: <20181202144223.42562-1-trond.myklebust@hammerspace.com>
To:     Trond Myklebust <trondmy@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Test report.

I applied this on top of the three v4 patches from Friday.

With TCP, I no longer see the soft IRQ warnings or bdev leaks.
However, at some point during my test (sec=krb5p,vers=3) the
mount stops responding and the client becomes idle. ^C leaves
the iozone processes in D state.


With RDMA, I can see a few issues:
 - A lot of -74 and server disconnects. I suspect this is due
   to GSS sequence window overruns
 - xprt_connect puts the calling task to sleep if the credit
   limit has been reached, and that can result in a deadlock
 - The reconnect logic appears to busy-wait until something
   else (eg, an RTO) forces a reconnect
 - A connect or disconnect wake-up that occurs while an RPC
   reply is being handled (ie, is still on xprt->pending)
   results in that RPC being retried unnecessarily.

I'm not sure how to address that last one. Sometimes RDMA has
to invalidate MRs, and that involves a wait/context swith which
opens the race window significantly.


> On Dec 2, 2018, at 9:42 AM, Trond Myklebust <trondmy@gmail.com> wrote:
> 
> If an asynchronous connection attempt completes while another task is
> in xprt_connect(), then the call to rpc_sleep_on() could end up
> racing with the call to xprt_wake_pending_tasks().
> So add a second test of the connection state after we've put the
> task to sleep and set the XPRT_CONNECTING flag, when we know that there
> can be no asynchronous connection attempts still in progress.
> 
> Fixes: 0b9e79431377d ("SUNRPC: Move the test for XPRT_CONNECTING into...")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> net/sunrpc/xprt.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 122c91c28e7c..ce927002862a 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -826,8 +826,15 @@ void xprt_connect(struct rpc_task *task)
> 			return;
> 		if (xprt_test_and_set_connecting(xprt))
> 			return;
> -		xprt->stat.connect_start = jiffies;
> -		xprt->ops->connect(xprt, task);
> +		/* Race breaker */
> +		if (!xprt_connected(xprt)) {
> +			xprt->stat.connect_start = jiffies;
> +			xprt->ops->connect(xprt, task);
> +		} else {
> +			xprt_clear_connecting(xprt);
> +			task->tk_status = 0;
> +			rpc_wake_up_queued_task(&xprt->pending, task);
> +		}
> 	}
> 	xprt_release_write(xprt, task);
> }
> -- 
> 2.19.2
> 

--
Chuck Lever