Re: [RFC] parent in ->d_compare() arguments

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

On Fri, Jul 29, 2016 at 6:07 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         We are passing to ->d_compare() instances parent, dentry itself,
> and a consistent snapshot of its ->d_name.name and ->d_name.len.  In all
> but one instance (ncpfs one) the only thing we need the parent for is
> finding the superblock.  Which is available as dentry->d_sb.  ncpfs one
> is weird, but it actually wants parent's ->d_inode, so it has to be
> careful about the RCU case anyway.
>
>         Do you have any objections to trimming the arguments list?  I want
> to kill the 'parent' argument there and let ncpfs carefully walk
> dentry->d_parent->d_inode.

No objection. That's all the slowpath, and simplifying it (at the
expense of possibly having to make ncpfs slightly more expensive)
sounds fine.


> Another thing in the same area: __d_lookup_rcu() does
> hlist_bl_for_each_entry_rcu(dentry, node, b, d_hash) {
>                 unsigned seq;
>                 seq = raw_seqcount_begin(&dentry->d_seq);
>                 if (dentry->d_parent != parent)
>                         continue;
> and raw_seqcount_begin() contains smp_rmb().  Seeing that we hit ->d_parent
> mismatch often enough and that we are fine with false negatives anyway,
> let's turn that into
>                 if (dentry->d_parent != parent)
>                         continue;
>                 seq = raw_seqcount_begin(&dentry->d_seq);
>                 if (unlikely(dentry->d_parent != parent))
>                         continue;
> and cut down on the number of smp_rmb() per __d_lookup_rcu().

No. Let's not. "smp_rmb()" is completely free on x86 (ok, so it's a
instruction scheduling barrer - close enough), so trying to optimize
away rmb's and replacing them with double compares sounds entirely
misdesigned.

Yes, yes, there are other architectures where rmb is much more
expensive. But quite frankly, in most cases those architectures have
broken synchronization to begin with ("synchronization is unusual, so
let's not optimize it"). They'll fix it eventually.

Instead, what we should look at, is to make raw_seqcount_begin() use a
smp_load_acquire() on architectures where that is cheaper than the
rmb.

But again, I don't see the point of double-testing "parent" when a
load-acquire or load+rmb _should_ be cheap (and absolutely is on x86).

                Linus