| Message ID | 20231109062056.3181775-14-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/22] struct dentry: get rid of randomize_layout idiocy | expand |
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, Reviewed-by: Christian Brauner <brauner@kernel.org>
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".
diff --git a/fs/dcache.c b/fs/dcache.c index d9466cab4884..916b978bfd98 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -739,14 +739,10 @@ static struct dentry *dentry_kill(struct dentry *dentry) spin_lock(&dentry->d_lock); parent = lock_parent(dentry); got_locks: - if (unlikely(dentry->d_lockref.count != 1)) { - dentry->d_lockref.count--; - } else if (likely(!retain_dentry(dentry))) { - dentry->d_lockref.count--; + dentry->d_lockref.count--; + if (likely(dentry->d_lockref.count == 0)) { __dentry_kill(dentry); return parent; - } else { - dentry->d_lockref.count--; } /* we are keeping it, after all */ if (inode)
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> --- fs/dcache.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)