Re: [PATCH v5 6/7] nfsd: handle CB_SEQUENCE NFS4ERR_SEQ_MISORDERED error better

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2025-02-08 at 20:24 -0500, Tom Talpey wrote:
> On 2/8/2025 4:07 PM, Chuck Lever wrote:
> > On 2/8/25 3:45 PM, Jeff Layton wrote:
> > > On Sat, 2025-02-08 at 14:18 -0500, Tom Talpey wrote:
> > > > On 2/8/2025 11:08 AM, Jeff Layton wrote:
> > > > > On Sat, 2025-02-08 at 13:40 -0500, Tom Talpey wrote:
> > > > > > On 2/8/2025 10:02 AM, Jeff Layton wrote:
> > > > > > > On Sat, 2025-02-08 at 12:01 -0500, Chuck Lever wrote:
> > > > > > > > On 2/7/25 4:53 PM, Jeff Layton wrote:
> > > > > > > > > For NFS4ERR_SEQ_MISORDERED, do one attempt with a seqid of 1, and then
> > > > > > > > > fall back to treating it like a BADSLOT if that fails.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > > > > ---
> > > > > > > > >     fs/nfsd/nfs4callback.c | 16 ++++++++++------
> > > > > > > > >     1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > > > > > > > > index 10067a34db3afff8d4e4383854ab9abd9767c2d6..d6e3e8bb2efabadda9f922318880e12e1cb2c23f 100644
> > > > > > > > > --- a/fs/nfsd/nfs4callback.c
> > > > > > > > > +++ b/fs/nfsd/nfs4callback.c
> > > > > > > > > @@ -1393,6 +1393,16 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > > > > > > > >     			goto requeue;
> > > > > > > > >     		rpc_delay(task, 2 * HZ);
> > > > > > > > >     		return false;
> > > > > > > > > +	case -NFS4ERR_SEQ_MISORDERED:
> > > > > > > > > +		/*
> > > > > > > > > +		 * Reattempt once with seq_nr 1. If that fails, treat this
> > > > > > > > > +		 * like BADSLOT.
> > > > > > > > > +		 */
> > > > > > > > 
> > > > > > > > Nit: this comment says exactly what the code says. If it were me, I'd
> > > > > > > > remove it. Is there a "why" statement that could be made here? Like,
> > > > > > > > why retry with a seq_nr of 1 instead of just failing immediately?
> > > > > > > > 
> > > > > > > 
> > > > > > > There isn't one that I know of. It looks like Kinglong Mee added it in
> > > > > > > 7ba6cad6c88f, but there is no real mention of that in the changelog.
> > > > > > > 
> > > > > > > TBH, I'm not enamored with this remedy either. What if the seq_nr was 2
> > > > > > > when we got this error, and we then retry with a seq_nr of 1? Does the
> > > > > > > server then treat that as a retransmission?
> > > > > > 
> > > > > > So I assume you mean the requester sent seq_nr 1, saw a reply and sent a
> > > > > > subsequent seq_nr 2, to which it gets SEQ_MISORDERED.
> > > > > > 
> > > > > > If so, yes definitely backing up the seq_nr to 1 will result in the
> > > > > > peer considering it to be a retransmission, which will be bad.
> > > > > > 
> > > > > 
> > > > > Yes, that's what I meant.
> > > > > 
> > > > > > > We might be best off
> > > > > > > dropping this and just always treating it like BADSLOT.
> > > > > > 
> > > > > > But, why would this happen? Usually I'd think the peer sent seq_nr X
> > > > > > before it received a reply to seq_nr X-1, which would be a peer bug.
> > > > > > 
> > > > > > OTOH, SEQ_MISORDERED is a valid response to an in-progress retry. So,
> > > > > > how does the requester know the difference?
> > > > > > 
> > > > > > If treating it as BADSLOT completely resets the sequence, then sure,
> > > > > > but either a) the request is still in-progress, or b) if a bug is
> > > > > > causing the situation, well it's not going to converge on a functional
> > > > > > session.
> > > > > > 
> > > > > 
> > > > > With this patchset, on BADSLOT, we'll set SEQ4_STATUS_BACKCHANNEL_FAULT
> > > > > in the next forechannel SEQUENCE on the session. That should cause the
> > > > > client to (eventually) send a DESTROY_SESSION and create a new one.
> > > > > 
> > > > > Unfortunately, in the meantime, because of the way the callback channel
> > > > > update works, the server can end up trying to send the callback again
> > > > > on the same session (and maybe more than once). I'm not sure that
> > > > > that's a real problem per-se, but it's less than ideal.
> > > > > 
> > > > > > Not sure I have a solid suggestion right now. Whatever the fix, it
> > > > > > should capture any subtlety in a comment.
> > > > > > 
> > > > > 
> > > > > At this point, I'm leaning toward just treating it like BADSLOT.
> > > > > Basically, mark the backchannel faulty, and leak the slot so that
> > > > > nothing else uses it. That allows us to send backchannel requests on
> > > > > the other slots until the session gets recreated.
> > > > 
> > > > Hmm, leaking the slot is a workable approach, as long as it doesn't
> > > > cascade more than a time or two. Some sort of trigger should be armed
> > > > to prevent runaway retries.
> > > > 
> > > > It's maybe worth considering what state the peer might be in when this
> > > > happens. It too may effectively leak a slot, and if is retaining some
> > > > bogus state either as a result of or because of the previous exchange(s)
> > > > then this may lead to future hangs/failures. Not pretty, and maybe not
> > > > worth trying to guess.
> > > > 
> > > > Tom.
> > > > 
> > > 
> > > 
> > > The idea here is that eventually the client should figure out that
> > > something is wrong and reestablish the session. Currently we don't
> > > limit the number of retries on a callback.
> > > 
> > > Maybe they should time out after a while? If we've retried a callback
> > > for more than two lease periods, give up and log something?
> > > 
> > > Either way, I'd consider that to be follow-on work to this set.
> > 
> > As a general comment, I think making a heroic effort to recover in any
> > of these cases is probably not worth the additional complexity. Where it
> > is required or where we believe it is worth the trouble, that's where we
> > want a detailed comment.
> > 
> > What we want to do is ensure forward progress. I'm guessing that error
> > conditions are going to be rare, so leaking the slot until a certain
> > portion of them are gone, and then indicating a session fault to force
> > the client to start over from scratch, is probably the most
> > straightforward approach.
> > 
> > So, is there a good reason to retry? There doesn't appear to be any
> > reasoning mentioned in the commit log or in nearby comments.
> 
> Agreed on the general comment.
> 
> As for the "any reason to retry" - maybe. If it's a transient error we
> don't want to give up early. Unfortunately that appears to be an
> ambiguous situation, because SEQ_MISORDERED is allowed in place of
> ERR_DELAY. I don't have any great suggestion however.
> 

