[PATCH v3 0/2] nfds: fix up v4.1 slot-based replay handling

[PATCH v3 0/2] nfds: fix up v4.1 slot-based replay handling

[NeilBrown]

v3 - with the fix to nfsd4_encode_sequence() in the first patch
plus some minor cosmetic changes as requested.

NeilBrown

 [PATCH v3 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.
 [PATCH v3 2/2] nfsd: stop pretending that we cache the SEQUENCE

[PATCH v3 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 replayed 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

The incorrect maxslots value, in particular, can cause the client to
think the slot table has been reduced in size so it can discard its
knowledge of current sequence number of the later slots, though the
server has not discarded those slots.  When the client later wants to
use a later slot, it can get NFS4ERR_SEQ_MISORDERED from the server.

This patch moves the setup of the reply into a new helper function and
call it *before* nfsd4_replay_cache_entry() is called.  Only one of the
updated fields was used after this point - maxslots.  So the
nfsd4_sequence struct has been extended to have separate maxslots for
the request and the response.

Reported-by: Olga Kornievskaia <okorniev@redhat.com>
Closes: https://lore.kernel.org/linux-nfs/20251010194449.10281-1-okorniev@redhat.com/
Tested-by: Olga Kornievskaia <okorniev@redhat.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++---------------
 fs/nfsd/nfs4xdr.c   |  2 +-
 fs/nfsd/xdr4.h      |  3 ++-
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c9053ef4d79f..60451fd98bdf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4349,6 +4349,36 @@ static bool replay_matches_cache(struct svc_rqst *rqstp,
 	return true;
 }
 
+/*
+ * Note that the response is constructed here both for the case
+ * of a new SEQUENCE request and for a replayed SEQUENCE request.
+ * We do not cache SEQUENCE responses as SEQUENCE is idempotent.
+ */
+static void nfsd4_construct_sequence_response(struct nfsd4_session *session,
+					      struct nfsd4_sequence *seq)
+{
+	struct nfs4_client *clp = session->se_client;
+
+	seq->maxslots_response = 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;
+}
+
 __be32
 nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		union nfsd4_op_u *u)
@@ -4398,6 +4428,9 @@ 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);
+
+	nfsd4_construct_sequence_response(session, seq);
+
 	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
 	if (status == nfserr_replay_cache) {
 		status = nfserr_seq_misordered;
@@ -4495,23 +4528,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)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 230bf53e39f7..6135b896b3fe 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5085,7 +5085,7 @@ nfsd4_encode_sequence(struct nfsd4_compoundres *resp, __be32 nfserr,
 		return nfserr;
 	/* Note slotid's are numbered from zero: */
 	/* sr_highest_slotid */
-	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots - 1);
+	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots_response - 1);
 	if (nfserr != nfs_ok)
 		return nfserr;
 	/* sr_target_highest_slotid */
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ee0570cbdd9e..1ce8e12ae335 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -574,8 +574,9 @@ struct nfsd4_sequence {
 	struct nfs4_sessionid	sessionid;		/* request/response */
 	u32			seqid;			/* request/response */
 	u32			slotid;			/* request/response */
-	u32			maxslots;		/* request/response */
+	u32			maxslots;		/* request */
 	u32			cachethis;		/* request */
+	u32			maxslots_response;	/* response */
 	u32			target_maxslots;	/* response */
 	u32			status_flags;		/* response */
 };
-- 
2.50.0.107.gf914562f5916.dirty

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

[Jeff Layton]

