[PATCH 0/2] nfsd: fix up SEQUENCE replay

[PATCH 0/2] nfsd: fix up SEQUENCE replay

[NeilBrown]

I think the sequence replay code is confused and wrong.
These untested patches are a suggestion of how to fix it.

I and do some testing later it people think it makes sense.

NeilBrown

[PATCH 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_enc_sequence_replay() uses nfsd4_encode_operation() to encode a
new SEQUENCE reply when replaying a request from the slot cache - only
ops after the SEQUENCE are replay from the cache in ->sl_data.

However it does this in nfsd4_replay_cache_entry() which is called
*before* nfsd4_sequence() has filled in reply fields.

This means that in the replayed SEQUENCE reply:
 maxslots will be whatever the client sent
 target_maxslots will be -1 (assuming init to zero, and
      nfsd4_encode_sequence() subtracts 1)
 status_flags will be zero

which might mislead the client.

This patch moves the setup of the reply to *before*
nfsd4_replay_cache_entry() is called.  Only one of the updated fields is
used after this point - maxslots.  So that field is copied to
client_maxslots so that can be used as needed.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c9053ef4d79f..1c01836e8507 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4360,6 +4360,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfs4_client *clp;
 	struct nfsd4_slot *slot;
 	struct nfsd4_conn *conn;
+	u32 client_maxslots;
 	__be32 status;
 	int buflen;
 	struct net *net = SVC_NET(rqstp);
@@ -4398,6 +4399,27 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	dprintk("%s: slotid %d\n", __func__, seq->slotid);
 
 	trace_nfsd_slot_seqid_sequence(clp, seq, slot);
+
+	/* prepare reply so that it is ready for nfsd4_replay_cache_entry() */
+	client_maxslots = seq->maxslots;
+	seq->maxslots = max(session->se_target_maxslots, client_maxslots);
+	seq->target_maxslots = session->se_target_maxslots;
+
+	switch (clp->cl_cb_state) {
+	case NFSD4_CB_DOWN:
+		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
+		break;
+	case NFSD4_CB_FAULT:
+		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
+		break;
+	default:
+		seq->status_flags = 0;
+	}
+	if (!list_empty(&clp->cl_revoked))
+		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
+	if (atomic_read(&clp->cl_admin_revoked))
+		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
+
 	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
 	if (status == nfserr_replay_cache) {
 		status = nfserr_seq_misordered;
@@ -4425,7 +4447,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
 	    slot->sl_generation == session->se_slot_gen &&
-	    seq->maxslots <= session->se_target_maxslots)
+	    client_maxslots <= session->se_target_maxslots)
 		/* Client acknowledged our reduce maxreqs */
 		free_session_slots(session, session->se_target_maxslots);
 
@@ -4495,23 +4517,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	}
 
 out:
-	seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
-	seq->target_maxslots = session->se_target_maxslots;
-
-	switch (clp->cl_cb_state) {
-	case NFSD4_CB_DOWN:
-		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
-		break;
-	case NFSD4_CB_FAULT:
-		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
-		break;
-	default:
-		seq->status_flags = 0;
-	}
-	if (!list_empty(&clp->cl_revoked))
-		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
-	if (atomic_read(&clp->cl_admin_revoked))
-		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
 	trace_nfsd_seq4_status(rqstp, seq);
 out_no_session:
 	if (conn)
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 2/2] nfsd: stop pretending that we cache the SEQUENCE reply.

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd does not cache the reply to a SEQUENCE.  When replayed, the
SEQUENCE op itself is replay.

