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

[Dai Ngo <dai.ngo@oracle.com>]

On 12/18/25 1:25 AM, Christoph Hellwig wrote:
> 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?

I'll drop this patch since I will use a list that hangs off the nfs4_client
structure to keep track of the block devices, as you suggested.

-Dai