Re: [PATCH] SUNRPC: Fix a potential race in xprt_connect()

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2018-12-03 at 10:56 -0500, Chuck Lever wrote:
> > On Dec 2, 2018, at 12:26 PM, Trond Myklebust <trondmy@gmail.com>
> > wrote:
> > 
> > On Sun, 2018-12-02 at 12:07 -0500, Chuck Lever wrote:
> > > 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.
> > 
> > Does /proc/*/stack show any tasks with any interesting stack
> > behaviour?
> > 
> > > 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.
> > 
> > In principle the latter issue is supposed to be handled by the
> > connect
> > cookies. Are the updates perhaps being ordered incorrectly w.r.t.
> > the
> > xprt->pending wakeup? If so, then that could cause races.
> > 
> > I'd expect the cookie would need to be updated before the call to
> > xprt-
> > pending both on connect and disconnect.
> 
> I've found one peculiar cookie-related behavior.
> 
> At some point, xprt_request_retransmit_after_disconnect() starts
> to claim that all requests have to be retransmitted and that the
> connection is closed. Closer inspection shows that the cookie
> check here is failing because it seems to be testing a request
> that hasn't been transmitted yet.

Right, but as far as the transport layer is concerned, there should be
no difference between 'need transmit' and 'need retransmit'. It is up
to the higher RPC client layer to decide whether or not there is a
policy difference in behaviour, and AFAICS rpc_task_need_encode()
should be doing the right thing.

> > > > 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
> > > > 
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com