So removing the code and comments which seem to imply that we do, and
detect the case of a SOLO sequence to avoid looking a the cache.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c |  7 ++-----
 fs/nfsd/xdr4.h      | 20 ++++++--------------
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1c01836e8507..b51a37ad70aa 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3530,11 +3530,8 @@ nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,
 		return op->status;
 	if (args->opcnt == 1) {
 		/*
-		 * The original operation wasn't a solo sequence--we
-		 * always cache those--so this retry must not match the
-		 * original:
+		 * We will simply replay the SEQUENCE.
 		 */
-		op->status = nfserr_seq_false_retry;
 	} else {
 		op = &args->ops[resp->opcnt++];
 		op->status = nfserr_retry_uncached_rep;
@@ -3559,7 +3556,7 @@ nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
 	dprintk("--> %s slot %p\n", __func__, slot);
 
 	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
-	if (status)
+	if (status || !slot->sl_datalen)
 		return status;
 
 	p = xdr_reserve_space(xdr, slot->sl_datalen);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ee0570cbdd9e..390bfb0ba13f 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -923,25 +923,17 @@ struct nfsd4_compoundres {
 	struct nfsd4_compound_state	cstate;
 };
 
-static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
-{
-	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
-	return resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
-}
-
 /*
  * The session reply cache only needs to cache replies that the client
- * actually asked us to.  But it's almost free for us to cache compounds
- * consisting of only a SEQUENCE op, so we may as well cache those too.
- * Also, the protocol doesn't give us a convenient response in the case
- * of a replay of a solo SEQUENCE op that wasn't cached
- * (RETRY_UNCACHED_REP can only be returned in the second op of a
- * compound).
+ * actually asked us to.
+ * This doesn't apply to SEQUENCE.  We must always record in the slot
+ * that we have received a SEQUENCE, and must always respond
+ * successfully to a replayed successful SEQUENCE - that is handled
+ * separately from the sl_data cache.
  */
 static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
 {
-	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
-		|| nfsd4_is_solo_sequence(resp);
+	return resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS;
 }
 
 static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.

[Jeff Layton]

On Tue, 2025-10-14 at 11:04 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_enc_sequence_replay() uses nfsd4_encode_operation() to encode a
> new SEQUENCE reply when replaying a request from the slot cache - only
> ops after the SEQUENCE are replay from the cache in ->sl_data.
> 
> However it does this in nfsd4_replay_cache_entry() which is called
> *before* nfsd4_sequence() has filled in reply fields.
> 
> This means that in the replayed SEQUENCE reply:
>  maxslots will be whatever the client sent
>  target_maxslots will be -1 (assuming init to zero, and
>       nfsd4_encode_sequence() subtracts 1)
>  status_flags will be zero
> 
> which might mislead the client.
> 
> This patch moves the setup of the reply to *before*
> nfsd4_replay_cache_entry() is called.  Only one of the updated fields is
> used after this point - maxslots.  So that field is copied to
> client_maxslots so that can be used as needed.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..1c01836e8507 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4360,6 +4360,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_client *clp;
>  	struct nfsd4_slot *slot;
>  	struct nfsd4_conn *conn;
> +	u32 client_maxslots;
>  	__be32 status;
>  	int buflen;
>  	struct net *net = SVC_NET(rqstp);
> @@ -4398,6 +4399,27 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
>  
>  	trace_nfsd_slot_seqid_sequence(clp, seq, slot);
> +
> +	/* prepare reply so that it is ready for nfsd4_replay_cache_entry() */
> +	client_maxslots = seq->maxslots;
> +	seq->maxslots = max(session->se_target_maxslots, client_maxslots);
> +	seq->target_maxslots = session->se_target_maxslots;
> +
> +	switch (clp->cl_cb_state) {
> +	case NFSD4_CB_DOWN:
> +		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> +		break;
> +	case NFSD4_CB_FAULT:
> +		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> +		break;
> +	default:
> +		seq->status_flags = 0;
> +	}
> +	if (!list_empty(&clp->cl_revoked))
> +		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> +	if (atomic_read(&clp->cl_admin_revoked))
> +		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
> +
>  	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
> @@ -4425,7 +4447,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
>  	    slot->sl_generation == session->se_slot_gen &&
> -	    seq->maxslots <= session->se_target_maxslots)
> +	    client_maxslots <= session->se_target_maxslots)
>  		/* Client acknowledged our reduce maxreqs */
>  		free_session_slots(session, session->se_target_maxslots);
>  
> @@ -4495,23 +4517,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  
>  out:
> -	seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
> -	seq->target_maxslots = session->se_target_maxslots;
> -
> -	switch (clp->cl_cb_state) {
> -	case NFSD4_CB_DOWN:
> -		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> -		break;
> -	case NFSD4_CB_FAULT:
> -		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> -		break;
> -	default:
> -		seq->status_flags = 0;
> -	}
> -	if (!list_empty(&clp->cl_revoked))
> -		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> -	if (atomic_read(&clp->cl_admin_revoked))
> -		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
>  	trace_nfsd_seq4_status(rqstp, seq);
>  out_no_session:
>  	if (conn)

