Re: [PATCH v5 07/11] nfsd: simplify clearing of current-state-id

[Chuck Lever <chuck.lever@oracle.com>]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> In NFSv4.1 the current and saved filehandle each have a corresponding
> stateid.  RFC 8881 requires that in certain circumstances - particularly
> when the current filehandle is changed - the current stateid should be
> set to the anonymous stateid (all zeros).  It also requires that when a
> request tries to use the current stateid, if that stateid is "special"
> (which includes the anonymous stateid), then a BAD_STATEID error must
> be produced.
> 
> Linux nfsd current implements these requirements using a flag to record
> if the stateid (current or saved) is valid rather than actually storing
> the anon stateid when invalid.  This has the same net effect.
> 
> The aim of this patch is to simplify this code and particularly to
> remove the per-op OP_CLEAR_STATEID flag which is unnecessary.
> 
> The "is valid" flag is moved from 'struct nfsd4_compound_state' to
> 'struct svc_fh' which already has a number of flags related to the
> filehandle.  This flag will only ever be set for the v4 current_fh and
> saved_fh and records if the corresponding stateid is valid.
> 
> fh_put() is changed to clear this flag.

I don't see a hunk in the patch that implements this.


> As this is called on current_fh
> in each op that currently has OP_CLEAR_STATEID set, this automatically
> clears the stored stateid when required so OP_CLEAR_STATEID is no longer
> needed.
> 
> As fh_dup2() already copies all of the svc_fh, it now copies the new
> fh_have_stateid flag as well, so savefh and restorefh are simplified.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/current_stateid.h |  1 -
>  fs/nfsd/nfs4proc.c        | 25 ++++++++-----------------
>  fs/nfsd/nfs4state.c       | 10 ++--------
>  fs/nfsd/nfsfh.h           |  1 +
>  fs/nfsd/xdr4.h            | 13 -------------
>  5 files changed, 11 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
> index c28540d86742..24d769043207 100644
> --- a/fs/nfsd/current_stateid.h
> +++ b/fs/nfsd/current_stateid.h
> @@ -5,7 +5,6 @@
>  #include "state.h"
>  #include "xdr4.h"
>  
> -extern void clear_current_stateid(struct nfsd4_compound_state *cstate);
>  /*
>   * functions to set current state id
>   */
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e61b1ee6c8d8..ae4094ff12dc 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -737,10 +737,7 @@ nfsd4_restorefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		return nfserr_restorefh;
>  
>  	fh_dup2(&cstate->current_fh, &cstate->save_fh);
> -	if (HAS_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG)) {
> -		memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
> -		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
> -	}
> +	memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
>  	return nfs_ok;
>  }
>  
> @@ -757,10 +754,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		return nfserr_nofilehandle;
>  
>  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
> -	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
> -		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
> -		SET_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG);
> -	}
> +	memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
>  	return nfs_ok;
>  }
>  
> @@ -2954,9 +2948,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  			if (op->opdesc->op_set_currentstateid)
>  				op->opdesc->op_set_currentstateid(cstate, &op->u);
>  
> -			if (op->opdesc->op_flags & OP_CLEAR_STATEID)
> -				clear_current_stateid(cstate);
> -
>  			if (current_fh->fh_export &&
>  					need_wrongsec_check(rqstp))
>  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> @@ -3401,7 +3392,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_CREATE] = {
>  		.op_func = nfsd4_create,
> -		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME | OP_CLEAR_STATEID,
> +		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
>  		.op_name = "OP_CREATE",
>  		.op_rsize_bop = nfsd4_create_rsize,
>  	},
> @@ -3455,13 +3446,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_LOOKUP] = {
>  		.op_func = nfsd4_lookup,
> -		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
> +		.op_flags = OP_HANDLES_WRONGSEC,
>  		.op_name = "OP_LOOKUP",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_LOOKUPP] = {
>  		.op_func = nfsd4_lookupp,
> -		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
> +		.op_flags = OP_HANDLES_WRONGSEC,
>  		.op_name = "OP_LOOKUPP",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> @@ -3494,21 +3485,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	[OP_PUTFH] = {
>  		.op_func = nfsd4_putfh,
>  		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
> -				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
> +				| OP_IS_PUTFH_LIKE,
>  		.op_name = "OP_PUTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTPUBFH] = {
>  		.op_func = nfsd4_putrootfh,
>  		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
> -				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
> +				| OP_IS_PUTFH_LIKE,
>  		.op_name = "OP_PUTPUBFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTROOTFH] = {
>  		.op_func = nfsd4_putrootfh,
>  		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
> -				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
> +				| OP_IS_PUTFH_LIKE,
>  		.op_name = "OP_PUTROOTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f92b01bdb4dd..7ffe0b8e44de 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -9106,7 +9106,7 @@ nfs4_state_shutdown(void)
>  static void
>  get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>  {
> -	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
> +	if (cstate->current_fh.fh_have_stateid &&
>  	    is_current_stateid(stateid))
>  		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
>  }
> @@ -9116,16 +9116,10 @@ put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>  {
>  	if (cstate->minorversion) {
>  		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
> -		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
> +		cstate->current_fh.fh_have_stateid = true;
>  	}
>  }
>  
> -void
> -clear_current_stateid(struct nfsd4_compound_state *cstate)
> -{
> -	CLEAR_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
> -}
> -
>  /*
>   * functions to set current state id
>   */
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 43fcc1dcf69a..0142227c90e6 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -93,6 +93,7 @@ typedef struct svc_fh {
>  						 */
>  	bool			fh_use_wgather;	/* NFSv2 wgather option */
>  	bool			fh_64bit_cookies;/* readdir cookie size */
> +	bool			fh_have_stateid; /* associated v4.1 stateid is not special */
>  	bool			fh_post_saved;	/* post-op attrs saved */
>  	bool			fh_pre_saved;	/* pre-op attrs saved */
>  
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index bcaf631ec12d..b6ab32c7b243 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -43,13 +43,6 @@
>  #define NFSD4_MAX_TAGLEN	128
>  #define XDR_LEN(n)                     (((n) + 3) & ~3)
>  
> -#define CURRENT_STATE_ID_FLAG (1<<0)
> -#define SAVED_STATE_ID_FLAG (1<<1)
> -
> -#define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
> -#define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
> -#define CLEAR_CSTATE_FLAG(c, f) ((c)->sid_flags &= ~(f))
> -
>  /**
>   * nfsd4_encode_bool - Encode an XDR bool type result
>   * @xdr: target XDR stream
> @@ -194,8 +187,6 @@ struct nfsd4_compound_state {
>  	__be32			saved_status;
>  	stateid_t	current_stateid;
>  	stateid_t	save_stateid;
> -	/* to indicate current and saved state id presents */
> -	u32		sid_flags;
>  };
>  
>  static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
> @@ -1030,10 +1021,6 @@ enum nfsd4_op_flags {
>  	 * the v4.0 case).
>  	 */
>  	OP_CACHEME = 1 << 6,
> -	/*
> -	 * These are ops which clear current state id.
> -	 */
> -	OP_CLEAR_STATEID = 1 << 7,
>  	/* Most ops return only an error on failure; some may do more: */
>  	OP_NONTRIVIAL_ERROR_ENCODE = 1 << 8,
>  };


-- 
Chuck Lever