Re: [PATCH v3 17/24] pNFS: Add deviceid reference query and collection walkers

[Benjamin Coddington <ben.coddington@hammerspace.com>]

On 10 Sep 2026, at 12:58, Anna Schumaker wrote:

> Hi Ben,
>
> On Fri, Sep 4, 2026, at 12:53 PM, Benjamin Coddington wrote:
>> The CB_NOTIFY_DEVICEID DELETE race recovery (RFC 8881 Section
>> 18.40.4) needs to ask whether any live layout still references a
>> deviceID, and to enumerate those layouts for TEST_STATEID.  Add a
>> layout_references_deviceid hook (sibling of reresolve_deviceid; the
>> flexfiles implementation memcmps each mirror stripe's decoded devid,
>> valid independent of the pinned device node) and two walkers over
>> the byserver pattern:
>>
>> - pnfs_layout_deviceid_referenced_byclid(): boolean existence query,
>>   early-stopping, entirely under i_lock.
>> - pnfs_layout_collect_deviceid_refs(): collects each matching layout
>>   with the hdr pinned, the inode grabbed with its superblock active
>>   (a pinned hdr does not hold its inode -- same discipline as the
>>   bulk-destroy walker), and the layout stateid and cred snapshotted
>>   under i_lock, so the caller can issue sleeping RPCs against the
>>   collection.
>>
>> The collection walker pins each matching header with a plain
>> pnfs_get_layout_hdr(), which cannot resurrect a dying header because
>> the walker holds i_lock and has checked NFS_I()->layout == lo.
>> pnfs_put_layout_hdr() drops the last reference under i_lock --
>> refcount_dec_and_lock() only decrements one to zero once it holds the
>> lock -- and clears NFS_I()->layout in that same critical section,
>> before it unlocks and frees.  So a header still installed on its inode
>> cannot have reached a zero refcount.
>>
>> That argument deliberately does not rest on NFS_LAYOUT_INVALID_STID.
>> A header can reach its final put while still valid: a full
>> LAYOUTRETURN ends in pnfs_layoutreturn_free_lsegs(), which resets the
>> layout stateid rather than invalidating it, and that is the ordinary
>> end of life for a return-on-close layout.  The validity check the
>> walkers do apply is a policy filter, not a lifetime guarantee.
>>
>> Because pnfs_put_layout_hdr() can send a layoutreturn and sleep, a
>> header pinned for a layout whose inode can no longer be grabbed is put
>> after the RCU read-side critical section, not within it.
>>
>> Both ways the walk can end early report it.  A GFP_ATOMIC allocation
>> failure aborts with -ENOMEM, and an inode that can no longer be
>> grabbed -- igrab() fails from I_FREEING on, while the layout may still
>> be valid and still name the deviceID -- aborts with -EAGAIN.  Either
>> way the caller is told the collection is partial instead of receiving
>> a short list it would read as "no references".
>>
>> No callers yet; no behavior change.
>>
>> Assisted-by: Claude:claude-fable-5
>> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
>> ---
>>  fs/nfs/flexfilelayout/flexfilelayout.c |  17 +++
>>  fs/nfs/pnfs.c                          | 164 +++++++++++++++++++++++++
>>  fs/nfs/pnfs.h                          |  29 +++++
>>  3 files changed, 210 insertions(+)
>>
>> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c
>> b/fs/nfs/flexfilelayout/flexfilelayout.c
>> index 9caf4b3b45ae..12b7be95a9c3 100644
>> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
>> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
>> @@ -2527,6 +2527,22 @@ static void ff_layout_cancel_io(struct
>> pnfs_layout_segment *lseg)
>>  	}
>>  }
>>
>> +/* Called under @lo's inode i_lock. */
>> +static bool ff_layout_references_deviceid(struct pnfs_layout_hdr *lo,
>> +					  const struct nfs4_deviceid *id)
>> +{
>> +	struct nfs4_flexfile_layout *flo = FF_LAYOUT_FROM_HDR(lo);
>> +	struct nfs4_ff_layout_mirror *mirror;
>> +	u32 dss_id;
>> +
>> +	list_for_each_entry(mirror, &flo->mirrors, mirrors)
>> +		for (dss_id = 0; dss_id < mirror->dss_count; dss_id++)
>> +			if (memcmp(&mirror->dss[dss_id].devid, id,
>> +				   sizeof(*id)) == 0)
>> +				return true;
>> +	return false;
>> +}
>> +
>>  /*
>>   * Un-pin every stripe node resolved from @id: in-flight I/O drains on
>> the
>>   * old node through its own reference, the next I/O re-resolves.
>> @@ -3155,6 +3171,7 @@ static struct pnfs_layoutdriver_type
>> flexfilelayout_type = {
>>  	.get_ds_info		= ff_layout_get_ds_info,
>>  	.free_deviceid_node	= ff_layout_free_deviceid_node,
>>  	.reresolve_deviceid	= ff_layout_reresolve_deviceid,
>> +	.layout_references_deviceid = ff_layout_references_deviceid,
>>  	.read_pagelist		= ff_layout_read_pagelist,
>>  	.write_pagelist		= ff_layout_write_pagelist,
>>  	.alloc_deviceid_node    = ff_layout_alloc_deviceid_node,
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 2f54b22d0e21..23f12eec99d7 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -2937,6 +2937,170 @@ pnfs_layout_reresolve_deviceid_byclid(struct
>> nfs_client *clp,
>>  	}
>>  }
>>
>> +struct pnfs_deviceid_ref_args {
>> +	const struct pnfs_layoutdriver_type *ld;
>> +	const struct nfs4_deviceid *id;
>> +	struct list_head *result;
>> +	bool found;
>> +};
>> +
>> +static int pnfs_layout_deviceid_referenced_byserver(
>> +		struct nfs_server *server, void *data)
>> +{
>> +	struct pnfs_deviceid_ref_args *args = data;
>> +	struct pnfs_layout_hdr *lo;
>> +	struct inode *inode;
>> +
>> +	if (server->pnfs_curr_ld != args->ld)
>> +		return 0;
>> +
>> +	rcu_read_lock();
>> +	list_for_each_entry_rcu(lo, &server->layouts, plh_layouts) {
>> +		inode = lo->plh_inode;
>> +		if (!inode)
>> +			continue;
>> +		spin_lock(&inode->i_lock);
>> +		if (NFS_I(inode)->layout == lo && pnfs_layout_is_valid(lo) &&
>> +		    args->ld->layout_references_deviceid(lo, args->id))
>> +			args->found = true;
>> +		spin_unlock(&inode->i_lock);
>> +		if (args->found)
>> +			break;
>> +	}
>> +	rcu_read_unlock();
>> +	return args->found;
>> +}
>> +
>> +/*
>> + * pnfs_layout_deviceid_referenced_byclid - does any live layout of
>> + * @clp's servers using @ld still reference deviceid @id?
>> + */
>> +bool
>> +pnfs_layout_deviceid_referenced_byclid(struct nfs_client *clp,
>> +				const struct pnfs_layoutdriver_type *ld,
>> +				const struct nfs4_deviceid *id)
>> +{
>> +	struct pnfs_deviceid_ref_args args = {
>> +		.ld = ld,
>> +		.id = id,
>> +	};
>
> I was wondering if we could go with different names for the members
> of this struct? "ld" and "id" look very similar, and at first glance
> I thought you were assigning the same values twice here.

Sure we can.. there's precedent for "ld" being used to refer to
pnfs_layoutdriver_type.  Should we change them all?  Happy to reversion and
resend, or let you re-write it as well.  Let me know what's most helpful.

Ben