This does seem like the right thing to do.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] nfsd: stop pretending that we cache the SEQUENCE reply.

[Jeff Layton]

On Tue, 2025-10-14 at 11:04 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd does not cache the reply to a SEQUENCE.  When replayed, the
> SEQUENCE op itself is replay.
> 
> So removing the code and comments which seem to imply that we do, and
> detect the case of a SOLO sequence to avoid looking a the cache.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c |  7 ++-----
>  fs/nfsd/xdr4.h      | 20 ++++++--------------
>  2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1c01836e8507..b51a37ad70aa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3530,11 +3530,8 @@ nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,
>  		return op->status;
>  	if (args->opcnt == 1) {
>  		/*
> -		 * The original operation wasn't a solo sequence--we
> -		 * always cache those--so this retry must not match the
> -		 * original:
> +		 * We will simply replay the SEQUENCE.
>  		 */
> -		op->status = nfserr_seq_false_retry;
>  	} else {
>  		op = &args->ops[resp->opcnt++];
>  		op->status = nfserr_retry_uncached_rep;
> @@ -3559,7 +3556,7 @@ nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
>  	dprintk("--> %s slot %p\n", __func__, slot);
>  
>  	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
> -	if (status)
> +	if (status || !slot->sl_datalen)
>  		return status;
>  
>  	p = xdr_reserve_space(xdr, slot->sl_datalen);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ee0570cbdd9e..390bfb0ba13f 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -923,25 +923,17 @@ struct nfsd4_compoundres {
>  	struct nfsd4_compound_state	cstate;
>  };
>  
> -static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
> -{
> -	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
> -	return resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
> -}
> -
>  /*
>   * The session reply cache only needs to cache replies that the client
> - * actually asked us to.  But it's almost free for us to cache compounds
> - * consisting of only a SEQUENCE op, so we may as well cache those too.
> - * Also, the protocol doesn't give us a convenient response in the case
> - * of a replay of a solo SEQUENCE op that wasn't cached
> - * (RETRY_UNCACHED_REP can only be returned in the second op of a
> - * compound).
> + * actually asked us to.
> + * This doesn't apply to SEQUENCE.  We must always record in the slot
> + * that we have received a SEQUENCE, and must always respond
> + * successfully to a replayed successful SEQUENCE - that is handled
> + * separately from the sl_data cache.
>   */
>  static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
>  {
> -	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
> -		|| nfsd4_is_solo_sequence(resp);
> +	return resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS;
>  }
>  
>  static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)

Again, this makes more sense to me.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.

[Chuck Lever]

On 10/13/25 8:04 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_enc_sequence_replay() uses nfsd4_encode_operation() to encode a
> new SEQUENCE reply when replaying a request from the slot cache - only
> ops after the SEQUENCE are replay from the cache in ->sl_data.


s/replay /replayed /

You might consider moving reply construction to a helper, where it
can be more extensively documented without adding more clutter to
nfsd4_sequence().


> However it does this in nfsd4_replay_cache_entry() which is called
> *before* nfsd4_sequence() has filled in reply fields.
> 
> This means that in the replayed SEQUENCE reply:
>  maxslots will be whatever the client sent
>  target_maxslots will be -1 (assuming init to zero, and
>       nfsd4_encode_sequence() subtracts 1)
>  status_flags will be zero
> 
> which might mislead the client.
> 
> This patch moves the setup of the reply to *before*
> nfsd4_replay_cache_entry() is called.  Only one of the updated fields is
> used after this point - maxslots.  So that field is copied to
> client_maxslots so that can be used as needed.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..1c01836e8507 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4360,6 +4360,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_client *clp;
>  	struct nfsd4_slot *slot;
>  	struct nfsd4_conn *conn;
> +	u32 client_maxslots;
>  	__be32 status;
>  	int buflen;
>  	struct net *net = SVC_NET(rqstp);
> @@ -4398,6 +4399,27 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
>  
>  	trace_nfsd_slot_seqid_sequence(clp, seq, slot);
> +
> +	/* prepare reply so that it is ready for nfsd4_replay_cache_entry() */
> +	client_maxslots = seq->maxslots;
> +	seq->maxslots = max(session->se_target_maxslots, client_maxslots);
> +	seq->target_maxslots = session->se_target_maxslots;
> +
> +	switch (clp->cl_cb_state) {
> +	case NFSD4_CB_DOWN:
> +		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> +		break;
> +	case NFSD4_CB_FAULT:
> +		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> +		break;
> +	default:
> +		seq->status_flags = 0;
> +	}
> +	if (!list_empty(&clp->cl_revoked))
> +		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> +	if (atomic_read(&clp->cl_admin_revoked))
> +		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
> +
>  	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
> @@ -4425,7 +4447,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
>  	    slot->sl_generation == session->se_slot_gen &&
> -	    seq->maxslots <= session->se_target_maxslots)
> +	    client_maxslots <= session->se_target_maxslots)
>  		/* Client acknowledged our reduce maxreqs */
>  		free_session_slots(session, session->se_target_maxslots);
>  
> @@ -4495,23 +4517,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  
>  out:
> -	seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
> -	seq->target_maxslots = session->se_target_maxslots;
> -
> -	switch (clp->cl_cb_state) {
> -	case NFSD4_CB_DOWN:
> -		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> -		break;
> -	case NFSD4_CB_FAULT:
> -		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> -		break;
> -	default:
> -		seq->status_flags = 0;
> -	}
> -	if (!list_empty(&clp->cl_revoked))
> -		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> -	if (atomic_read(&clp->cl_admin_revoked))
> -		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
>  	trace_nfsd_seq4_status(rqstp, seq);
>  out_no_session:
>  	if (conn)


