Re: [RFC PATCH v2 4/4] get rid of busy-waiting in shrink_dcache_tree()

[Al Viro <viro@zeniv.linux.org.uk>]

On Sat, Apr 04, 2026 at 12:04:42PM -0700, Linus Torvalds wrote:

> > static bool d_deathwatch(struct dentry *dentry, struct completion_list *node)
> 
> This looks odd to me. Particularly the "complete it myself" case where
> you return true. and then shrink the dentry list because you *think*
> you'll be waiting even if you don't.

Not really - we shrink the list before each rescan anyway.

We have collected some dentries into data.dispose and perhaps we'd stopped at
finding a victim already in somebody else's shrink list or already dying.

1) no such victim found => shrink data.dispose, rescan
2) victim found and successfully stolen from whatever shrink list it was on =>
evict the victim, shrink data.dispose, rescan
3) victim was busy dying => shrink data.dispose, wait until it finishes dying,
rescan
4) victim has managed to die while we'd been returning from d_walk() =>
shrink data.dispose, rescan

In (3) we could do wait_for_completion() before shrinking data.dispose; that
would merge the tails of all 4 cases.  However, that would add pointless waits -
basically, that's the scenario when another thread is sitting inside iput()
and we would be better off evicting whatever else we'd collected.

So (3) gets shrinking first, then wait, then go for the next iteration.
Sure, we can have (4) as a separate case is the same 3-way switch, but
what would it do?
		spin_unlock(&v->d_lock);
		rcu_read_unlock();
		break;	// proceeding to shrink_dentry_list() between the
			// end of if (data.victim) and the end of loop body
And right next to it we'd have
		spin_unlock(&v->d_lock);
		rcu_read_unlock();
		shrink data.dispose
		wait_for_completion(&wait.complete);
		continue;
for case (3), which differs only in having shrink_dentry_list() right there
and followed by wait_for_completion().  Put another way,
(4) is equivalent to
		spin_unlock(&v->d_lock);
		rcu_read_unlock();
		shrink data.dispose
		continue;

> IOW, if you go down this case, wouldn't it make more sense to have
> d_deathwatch() return a ternary value,  and have a switch case in the
> caller, and just enumerate the different cases?

See above.

> But I don't know. That's really more of a "that looks like an odd
> pattern" to me than any kind of objection.

FWIW, the thing that had me flipping back and forth between the variants
of calling conventions until I went "I've been staring at that for too
long to trust my taste" and asked that question was not the difference
in location of shrinking - it was self-called complete().

Hell knows; I'll probably leave it as-is for now and return to that
next week - it doesn't affect correctness, after all, and I'd rather
have that thing in -next in the current form seeing that it fixes
a real bug and further modifications would be local to fs/dcache.c
anyway.