Re: [PATCH 1/3] NFSD: Move clientid_hashval and same_clid to header files

[Christoph Hellwig <hch@lst.de>]

On Mon, Dec 15, 2025 at 01:58:49PM -0500, Chuck Lever wrote:
> >  	return bmval_is_subset(bmval, nfsd_suppattrs[minorversion]);
> >  }
> > 
> > +static inline unsigned int clientid_hashval(u32 id)
> > +{
> > +	return id & CLIENT_HASH_MASK;
> > +}
> > +
> 
> I can't comment on the overall purpose of this series yet, but
> there are one or two mechanical issues that I see already.
> 
> Let's not add NFSv4- or pNFS-specific functions to fs/nfsd/nfsd.h.
> Same comment applies to the function declarations this series moves
> in a subsequent patch.
> 
> Why can't clientid_hashval() go into fs/nfsd/state.h instead?

Or why do we need it at all?  It's completly trivial, and there is
no inherent advantage in sharing an arbiteary masking for hasheѕ.

> Also, when a function becomes accessible outside of one source
> file (like a "static inline" function or a callback function),
> it needs to get a kdoc comment that documents its API contract.

And a nfsd_ prefix would also be useful.  All good arguments for
just duplicating this bit.

> > +static inline int same_clid(clientid_t *cl1, clientid_t *cl2)
> > +{
> > +	return (cl1->cl_boot == cl2->cl_boot) && (cl1->cl_id == cl2->cl_id);
> > +}

Now this might have some more reason to be shared as we need two
values to establish the client identify.  Ńo need for the braces
here even wen they are copied from the existing code.

Also I wonder why clientid_t is a typedef instead of a struct?  And
if we want to treat it as opaque, why we're comparing the fields instead
of a memcmp?