Re: [RFC] weird stuff in exfat_lookup()

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

[with apologies for very late reply]

On Thu, Mar 13, 2025 at 09:39:39PM +0900, Sungjong Seo wrote:
> - thread1: while(1) { mkdir(A) and rmdir(A) }
> - thread2: while(1) { stat(A) }
> 
> This is due to the characteristics of exfat allowing negative dentry and
> considering CI in d_revalidate. As mentioned in the comment,
> unhashed-positive dentry can exist in a situation where mkdir
> and stat are competing, and it can be dropped, but exfat_lookup has
> been implemented to reuse(rehash) this dentry.

That's an interesting scenario, but I still don't see why would we bother.

Note that in your example we don't need to even look for aliases - it's
a directory inode, so d_splice_alias() would do the right thing, no matter
what.  And for non-directories you
	* already have d_move(alias, dentry) there, which would do the right
thing as well and
	* won't get an unhashed alias from d_find_alias() to start with.

Frankly, I would skip the entire "look for aliases" thing in case of
directory inodes - just let d_splice_alias() handle it.  That has
another fun benefit - exfat_d_anon_disconn() check becomes completely
pointless.  By the time we call it we have already verified that
alias->d_parent == dentry->d_parent, so the only way to get
exfat_d_anon_disconn(alias) to be true is to have IS_ROOT(alias),
i.e. alias->d_parent == alias and thus alias == dentry->d_parent
and at the very least the inode is a directory one.  We obvously
want to have it fail with -ELOOP in such case and d_splice_alias()
does just that, so if we bypass the entire "look for an alias"
thing for directories, the check becomes identical to
	if (alias && alias->d_parent == dentry->d_parent)
since the last term in the current variant (!exfat_d_anon_disconn(...))
can be dropped, along with the helper itself.

Does anybody have a problem with patch below?

[PATCH] simplify exfat_lookup()

1) d_splice_alias() handles ERR_PTR() for inode just fine
2) no need to even look for existing aliases in case of directory inodes;
just punt to d_splice_alias(), it'll do the right thing
3) no need to bother with 'd_unhashed(alias)' case - d_find_alias()
would've returned that only in case of a directory, and d_splice_alias()
will handle that just fine on its own.
4) exfat_d_anon_disconn() is entirely pointless now - we only get to
evaluating it in case dentry->d_parent == alias->d_parent and
alias being a non-directory.  But in that case IS_ROOT(alias) can't
possibly be true - that would've reqiured alias == alias->d_parent,
i.e alias == dentry->d_parent and dentry->d_parent is guaranteed to
be a directory.  So exfat_d_anon_disconn() would always return false
when it's called, which makes && !exfat_d_anon_disconn(alias)
a no-op.
    
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index 670116ae9ec8..8fac39f2bcb3 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -711,71 +711,44 @@ static int exfat_find(struct inode *dir, const struct qstr *qname,
 	return 0;
 }
 
-static int exfat_d_anon_disconn(struct dentry *dentry)
-{
-	return IS_ROOT(dentry) && (dentry->d_flags & DCACHE_DISCONNECTED);
-}
-
 static struct dentry *exfat_lookup(struct inode *dir, struct dentry *dentry,
 		unsigned int flags)
 {
 	struct super_block *sb = dir->i_sb;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct dentry *alias;
 	struct exfat_dir_entry info;
 	int err;
 	loff_t i_pos;
-	mode_t i_mode;
 
 	mutex_lock(&EXFAT_SB(sb)->s_lock);
 	err = exfat_find(dir, &dentry->d_name, &info);
 	if (err) {
-		if (err == -ENOENT) {
-			inode = NULL;
-			goto out;
-		}
-		goto unlock;
+		if (unlikely(err != -ENOENT))
+			inode = ERR_PTR(err);
+		goto out;
 	}
 
 	i_pos = exfat_make_i_pos(&info);
 	inode = exfat_build_inode(sb, &info, i_pos);
-	err = PTR_ERR_OR_ZERO(inode);
-	if (err)
-		goto unlock;
+	if (IS_ERR(inode) || S_ISDIR(inode->i_mode))
+		goto out;
 
-	i_mode = inode->i_mode;
 	alias = d_find_alias(inode);
 
 	/*
 	 * Checking "alias->d_parent == dentry->d_parent" to make sure
 	 * FS is not corrupted (especially double linked dir).
 	 */
-	if (alias && alias->d_parent == dentry->d_parent &&
-			!exfat_d_anon_disconn(alias)) {
-
+	if (alias && alias->d_parent == dentry->d_parent) {
 		/*
-		 * Unhashed alias is able to exist because of revalidate()
-		 * called by lookup_fast. You can easily make this status
-		 * by calling create and lookup concurrently
-		 * In such case, we reuse an alias instead of new dentry
+		 * This inode has non anonymous-DCACHE_DISCONNECTED
+		 * dentry. This means, the user did ->lookup() by an
+		 * another name (longname vs 8.3 alias of it) in past.
+		 *
+		 * Switch to new one for reason of locality if possible.
 		 */
-		if (d_unhashed(alias)) {
-			WARN_ON(alias->d_name.hash_len !=
-				dentry->d_name.hash_len);
-			exfat_info(sb, "rehashed a dentry(%p) in read lookup",
-				   alias);
-			d_drop(dentry);
-			d_rehash(alias);
-		} else if (!S_ISDIR(i_mode)) {
-			/*
-			 * This inode has non anonymous-DCACHE_DISCONNECTED
-			 * dentry. This means, the user did ->lookup() by an
-			 * another name (longname vs 8.3 alias of it) in past.
-			 *
-			 * Switch to new one for reason of locality if possible.
-			 */
-			d_move(alias, dentry);
-		}
+		d_move(alias, dentry);
 		iput(inode);
 		mutex_unlock(&EXFAT_SB(sb)->s_lock);
 		return alias;
@@ -787,9 +760,6 @@ static struct dentry *exfat_lookup(struct inode *dir, struct dentry *dentry,
 		exfat_d_version_set(dentry, inode_query_iversion(dir));
 
 	return d_splice_alias(inode, dentry);
-unlock:
-	mutex_unlock(&EXFAT_SB(sb)->s_lock);
-	return ERR_PTR(err);
 }
 
 /* remove an entry, BUT don't truncate */