Re: [viro-vfs:work.dcache2] [__dentry_kill()] 1b738f196e: stress-ng.sysinfo.ops_per_sec -27.2% regression

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

On Wed, Dec 06, 2023 at 10:40:15AM +0800, Oliver Sang wrote:
> hi, Al Viro,

> > Oh, well - let's see what profiles show...  I still hope that it's not where
> > the trouble comes from - it would've lead the extra cycles in shrink_dcache_parent()
> > or d_walk() called from it and profiles you've posted do not show that, so...
> > 
> 
> our auto-bisect pointed to
> 3f99656f7bc2f step 5: call __dentry_kill() without holding a lock on parent

Nuts.  All that commit is doing is
	* don't bother with taking parent's ->d_lock in lock_for_kill()
	* don't bother with dropping it in __dentry_kill(), since now we don't have it
	  taken.

And that somehow manages to give a 30% performance drop on that test?

Seriously, not touching parent's ->d_lock aside, there is only one change
I see in there.  Namely,
	having failed trylock on inode
	unlocked dentry
	locked inode
	relocked dentry
	noticed that inode does not match ->d_inode
	... and it turns out that ->d_inode is NULL now
In that situation new variant treats goes "OK, unlock the old
inode and we are done".  The old one unlocked old inode,
unlocked dentry, relocked it, checked that its ->d_inode has
not become non-NULL while we had it unlocked and only then
succeeded.  If ->d_inode has changed, it repeated the whole
loop (unlocked dentry, etc.)

Let's try to isolate that; step 5 split in two and branch
force-pushed.  Instead of
dc3cf789eb259 step 4: make shrink_kill() keep the parent pinned until after __dentry_kill() of victim
3f99656f7bc2f step 5: call __dentry_kill() without holding a lock on parent
we have
dc3cf789eb259 step 4: make shrink_kill() keep the parent pinned until after __dentry_kill() of victim
854e9f938aafe step 4.5: call __dentry_kill() without holding a lock on parent
e2797564725a5 step 5: clean lock_for_kill()
;  git diff e2797564725a5 3f99656f7bc2f
;

Intermediate step preserves the control flow in lock_for_kill() -
the only change in that one is "don't bother with parent's ->d_lock".

The step after it does the change in lock_for_kill() described above.

Could you profile 854e9f938aafe and see where does it fall - is it like
dc3cf789eb259 or like 3f99656f7bc2f?

I really don't get it...