Re: Q. hlist_bl_add_head_rcu() in d_alloc_parallel()

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

[Linus and George Cc'd since it's close to the area affected by hash
rework and friends]

On Thu, Jun 23, 2016 at 10:19:16AM +0900, J. R. Okajima wrote:
> 
> Al Viro:
> > That check is definitely bogus and I'm completely at loss as to WTF is it
> > doing there.  Thanks for catching that; this kind of idiotic braino can
> > escape notice when rereading the code again and again, unfortunately ;-/
> >
> > Fixed, will push to Linus tonight or tomorrow.
> 
> Thank you too for fixing.
> I've confirmed the patch was merged and it passed my local tests
> too. Happy.
> I have another and relatively less important suggestion. Since the two
> groups tests in the loop are very similar, how about extract them as a
> new single static function?
> Do you think it will invite a performance down?

We do have too many duplicates, especially if you count the related but not
identical ones as well.

1) __d_lookup():
	check hash
	grab d_lock to stabilize the rest
	check parent
	check if it's hashed
	check name/length
2) d_alloc_parallel(), search in in-lookup hash:
	hash/parent/name stable for everything in in-lookup hash, need no locks
	check hash
	check parent
	check name/length
3) d_alloc_parallel(), check after waiting:
	d_lock already held
	check hash
	check parent
	check if it's hashed
	check name/length
4) d_exact_alias():
	check hash
	check parent
	check name/length (simplified since at the moment it's only used for
filesystems that do not have ->d_compare()).

FWIW, now that I look at d_exact_alias()... the comment in there needs
an update; the code is correct, but only because we don't call
that thing for directories.  Which means that d_splice_alias()-induced
moves do not affect us, leaving only d_move(), which is always called
with parent locked exclusive.

Hell knows...  Order of checks can be delicate; out of those cases, (1) and (2)
are on the hottest paths.  We certainly can do this:

static always_inline bool d_same_name(const struct dentry *dentry,
				      const struct dentry *parent,
				      const struct qstr *name)
{
	if (parent->d_flags & DCACHE_OP_COMPARE) {
		int tlen = dentry->d_name.len;
		const char *tname = dentry->d_name.name;
		if (parent->d_op->d_compare(parent, dentry, tlen, tname, name))
			return false;
	} else {
		if (dentry->d_name.len != qstr->len)
			return false;
		if (dentry_cmp(dentry, qstr->name, qstr->len))
			return false;
	}
	return true;
}

then switch all four to this.  That would reduce the amount of boilerplate;
I would hesitate to replace always_inline with inline, since we don't want
(1) and (2) to become uninlined, but maybe even that doesn't matter all that
much - most of rejects will happen without looking at the names.  It really
needs profiling...