Re: [PATCH 2/2] SUNRPC: Reconnect with new port on server initiated connection termination

[Vallish Vaidyeshwara <vallish@amazon.com>]

On Thu, May 10, 2018 at 05:26:12PM +0000, Trond Myklebust wrote:
> On Thu, 2018-05-10 at 16:22 +0000, Vallish Vaidyeshwara wrote:
> > On Thu, May 10, 2018 at 03:25:14PM +0000, Trond Myklebust wrote:
> > > On Thu, 2018-05-10 at 06:12 +0000, Vallish Vaidyeshwara wrote:
> > > > Server initiated socket close can corrupt connection state
> > > > tracking
> > > > table in conjunction with other network middle boxes. In
> > > > situations
> > > > like these, client connection hangs till connection state
> > > > tracking
> > > > table entries age out and get purged. Client reconnection with a
> > > > new
> > > > port in such a situation will avoid connection hang.
> > > > 
> > > > Reviewed-by: Jacob Strauss <jsstraus@amazon.com>
> > > > Reviewed-by: Alakesh Haloi <alakeshh@amazon.com>
> > > > Signed-off-by: Vallish Vaidyeshwara <vallish@amazon.com>
> > > > ---
> > > >  net/sunrpc/xprtsock.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > > > index 5bf75b3..d293c8d 100644
> > > > --- a/net/sunrpc/xprtsock.c
> > > > +++ b/net/sunrpc/xprtsock.c
> > > > @@ -1629,6 +1629,8 @@ static void xs_tcp_state_change(struct sock
> > > > *sk)
> > > >  		/* The server initiated a shutdown of the socket
> > > > */
> > > >  		xprt->connect_cookie++;
> > > >  		clear_bit(XPRT_CONNECTED, &xprt->state);
> > > > +		/* Server sent FIN, reconnect with a new port */
> > > > +		transport->srcport = 0;
> > > >  		xs_tcp_force_close(xprt);
> > > >  		/* fall through */
> > > >  	case TCP_CLOSING:
> > > > @@ -1650,6 +1652,9 @@ static void xs_tcp_state_change(struct sock
> > > > *sk)
> > > >  					&transport->sock_state))
> > > >  			xprt_clear_connecting(xprt);
> > > >  		clear_bit(XPRT_CLOSING, &xprt->state);
> > > > +		/* Server sent RST, reconnect with a new port */
> > > > +		if (sk->sk_err == ECONNRESET)
> > > > +			transport->srcport = 0;
> > > >  		if (sk->sk_err)
> > > >  			xprt_wake_pending_tasks(xprt, -sk-
> > > > >sk_err);
> > > >  		/* Trigger the socket release */
> > > 
> > > NACK. This will utterly break NFSv2, NFSv3 and NFSv4.0 duplicate
> > > replay
> > > cache semantics. 
> > > 
> > > Cheers
> > >   Trond
> > > -- 
> > > Trond Myklebust
> > > Linux NFS client maintainer, Hammerspace
> > > trond.myklebust@hammerspace.com
> > 
> > Hello Trond,
> > 
> > The first patch in this series is actually helping restore DRC
> > behavior in
> > cases like network partition where packets are dropped:
> > [PATCH 1/2] SUNRPC: Need to reuse non-reserved port for reconnect
> > Patch 1 starts reusing port in all cases.
> > 
> > The second patch is still not breaking DRC semantics, to quote from
> > source
> > code:
> > 
> > <code snip>
> >   /**
> >    * xs_close - close a socket
> >    * @xprt: transport
> >    *
> >    * This is used when all requests are complete; ie, no DRC state
> > remains
> >    * on the server we want to save.
> >    *
> >    * The caller _must_ be holding XPRT_LOCKED in order to avoid
> > issues with
> >    * xs_reset_transport() zeroing the socket from underneath a
> > writer.
> >    */
> >   static void xs_close(struct rpc_xprt *xprt)
> > <code snip>
> > 
> > If the server has closed a connection, then no DRC state remains on
> > the server
> > we want to use.
> > 
> > The second patch is exploiting this semantics and using a new port in
> > following
> > 2 cases:
> > a) RST from server implies the connection was torn down & no useful
> > DRC exists
> > on server
> > b) FIN from server implies that server is shutting down the
> > connection as part
> > of close and no DRC state remains
> > 
> > Please let me know if I have missed something obvious, I definitely
> > do not want
> > to break DRC as that is not the intention of this patch series. Is
> > there a
> > situation where server can close a connection and still keep DRC?
> > 
> 
> The DRC does not exist for the benefit of the server, but for the
> benefit of the client. It is there to ensure that if the client replays
> the request, then it gets the exact same reply as it should have
> received when the first request was sent. Bearing that in mind:
> 
>    1. What guarantees that all servers behave correctly w.r.t. ensuring
>       that they have sent all replies to any outstanding RPC call before
>       shutting down the connection. I'm not sure that even the Linux
>       server does that.
>    2. How would a server even know whether or not the client may need to
>       replay a request?
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

Hello Trond,

Thanks for the explanation, I was kind of misled by code comments in
xs_close.I will probably submit a different patch to clean up these code
comments which can mislead others as well.

Regards,
-Vallish