IMO, we should retry callbacks (basically) indefinitely, unless the
NFSv4 client is being torn down (i.e. lease expires or an unmount
happened, etc).

> Jeff, to your point that the "client should figure out something is
> wrong", I'm not sure how you think that will happen. If the server is
> making a delegation recall and the client receive code chooses to reject 
> it at the sequence check, how would that eventually cause the client to
> reestablish the session (on the forechannel)?
> 
> 

In the BADSLOT case, it calls nfsd4_mark_cb_fault(cb->cb_clp), which
sets a flag in the client that makes it set
SEQ4_STATUS_BACKCHANNEL_FAULT in the next SEQUENCE call.

The client should take that as an indication that there is a problem
and reestablish a new session (and maybe a new connection). Granted, it
might take up to the next lease renewal, but there's not much else we
can do if the client won't talk to us.

That's why I was suggesting that we might time out the backchannel
calls after two lease periods. OTOH, maybe it's sufficient to not queue
any callbacks for courtesy clients?

> > 
> > 
> > > > > > > Thoughts?
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +		if (session->se_cb_seq_nr[cb->cb_held_slot] != 1) {
> > > > > > > > > +			session->se_cb_seq_nr[cb->cb_held_slot] = 1;
> > > > > > > > > +			goto retry_nowait;
> > > > > > > > > +		}
> > > > > > > > > +		fallthrough;
> > > > > > > > >     	case -NFS4ERR_BADSLOT:
> > > > > > > > >     		/*
> > > > > > > > >     		 * BADSLOT means that the client and server are out of sync
> > > > > > > > > @@ -1403,12 +1413,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > > > > > > > >     		nfsd4_mark_cb_fault(cb->cb_clp);
> > > > > > > > >     		cb->cb_held_slot = -1;
> > > > > > > > >     		goto retry_nowait;
> > > > > > > > > -	case -NFS4ERR_SEQ_MISORDERED:
> > > > > > > > > -		if (session->se_cb_seq_nr[cb->cb_held_slot] != 1) {
> > > > > > > > > -			session->se_cb_seq_nr[cb->cb_held_slot] = 1;
> > > > > > > > > -			goto retry_nowait;
> > > > > > > > > -		}
> > > > > > > > > -		break;
> > > > > > > > >     	default:
> > > > > > > > >     		nfsd4_mark_cb_fault(cb->cb_clp);
> > > > > > > > >     	}
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>