On Tue, May 18, 2021 at 05:53:47PM +0000, Trond Myklebust wrote:

On Sat, 2021-05-15 at 08:02 +0100, Christoph Hellwig wrote:

quoted

On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:

quoted

Why leave the commit_metadata() call under the lock? If you're
concerned about latency, then it makes more sense to call
fh_unlock()
before flushing those metadata updates to disk.

Also I'm not sure why the extra inode reference is needed.  What
speaks
against just moving the dput out of the locked section?

Isn't the inode reference taken just in order to ensure that the call
to iput_final() (and in particular the call to
truncate_inode_pages_final()) is performed outside the lock?

The dput() is presumably usually not particularly expensive, since the
dentry is just a completely ordinary negative dentry at this point.

Right, but why bother with the extra ihold/iput instead of just
postponing the dput?

diff --git a/fs/namei.c b/fs/namei.c
index 79b0ff9b151e..aeed93c9874c 100644
--- a/fs/namei.c
+++ b/fs/namei.c

@@ -4084,7 +4084,6 @@ long do_unlinkat(int dfd, struct filename *name)
 		inode = dentry->d_inode;
 		if (d_is_negative(dentry))
 			goto slashes;
-		ihold(inode);
 		error = security_path_unlink(&path, dentry);
 		if (error)
 			goto exit2;

@@ -4092,11 +4091,10 @@ long do_unlinkat(int dfd, struct filename *name)
 		error = vfs_unlink(mnt_userns, path.dentry->d_inode, dentry,
 				   &delegated_inode);
 exit2:
-		dput(dentry);
 	}
 	inode_unlock(path.dentry->d_inode);
-	if (inode)
-		iput(inode);	/* truncate the inode here */
+	if (!IS_ERR(dentry))
+		dput(dentry);	/* truncate the inode here */
 	inode = NULL;
 	if (delegated_inode) {
 		error = break_deleg_wait(&delegated_inode);

?

--b.