Re: [PATCH 1/8] nfsd: don't restart v4.1+ callback when RPC_SIGNALLED is set

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2025-01-26 at 11:41 -0500, Chuck Lever wrote:
> On 1/26/25 6:18 AM, Jeff Layton wrote:
> > On Sun, 2025-01-26 at 10:01 +1100, NeilBrown wrote:
> > > On Fri, 24 Jan 2025, Jeff Layton wrote:
> > > > This is problematic, since the RPC might have been entirely successful.
> > > > There is no point in restarting a v4.1+ callback just because
> > > > RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for
> > > > detecting when it should retransmit the RPC.
> > > 
> > > But why might RPC_SIGNALLED() ever be true?
> > > The flag RPC_TASK_SIGNALLED is only ever set by rpc_signal_task() which
> > > is only called from rpc_killall_tasks() and __rpc_execute() for
> > > non-async tasks which doesn't apply to nfsd callbacks as they are
> > > started with rpc_call_async().
> > > 
> > > rpc_killall_tasks() is called by fs/nfs/ which isn't relevant for us,
> > > and from rpc_shutdown_client().  In those cases we certainly don't want
> > > the request to be retried, though the nfsd4_process_cb_update() case is
> > > a little interesting as we do want it to be retried, but in a different
> > > client.
> > > 
> > > So the code you are removing is either dead code because something else
> > > will prevent the restart when a client is being shut down, or it is bad
> > > code because it would delay rpc_shutdown_client() while the request is
> > > retried.
> > > 
> > > I haven't dug the extra step to figure out which, but either way I think
> > > the code should go.
> > 
> > Thanks. That was my analysis too.
> 
> Agreed, this code is problematic, but it appears to me that some of
> these problems are not resolved by simply removing the signal check.
> 
> 
> > rpc_shutdown_client() is called when we tear down and rebuild the
> > rpc_client. nfsd does this in setup_callback_client(), which gets
> > called from nfsd4_process_cb_update() (basically when we detect that
> > the backchannel is having problems).
> > 
> > There are really only two states: We either got a reply from the server
> > before the client went down, or we didn't. In the case where we got a
> > reply, there is no need to retry anything. In the case where we didn't,
> > the tk_status will be '1', so there is no need to check RPC_SIGNALLED()
> > at all here.
> 
> Fwiw, the "cb_seq_status == 1" arm skips the signal check in the current
> code.
> 
> 
> > The existing code could lead to the call being retried when we had
> > already gotten a perfectly valid reply.
> 
> Here's a case-by-case audit:
> 
>   - NFS_OK: SEQUENCE was decoded and passed sanity checks. So this should
>     not ever requeue in here. It might be requeued during subsequent
>     processing.
> 
>   - ESERVERFAULT: SEQUENCE was decoded but failed sanity checking. The
>     reply should be dropped now, and the session marked FAULT. No requeue
>     is ever needed here.
> 
>     [ I question whether the sequence number should be bumped in this
>       case -- the client's callback server replied with the identity of
>       some other slot. And anyway, this session is about to become
>       toast. ]
> 

It didn't necessarily reply with the ID of a different slot. It's just
that the decoding failed in some way. It could have been any of the
cases in decode_cb_sequence4resok(). Maybe that function needs to
return more distinct error codes so we know what was mangled.

>   - 1: The timeout case. We want a fresh session here, so it falls
>     through to BADSESSION.
> 

Ok.

>   - NFS4ERR_BADSESSION: This needs a requeue so that the operation can
>     be retried with a fresh session. But it should always check if the
>     rpc_clnt is shutting down before doing so. This is a problem in the
>     current code.
> 

I'm not sure I understand the problem you see with that in the existing
code. There's a rather complicated dance in nfsd4_process_cb_update(),
but if the nfs4_client is shutting down, then clp->cl_cb_client will be
NULL after it, and the callback will end.

You said "rpc_clnt" though, so I'm not sure I understand the scenario
you mean.

>   - NFS4ERR_DELAY: Skips the signal check, but shouldn't. If the rpc_clnt
>     is shutting down, this RPC should not be requeued.
> 

Good point -- ot sure how we deal with that in a non-racy way. I'll
think about it.

>   - NFS4ERR_BAD_SLOT: Skips the signal check, but shouldn't. I need to
>     think more about BAD_SLOT recovery best practice.
> 

RPC_SIGNALLED() is irrelevant here. I think what we want to do is mark
the backchannel as faulty, _leak_ the slot and retry via the workqueue
(not just requeue the rpc_task). That should just cause the callback to
exit once it runs again.

We should also mark the backchannel as faulty, since the client and
server no longer agree on the size of the slot table.

>   - NFS4ERR_SEQ_MISORDERED: Does one retry with a seq_nr of 1. It
>     probably should terminate if that fails. IMO this should check for
>     rpc_clnt shutdown before requeuing the retry.
> 

Fair enough. There is a frustrating lack of guidance in the spec about
SEQ_MISORDERED. We should probably mark the BC as having a FAULT too if
the retry fails.

>   - default: I don't think this case should ever be requeued, but it
>     appears that it could be if the rpc_clnt is shutting down.
> 

Yeah. Might not hurt to throw a pr_warn() here too. I think we never
want to fall into this case.

In any case, my intention is to fix up the cb_session lifetime problem
first, and then we can rework the error handling from the callbacks on
top of that.
-- 
Jeff Layton <jlayton@kernel.org>