-- 
Chuck Lever

Re: [PATCH 2/2] nfsd: stop pretending that we cache the SEQUENCE reply.

[Chuck Lever]

On 10/13/25 8:04 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd does not cache the reply to a SEQUENCE.  When replayed, the
> SEQUENCE op itself is replay.


s/replay./replayed./


> So removing the code and comments which seem to imply that we do, and


s/removing/remove/


> detect the case of a SOLO sequence to avoid looking a the cache.


s/looking a the cache./looking at the slot replay cache./


> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c |  7 ++-----
>  fs/nfsd/xdr4.h      | 20 ++++++--------------
>  2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1c01836e8507..b51a37ad70aa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3530,11 +3530,8 @@ nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,


The function's comments (including its documenting comment) seem
repetitive. Let's remove the function's documenting comment, and
then disperse those remarks inside the function like so:

-       /* Encode the replayed sequence operation */
+	/* Encode the replayed SEQUENCE response from current slot values. */


>  		return op->status;
>  	if (args->opcnt == 1) {
>  		/*
> -		 * The original operation wasn't a solo sequence--we
> -		 * always cache those--so this retry must not match the
> -		 * original:
> +		 * We will simply replay the SEQUENCE.
>  		 */


The now empty set of braces here with just a comment is perhaps
confusing.


> -		op->status = nfserr_seq_false_retry;
>  	} else {


From above, add:

+		/*
+		 * Encode the uncached_rep error on the next
+		 * operation, and set resp->p and increment
+		 * resp->opcnt for nfs4svc_encode_compoundres.
+		 */


>  		op = &args->ops[resp->opcnt++];
>  		op->status = nfserr_retry_uncached_rep;
> @@ -3559,7 +3556,7 @@ nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
>  	dprintk("--> %s slot %p\n", __func__, slot);
>  
>  	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
> -	if (status)
> +	if (status || !slot->sl_datalen)


This seems subtle. Maybe a comment could explain what this check
is trying to detect. I assume it is either the solo sequence case,
or the case where there is no replay cache?


>  		return status;
>  
>  	p = xdr_reserve_space(xdr, slot->sl_datalen);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ee0570cbdd9e..390bfb0ba13f 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -923,25 +923,17 @@ struct nfsd4_compoundres {
>  	struct nfsd4_compound_state	cstate;
>  };
>  
> -static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
> -{
> -	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
> -	return resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
> -}
> -
>  /*
>   * The session reply cache only needs to cache replies that the client
> - * actually asked us to.  But it's almost free for us to cache compounds
> - * consisting of only a SEQUENCE op, so we may as well cache those too.
> - * Also, the protocol doesn't give us a convenient response in the case
> - * of a replay of a solo SEQUENCE op that wasn't cached
> - * (RETRY_UNCACHED_REP can only be returned in the second op of a
> - * compound).
> + * actually asked us to.
> + * This doesn't apply to SEQUENCE.  We must always record in the slot
> + * that we have received a SEQUENCE, and must always respond
> + * successfully to a replayed successful SEQUENCE - that is handled
> + * separately from the sl_data cache.

Once is_solo_sequence is gone, I'm not convinced this is the right spot
for your new explanation. (ie, the comment seems sensible, but maybe it
belongs somewhere else).

The only thing this helper is doing is looking at how the client
set sa_cachethis in the original request, and that's already
obvious from the code -- no comment is needed. And maybe even this
helper could be removed, and the now smaller "if" statement can be
relocated to its only call site.


>   */
>  static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
>  {
> -	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
> -		|| nfsd4_is_solo_sequence(resp);
> +	return resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS;
>  }
>  
>  static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)


