[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...
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
