On Mon, Oct 30, 2023 at 12:18:28PM -1000, Linus Torvalds wrote:
> On Mon, 30 Oct 2023 at 11:53, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > After fixing a couple of brainos, it seems to work.
>
> This all makes me unnaturally nervous, probably because it;s overly
> subtle, and I have lost the context for some of the rules.
A bit of context: I started to look at the possibility of refcount overflows.
Writing the current rules for dentry refcounting and lifetime down was the
obvious first step, and that immediately turned into an awful mess.
It is overly subtle. Even more so when you throw the shrink lists into
the mix - shrink_lock_dentry() got too smart for its own good, and that
leads to really awful correctness proofs. The next thing in the series
is getting rid of the "it had been moved around, so somebody had clearly
been taking/dropping references and we can just evict it from the
shrink list and be done with that" crap - the things get much simpler
if the rules become
* call it under rcu_read_lock, with dentry locked
* if returned true
dentry, parent, inode locked, refcount is zero.
* if returned false
dentry locked, refcount is non-zero.
It used to be that way, but removal of trylock loops had turned that
into something much more subtle. Restoring the old semantics without
trylocks on the slow path is doable and it makes analysis much simpler.
BTW, where how aggressive do we want to be with d_lru_del()?
We obviously do not do that on non-final dput, even if we have
a dentry with positive refcount in LRU list. But when we hit e.g.
shrink_dcache_parent(), all dentries in the subtree get d_lru_del(),
whether they are busy or not. I'm not sure it's a good idea...
Sure, we want __dentry_kill() to remove the victim from LRU and
we want the same done to anything moved to a shrink list.
Having LRU scanners ({prune,shrink}_dcache_sb() do that to
dentries with positive refcount also makes sense. Do we really need
the other cases?