-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.

[Olga Kornievskaia]

On Mon, Oct 13, 2025 at 8:06 PM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> nfsd4_enc_sequence_replay() uses nfsd4_encode_operation() to encode a
> new SEQUENCE reply when replaying a request from the slot cache - only
> ops after the SEQUENCE are replay from the cache in ->sl_data.
>
> However it does this in nfsd4_replay_cache_entry() which is called
> *before* nfsd4_sequence() has filled in reply fields.
>
> This means that in the replayed SEQUENCE reply:
>  maxslots will be whatever the client sent
>  target_maxslots will be -1 (assuming init to zero, and
>       nfsd4_encode_sequence() subtracts 1)
>  status_flags will be zero
>
> which might mislead the client.

This also fixes the problem I described in my proposed patch. I would
have liked to see a bit more detail mentioning that it leads to the
client shrinking its slot table and then a hung client due to the
server's SEQ_MISORDERED error. I think having details in the commit
message might be useful for later connecting the symptoms to this
patch.

Tested-by: Olga Kornievskaia <okorniev@redhat.com>

> This patch moves the setup of the reply to *before*
> nfsd4_replay_cache_entry() is called.  Only one of the updated fields is
> used after this point - maxslots.  So that field is copied to
> client_maxslots so that can be used as needed.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..1c01836e8507 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4360,6 +4360,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         struct nfs4_client *clp;
>         struct nfsd4_slot *slot;
>         struct nfsd4_conn *conn;
> +       u32 client_maxslots;
>         __be32 status;
>         int buflen;
>         struct net *net = SVC_NET(rqstp);
> @@ -4398,6 +4399,27 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         dprintk("%s: slotid %d\n", __func__, seq->slotid);
>
>         trace_nfsd_slot_seqid_sequence(clp, seq, slot);
> +
> +       /* prepare reply so that it is ready for nfsd4_replay_cache_entry() */
> +       client_maxslots = seq->maxslots;
> +       seq->maxslots = max(session->se_target_maxslots, client_maxslots);
> +       seq->target_maxslots = session->se_target_maxslots;
> +
> +       switch (clp->cl_cb_state) {
> +       case NFSD4_CB_DOWN:
> +               seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> +               break;
> +       case NFSD4_CB_FAULT:
> +               seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> +               break;
> +       default:
> +               seq->status_flags = 0;
> +       }
> +       if (!list_empty(&clp->cl_revoked))
> +               seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> +       if (atomic_read(&clp->cl_admin_revoked))
> +               seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
> +
>         status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>         if (status == nfserr_replay_cache) {
>                 status = nfserr_seq_misordered;
> @@ -4425,7 +4447,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>
>         if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
>             slot->sl_generation == session->se_slot_gen &&
> -           seq->maxslots <= session->se_target_maxslots)
> +           client_maxslots <= session->se_target_maxslots)
>                 /* Client acknowledged our reduce maxreqs */
>                 free_session_slots(session, session->se_target_maxslots);
>
> @@ -4495,23 +4517,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         }
>
>  out:
> -       seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
> -       seq->target_maxslots = session->se_target_maxslots;
> -
> -       switch (clp->cl_cb_state) {
> -       case NFSD4_CB_DOWN:
> -               seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> -               break;
> -       case NFSD4_CB_FAULT:
> -               seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> -               break;
> -       default:
> -               seq->status_flags = 0;
> -       }
> -       if (!list_empty(&clp->cl_revoked))
> -               seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> -       if (atomic_read(&clp->cl_admin_revoked))
> -               seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
>         trace_nfsd_seq4_status(rqstp, seq);
>  out_no_session:
>         if (conn)
> --
> 2.50.0.107.gf914562f5916.dirty
>
>

