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 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.


> > 
> > > > 
> > > > 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>