On Mon, Jul 07, 2025 at 07:20:28PM +0200, Max Kellermann wrote:
> On Mon, Jul 7, 2025 at 7:04 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> > @@ -1478,6 +1444,8 @@ static enum d_walk_ret select_collect(void *_data, struct dentry *dentry)
> > } else if (!dentry->d_lockref.count) {
> > to_shrink_list(dentry, &data->dispose);
> > data->found++;
> > + } else if (dentry->d_lockref.count < 0) {
> > + data->found++;
> > }
> > /*
> > * We can return to the caller if we have found some (this
>
> I have doubts about this part of your patch. (Warning: I don't really
> know what your patch is about.)
>
> Why does this new check exist?
> (It checks for "dead" or "killed" entries, but why aren't you using
> __lockref_is_dead() here?)
What's the difference? It checks for dentries currently still going through
->d_prune()/->d_iput()/->d_release().
> Actual problem why I found this: while debugging (yet another) Ceph
> bug, I found that a kworker is busy-looping inside
> shrink_dcache_parent(). Each iteration finds a dead/killed dentry,
> thus "found=true" and the loop keeps on looping forever, yet nothing
> ever gets done.
> It does this because a userspace process is trying to write to Ceph
> file, that write() system call invokes the shrinker (via
> filmap_add_folio() / memcg). The shrinker calls shrink_dentry_list(),
> __dentry_kill() - now that dentry is dead/killed, but it remains
> listed in the parent because the thread is stuck in ceph_evict_inode()
> / netfs_wait_for_outstanding_io().
>
> I am seeing this because Ceph doesn't finish I/O on the inode, which
> causes the kworker to busy-loop forever without making any progress.
> But even if Ceph weren't as buggy as it is, there may still be some
> time waiting for the Ceph server, and that will always cause brief
> periods of busy-looping in the kworker, won't it?
>
> I don't know how to solve this (I have no idea about the dcache,
> having opened its source for the first time today), but I wonder why
> select_collect() ever cares about dead/killed dentries. Removing that
> check seems like the obvious solution, but there must be a reason why
> you added it.
shrink_dcache_parent() is "evict everything that can be evicted in the
subtree"; no idea whether it's the right primitive for your usecase.
Note that these suckers *do* keep their ancestors pinned; as the result
we are, e.g., guaranteed sane ordering on RCU grace periods for their
freeing, etc. One thing we definitely do not want is eviction of parent
started before its child is done with __dentry_kill()...
What are you using shrink_dcache_parent() for?
