[PATCH RFC 0/2] nfs: free leftover lsegs before freeing a layout in pnfs_put_layout_hdr

[PATCH RFC 0/2] nfs: free leftover lsegs before freeing a layout in pnfs_put_layout_hdr

[Jeff Layton]

Sending this as an RFC as I don't have a reliable reproducer for the
problem that Omar reported. I'm also not sure this is the best fix for
the problem. There is probably a case to be made that the real bug is in
the error handling for pnfs_layoutreturn_before_put_layout_hdr().

My guess is that the issue is that we end up with entries on the
plh_return_segs list just before the network goes down. That causes the
LAYOUTRETURN to fail with something that looks retryable, and the lsegs
on the list aren't freed.

It's possible that we just need to catch ENETUNREACH in the LAYOUTRETURN
error handling, but I'm not sure I correctly understand the problem. If
entries are racing onto the list just before the refcount decrement,
then that wouldn't fix it.

The first patch should fix the issue of the leaked lsegs, and the second
should let us know if it ever crops up again.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (2):
      nfs: free leftover lsegs before freeing a layout in pnfs_put_layout_hdr
      nfs: pr_warn if plh_segs or plh_return_segs are non-empty when freeing

 fs/nfs/pnfs.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
---
base-commit: 5bc1018675ec28a8a60d83b378d8c3991faa5a27
change-id: 20250428-nfs-6-16-87062aa2989d

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH RFC 1/2] nfs: free leftover lsegs before freeing a layout in pnfs_put_layout_hdr

[Jeff Layton]

Entries on the plh_return_segs list have already had their refcount go
to zero, and are only hanging around so that a LAYOUTRETURN can be
issued.

If there are still leftover lsegs on the plh_return_segs list when the
last pnfs_layout_hdr reference is put, then that means that the
layoutreturn just before the final refcount_dec_and_lock() has failed,
or that new lsegs have somehow raced onto plh_return_segs.

In either case, nothing else will be aware of the entries on that list,
since there are no more outstanding references. On the last
pnfs_layout_put(), do a final call to pnfs_mark_layout_stateid_invalid()
to shuffle any leftover segments to a tmp_list. Then free that list
after dropping the i_lock.

Reported-by: Omar Sandoval <osandov@osandov.com>
Closes: https://lore.kernel.org/linux-nfs/Z_ArpQC_vREh_hEA@telecaster/
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/pnfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 5f582713bf05eb72eb34b2e2c06d1edcd3c258d3..0bcb5a4bd420c157069ee63457518b206223b7cb 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -307,6 +307,7 @@ pnfs_put_layout_hdr(struct pnfs_layout_hdr *lo)
 {
 	struct inode *inode;
 	unsigned long i_state;
+	LIST_HEAD(tmp_list);
 
 	if (!lo)
 		return;
@@ -316,9 +317,11 @@ pnfs_put_layout_hdr(struct pnfs_layout_hdr *lo)
 	if (refcount_dec_and_lock(&lo->plh_refcount, &inode->i_lock)) {
 		if (!list_empty(&lo->plh_segs))
 			WARN_ONCE(1, "NFS: BUG unfreed layout segments.\n");
+		pnfs_mark_layout_stateid_invalid(lo, &tmp_list);
 		pnfs_detach_layout_hdr(lo);
 		i_state = inode->i_state;
 		spin_unlock(&inode->i_lock);
+		pnfs_free_lseg_list(&tmp_list);
 		pnfs_free_layout_hdr(lo);
 		/* Notify pnfs_destroy_layout_final() that we're done */
 		if (i_state & (I_FREEING | I_CLEAR))

-- 
2.49.0

[PATCH RFC 2/2] nfs: pr_warn if plh_segs or plh_return_segs are non-empty when freeing

[Jeff Layton]

pnfs_layoutreturn_before_put_layout_hdr() currently throws a WARN if
plh_segs still has entries when the layout_hdr is freed.

Add a new routine that prints info about leftover segments when this
occurs, before dumping the stack. Call that for both plh_segs and
plh_return_segs lists.

Suggested-by: Omar Sandoval <osandov@osandov.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/pnfs.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 0bcb5a4bd420c157069ee63457518b206223b7cb..4387b1f77a3140b88a0644db1a565a4b71ab64bf 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -302,6 +302,22 @@ pnfs_detach_layout_hdr(struct pnfs_layout_hdr *lo)
 	nfsi->read_io = 0;
 }
 
+static void
+pnfs_warn_dangling_lsegs(struct list_head *list, const char *name)
+{
+	struct pnfs_layout_segment *lseg;
+
+	if (list_empty(list))
+		return;
+
+	pr_warn("NFS: BUG unfreed layout segments on %s\n", name);
+	list_for_each_entry(lseg, list, pls_list)
+		pr_warn("%p: refs=%d flags=0x%lx seq=%u\n", lseg,
+			refcount_read(&lseg->pls_refcount), lseg->pls_flags,
+			lseg->pls_seq);
+	dump_stack();
+}
+
 void
 pnfs_put_layout_hdr(struct pnfs_layout_hdr *lo)
 {
@@ -315,8 +331,8 @@ pnfs_put_layout_hdr(struct pnfs_layout_hdr *lo)
 	pnfs_layoutreturn_before_put_layout_hdr(lo);
 
 	if (refcount_dec_and_lock(&lo->plh_refcount, &inode->i_lock)) {
-		if (!list_empty(&lo->plh_segs))
-			WARN_ONCE(1, "NFS: BUG unfreed layout segments.\n");
+		pnfs_warn_dangling_lsegs(&lo->plh_segs, "plh_segs");
+		pnfs_warn_dangling_lsegs(&lo->plh_return_segs, "plh_return_segs");
 		pnfs_mark_layout_stateid_invalid(lo, &tmp_list);
 		pnfs_detach_layout_hdr(lo);
 		i_state = inode->i_state;

-- 
2.49.0

Re: [PATCH RFC 0/2] nfs: free leftover lsegs before freeing a layout in pnfs_put_layout_hdr

[Jeff Layton]

On Mon, 2025-04-28 at 13:24 -0700, Jeff Layton wrote:
> Sending this as an RFC as I don't have a reliable reproducer for the
> problem that Omar reported. I'm also not sure this is the best fix for
> the problem. There is probably a case to be made that the real bug is in
> the error handling for pnfs_layoutreturn_before_put_layout_hdr().
> 
> My guess is that the issue is that we end up with entries on the
> plh_return_segs list just before the network goes down. That causes the
> LAYOUTRETURN to fail with something that looks retryable, and the lsegs
> on the list aren't freed.
> 
> It's possible that we just need to catch ENETUNREACH in the LAYOUTRETURN
> error handling, but I'm not sure I correctly understand the problem. If
> entries are racing onto the list just before the refcount decrement,
> then that wouldn't fix it.
> 
> The first patch should fix the issue of the leaked lsegs, and the second
> should let us know if it ever crops up again.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Jeff Layton (2):
>       nfs: free leftover lsegs before freeing a layout in pnfs_put_layout_hdr
>       nfs: pr_warn if plh_segs or plh_return_segs are non-empty when freeing
> 
>  fs/nfs/pnfs.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> ---
> base-commit: 5bc1018675ec28a8a60d83b378d8c3991faa5a27
> change-id: 20250428-nfs-6-16-87062aa2989d
> 
> Best regards,

Trond, Anna, ping? This seems like the right thing to do, but I'd
appreciate a second (and third) set of eyes on this.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>