Re: [PATCH v5 06/11] nfsd: revise names of special stateid, and predicate functions.

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

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> When I see "CURRENT_STATEID(foo)" in the code it is not clear that this
> is testing the stateid to see if it is a special stateid.  I find that
> IS_CURRENT_STATEID(foo) is clearer.  But an inline function is even
> better, so is_current_stateid().
> 
> There are other special stateids which are described in RFC 8881 Section
> 8.2.3 as "anonymous", "READ bypass", and "invalid".  The nfsd code
> currently names them "zero", "one" and "close" which doesn't help with
> comparing the code to the RFC.
> 
> So this patch changes the names of those special stateids and adds
> "is_" to the front of the predicates.
> 
> As CLOSE_STATEID() was not needed, it is discarded rather than replacing
> with is_invalid_stateid().
> 
> I felt that is_read_bypass_stateid() was a little too verbose, so I made
> it is_bypass_stateid().
> 
> For consistency, invalid_stateid is changed to use ~0 rather than
> 0xffffffffU for the generation number.  (RFC 8881 say to use
> "NFS4_UINT32_MAX" for the generation number here, and "all ones" for the
> generation and opaque of anon_stateid).
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ea931e606f40..f92b01bdb4dd 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -60,18 +60,18 @@
>  #define NFSDDBG_FACILITY                NFSDDBG_PROC
>  
>  #define all_ones {{ ~0, ~0}, ~0}
> -static const stateid_t one_stateid = {
> +static const stateid_t read_bypass_stateid = {
>  	.si_generation = ~0,
>  	.si_opaque = all_ones,
>  };
> -static const stateid_t zero_stateid = {
> +static const stateid_t anon_stateid = {
>  	/* all fields zero */
>  };
> -static const stateid_t currentstateid = {
> +static const stateid_t current_stateid = {
>  	.si_generation = 1,
>  };
> -static const stateid_t close_stateid = {
> -	.si_generation = 0xffffffffU,
> +static const stateid_t invalid_stateid = {
> +	.si_generation = ~0,
>  };
>  
>  /*
> @@ -93,10 +93,16 @@ static inline bool stateid_well_formed(stateid_t *stid)
>  
>  static u64 current_sessionid = 1;
>  
> -#define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
> -#define ONE_STATEID(stateid)  (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
> -#define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
> -#define CLOSE_STATEID(stateid)  (!memcmp((stateid), &close_stateid, sizeof(stateid_t)))
> +/* These special stateid are defined in RFC 8881 Section 8.2.3 */
> +static inline bool is_anon_stateid(stateid_t *stateid) {
> +	return memcmp(stateid, &anon_stateid, sizeof(stateid_t));
> +}
> +static inline bool is_bypass_stateid(stateid_t *stateid) {
> +	return memcmp(stateid, &read_bypass_stateid, sizeof(stateid_t));
> +}
> +static inline bool is_current_stateid(stateid_t *stateid) {
> +	return memcmp(stateid, &current_stateid, sizeof(stateid_t));
> +}

The new static inline functions appear to invert the logic -- the macros
use "!memcmp" but the new functions omit the "!". memcmp() returns an
int, so there is an implicit type conversion here as well. So maybe you
want "memcmp(stateid, ... ) == 0" ?

And now we can use "sizeof(*stateid)" here which is slightly less
brittle.


>  /* forward declarations */
>  static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
> @@ -388,7 +394,7 @@ nfsd4_cb_notify_lock_prepare(struct nfsd4_callback *cb)
>  static int
>  nfsd4_cb_notify_lock_done(struct nfsd4_callback *cb, struct rpc_task *task)
>  {
> -	trace_nfsd_cb_notify_lock_done(&zero_stateid, task);
> +	trace_nfsd_cb_notify_lock_done(&anon_stateid, task);
>  
>  	/*
>  	 * Since this is just an optimization, we don't try very hard if it
> @@ -6512,7 +6518,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>  	 * open stateid would have to be created.
>  	 */
>  	if (new_stp && open_xor_delegation(open)) {
> -		memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid));
> +		memcpy(&open->op_stateid, &anon_stateid, sizeof(open->op_stateid));
>  		open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID;
>  		release_open_stateid(stp);
>  	}
> @@ -7076,7 +7082,7 @@ __be32 nfs4_check_openmode(struct nfs4_ol_stateid *stp, int flags)
>  static inline __be32
>  check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid, int flags)
>  {
> -	if (ONE_STATEID(stateid) && (flags & RD_STATE))
> +	if (is_bypass_stateid(stateid) && (flags & RD_STATE))
>  		return nfs_ok;
>  	else if (opens_in_grace(net)) {
>  		/* Answer in remaining cases depends on existence of
> @@ -7085,7 +7091,7 @@ check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid,
>  	} else if (flags & WR_STATE)
>  		return nfs4_share_conflict(current_fh,
>  				NFS4_SHARE_DENY_WRITE);
> -	else /* (flags & RD_STATE) && ZERO_STATEID(stateid) */
> +	else /* (flags & RD_STATE) && is_anon_stateid(stateid) */
>  		return nfs4_share_conflict(current_fh,
>  				NFS4_SHARE_DENY_READ);
>  }
> @@ -7401,7 +7407,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  	if (nfp)
>  		*nfp = NULL;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
> +	if (is_anon_stateid(stateid) || is_bypass_stateid(stateid)) {
>  		status = check_special_stateids(net, fhp, stateid, flags);
>  		goto done;
>  	}
> @@ -7823,12 +7829,12 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	/* v4.1+ suggests that we send a special stateid in here, since the
>  	 * clients should just ignore this anyway. Since this is not useful
> -	 * for v4.0 clients either, we set it to the special close_stateid
> +	 * for v4.0 clients either, we set it to the special invalid_stateid
>  	 * universally.
>  	 *
>  	 * See RFC5661 section 18.2.4, and RFC7530 section 16.2.5
>  	 */
> -	memcpy(&close->cl_stateid, &close_stateid, sizeof(close->cl_stateid));
> +	memcpy(&close->cl_stateid, &invalid_stateid, sizeof(close->cl_stateid));
>  
>  	/* put reference from nfs4_preprocess_seqid_op */
>  	nfs4_put_stid(&stp->st_stid);
> @@ -9101,7 +9107,7 @@ static void
>  get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>  {
>  	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
> -	    CURRENT_STATEID(stateid))
> +	    is_current_stateid(stateid))
>  		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
>  }
>  


-- 
Chuck Lever