Re: [PATCH 14/22] dentry_kill(): don't bother with retain_dentry() on slow path

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

On Thu, Nov 09, 2023 at 04:53:07PM +0100, Christian Brauner wrote:
> On Thu, Nov 09, 2023 at 06:20:48AM +0000, Al Viro wrote:
> > We have already checked it and dentry used to look not worthy
> > of keeping.  The only hard obstacle to evicting dentry is
> > non-zero refcount; everything else is advisory - e.g. memory
> > pressure could evict any dentry found with refcount zero.
> > On the slow path in dentry_kill() we had dropped and regained
> > ->d_lock; we must recheck the refcount, but everything else
> > is not worth bothering with.
> > 
> > Note that filesystem can not count upon ->d_delete() being
> > called for dentry - not even once.  Again, memory pressure
> > (as well as d_prune_aliases(), or attempted rmdir() of ancestor,
> > or...) will not call ->d_delete() at all.
> > 
> > So from the correctness point of view we are fine doing the
> > check only once.  And it makes things simpler down the road.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> 
> Ok, that again relies on earlier patches that ensure that dentry_kill()
> isn't called with refcount == 0 afaiu,

Huh?

There are two reasons to keep dentry alive - positive refcount and
a bunch of heuristics for "it might be nice to keep it around in
hash, even though its refcount is down to zero now".

Breakage on underflows aside, dentry_kill() had always been called with
refcount 1, victim locked and those heuristics saying "no point keeping
it around".  Then it grabs the rest of locks needed for actual killing;
if we are lucky and that gets done just on trylocks, that's it - we
decrement refcount (to 0 - we held ->d_lock all along) and pass the
sucker to __dentry_kill().  RIP.  If we had to drop and regain ->d_lock,
it is possible that somebody took an extra reference and it's no longer
possible to kill the damn thing.  In that case we just decrement the
refcount, drop the locks and that's it - we are done.

So far, so good, but there's an extra twist - in case we had to drop
and regain ->d_lock, dentry_kill() rechecks the "might be nice to
keep it around" heuristics and treats "it might be" same way as it
would deal with finding extra references taken by somebody while
->d_lock had not been held.  That is to say, it does refcount decrement
(to 0 - we'd just checked that it hadn't been increased from 1),
drops the locks and that's it.

The thing is, those heuristics are really "it might be nice to keep" -
there are trivial ways to force eviction of any unlocked dentry with
zero refcount.  So why bother rechecking those?  We have already
checked them just before calling dentry_kill() and got "nah, don't
bother keeping it", after all.  And we would be leaving it in the
state where it could be instantly evicted, heuristics nonwithstanding,
so from correctness standpoint might as well decide not to keep
it and act as if that second call of retain_dentry() returned false.

Previous patches have very little to do with that - the only thing
that affects dentry_kill() is the (now gone) possibility of hitting
an underflow here.  If underflow happened, we were already screwed;
yes, this would've been one of the places where the breakage would
show up, but that's basically "what amusing kinds of behaviour would
that function exhibit on FUBAR data structures".