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

["Anna Schumaker" <anna@kernel.org>]

On Thu, Sep 10, 2026, at 1:24 PM, Benjamin Coddington wrote:
> 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.

I think just adjusting the naming in this patch is enough. You could even
leave "ld" alone and change "id" to something like "devid" and I'd be happy.

Anna

>
> Ben