Re: [PATCH 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.

[NeilBrown]

On Wed, 15 Oct 2025, Olga Kornievskaia wrote:
> On Mon, Oct 13, 2025 at 8:06 PM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > nfsd4_enc_sequence_replay() uses nfsd4_encode_operation() to encode a
> > new SEQUENCE reply when replaying a request from the slot cache - only
> > ops after the SEQUENCE are replay from the cache in ->sl_data.
> >
> > However it does this in nfsd4_replay_cache_entry() which is called
> > *before* nfsd4_sequence() has filled in reply fields.
> >
> > This means that in the replayed SEQUENCE reply:
> >  maxslots will be whatever the client sent
> >  target_maxslots will be -1 (assuming init to zero, and
> >       nfsd4_encode_sequence() subtracts 1)
> >  status_flags will be zero
> >
> > which might mislead the client.
> 
> This also fixes the problem I described in my proposed patch. I would
> have liked to see a bit more detail mentioning that it leads to the
> client shrinking its slot table and then a hung client due to the
> server's SEQ_MISORDERED error. I think having details in the commit
> message might be useful for later connecting the symptoms to this
> patch.

I had seen that patch of yours but hadn't looked at it properly yet.  I
does indeed address the same problem.  I'll add relevant parts of your
commit message to me next posting.

> 
> Tested-by: Olga Kornievskaia <okorniev@redhat.com>

Thanks!

NeilBrown

> 
> > This patch moves the setup of the reply to *before*
> > nfsd4_replay_cache_entry() is called.  Only one of the updated fields is
> > used after this point - maxslots.  So that field is copied to
> > client_maxslots so that can be used as needed.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
> >  1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c9053ef4d79f..1c01836e8507 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4360,6 +4360,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >         struct nfs4_client *clp;
> >         struct nfsd4_slot *slot;
> >         struct nfsd4_conn *conn;
> > +       u32 client_maxslots;
> >         __be32 status;
> >         int buflen;
> >         struct net *net = SVC_NET(rqstp);
> > @@ -4398,6 +4399,27 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >         dprintk("%s: slotid %d\n", __func__, seq->slotid);
> >
> >         trace_nfsd_slot_seqid_sequence(clp, seq, slot);
> > +
> > +       /* prepare reply so that it is ready for nfsd4_replay_cache_entry() */
> > +       client_maxslots = seq->maxslots;
> > +       seq->maxslots = max(session->se_target_maxslots, client_maxslots);
> > +       seq->target_maxslots = session->se_target_maxslots;
> > +
> > +       switch (clp->cl_cb_state) {
> > +       case NFSD4_CB_DOWN:
> > +               seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> > +               break;
> > +       case NFSD4_CB_FAULT:
> > +               seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> > +               break;
> > +       default:
> > +               seq->status_flags = 0;
> > +       }
> > +       if (!list_empty(&clp->cl_revoked))
> > +               seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> > +       if (atomic_read(&clp->cl_admin_revoked))
> > +               seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
> > +
> >         status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
> >         if (status == nfserr_replay_cache) {
> >                 status = nfserr_seq_misordered;
> > @@ -4425,7 +4447,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >
> >         if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
> >             slot->sl_generation == session->se_slot_gen &&
> > -           seq->maxslots <= session->se_target_maxslots)
> > +           client_maxslots <= session->se_target_maxslots)
> >                 /* Client acknowledged our reduce maxreqs */
> >                 free_session_slots(session, session->se_target_maxslots);
> >
> > @@ -4495,23 +4517,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >         }
> >
> >  out:
> > -       seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
> > -       seq->target_maxslots = session->se_target_maxslots;
> > -
> > -       switch (clp->cl_cb_state) {
> > -       case NFSD4_CB_DOWN:
> > -               seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> > -               break;
> > -       case NFSD4_CB_FAULT:
> > -               seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> > -               break;
> > -       default:
> > -               seq->status_flags = 0;
> > -       }
> > -       if (!list_empty(&clp->cl_revoked))
> > -               seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> > -       if (atomic_read(&clp->cl_admin_revoked))
> > -               seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
> >         trace_nfsd_seq4_status(rqstp, seq);
> >  out_no_session:
> >         if (conn)
> > --
> > 2.50.0.107.gf914562f5916.dirty
> >
> >
>