Re: Q. hlist_bl_add_head_rcu() in d_alloc_parallel()

["J. R. Okajima" <hooanon05g@gmail.com>]

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?


J. R. Okajima


diff --git a/fs/dcache.c b/fs/dcache.c
index 76afffd..3a9ebbc 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2462,14 +2462,42 @@ static void d_wait_lookup(struct dentry *dentry)
 	}
 }
 
+/* tests for d_alloc_parallel() */
+static bool dap_test(struct dentry *dentry, const struct qstr *name,
+		     struct dentry *parent, bool do_unhashed_test)
+{
+	bool ret;
+
+	ret = false;
+	if (dentry->d_name.hash != name->hash)
+		goto out;
+	if (dentry->d_parent != parent)
+		goto out;
+	if (do_unhashed_test && d_unhashed(dentry))
+		goto out;
+	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))
+			goto out;
+	} else {
+		unsigned int len = name->len;
+		if (dentry->d_name.len != len)
+			goto out;
+		if (dentry_cmp(dentry, name->name, len))
+			goto out;
+	}
+	ret = true;
+
+out:
+	return ret;
+}
+
 struct dentry *d_alloc_parallel(struct dentry *parent,
 				const struct qstr *name,
 				wait_queue_head_t *wq)
 {
-	unsigned int len = name->len;
-	unsigned int hash = name->hash;
-	const unsigned char *str = name->name;
-	struct hlist_bl_head *b = in_lookup_hash(parent, hash);
+	struct hlist_bl_head *b = in_lookup_hash(parent, name->hash);
 	struct hlist_bl_node *node;
 	struct dentry *new = d_alloc(parent, name);
 	struct dentry *dentry;
@@ -2515,21 +2543,8 @@ retry:
 	 * we encounter.
 	 */
 	hlist_bl_for_each_entry(dentry, node, b, d_u.d_in_lookup_hash) {
-		if (dentry->d_name.hash != hash)
-			continue;
-		if (dentry->d_parent != parent)
+		if (!dap_test(dentry, name, parent, /*do_unhashed_test*/false))
 			continue;
-		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))
-				continue;
-		} else {
-			if (dentry->d_name.len != len)
-				continue;
-			if (dentry_cmp(dentry, str, len))
-				continue;
-		}
 		hlist_bl_unlock(b);
 		/* now we can try to grab a reference */
 		if (!lockref_get_not_dead(&dentry->d_lockref)) {
@@ -2550,23 +2565,9 @@ retry:
 		 * d_lookup() would've found anyway.  If it is, just return it;
 		 * otherwise we really have to repeat the whole thing.
 		 */
-		if (unlikely(dentry->d_name.hash != hash))
-			goto mismatch;
-		if (unlikely(dentry->d_parent != parent))
+		if (unlikely(!dap_test(dentry, name, parent,
+				       /*do_unhashed_test*/true)))
 			goto mismatch;
-		if (unlikely(d_unhashed(dentry)))
-			goto mismatch;
-		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))
-				goto mismatch;
-		} else {
-			if (unlikely(dentry->d_name.len != len))
-				goto mismatch;
-			if (unlikely(dentry_cmp(dentry, str, len)))
-				goto mismatch;
-		}
 		/* OK, it *is* a hashed match; return it */
 		spin_unlock(&dentry->d_lock);
 		dput(new);