From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:35434 "EHLO
	ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750721AbcFWC6N (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Wed, 22 Jun 2016 22:58:13 -0400
Date: Thu, 23 Jun 2016 03:58:09 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: "J. R. Okajima" <hooanon05g@gmail.com>
Cc: linux-fsdevel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	George Spelvin <linux@sciencehorizons.net>
Subject: Re: Q. hlist_bl_add_head_rcu() in d_alloc_parallel()
Message-ID: <20160623025809.GT14480@ZenIV.linux.org.uk>
References: <13136.1466196630@jrobl>
 <20160617221614.GE14480@ZenIV.linux.org.uk>
 <2123.1466313884@jrobl>
 <20160619165557.GH14480@ZenIV.linux.org.uk>
 <28627.1466397254@jrobl>
 <20160620053530.GI14480@ZenIV.linux.org.uk>
 <20287.1466644756@jrobl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20287.1466644756@jrobl>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

[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...