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

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2019-05-02 at 07:35 -0400, Scott Mayhew wrote:
> 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?
> 

If you ever see them in rpc_call_done/nfsd4_cb_done, then it will be
due to an RPC bug which will need to be fixed.

call_status() should be handling those errors by retrying, and if it
runs out of retry attempts, then it should be setting either an EIO
error or an ETIMEDOUT error, depending on which rpc_task flags have
been specified.

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