Re: performance delta after VFS i_mutex=>i_rwsem conversion

[Linus Torvalds <torvalds@linux-foundation.org>]

On Mon, Jun 6, 2016 at 2:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, there's another fun issue on ramfs - dcache_readdir() is doing an
> obscene amount of grabbing/releasing ->d_lock

Yeah, I saw that report, and I'm not entirely surprised. There was no
real reason to worry about it before, since readdir() couldn't really
cause contention with the previous mutual exclusion at the upper
level.

> I think I have a kinda-sorta solution, but it has a problem.  What I want
> to do is
>         * list_move() only once per dcache_readdir()
>         * ->d_lock taken for that and only for that.
>         * list_move() itself surrounded with write_seqcount_{begin,end} on
> some seqcount
>         * traversal to the next real entry done under rcu_read_lock in a
> seqretry loop.
>
> The only problem is where to put that seqcount (unsigned int, really).
> ->i_dir_seq is an obvious candidate, but that'll need careful profiling
> on getdents/lookup mixes...

Let's look at smaller changes first.

In particular, why the f*ck do we take "next->d_lock" at all, much less twice?

Do we really need that? Both of them seem bogus:

 - the first one only protects the "simple_positive()" test.

    That seems stupid. We hold the parent d_lock _and_ we hold the
parent inode lock for reading, how the hell is simple_positive() going
to change? Yeah, yeah, *maybe* we could catch a lookup that just
happens to add the inode field in process, but it's not a race we even
care about. If we see the inode being non-NULL, it will now *stay*
non-NULL, and we already depend on that (that "d_inode(next)" is then
done without the lock held.

 - the second one only protects "list_move()" of the cursor. But since
it's the child list, the "next->d_lock" thing ends up being
irrelevant. It's the parent dentry lock we need to hold, nothing else.
Not the "next" one.

so I don't see the point of half the d_lock games we play.

And the thing about spinlock contention: having *nested* spinlocks be
contented turns contention into an exponential thing. I really suspect
that if we can just remove the nested spinlock, the dentry->d_lock
contention will go down by a huge amount, because then you completely
remove the "wait on lock while holding another lock" thing, which is
what tends to really suck.

So before you do something fancy, look at the simple parts. I think
getting rid of the nested d_lock might be really simple.

Maybe we might want to do some really careful

     inode = READ_ONCE(dentry->d_inode);

because we're reading the thing without locking, and maybe there's
something else subtle going on. But that really looks fairly
low-hanging, and it might make the problem go away in practice.

              Linus