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

[Trond Myklebust <trondmy@hammerspace.com>]

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.

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