Re: [PATCH] SUNRPC: Add missing support for RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT

[NeilBrown <neilb@suse.de>]

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

On Thu, 25 Sep 2014 21:23:35 -0400 Trond Myklebust
<trond.myklebust@primarydata.com> wrote:

> On Thu, Sep 25, 2014 at 9:10 PM, NeilBrown <neilb@suse.de> wrote:
> > On Wed, 24 Sep 2014 22:51:19 -0400 Trond Myklebust
> > <trond.myklebust@primarydata.com> wrote:
> >
> >> The flag RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT was intended introduced in
> >> order to allow NFSv4 clients to disable resend timeouts. Since those
> >> cause the RPC layer to break the connection, they mess up the duplicate
> >> reply caches that remain indexed on the port number in NFSv4..
> >>
> >> This patch includes the code that was missing in the original to
> >> set the appropriate flag in struct rpc_clnt, when the caller of
> >> rpc_create() sets RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT.
> >>
> >> Fixes: 8a19a0b6cb2e (SUNRPC: Add RPC task and client level options to...)
> >> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> >> ---
> >>  net/sunrpc/clnt.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >> index 488ddeed9363..841565450354 100644
> >> --- a/net/sunrpc/clnt.c
> >> +++ b/net/sunrpc/clnt.c
> >> @@ -461,6 +461,8 @@ struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
> >>
> >>       if (args->flags & RPC_CLNT_CREATE_AUTOBIND)
> >>               clnt->cl_autobind = 1;
> >> +     if (args->flags & RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT)
> >> +             clnt->cl_noretranstimeo = 1;
> >>       if (args->flags & RPC_CLNT_CREATE_DISCRTRY)
> >>               clnt->cl_discrtry = 1;
> >>       if (!(args->flags & RPC_CLNT_CREATE_QUIET))
> >
> > Hi Trond,
> >  do this relate to my observation in
> >     Subject: Re: NFS auto-reconnect tuning
> >
> >  that NFSv4 closes a connections after the first timeout?
> >  This patch seems to be a step towards changing that behaviour, though it
> >  doesn't succeed.
> >
> > You would need
> >
> > @@ -580,6 +582,7 @@ static struct rpc_clnt *__rpc_clone_client(struct rpc_create_args *args,
> >         new->cl_autobind = 0;
> >         new->cl_softrtry = clnt->cl_softrtry;
> >         new->cl_discrtry = clnt->cl_discrtry;
> > +       new->cl_noretranstimeo = clnt->cl_noretranstimeo;
> 
> D'oh! Yes, of course that is correct.
> 
> >         new->cl_chatty = clnt->cl_chatty;
> >         return new;
> >
> > as well.
> >
> > What do you think of having the client close the connection more quickly when
> > there is a timeout?  There does seem to be a case for closing sooner than 30
> > minutes...
> >
> > I must admit I'm a bit confused by these flags so I might be missing
> > something important.
> 
> Actually, the whole idea is to stop the client from disconnecting when
> it doesn't need to. Disconnect == very bad, since it typically breaks
> the duplicate reply cache semantics and it can interrupt RPC calls
> that may be in progress on the server.
> Furthermore, the NFSv4 server is supposed to guarantee to us that it
> will always reply to an RPC call (provided that the connection stays
> up), so the whole business of disconnecting was misguided in the first
> place. The only thing we need is to detect accidental disconnections
> (i.e. network partitions), and the way to do that is to use TCP
> keepalive. The new code will therefore interpret the NFSv4 'timeo'
> mount parameter as the TCP keepalive timeout rather than as a
> retransmission timeout.
> 

OK - that makes sense.

A problem is that TCP keepalives don't help when you change the local IP
address.
tcp_keepalive_timer() sees that tcp_send_head() is always non-NULL
(presumably a packet that it is trying to send but cannot be) and so it just
keeps waiting.
I guess I should raise this on net-dev ....

Thanks,
NeilBrown

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