On Thu, 2025-10-16 at 12:31 +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 replayed 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
> 
> The incorrect maxslots value, in particular, can cause the client to
> think the slot table has been reduced in size so it can discard its
> knowledge of current sequence number of the later slots, though the
> server has not discarded those slots.  When the client later wants to
> use a later slot, it can get NFS4ERR_SEQ_MISORDERED from the server.
> 
> This patch moves the setup of the reply into a new helper function and
> call it *before* nfsd4_replay_cache_entry() is called.  Only one of the
> updated fields was used after this point - maxslots.  So the
> nfsd4_sequence struct has been extended to have separate maxslots for
> the request and the response.
> 
> Reported-by: Olga Kornievskaia <okorniev@redhat.com>
> Closes: https://lore.kernel.org/linux-nfs/20251010194449.10281-1-okorniev@redhat.com/
> Tested-by: Olga Kornievskaia <okorniev@redhat.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++---------------
>  fs/nfsd/nfs4xdr.c   |  2 +-
>  fs/nfsd/xdr4.h      |  3 ++-
>  3 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..60451fd98bdf 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4349,6 +4349,36 @@ static bool replay_matches_cache(struct svc_rqst *rqstp,
>  	return true;
>  }
>  
> +/*
> + * Note that the response is constructed here both for the case
> + * of a new SEQUENCE request and for a replayed SEQUENCE request.
> + * We do not cache SEQUENCE responses as SEQUENCE is idempotent.
> + */
> +static void nfsd4_construct_sequence_response(struct nfsd4_session *session,
> +					      struct nfsd4_sequence *seq)
> +{
> +	struct nfs4_client *clp = session->se_client;
> +
> +	seq->maxslots_response = 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;
> +}
> +
>  __be32
>  nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		union nfsd4_op_u *u)
> @@ -4398,6 +4428,9 @@ 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);
> +
> +	nfsd4_construct_sequence_response(session, seq);
> +
>  	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
> @@ -4495,23 +4528,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)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 230bf53e39f7..6135b896b3fe 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5085,7 +5085,7 @@ nfsd4_encode_sequence(struct nfsd4_compoundres *resp, __be32 nfserr,
>  		return nfserr;
>  	/* Note slotid's are numbered from zero: */
>  	/* sr_highest_slotid */
> -	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots - 1);
> +	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots_response - 1);
>  	if (nfserr != nfs_ok)
>  		return nfserr;
>  	/* sr_target_highest_slotid */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ee0570cbdd9e..1ce8e12ae335 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -574,8 +574,9 @@ struct nfsd4_sequence {
>  	struct nfs4_sessionid	sessionid;		/* request/response */
>  	u32			seqid;			/* request/response */
>  	u32			slotid;			/* request/response */
> -	u32			maxslots;		/* request/response */
> +	u32			maxslots;		/* request */
>  	u32			cachethis;		/* request */
> +	u32			maxslots_response;	/* response */
>  	u32			target_maxslots;	/* response */
>  	u32			status_flags;		/* response */
>  };

I do like this version with the helper function better.

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

