[RFC PATCH] dcache: keep shrink_dcache_for_umount() making progress on busy roots

[RFC PATCH] dcache: keep shrink_dcache_for_umount() making progress on busy roots

[Karl Mehltretter]

Commit e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
moved secondary roots from ->d_hash to ->d_sib.  Secondary roots are
now d_unhashed(), so __d_drop() returns without removing them from
->s_roots.  Consequently, d_drop() in do_one_tree() no longer
guarantees progress through the list.

If a secondary root has an unexpected extra reference, do_one_tree()
reports it, but its final dput() cannot evict it.  The root remains
->s_roots.first and the loop selects it forever, holding ->s_umount for
write and repeatedly reporting the same dentry.

Before e9895609cb7f, ___d_drop() special-cased IS_ROOT dentries and
removed them from ->s_roots regardless of their refcount.  Commit
9c8c10e262e0 ("more graceful recovery in umount_collect()") deliberately
made busy dentries nonfatal: report them and finish the unmount rather
than BUG() while holding ->s_umount.

Detaching busy secondary roots from ->s_roots restores the previous
behavior.  A permanently leaked reference remains leaked after unmount;
if the extra reference is only delayed, its final dput() may run after
teardown has advanced and hit poisoned or freed filesystem state.  The
current code avoids that late cleanup only by looping indefinitely under
->s_umount for a real leak.

Restore that behavior by removing each live secondary root from
->s_roots after taking the temporary reference.  In the normal case,
dentry_unlist() finds ->d_sib already unhashed when eviction occurs.

Fixes: e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---

Notes:
    The reproducer mounts ramfs, allocates a fresh directory inode on its
    superblock, calls d_obtain_root(), and deliberately retains the returned
    dentry reference before unmounting.  The fresh inode ensures that no
    existing alias is returned.

                            d_unhashed()   reports   umount returns
      v7.1                             0         1              yes
      7.2-rc4+ unpatched               1     13013               no
      7.2-rc4+ patched                 1         1              yes

    All 13013 reports in the unpatched run identified the same dentry,
    confirming that the loop kept selecting ->s_roots.first.  With the patch,
    diagnostics match v7.1: one busy-dentry warning, one busy-inode warning,
    and no list-debug corruption.

    This injects a pre-existing refcount bug; no in-tree filesystem was found
    to create one by itself.

    Detaching the root guarantees progress for a permanent leak.  If the extra
    reference is merely delayed, however, its eventual dput() can run after
    teardown has advanced.  Leaving the root attached avoids that late cleanup
    but retries indefinitely for a permanent leak.

 fs/dcache.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/dcache.c b/fs/dcache.c
index 3e9af9de7074..2aff15ee07fb 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1832,6 +1832,9 @@ void shrink_dcache_for_umount(struct super_block *sb)
 				wait_for_completion(&wait.completion);
 		} else {
 			dget_dlock(dentry);
+			// A busy root survives do_one_tree(); unlink it so the
+			// ->s_roots loop keeps making progress.
+			unlink_secondary_root(dentry);
 			spin_unlock(&dentry->d_lock);
 			do_one_tree(dentry);
 		}
-- 
2.53.0

Re: [RFC PATCH] dcache: keep shrink_dcache_for_umount() making progress on busy roots

[Christian Brauner]

> Commit e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
> moved secondary roots from ->d_hash to ->d_sib.  Secondary roots are
> now d_unhashed(), so __d_drop() returns without removing them from
> ->s_roots.  Consequently, d_drop() in do_one_tree() no longer
> guarantees progress through the list.
> 
> If a secondary root has an unexpected extra reference, do_one_tree()
> reports it, but its final dput() cannot evict it.  The root remains
> ->s_roots.first and the loop selects it forever, holding ->s_umount for
> write and repeatedly reporting the same dentry.
> 
> Before e9895609cb7f, ___d_drop() special-cased IS_ROOT dentries and
> removed them from ->s_roots regardless of their refcount.  Commit
> 9c8c10e262e0 ("more graceful recovery in umount_collect()") deliberately
> made busy dentries nonfatal: report them and finish the unmount rather
> than BUG() while holding ->s_umount.
> 
> Detaching busy secondary roots from ->s_roots restores the previous
> behavior.  A permanently leaked reference remains leaked after unmount;
> if the extra reference is only delayed, its final dput() may run after
> teardown has advanced and hit poisoned or freed filesystem state.  The
> current code avoids that late cleanup only by looping indefinitely under
> ->s_umount for a real leak.
> 
> Restore that behavior by removing each live secondary root from
> ->s_roots after taking the temporary reference.  In the normal case,
> dentry_unlist() finds ->d_sib already unhashed when eviction occurs.
> 
> Fixes: e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

