Re: [PATCH 2/3] NFSD: Add infrastructure for tracking persistent SCSI registration keys

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

On 12/18/25 1:34 AM, Christoph Hellwig wrote:
> On Mon, Dec 15, 2025 at 10:13:34AM -0800, Dai Ngo wrote:
>> +
>> +int
>> +nfsd4_scsi_pr_init_hashtbl(struct nfsd_net *nn)
>> +{
>> +	int ix;
>> +
>> +	nn->client_pr_record_hashtbl = kmalloc_array(CLIENT_HASH_SIZE,
>> +					sizeof(struct list_head),
>> +					GFP_KERNEL);
>> +	if (!nn->client_pr_record_hashtbl)
>> +		return -ENOMEM;
>> +	spin_lock_init(&nn->client_pr_record_hashtbl_lock);
>> +	for (ix = 0; ix < CLIENT_HASH_SIZE; ix++)
>> +		INIT_LIST_HEAD(&nn->client_pr_record_hashtbl[ix]);
>> +	return 0;
> I guess there is precendent in the nfsd code in using this fixed size
> hash table, but they are not very scalable.  And using the rhastable
> API is actually relatively simple, so it might be easier to use that
> than rolling your own hash.
>
> If you stick to the fixes size open code hash, you should use a
> hlist_head here.  There is no advantage in having a pointer to the tail
> entry for hashes, and the hlist saves half of the memory usage and
> improves cache efficiency.
>
> But taking a step back:  why do we even need a new hash table here?
> Can't we jut hang off a list of block device for which a layout
> was granted off the nfs4_client structure given that we already
> have it available?

Yes, a simple list hang of the nfs4_client structure makes sense.

I don't think a NFS client would be configured to access too many
pNFS shares with SCSI layout type. Also, fencing is not expected
to be a frequent operation so traversing the list should not effect
the overall performance of the NFS server.

>
>> +static struct scsi_pr_record *
>> +nfsd4_pr_find_client(struct nfs4_client *clp, struct block_device *blkdev)
>> +{
>> +	struct scsi_pr_record *pr_rec = NULL;
>> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>> +	unsigned int idhashval;
>> +	dev_t bdev = blkdev->bd_dev;
>> +
>> +	assert_spin_locked(&nn->client_pr_record_hashtbl_lock);
>> +	idhashval = clientid_hashval(clp->cl_clientid.cl_id);
>> +	list_for_each_entry(pr_rec, &nn->client_pr_record_hashtbl[idhashval],
>> +					spr_hash) {
>> +		if (same_clid(&pr_rec->spr_clid, &clp->cl_clientid) &&
>> +				pr_rec->spr_bdev == bdev) {
>> +			return pr_rec;
>> +		}
> This ensures that you always have collisions for multiple bdevs of the
> same client.  Why not use a hash the mixes entropy from the client id
> and the bdev?

This is no longer needed when switching to use a simple list.

>
>> +bool
>> +nfsd4_scsi_pr_fence(struct nfs4_client *clp, struct block_device *blkdev)
>> +{
>> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>> +	struct scsi_pr_record *rec;
>> +
>> +	assert_spin_locked(&nn->client_pr_record_hashtbl_lock);
>> +	rec = nfsd4_pr_find_client(clp, blkdev);
>> +	if (rec && !rec->spr_fenced) {
>> +		rec->spr_fenced = true;
>> +		return true;
>> +	}
>> +	return false;
>> +}
> This function seems misnamed.  It doesn't do any actualy fencing.

will fix in v2.

>
>> +extern int nfsd4_scsi_pr_init_hashtbl(struct nfsd_net *net);
>> +extern void nfsd4_scsi_pr_shutdown(struct nfsd_net *net);
>> +struct nfs4_client;
>> +extern void nfsd4_scsi_pr_del_client(struct nfs4_client *clp);
>> +extern int nfsd4_scsi_pr_add_client(struct nfs4_client *clp,
>> +		struct block_device *blkdev);
>> +extern bool nfsd4_scsi_pr_fence(struct nfs4_client *clp,
>> +		struct block_device *blkdev);
> Some of these are only used inside of blocklayout, so mark them
> static.  For the others drop the extern.  Also please keep
> struct forward declarations at the top of the file.

will fix in v2.

>
>> +
>>   #else /* CONFIG_NFSD_V4 */
>>   static inline int nfsd4_is_junction(struct dentry *dentry)
>>   {
>> @@ -578,6 +587,13 @@ static inline int nfsd4_is_junction(struct dentry *dentry)
>>   
>>   static inline void nfsd4_init_leases_net(struct nfsd_net *nn) { };
>>   
>> +extern inline int nfsd4_scsi_pr_init_hashtbl(struct nfsd_net *nn)
> This should be static inline.

no longer need this with a simple list.

>
>> index db9af780438b..9ae02b6d922d 100644
>> --- a/fs/nfsd/pnfs.h
>> +++ b/fs/nfsd/pnfs.h
>> @@ -67,6 +67,17 @@ __be32 nfsd4_return_client_layouts(struct svc_rqst *rqstp,
>>   int nfsd4_set_deviceid(struct nfsd4_deviceid *id, const struct svc_fh *fhp,
>>   		u32 device_generation);
>>   struct nfsd4_deviceid_map *nfsd4_find_devid_map(int idx);
>> +
>> +int nfsd4_scsi_pr_init_hashtbl(struct nfsd_net *nn);
>> +void nfsd4_scsi_pr_shutdown(struct nfsd_net *nn);
>> +void nfsd4_scsi_pr_del_client(struct nfs4_client *clp);
> These duplicate the prototypes in nfsd.h.

will fix in v2.

>
>> +struct scsi_pr_record {
>> +	struct list_head spr_hash;
>> +	clientid_t spr_clid;
>> +	dev_t spr_bdev;
>> +	bool spr_fenced;
>> +};
> This can be kept static in blocklayout.c

will fix in v2.

Thanks,
-Dai