[PATCH v3 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.  As the comment above
nfsd4_replay_cache_entry() says:

 * The sequence operation is not cached because we can use the slot and
 * session values.

The comment above nfsd4_cache_this() suggests otherwise.

 * 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

The code in nfsd4_store_cache_entry() makes it clear that only responses
beyond 'cstate.data_offset' are actually cached, and data_offset is set
at the end of nfsd4_encode_sequence() *after* the sequence response has
been encoded.

This patch simplifies code and removes the confusing comments.

- nfsd4_is_solo_sequence() is discarded as not-useful.
- nfsd4_cache_this() is now trivial so it too is discarded with the
  code placed in-line at the one call-site in nfsd4_store_cache_entry().
- nfsd4_enc_sequence_replay() is open-coded in to
  nfsd4_replay_cache_entry(), and then simplified to (hopefully) make
  the process of replaying a reply clearer.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 57 ++++++++++++++-------------------------------
 fs/nfsd/xdr4.h      | 21 -----------------
 2 files changed, 17 insertions(+), 61 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 60451fd98bdf..7bd20c9efd29 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3494,7 +3494,7 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
 	free_svc_cred(&slot->sl_cred);
 	copy_cred(&slot->sl_cred, &resp->rqstp->rq_cred);
 
-	if (!nfsd4_cache_this(resp)) {
+	if (!(resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)) {
 		slot->sl_flags &= ~NFSD4_SLOT_CACHED;
 		return;
 	}
@@ -3508,41 +3508,6 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
 	return;
 }
 
-/*
- * Encode the replay sequence operation from the slot values.
- * If cachethis is FALSE encode the uncached rep error on the next
- * operation which sets resp->p and increments resp->opcnt for
- * nfs4svc_encode_compoundres.
- *
- */
-static __be32
-nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,
-			  struct nfsd4_compoundres *resp)
-{
-	struct nfsd4_op *op;
-	struct nfsd4_slot *slot = resp->cstate.slot;
-
-	/* Encode the replayed sequence operation */
-	op = &args->ops[resp->opcnt - 1];
-	nfsd4_encode_operation(resp, op);
-
-	if (slot->sl_flags & NFSD4_SLOT_CACHED)
-		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:
-		 */
-		op->status = nfserr_seq_false_retry;
-	} else {
-		op = &args->ops[resp->opcnt++];
-		op->status = nfserr_retry_uncached_rep;
-		nfsd4_encode_operation(resp, op);
-	}
-	return op->status;
-}
-
 /*
  * The sequence operation is not cached because we can use the slot and
  * session values.
@@ -3551,17 +3516,29 @@ static __be32
 nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
 			 struct nfsd4_sequence *seq)
 {
+	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
 	struct nfsd4_slot *slot = resp->cstate.slot;
 	struct xdr_stream *xdr = resp->xdr;
 	__be32 *p;
-	__be32 status;
 
 	dprintk("--> %s slot %p\n", __func__, slot);
 
-	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
-	if (status)
-		return status;
+	/* Always encode the SEQUENCE response. */
+	nfsd4_encode_operation(resp, &args->ops[0]);
+	if (args->opcnt == 1)
+		/* A solo SEQUENCE - nothing was cached */
+		return args->ops[0].status;
+
+	if (!(slot->sl_flags & NFSD4_SLOT_CACHED)) {
+		/* We weren't asked to cache this. */
+		struct nfsd4_op *op;
+		op = &args->ops[resp->opcnt++];
+		op->status = nfserr_retry_uncached_rep;
+		nfsd4_encode_operation(resp, op);
+		return op->status;
+	}
 
+	/* return reply from cache */
 	p = xdr_reserve_space(xdr, slot->sl_datalen);
 	if (!p) {
 		WARN_ON_ONCE(1);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 1ce8e12ae335..ae75846b3cd7 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -924,27 +924,6 @@ 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).