Wouldn't it make more sense to move this into do_one_tree()?

From 6c955dad4ba4bc1c1e47426697825d041e591d8e Mon Sep 17 00:00:00 2001
From: Karl Mehltretter <kmehltretter@gmail.com>
Date: Wed, 29 Jul 2026 02:59:33 +0200
Subject: [PATCH] dcache: keep shrink_dcache_for_umount() making progress on
 busy roots

Commit e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
moved secondary roots from ->d_hash to ->d_sib.  Secondary roots are
now d_unhashed(), so __d_drop() returns without removing them from
->s_roots.  Consequently, d_drop() in do_one_tree() no longer
guarantees progress through the list.

If a secondary root is still busy once do_one_tree() is done with it,
its final dput() cannot evict it.  The root remains ->s_roots.first
and the loop selects it forever, holding ->s_umount for write and
repeatedly reporting the same dentry.

The root does not need a leaked reference of its own for that.  Every
child pins its parent (d_alloc() takes a reference on it) and
umount_check() deliberately reports a busy descendant instead of
complaining about its ancestors, so a single leaked dentry reference
anywhere below a secondary root is enough.  For filesystems that build
->s_root with d_obtain_root() - nfs, ceph, nilfs2 snapshot mounts -
that is the entire tree.

Before e9895609cb7f, ___d_drop() special-cased IS_ROOT dentries and
removed them from ->s_roots regardless of their refcount, so the
d_drop() in do_one_tree() detached the root from the superblock no
matter what.  Commit 9c8c10e262e0 ("more graceful recovery in
umount_collect()") deliberately made busy dentries nonfatal: report
them and finish the unmount rather than BUG() while holding
->s_umount.

Restore that by detaching the root in do_one_tree() itself, next to
the d_drop() that used to do it.  That covers both callers - the
->s_roots loop and ->s_root, which for the filesystems above is a
secondary root as well.  In the normal case dentry_unlist() finds
->d_sib already unhashed when eviction occurs.

A permanently leaked reference remains leaked after unmount, as it did
before e9895609cb7f; if the extra reference is merely delayed, its
final dput() may run after teardown has advanced.  Leaving the root on
->s_roots is not an alternative: the superblock would then be freed
with a live dentry still linked into it, and that dentry's
dentry_unlist() would take ->s_roots_lock on freed memory.

Christian Brauner <brauner@kernel.org> says:
Moved the ->s_roots removal from the shrink_dcache_for_umount() loop
into do_one_tree(), so a busy ->s_root obtained from d_obtain_root() is
detached on the first pass instead of being reported a second time when
the loop picks it off ->s_roots.  Extended the commit message with the
pinned-ancestor case.

Fixes: e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
Link: https://patch.msgid.link/20260729005933.15858-1-kmehltretter@gmail.com
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/dcache.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 3e9af9de7074..aded2564b589 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1794,7 +1794,12 @@ static void do_one_tree(struct dentry *dentry)
 {
 	shrink_dcache_tree(dentry, true);
 	d_walk(dentry, dentry, umount_check);
-	d_drop(dentry);
+	spin_lock(&dentry->d_lock);
+	__d_drop(dentry);
+	// a busy root survives the dput() below; don't leave it on ->s_roots
+	if (unlikely(!hlist_unhashed(&dentry->d_sib)))
+		unlink_secondary_root(dentry);
+	spin_unlock(&dentry->d_lock);
 	dput(dentry);
 }
 
-- 
2.53.0

Re: [RFC PATCH] dcache: keep shrink_dcache_for_umount() making progress on busy roots

[Karl Mehltretter]

On Wed, Jul 29, 2026 at 12:01:48PM +0100, Christian Brauner wrote:
> 
> Wouldn't it make more sense to move this into do_one_tree()?
> 

Yes I agree. Your version is a clear improvement.

Thanks,
Karl