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

[Christian Brauner <brauner@kernel.org>]

> 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