- */
-static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
-{
-	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
-		|| nfsd4_is_solo_sequence(resp);
-}
-
 static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
 {
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
-- 
2.50.0.107.gf914562f5916.dirty

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

[Jeff Layton]

On Thu, 2025-10-16 at 12:31 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd does not cache the reply to a SEQUENCE.  As the comment above
> nfsd4_replay_cache_entry() says:
> 
>  * The sequence operation is not cached because we can use the slot and
>  * session values.
> 
> The comment above nfsd4_cache_this() suggests otherwise.
> 
>  * 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
> 
> The code in nfsd4_store_cache_entry() makes it clear that only responses
> beyond 'cstate.data_offset' are actually cached, and data_offset is set
> at the end of nfsd4_encode_sequence() *after* the sequence response has
> been encoded.
> 
> This patch simplifies code and removes the confusing comments.
> 
> - nfsd4_is_solo_sequence() is discarded as not-useful.
> - nfsd4_cache_this() is now trivial so it too is discarded with the
>   code placed in-line at the one call-site in nfsd4_store_cache_entry().
> - nfsd4_enc_sequence_replay() is open-coded in to
>   nfsd4_replay_cache_entry(), and then simplified to (hopefully) make
>   the process of replaying a reply clearer.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 57 ++++++++++++++-------------------------------
>  fs/nfsd/xdr4.h      | 21 -----------------
>  2 files changed, 17 insertions(+), 61 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 60451fd98bdf..7bd20c9efd29 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3494,7 +3494,7 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  	free_svc_cred(&slot->sl_cred);
>  	copy_cred(&slot->sl_cred, &resp->rqstp->rq_cred);
>  
> -	if (!nfsd4_cache_this(resp)) {
> +	if (!(resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)) {
>  		slot->sl_flags &= ~NFSD4_SLOT_CACHED;
>  		return;
>  	}
> @@ -3508,41 +3508,6 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  	return;
>  }
>  
> -/*
> - * Encode the replay sequence operation from the slot values.
> - * If cachethis is FALSE encode the uncached rep error on the next
> - * operation which sets resp->p and increments resp->opcnt for
> - * nfs4svc_encode_compoundres.
> - *
> - */
> -static __be32
> -nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,
> -			  struct nfsd4_compoundres *resp)
> -{
> -	struct nfsd4_op *op;
> -	struct nfsd4_slot *slot = resp->cstate.slot;
> -
> -	/* Encode the replayed sequence operation */
> -	op = &args->ops[resp->opcnt - 1];
> -	nfsd4_encode_operation(resp, op);
> -
> -	if (slot->sl_flags & NFSD4_SLOT_CACHED)
> -		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:
> -		 */
> -		op->status = nfserr_seq_false_retry;
> -	} else {
> -		op = &args->ops[resp->opcnt++];
> -		op->status = nfserr_retry_uncached_rep;
> -		nfsd4_encode_operation(resp, op);
> -	}
> -	return op->status;
> -}
> -
>  /*
>   * The sequence operation is not cached because we can use the slot and
>   * session values.
> @@ -3551,17 +3516,29 @@ static __be32
>  nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
>  			 struct nfsd4_sequence *seq)
>  {
> +	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
>  	struct nfsd4_slot *slot = resp->cstate.slot;
>  	struct xdr_stream *xdr = resp->xdr;
>  	__be32 *p;
> -	__be32 status;
>  
>  	dprintk("--> %s slot %p\n", __func__, slot);
>  
> -	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
> -	if (status)
> -		return status;
> +	/* Always encode the SEQUENCE response. */
> +	nfsd4_encode_operation(resp, &args->ops[0]);
> +	if (args->opcnt == 1)
> +		/* A solo SEQUENCE - nothing was cached */
> +		return args->ops[0].status;
> +
> +	if (!(slot->sl_flags & NFSD4_SLOT_CACHED)) {
> +		/* We weren't asked to cache this. */
> +		struct nfsd4_op *op;
> +		op = &args->ops[resp->opcnt++];
> +		op->status = nfserr_retry_uncached_rep;
> +		nfsd4_encode_operation(resp, op);
> +		return op->status;
> +	}
>  
> +	/* return reply from cache */
>  	p = xdr_reserve_space(xdr, slot->sl_datalen);
>  	if (!p) {
>  		WARN_ON_ONCE(1);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 1ce8e12ae335..ae75846b3cd7 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -924,27 +924,6 @@ 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).
> - */
> -static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
> -{
> -	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
> -		|| nfsd4_is_solo_sequence(resp);
> -}
> -
>  static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
>  {
>  	struct nfsd4_compoundres *resp = rqstp->rq_resp;

Seems sane, and I like the simplification.

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

Re: [PATCH v3 0/2] nfds: fix up v4.1 slot-based replay handling

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Thu, 16 Oct 2025 12:31:12 +1100, NeilBrown wrote:
> v3 - with the fix to nfsd4_encode_sequence() in the first patch
> plus some minor cosmetic changes as requested.
> 
> NeilBrown
> 
> [PATCH v3 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.
>  [PATCH v3 2/2] nfsd: stop pretending that we cache the SEQUENCE
> 
> [...]

Applied to nfsd-testing, thanks!

[1/2] nfsd: ensure SEQUENCE replay sends a valid reply.
      commit: 337a6c1fcbfb6612067fc75e2668739e3ed7115a
[2/2] nfsd: stop pretending that we cache the SEQUENCE reply.
      commit: e51cf313aae1b04edb5953751028bda51c5d5952

--
Chuck Lever