Re: [PATCH] nfsd: CB_RECALL can race with FREE_STATEID

[Scott Mayhew <smayhew@redhat.com>]

On Tue, 30 Apr 2019, Trond Myklebust wrote:

> On Tue, 2019-04-30 at 14:58 -0400, Scott Mayhew wrote:
> > On Thu, 18 Apr 2019, J. Bruce Fields wrote:
> > 
> > > On Thu, Apr 18, 2019 at 04:50:24PM -0400, Scott Mayhew wrote:
> > > > On Thu, 18 Apr 2019, J. Bruce Fields wrote:
> > > > 
> > > > > On Thu, Apr 18, 2019 at 09:24:00AM -0400, Scott Mayhew wrote:
> > > > > > While trying to track down some issues involving large
> > > > > > numbers of
> > > > > > delegations being recalled/revoked, I caught the server
> > > > > > setting
> > > > > > SEQ4_STATUS_CB_PATH_DOWN while the client was actively
> > > > > > responding to
> > > > > > CB_RECALLs.  It turns out that the client had already done a
> > > > > > TEST_STATEID and FREE_STATEID for a delegation being recalled
> > > > > > by the
> > > > > > time it received the CB_RECALL.
> > > > > 
> > > > > That's interesting, thanks!
> > > > > 
> > > > > This exception seems awfully narrow, though.
> > > > > 
> > > > > If we get back any NFS-level error at all, then I think the
> > > > > callback
> > > > > channel is working (am I wrong?)
> > > > 
> > > > Correct, if the client replies with either NFS4ERR_DELAY or
> > > > NFS4ERR_BAD_STATEID, the server will retry 1 time (see
> > > > dl_retries).
> > > > After that, we fall thru and nfsd4_cb_recall_done() returns -1
> > > > which
> > > > causes the SEQ4_STATUS_CB_PATH_DOWN flag to be set.
> > > > 
> > > > > and telling the client to set up a new
> > > > > one is probably not going to help.  The best we can do is
> > > > > probably just
> > > > > give up
> > > > 
> > > > That's what the patch is essentially doing.  Or are you saying
> > > > don't
> > > > even bother with the checks but still return 1 so we don't set
> > > > the
> > > > SEQ4_STATUS_CB_PATH_DOWN flag?
> > > 
> > > Right, I don't see any point returning -1 (which ends up setting
> > > CB_PATH_DOWN) in any case where we get an nfs-level error.  If the
> > > client got so far as returning an error, then the callback path is
> > > working.
> > > 
> > > I'm not sure exactly what errors *should* result in CB_PATH_DOWN,
> > > though.  ETIMEDOUT, ENOTCONN, EIO?
> > 
> > I'm not sure either.  Looking at
> > call_status/call_timeout/rpc_check_timeout, it looks to me like
> > ENOTCONN
> > will be translated to ETIMEDOUT because nfsd4_run_cb_work sets the 
> > RPC_TASK_SOFTCONN flag in the call to rpc_call_async.
> > 
> > It looks like call_status can return EHOSTDOWN, ENETDOWN,
> > EHOSTUNREACH,
> > ENETUNREACH, and EPERM... should those be handled as well?
> 
> Those errors should never be passed back to applications.

I'm confused.  If call_status passes any of those errors to rpc_exit,
then I'll see them in rpc_call_done/nfsd4_cb_done, won't I?

-Scott

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>