[PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[NeilBrown]

More excellent review feedback - more patches :-)

I've chosen to use ovl_parent_lock() here as a temporary and leave the
debate over naming for the VFS version of the function until all the new
names are introduced later.


Original description:

This series of patches for overlayfs is primarily focussed on preparing
for some proposed changes to directory locking.  In the new scheme we
will lock individual dentries in a directory rather than the whole
directory.

ovl currently will sometimes lock a directory on the upper filesystem
and do a few different things while holding the lock.  This is
incompatible with the new scheme.

This series narrows the region of code protected by the directory lock,
taking it multiple times when necessary.  This theoretically open up the
possibilty of other changes happening on the upper filesytem between the
unlock and the lock.  To some extent the patches guard against that by
checking the dentries still have the expect parent after retaking the
lock.  In general, I think ovl would have trouble if upperfs were being
changed independantly, and I don't think the changes here increase the
problem in any important way.

After this series (with any needed changes) lands I will resubmit my
change to vfs_rmdir() behaviour to have it drop the lock on error.  ovl
will be much better positioned to handle that change.  It will come with
the new "lookup_and_lock" API that I am proposing.

Thanks,
NeilBrown

 [PATCH v3 01/21] ovl: simplify an error path in ovl_copy_up_workdir()
 [PATCH v3 02/21] ovl: change ovl_create_index() to take dir locks
 [PATCH v3 03/21] ovl: Call ovl_create_temp() without lock held.
 [PATCH v3 04/21] ovl: narrow the locked region in
 [PATCH v3 05/21] ovl: narrow locking in ovl_create_upper()
 [PATCH v3 06/21] ovl: narrow locking in ovl_clear_empty()
 [PATCH v3 07/21] ovl: narrow locking in ovl_create_over_whiteout()
 [PATCH v3 08/21] ovl: simplify gotos in ovl_rename()
 [PATCH v3 09/21] ovl: narrow locking in ovl_rename()
 [PATCH v3 10/21] ovl: narrow locking in ovl_cleanup_whiteouts()
 [PATCH v3 11/21] ovl: narrow locking in ovl_cleanup_index()
 [PATCH v3 12/21] ovl: narrow locking in ovl_workdir_create()
 [PATCH v3 13/21] ovl: narrow locking in ovl_indexdir_cleanup()
 [PATCH v3 14/21] ovl: narrow locking in ovl_workdir_cleanup_recurse()
 [PATCH v3 15/21] ovl: change ovl_workdir_cleanup() to take dir lock
 [PATCH v3 16/21] ovl: narrow locking on ovl_remove_and_whiteout()
 [PATCH v3 17/21] ovl: change ovl_cleanup_and_whiteout() to take
 [PATCH v3 18/21] ovl: narrow locking in ovl_whiteout()
 [PATCH v3 19/21] ovl: narrow locking in ovl_check_rename_whiteout()
 [PATCH v3 20/21] ovl: change ovl_create_real() to receive dentry
 [PATCH v3 21/21] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()

[PATCH v3 01/21] ovl: simplify an error path in ovl_copy_up_workdir()

[NeilBrown]

If ovl_copy_up_data() fails the error is not immediately handled but the
code continues on to call ovl_start_write() and lock_rename(),
presumably because both of these locks are needed for the cleanup.
Only then (if the lock was successful) is the error checked.

This makes the code a little hard to follow and could be fragile.

This patch changes to handle the error after the ovl_start_write()
(which cannot fail, so there aren't multiple errors to deail with).  A
new ovl_cleanup_unlocked() is created which takes the required directory
lock.  This will be used extensively in later patches.

In general we need to check the parent is still correct after taking the
lock (as ovl_copy_up_workdir() does after a successful lock_rename()) so
that is included in ovl_cleanup_unlocked() using new ovl_parent_lock()
and ovl_parent_unlock() calls (it is planned to move this API into VFS code
eventually, though in a slightly different form).

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/copy_up.c   | 20 +++++++++++---------
 fs/overlayfs/dir.c       | 15 +++++++++++++++
 fs/overlayfs/overlayfs.h |  6 ++++++
 fs/overlayfs/util.c      | 10 ++++++++++
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8a3c0d18ec2e..79f41ef6ffa7 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -794,23 +794,24 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	 */
 	path.dentry = temp;
 	err = ovl_copy_up_data(c, &path);
+	ovl_start_write(c->dentry);
+	if (err)
+		goto cleanup_unlocked;
+
 	/*
 	 * We cannot hold lock_rename() throughout this helper, because of
 	 * lock ordering with sb_writers, which shouldn't be held when calling
 	 * ovl_copy_up_data(), so lock workdir and destdir and make sure that
 	 * temp wasn't moved before copy up completion or cleanup.
 	 */
-	ovl_start_write(c->dentry);
 	trap = lock_rename(c->workdir, c->destdir);
 	if (trap || temp->d_parent != c->workdir) {
 		/* temp or workdir moved underneath us? abort without cleanup */
 		dput(temp);
 		err = -EIO;
-		if (IS_ERR(trap))
-			goto out;
-		goto unlock;
-	} else if (err) {
-		goto cleanup;
+		if (!IS_ERR(trap))
+			unlock_rename(c->workdir, c->destdir);
+		goto out;
 	}
 
 	err = ovl_copy_up_metadata(c, temp);
@@ -846,7 +847,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	ovl_inode_update(inode, temp);
 	if (S_ISDIR(inode->i_mode))
 		ovl_set_flag(OVL_WHITEOUTS, inode);
-unlock:
 	unlock_rename(c->workdir, c->destdir);
 out:
 	ovl_end_write(c->dentry);
@@ -854,9 +854,11 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	return err;
 
 cleanup:
-	ovl_cleanup(ofs, wdir, temp);
+	unlock_rename(c->workdir, c->destdir);
+cleanup_unlocked:
+	ovl_cleanup_unlocked(ofs, c->workdir, temp);
 	dput(temp);
-	goto unlock;
+	goto out;
 }
 
 /* Copyup using O_TMPFILE which does not require cross dir locking */
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 4fc221ea6480..67cad3dba8ad 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -43,6 +43,21 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
 	return err;
 }
 
+int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
+			 struct dentry *wdentry)
+{
+	int err;
+
+	err = ovl_parent_lock(workdir, wdentry);
+	if (err)
+		return err;
+
+	ovl_cleanup(ofs, workdir->d_inode, wdentry);
+	ovl_parent_unlock(workdir);
+
+	return 0;
+}
+
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
 {
 	struct dentry *temp;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 42228d10f6b9..6737a4692eb2 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -416,6 +416,11 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
 }
 
 /* util.c */
+int ovl_parent_lock(struct dentry *parent, struct dentry *child);
+static inline void ovl_parent_unlock(struct dentry *parent)
+{
+	inode_unlock(parent->d_inode);
+}
 int ovl_get_write_access(struct dentry *dentry);
 void ovl_put_write_access(struct dentry *dentry);
 void ovl_start_write(struct dentry *dentry);
@@ -843,6 +848,7 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs,
 			       struct inode *dir, struct dentry *newdentry,
 			       struct ovl_cattr *attr);
 int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
+int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
 struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
 			       struct ovl_cattr *attr);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 2b4754c645ee..f4944f11d5eb 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1544,3 +1544,13 @@ void ovl_copyattr(struct inode *inode)
 	i_size_write(inode, i_size_read(realinode));
 	spin_unlock(&inode->i_lock);
 }
+
+int ovl_parent_lock(struct dentry *parent, struct dentry *child)
+{
+	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
+	if (!child || child->d_parent == parent)
+		return 0;
+
+	inode_unlock(parent->d_inode);
+	return -EINVAL;
+}
-- 
2.49.0

[PATCH v3 02/21] ovl: change ovl_create_index() to take dir locks

[NeilBrown]

ovl_copy_up_workdir() currently take a rename lock on two directories,
then use the lock to both create a file in one directory, perform a
rename, and possibly unlink the file for cleanup.  This is incompatible
with proposed changes which will lock just the dentry of objects being
acted on.

This patch moves the call to ovl_create_index() earlier in
ovl_copy_up_workdir() to before the lock is taken.

ovl_create_index() then takes the required lock only when needed.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/copy_up.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 79f41ef6ffa7..d485bd7edd8f 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -517,8 +517,6 @@ static int ovl_set_upper_fh(struct ovl_fs *ofs, struct dentry *upper,
 
 /*
  * Create and install index entry.
- *
- * Caller must hold i_mutex on indexdir.
  */
 static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 			    struct dentry *upper)
@@ -550,7 +548,9 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	if (err)
 		return err;
 
+	inode_lock(dir);
 	temp = ovl_create_temp(ofs, indexdir, OVL_CATTR(S_IFDIR | 0));
+	inode_unlock(dir);
 	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
 		goto free_name;
@@ -559,6 +559,9 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	if (err)
 		goto out;
 
+	err = ovl_parent_lock(indexdir, temp);
+	if (err)
+		goto out;
 	index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
 	if (IS_ERR(index)) {
 		err = PTR_ERR(index);
@@ -566,9 +569,10 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 		err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
 		dput(index);
 	}
+	ovl_parent_unlock(indexdir);
 out:
 	if (err)
-		ovl_cleanup(ofs, dir, temp);
+		ovl_cleanup_unlocked(ofs, indexdir, temp);
 	dput(temp);
 free_name:
 	kfree(name.name);
@@ -798,6 +802,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	if (err)
 		goto cleanup_unlocked;
 
+	if (S_ISDIR(c->stat.mode) && c->indexed) {
+		err = ovl_create_index(c->dentry, c->origin_fh, temp);
+		if (err)
+			goto cleanup_unlocked;
+	}
+
 	/*
 	 * We cannot hold lock_rename() throughout this helper, because of
 	 * lock ordering with sb_writers, which shouldn't be held when calling
@@ -818,12 +828,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	if (err)
 		goto cleanup;
 
-	if (S_ISDIR(c->stat.mode) && c->indexed) {
-		err = ovl_create_index(c->dentry, c->origin_fh, temp);
-		if (err)
-			goto cleanup;
-	}
-
 	upper = ovl_lookup_upper(ofs, c->destname.name, c->destdir,
 				 c->destname.len);
 	err = PTR_ERR(upper);
-- 
2.49.0

[PATCH v3 03/21] ovl: Call ovl_create_temp() without lock held.

[NeilBrown]

ovl currently locks a directory or two and then performs multiple actions
in one or both directories.  This is incompatible with proposed changes
which will lock just the dentry of objects being acted on.

This patch moves calls to ovl_create_temp() out of the locked regions and
has it take and release the relevant lock itself.

The lock that was taken before this function was called is now taken
after.  This means that any code between where the lock was taken and
ovl_create_temp() is now unlocked.  This necessitates the use of
ovl_cleanup_unlocked() and the creation of ovl_lookup_upper_unlocked().
These will be used more widely in future patches.

Now that the file is created before the lock is taken for rename, we
need to ensure the parent wasn't changed before the lock was gained.
ovl_lock_rename_workdir() is changed to optionally receive the dentries
that will be involved in the rename.  If either is present but has the
wrong parent, an error is returned.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/copy_up.c   |  6 -----
 fs/overlayfs/dir.c       | 54 +++++++++++++++++++++-------------------
 fs/overlayfs/overlayfs.h | 12 ++++++++-
 fs/overlayfs/super.c     | 11 +++++---
 fs/overlayfs/util.c      |  7 +++++-
 5 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d485bd7edd8f..fef873d18b2d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -523,7 +523,6 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 {
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
-	struct inode *dir = d_inode(indexdir);
 	struct dentry *index = NULL;
 	struct dentry *temp = NULL;
 	struct qstr name = { };
@@ -548,9 +547,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	if (err)
 		return err;
 
-	inode_lock(dir);
 	temp = ovl_create_temp(ofs, indexdir, OVL_CATTR(S_IFDIR | 0));
-	inode_unlock(dir);
 	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
 		goto free_name;
@@ -766,7 +763,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct inode *inode;
-	struct inode *wdir = d_inode(c->workdir);
 	struct path path = { .mnt = ovl_upper_mnt(ofs) };
 	struct dentry *temp, *upper, *trap;
 	struct ovl_cu_creds cc;
@@ -783,9 +779,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 		return err;
 
 	ovl_start_write(c->dentry);
-	inode_lock(wdir);
 	temp = ovl_create_temp(ofs, c->workdir, &cattr);
-	inode_unlock(wdir);
 	ovl_end_write(c->dentry);
 	ovl_revert_cu_creds(&cc);
 
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 67cad3dba8ad..373335e420fd 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -214,8 +214,12 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
 struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
 			       struct ovl_cattr *attr)
 {
-	return ovl_create_real(ofs, d_inode(workdir),
-			       ovl_lookup_temp(ofs, workdir), attr);
+	struct dentry *ret;
+	inode_lock(workdir->d_inode);
+	ret = ovl_create_real(ofs, d_inode(workdir),
+			      ovl_lookup_temp(ofs, workdir), attr);
+	inode_unlock(workdir->d_inode);
+	return ret;
 }
 
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
@@ -353,7 +357,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	struct dentry *workdir = ovl_workdir(dentry);
 	struct inode *wdir = workdir->d_inode;
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
-	struct inode *udir = upperdir->d_inode;
 	struct path upperpath;
 	struct dentry *upper;
 	struct dentry *opaquedir;
@@ -363,27 +366,25 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (WARN_ON(!workdir))
 		return ERR_PTR(-EROFS);
 
-	err = ovl_lock_rename_workdir(workdir, upperdir);
-	if (err)
-		goto out;
-
 	ovl_path_upper(dentry, &upperpath);
 	err = vfs_getattr(&upperpath, &stat,
 			  STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
 	if (err)
-		goto out_unlock;
+		goto out;
 
 	err = -ESTALE;
 	if (!S_ISDIR(stat.mode))
-		goto out_unlock;
+		goto out;
 	upper = upperpath.dentry;
-	if (upper->d_parent->d_inode != udir)
-		goto out_unlock;
 
 	opaquedir = ovl_create_temp(ofs, workdir, OVL_CATTR(stat.mode));
 	err = PTR_ERR(opaquedir);
 	if (IS_ERR(opaquedir))
-		goto out_unlock;
+		goto out;
+
+	err = ovl_lock_rename_workdir(workdir, opaquedir, upperdir, upper);
+	if (err)
+		goto out_cleanup_unlocked;
 
 	err = ovl_copy_xattr(dentry->d_sb, &upperpath, opaquedir);
 	if (err)
@@ -413,10 +414,10 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	return opaquedir;
 
 out_cleanup:
-	ovl_cleanup(ofs, wdir, opaquedir);
-	dput(opaquedir);
-out_unlock:
 	unlock_rename(workdir, upperdir);
+out_cleanup_unlocked:
+	ovl_cleanup_unlocked(ofs, workdir, opaquedir);
+	dput(opaquedir);
 out:
 	return ERR_PTR(err);
 }
@@ -454,15 +455,11 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 			return err;
 	}
 
-	err = ovl_lock_rename_workdir(workdir, upperdir);
-	if (err)
-		goto out;
-
-	upper = ovl_lookup_upper(ofs, dentry->d_name.name, upperdir,
-				 dentry->d_name.len);
+	upper = ovl_lookup_upper_unlocked(ofs, dentry->d_name.name, upperdir,
+					  dentry->d_name.len);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
-		goto out_unlock;
+		goto out;
 
 	err = -ESTALE;
 	if (d_is_negative(upper) || !ovl_upper_is_whiteout(ofs, upper))
@@ -473,6 +470,10 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(newdentry))
 		goto out_dput;
 
+	err = ovl_lock_rename_workdir(workdir, newdentry, upperdir, upper);
+	if (err)
+		goto out_cleanup_unlocked;
+
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
 	 */
@@ -523,10 +524,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		ovl_cleanup(ofs, udir, newdentry);
 		dput(newdentry);
 	}
+	unlock_rename(workdir, upperdir);
 out_dput:
 	dput(upper);
-out_unlock:
-	unlock_rename(workdir, upperdir);
 out:
 	if (!hardlink) {
 		posix_acl_release(acl);
@@ -535,7 +535,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	return err;
 
 out_cleanup:
-	ovl_cleanup(ofs, wdir, newdentry);
+	unlock_rename(workdir, upperdir);
+out_cleanup_unlocked:
+	ovl_cleanup_unlocked(ofs, workdir, newdentry);
 	dput(newdentry);
 	goto out_dput;
 }
@@ -772,7 +774,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 			goto out;
 	}
 
-	err = ovl_lock_rename_workdir(workdir, upperdir);
+	err = ovl_lock_rename_workdir(workdir, NULL, upperdir, NULL);
 	if (err)
 		goto out_dput;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6737a4692eb2..cff5bb625e9d 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -407,6 +407,15 @@ static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
 	return lookup_one(ovl_upper_mnt_idmap(ofs), &QSTR_LEN(name, len), base);
 }
 
+static inline struct dentry *ovl_lookup_upper_unlocked(struct ovl_fs *ofs,
+						       const char *name,
+						       struct dentry *base,
+						       int len)
+{
+	return lookup_one_unlocked(ovl_upper_mnt_idmap(ofs),
+				   &QSTR_LEN(name, len), base);
+}
+
 static inline bool ovl_open_flags_need_copy_up(int flags)
 {
 	if (!flags)
@@ -540,7 +549,8 @@ bool ovl_is_inuse(struct dentry *dentry);
 bool ovl_need_index(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry);
 void ovl_nlink_end(struct dentry *dentry);
-int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
+int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
+			    struct dentry *upperdir, struct dentry *upper);
 int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
 			     struct ovl_metacopy *data);
 int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index cf99b276fdfb..2e6b25bde83f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -564,13 +564,16 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 	struct name_snapshot name;
 	int err;
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-
 	temp = ovl_create_temp(ofs, workdir, OVL_CATTR(S_IFREG | 0));
 	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
-		goto out_unlock;
+		return err;
 
+	err = ovl_parent_lock(workdir, temp);
+	if (err) {
+		dput(temp);
+		return err;
+	}
 	dest = ovl_lookup_temp(ofs, workdir);
 	err = PTR_ERR(dest);
 	if (IS_ERR(dest)) {
@@ -606,7 +609,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 	dput(dest);
 
 out_unlock:
-	inode_unlock(dir);
+	ovl_parent_unlock(workdir);
 
 	return err;
 }
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f4944f11d5eb..fc229f5fb4e9 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1220,7 +1220,8 @@ void ovl_nlink_end(struct dentry *dentry)
 	ovl_inode_unlock(inode);
 }
 
-int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
+int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
+			    struct dentry *upperdir, struct dentry *upper)
 {
 	struct dentry *trap;
 
@@ -1234,6 +1235,10 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
 		goto err;
 	if (trap)
 		goto err_unlock;
+	if (work && work->d_parent != workdir)
+		goto err_unlock;
+	if (upper && upper->d_parent != upperdir)
+		goto err_unlock;
 
 	return 0;
 
-- 
2.49.0

[PATCH v3 04/21] ovl: narrow the locked region in ovl_copy_up_workdir()

[NeilBrown]

In ovl_copy_up_workdir() unlock immediately after the rename.  There is
nothing else in the function that needs the lock.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/copy_up.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index fef873d18b2d..8f8dbe8a1d54 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -829,9 +829,10 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 		goto cleanup;
 
 	err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
+	unlock_rename(c->workdir, c->destdir);
 	dput(upper);
 	if (err)
-		goto cleanup;
+		goto cleanup_unlocked;
 
 	inode = d_inode(c->dentry);
 	if (c->metacopy_digest)
@@ -845,7 +846,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	ovl_inode_update(inode, temp);
 	if (S_ISDIR(inode->i_mode))
 		ovl_set_flag(OVL_WHITEOUTS, inode);
-	unlock_rename(c->workdir, c->destdir);
 out:
 	ovl_end_write(c->dentry);
 
-- 
2.49.0

[PATCH v3 05/21] ovl: narrow locking in ovl_create_upper()

[NeilBrown]

Drop the directory lock immediately after the ovl_create_real() call and
take a separate lock later for cleanup in ovl_cleanup_unlocked() - if
needed.

This makes way for future changes where locks are taken on individual
dentries rather than the whole directory.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 373335e420fd..1a146a71993a 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -326,9 +326,9 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 				    ovl_lookup_upper(ofs, dentry->d_name.name,
 						     upperdir, dentry->d_name.len),
 				    attr);
-	err = PTR_ERR(newdentry);
+	inode_unlock(udir);
 	if (IS_ERR(newdentry))
-		goto out_unlock;
+		return PTR_ERR(newdentry);
 
 	if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry) &&
 	    !ovl_allow_offline_changes(ofs)) {
@@ -340,14 +340,12 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	err = ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink, NULL);
 	if (err)
 		goto out_cleanup;
-out_unlock:
-	inode_unlock(udir);
-	return err;
+	return 0;
 
 out_cleanup:
-	ovl_cleanup(ofs, udir, newdentry);
+	ovl_cleanup_unlocked(ofs, upperdir, newdentry);
 	dput(newdentry);
-	goto out_unlock;
+	return err;
 }
 
 static struct dentry *ovl_clear_empty(struct dentry *dentry,
-- 
2.49.0

[PATCH v3 06/21] ovl: narrow locking in ovl_clear_empty()

[NeilBrown]

Drop the locks immediately after rename, and use a separate lock for
cleanup.

This makes way for future changes where locks are taken on individual
dentries rather than the whole directory.

Note that ovl_cleanup_whiteouts() operates on "upper", a child of
"upperdir" and does not require upperdir or workdir to be locked.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 1a146a71993a..540b67f5cdf5 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -353,7 +353,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 {
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct dentry *workdir = ovl_workdir(dentry);
-	struct inode *wdir = workdir->d_inode;
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
 	struct path upperpath;
 	struct dentry *upper;
@@ -399,12 +398,12 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 		goto out_cleanup;
 
 	err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
+	unlock_rename(workdir, upperdir);
 	if (err)
-		goto out_cleanup;
+		goto out_cleanup_unlocked;
 
 	ovl_cleanup_whiteouts(ofs, upper, list);
-	ovl_cleanup(ofs, wdir, upper);
-	unlock_rename(workdir, upperdir);
+	ovl_cleanup_unlocked(ofs, workdir, upper);
 
 	/* dentry's upper doesn't match now, get rid of it */
 	d_drop(dentry);
-- 
2.49.0

[PATCH v3 07/21] ovl: narrow locking in ovl_create_over_whiteout()

[NeilBrown]

Unlock the parents immediately after the rename, and use
ovl_cleanup_unlocked() for cleanup, which takes a separate lock.

This makes way for future changes where locks are taken on individual
dentries rather than the whole directory.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 540b67f5cdf5..7c92ffb6e312 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -433,9 +433,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 {
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct dentry *workdir = ovl_workdir(dentry);
-	struct inode *wdir = workdir->d_inode;
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
-	struct inode *udir = upperdir->d_inode;
 	struct dentry *upper;
 	struct dentry *newdentry;
 	int err;
@@ -506,22 +504,23 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 
 		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
 				    RENAME_EXCHANGE);
+		unlock_rename(workdir, upperdir);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_unlocked;
 
-		ovl_cleanup(ofs, wdir, upper);
+		ovl_cleanup_unlocked(ofs, workdir, upper);
 	} else {
 		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
+		unlock_rename(workdir, upperdir);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_unlocked;
 	}
 	ovl_dir_modified(dentry->d_parent, false);
 	err = ovl_instantiate(dentry, inode, newdentry, hardlink, NULL);
 	if (err) {
-		ovl_cleanup(ofs, udir, newdentry);
+		ovl_cleanup_unlocked(ofs, upperdir, newdentry);
 		dput(newdentry);
 	}
-	unlock_rename(workdir, upperdir);
 out_dput:
 	dput(upper);
 out:
-- 
2.49.0

[PATCH v3 08/21] ovl: simplify gotos in ovl_rename()

[NeilBrown]

Rather than having three separate goto label: out_unlock, out_dput_old,
and out_dput, make use of that fact that dput() happily accepts a NULL
pointer to reduce this to just one goto label: out_unlock.

olddentry and newdentry are initialised to NULL and only set once a
value dentry is found.  They are then dput() late in the function.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c | 54 +++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 7c92ffb6e312..138dd85d2242 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1082,9 +1082,9 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	int err;
 	struct dentry *old_upperdir;
 	struct dentry *new_upperdir;
-	struct dentry *olddentry;
-	struct dentry *newdentry;
-	struct dentry *trap;
+	struct dentry *olddentry = NULL;
+	struct dentry *newdentry = NULL;
+	struct dentry *trap, *de;
 	bool old_opaque;
 	bool new_opaque;
 	bool cleanup_whiteout = false;
@@ -1197,21 +1197,23 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 		goto out_revert_creds;
 	}
 
-	olddentry = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
-				     old->d_name.len);
-	err = PTR_ERR(olddentry);
-	if (IS_ERR(olddentry))
+	de = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
+			      old->d_name.len);
+	err = PTR_ERR(de);
+	if (IS_ERR(de))
 		goto out_unlock;
+	olddentry = de;
 
 	err = -ESTALE;
 	if (!ovl_matches_upper(old, olddentry))
-		goto out_dput_old;
+		goto out_unlock;
 
-	newdentry = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
-				     new->d_name.len);
-	err = PTR_ERR(newdentry);
-	if (IS_ERR(newdentry))
-		goto out_dput_old;
+	de = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
+			      new->d_name.len);
+	err = PTR_ERR(de);
+	if (IS_ERR(de))
+		goto out_unlock;
+	newdentry = de;
 
 	old_opaque = ovl_dentry_is_opaque(old);
 	new_opaque = ovl_dentry_is_opaque(new);
@@ -1220,28 +1222,28 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	if (d_inode(new) && ovl_dentry_upper(new)) {
 		if (opaquedir) {
 			if (newdentry != opaquedir)
-				goto out_dput;
+				goto out_unlock;
 		} else {
 			if (!ovl_matches_upper(new, newdentry))
-				goto out_dput;
+				goto out_unlock;
 		}
 	} else {
 		if (!d_is_negative(newdentry)) {
 			if (!new_opaque || !ovl_upper_is_whiteout(ofs, newdentry))
-				goto out_dput;
+				goto out_unlock;
 		} else {
 			if (flags & RENAME_EXCHANGE)
-				goto out_dput;
+				goto out_unlock;
 		}
 	}
 
 	if (olddentry == trap)
-		goto out_dput;
+		goto out_unlock;
 	if (newdentry == trap)
-		goto out_dput;
+		goto out_unlock;
 
 	if (olddentry->d_inode == newdentry->d_inode)
-		goto out_dput;
+		goto out_unlock;
 
 	err = 0;
 	if (ovl_type_merge_or_lower(old))
@@ -1249,7 +1251,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	else if (is_dir && !old_opaque && ovl_type_merge(new->d_parent))
 		err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
 	if (err)
-		goto out_dput;
+		goto out_unlock;
 
 	if (!overwrite && ovl_type_merge_or_lower(new))
 		err = ovl_set_redirect(new, samedir);
@@ -1257,12 +1259,12 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 		 ovl_type_merge(old->d_parent))
 		err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
 	if (err)
-		goto out_dput;
+		goto out_unlock;
 
 	err = ovl_do_rename(ofs, old_upperdir, olddentry,
 			    new_upperdir, newdentry, flags);
 	if (err)
-		goto out_dput;
+		goto out_unlock;
 
 	if (cleanup_whiteout)
 		ovl_cleanup(ofs, old_upperdir->d_inode, newdentry);
@@ -1284,10 +1286,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	if (d_inode(new) && ovl_dentry_upper(new))
 		ovl_copyattr(d_inode(new));
 
-out_dput:
-	dput(newdentry);
-out_dput_old:
-	dput(olddentry);
 out_unlock:
 	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
@@ -1297,6 +1295,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	else
 		ovl_drop_write(old);
 out:
+	dput(newdentry);
+	dput(olddentry);
 	dput(opaquedir);
 	ovl_cache_free(&list);
 	return err;
-- 
2.49.0

[PATCH v3 09/21] ovl: narrow locking in ovl_rename()

[NeilBrown]

Drop the rename lock immediately after the rename, and use
ovl_cleanup_unlocked() for cleanup.

This makes way for future changes where locks are taken on individual
dentries rather than the whole directory.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 138dd85d2242..e81be60f1125 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1263,11 +1263,12 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 
 	err = ovl_do_rename(ofs, old_upperdir, olddentry,
 			    new_upperdir, newdentry, flags);
+	unlock_rename(new_upperdir, old_upperdir);
 	if (err)
-		goto out_unlock;
+		goto out_revert_creds;
 
 	if (cleanup_whiteout)
-		ovl_cleanup(ofs, old_upperdir->d_inode, newdentry);
+		ovl_cleanup_unlocked(ofs, old_upperdir, newdentry);
 
 	if (overwrite && d_inode(new)) {
 		if (new_is_dir)
@@ -1286,8 +1287,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	if (d_inode(new) && ovl_dentry_upper(new))
 		ovl_copyattr(d_inode(new));
 
-out_unlock:
-	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
 	ovl_revert_creds(old_cred);
 	if (update_nlink)
@@ -1300,6 +1299,10 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	dput(opaquedir);
 	ovl_cache_free(&list);
 	return err;
+
+out_unlock:
+	unlock_rename(new_upperdir, old_upperdir);
+	goto out_revert_creds;
 }
 
 static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
-- 
2.49.0

[PATCH v3 10/21] ovl: narrow locking in ovl_cleanup_whiteouts()

[NeilBrown]

Rather than lock the directory for the whole operation, use
ovl_lookup_upper_unlocked() and ovl_cleanup_unlocked() to take the lock
only when needed.

This makes way for future changes where locks are taken on individual
dentries rather than the whole directory.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/readdir.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 68cca52ae2ac..2a222b8185a3 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1034,14 +1034,13 @@ void ovl_cleanup_whiteouts(struct ovl_fs *ofs, struct dentry *upper,
 {
 	struct ovl_cache_entry *p;
 
-	inode_lock_nested(upper->d_inode, I_MUTEX_CHILD);
 	list_for_each_entry(p, list, l_node) {
 		struct dentry *dentry;
 
 		if (WARN_ON(!p->is_whiteout || !p->is_upper))
 			continue;
 
-		dentry = ovl_lookup_upper(ofs, p->name, upper, p->len);
+		dentry = ovl_lookup_upper_unlocked(ofs, p->name, upper, p->len);
 		if (IS_ERR(dentry)) {
 			pr_err("lookup '%s/%.*s' failed (%i)\n",
 			       upper->d_name.name, p->len, p->name,
@@ -1049,10 +1048,9 @@ void ovl_cleanup_whiteouts(struct ovl_fs *ofs, struct dentry *upper,
 			continue;
 		}
 		if (dentry->d_inode)
-			ovl_cleanup(ofs, upper->d_inode, dentry);
+			ovl_cleanup_unlocked(ofs, upper, dentry);
 		dput(dentry);
 	}
-	inode_unlock(upper->d_inode);
 }
 
 static bool ovl_check_d_type(struct dir_context *ctx, const char *name,
-- 
2.49.0

[PATCH v3 11/21] ovl: narrow locking in ovl_cleanup_index()

[NeilBrown]

ovl_cleanup_index() takes a lock on the directory and then does a lookup
and possibly one of two different cleanups.
This patch narrows the locking to use the _unlocked() versions of the
lookup and one cleanup, and just takes the lock for the other cleanup.

A subsequent patch will take the lock into the cleanup.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/util.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index fc229f5fb4e9..b06136bbe170 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1071,7 +1071,6 @@ static void ovl_cleanup_index(struct dentry *dentry)
 {
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
-	struct inode *dir = indexdir->d_inode;
 	struct dentry *lowerdentry = ovl_dentry_lower(dentry);
 	struct dentry *upperdentry = ovl_dentry_upper(dentry);
 	struct dentry *index = NULL;
@@ -1107,21 +1106,22 @@ static void ovl_cleanup_index(struct dentry *dentry)
 		goto out;
 	}
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-	index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
+	index = ovl_lookup_upper_unlocked(ofs, name.name, indexdir, name.len);
 	err = PTR_ERR(index);
 	if (IS_ERR(index)) {
 		index = NULL;
 	} else if (ovl_index_all(dentry->d_sb)) {
 		/* Whiteout orphan index to block future open by handle */
-		err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
-					       indexdir, index);
+		err = ovl_parent_lock(indexdir, index);
+		if (!err) {
+			err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
+						       indexdir, index);
+			ovl_parent_unlock(indexdir);
+		}
 	} else {
 		/* Cleanup orphan index entries */
-		err = ovl_cleanup(ofs, dir, index);
+		err = ovl_cleanup_unlocked(ofs, indexdir, index);
 	}
-
-	inode_unlock(dir);
 	if (err)
 		goto fail;
 
-- 
2.49.0

[PATCH v3 12/21] ovl: narrow locking in ovl_workdir_create()

[NeilBrown]

In ovl_workdir_create() don't hold the dir lock for the whole time, but
only take it when needed.

It now gets taken separately for ovl_workdir_cleanup().  A subsequent
patch will move the locking into that function.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/super.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 2e6b25bde83f..cb2551a155d8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -299,8 +299,8 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 	int err;
 	bool retried = false;
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
 retry:
+	inode_lock_nested(dir, I_MUTEX_PARENT);
 	work = ovl_lookup_upper(ofs, name, ofs->workbasedir, strlen(name));
 
 	if (!IS_ERR(work)) {
@@ -311,23 +311,28 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 
 		if (work->d_inode) {
 			err = -EEXIST;
+			inode_unlock(dir);
 			if (retried)
 				goto out_dput;
 
 			if (persist)
-				goto out_unlock;
+				return work;
 
 			retried = true;
-			err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
-			dput(work);
-			if (err == -EINVAL) {
-				work = ERR_PTR(err);
-				goto out_unlock;
+			err = ovl_parent_lock(ofs->workbasedir, work);
+			if (!err) {
+				err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
+				ovl_parent_unlock(ofs->workbasedir);
 			}
+			dput(work);
+			if (err == -EINVAL)
+				return ERR_PTR(err);
+
 			goto retry;
 		}
 
 		work = ovl_do_mkdir(ofs, dir, work, attr.ia_mode);
+		inode_unlock(dir);
 		err = PTR_ERR(work);
 		if (IS_ERR(work))
 			goto out_err;
@@ -365,11 +370,10 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 		if (err)
 			goto out_dput;
 	} else {
+		inode_unlock(dir);
 		err = PTR_ERR(work);
 		goto out_err;
 	}
-out_unlock:
-	inode_unlock(dir);
 	return work;
 
 out_dput:
@@ -377,8 +381,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 out_err:
 	pr_warn("failed to create directory %s/%s (errno: %i); mounting read-only\n",
 		ofs->config.workdir, name, -err);
-	work = NULL;
-	goto out_unlock;
+	return NULL;
 }
 
 static int ovl_check_namelen(const struct path *path, struct ovl_fs *ofs,
-- 
2.49.0

[PATCH v3 13/21] ovl: narrow locking in ovl_indexdir_cleanup()

[NeilBrown]

Instead of taking the directory lock for the whole cleanup, only take it
when needed.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/readdir.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 2a222b8185a3..95d5284daf8d 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1194,7 +1194,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 	if (err)
 		goto out;
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
 	list_for_each_entry(p, &list, l_node) {
 		if (p->name[0] == '.') {
 			if (p->len == 1)
@@ -1202,7 +1201,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			if (p->len == 2 && p->name[1] == '.')
 				continue;
 		}
-		index = ovl_lookup_upper(ofs, p->name, indexdir, p->len);
+		index = ovl_lookup_upper_unlocked(ofs, p->name, indexdir, p->len);
 		if (IS_ERR(index)) {
 			err = PTR_ERR(index);
 			index = NULL;
@@ -1210,7 +1209,11 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 		}
 		/* Cleanup leftover from index create/cleanup attempt */
 		if (index->d_name.name[0] == '#') {
-			err = ovl_workdir_cleanup(ofs, dir, path.mnt, index, 1);
+			err = ovl_parent_lock(indexdir, index);
+			if (!err) {
+				err = ovl_workdir_cleanup(ofs, dir, path.mnt, index, 1);
+				ovl_parent_unlock(indexdir);
+			}
 			if (err)
 				break;
 			goto next;
@@ -1220,7 +1223,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			goto next;
 		} else if (err == -ESTALE) {
 			/* Cleanup stale index entries */
-			err = ovl_cleanup(ofs, dir, index);
+			err = ovl_cleanup_unlocked(ofs, indexdir, index);
 		} else if (err != -ENOENT) {
 			/*
 			 * Abort mount to avoid corrupting the index if
@@ -1233,10 +1236,14 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			 * Whiteout orphan index to block future open by
 			 * handle after overlay nlink dropped to zero.
 			 */
-			err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
+			err = ovl_parent_lock(indexdir, index);
+			if (!err) {
+				err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
+				ovl_parent_unlock(indexdir);
+			}
 		} else {
 			/* Cleanup orphan index entries */
-			err = ovl_cleanup(ofs, dir, index);
+			err = ovl_cleanup_unlocked(ofs, indexdir, index);
 		}
 
 		if (err)
@@ -1247,7 +1254,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 		index = NULL;
 	}
 	dput(index);
-	inode_unlock(dir);
 out:
 	ovl_cache_free(&list);
 	if (err)
-- 
2.49.0

[PATCH v3 14/21] ovl: narrow locking in ovl_workdir_cleanup_recurse()

[NeilBrown]

Only take the dir lock when needed, rather than for the whole loop.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/readdir.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 95d5284daf8d..b0f9e5a00c1a 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1122,7 +1122,6 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 	if (err)
 		goto out;
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
 	list_for_each_entry(p, &list, l_node) {
 		struct dentry *dentry;
 
@@ -1137,16 +1136,21 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 			err = -EINVAL;
 			break;
 		}
-		dentry = ovl_lookup_upper(ofs, p->name, path->dentry, p->len);
+		dentry = ovl_lookup_upper_unlocked(ofs, p->name, path->dentry, p->len);
 		if (IS_ERR(dentry))
 			continue;
-		if (dentry->d_inode)
-			err = ovl_workdir_cleanup(ofs, dir, path->mnt, dentry, level);
+		if (dentry->d_inode) {
+			err = ovl_parent_lock(path->dentry, dentry);
+			if (!err) {
+				err = ovl_workdir_cleanup(ofs, dir, path->mnt,
+							  dentry, level);
+				ovl_parent_unlock(path->dentry);
+			}
+		}
 		dput(dentry);
 		if (err)
 			break;
 	}
-	inode_unlock(dir);
 out:
 	ovl_cache_free(&list);
 	return err;
-- 
2.49.0

[PATCH v3 15/21] ovl: change ovl_workdir_cleanup() to take dir lock as needed.

[NeilBrown]

Rather than calling ovl_workdir_cleanup() with the dir already locked,
change it to take the dir lock only when needed.

Also change ovl_workdir_cleanup() to take a dentry for the parent rather
than an inode.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/readdir.c   | 36 +++++++++++++-----------------------
 fs/overlayfs/super.c     |  6 +-----
 3 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index cff5bb625e9d..f6023442a45c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -738,7 +738,7 @@ void ovl_cleanup_whiteouts(struct ovl_fs *ofs, struct dentry *upper,
 void ovl_cache_free(struct list_head *list);
 void ovl_dir_cache_free(struct inode *inode);
 int ovl_check_d_type_supported(const struct path *realpath);
-int ovl_workdir_cleanup(struct ovl_fs *ofs, struct inode *dir,
+int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
 			struct vfsmount *mnt, struct dentry *dentry, int level);
 int ovl_indexdir_cleanup(struct ovl_fs *ofs);
 
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index b0f9e5a00c1a..e2d0c314df6c 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1096,7 +1096,6 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 				       int level)
 {
 	int err;
-	struct inode *dir = path->dentry->d_inode;
 	LIST_HEAD(list);
 	struct ovl_cache_entry *p;
 	struct ovl_readdir_data rdd = {
@@ -1139,14 +1138,9 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 		dentry = ovl_lookup_upper_unlocked(ofs, p->name, path->dentry, p->len);
 		if (IS_ERR(dentry))
 			continue;
-		if (dentry->d_inode) {
-			err = ovl_parent_lock(path->dentry, dentry);
-			if (!err) {
-				err = ovl_workdir_cleanup(ofs, dir, path->mnt,
-							  dentry, level);
-				ovl_parent_unlock(path->dentry);
-			}
-		}
+		if (dentry->d_inode)
+			err = ovl_workdir_cleanup(ofs, path->dentry, path->mnt,
+						  dentry, level);
 		dput(dentry);
 		if (err)
 			break;
@@ -1156,24 +1150,25 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 	return err;
 }
 
-int ovl_workdir_cleanup(struct ovl_fs *ofs, struct inode *dir,
+int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
 			struct vfsmount *mnt, struct dentry *dentry, int level)
 {
 	int err;
 
-	if (!d_is_dir(dentry) || level > 1) {
-		return ovl_cleanup(ofs, dir, dentry);
-	}
+	if (!d_is_dir(dentry) || level > 1)
+		return ovl_cleanup_unlocked(ofs, parent, dentry);
 
-	err = ovl_do_rmdir(ofs, dir, dentry);
+	err = ovl_parent_lock(parent, dentry);
+	if (err)
+		return err;
+	err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
+	ovl_parent_unlock(parent);
 	if (err) {
 		struct path path = { .mnt = mnt, .dentry = dentry };
 
-		inode_unlock(dir);
 		err = ovl_workdir_cleanup_recurse(ofs, &path, level + 1);
-		inode_lock_nested(dir, I_MUTEX_PARENT);
 		if (!err)
-			err = ovl_cleanup(ofs, dir, dentry);
+			err = ovl_cleanup_unlocked(ofs, parent, dentry);
 	}
 
 	return err;
@@ -1184,7 +1179,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 	int err;
 	struct dentry *indexdir = ofs->workdir;
 	struct dentry *index = NULL;
-	struct inode *dir = indexdir->d_inode;
 	struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = indexdir };
 	LIST_HEAD(list);
 	struct ovl_cache_entry *p;
@@ -1213,11 +1207,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 		}
 		/* Cleanup leftover from index create/cleanup attempt */
 		if (index->d_name.name[0] == '#') {
-			err = ovl_parent_lock(indexdir, index);
-			if (!err) {
-				err = ovl_workdir_cleanup(ofs, dir, path.mnt, index, 1);
-				ovl_parent_unlock(indexdir);
-			}
+			err = ovl_workdir_cleanup(ofs, indexdir, path.mnt, index, 1);
 			if (err)
 				break;
 			goto next;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index cb2551a155d8..4c3736bf2db4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -319,11 +319,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 				return work;
 
 			retried = true;
-			err = ovl_parent_lock(ofs->workbasedir, work);
-			if (!err) {
-				err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
-				ovl_parent_unlock(ofs->workbasedir);
-			}
+			err = ovl_workdir_cleanup(ofs, ofs->workbasedir, mnt, work, 0);
 			dput(work);
 			if (err == -EINVAL)
 				return ERR_PTR(err);
-- 
2.49.0

[PATCH v3 16/21] ovl: narrow locking on ovl_remove_and_whiteout()

[NeilBrown]

This code:
  performs a lookup_upper
  creates a whiteout object
  renames the whiteout over the result of the lookup

The create and the rename must be locked separately for proposed
directory locking changes.  This patch takes a first step of moving the
lookup out of the locked region.  A subsequent patch will separate the
create from the rename.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index e81be60f1125..340f8679b6e7 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -770,15 +770,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 			goto out;
 	}
 
-	err = ovl_lock_rename_workdir(workdir, NULL, upperdir, NULL);
-	if (err)
-		goto out_dput;
-
-	upper = ovl_lookup_upper(ofs, dentry->d_name.name, upperdir,
-				 dentry->d_name.len);
+	upper = ovl_lookup_upper_unlocked(ofs, dentry->d_name.name, upperdir,
+					  dentry->d_name.len);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
-		goto out_unlock;
+		goto out_dput;
 
 	err = -ESTALE;
 	if ((opaquedir && upper != opaquedir) ||
@@ -787,17 +783,18 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 		goto out_dput_upper;
 	}
 
-	err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
+	err = ovl_lock_rename_workdir(workdir, NULL, upperdir, upper);
 	if (err)
-		goto out_d_drop;
+		goto out_dput_upper;
+
+	err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
+	if (!err)
+		ovl_dir_modified(dentry->d_parent, true);
 
-	ovl_dir_modified(dentry->d_parent, true);
-out_d_drop:
 	d_drop(dentry);
+	unlock_rename(workdir, upperdir);
 out_dput_upper:
 	dput(upper);
-out_unlock:
-	unlock_rename(workdir, upperdir);
 out_dput:
 	dput(opaquedir);
 out:
-- 
2.49.0

[PATCH v3 17/21] ovl: change ovl_cleanup_and_whiteout() to take rename lock as needed

[NeilBrown]

Rather than locking the directory(s) before calling
ovl_cleanup_and_whiteout(), change it (and ovl_whiteout()) to do the
locking, so the locking can be fine grained as will be needed for
proposed locking changes.

Sometimes this is called to whiteout something in the index dir, in
which case only that dir must be locked.  In one case it is called on
something in an upperdir, so two directories must be locked.  We use
ovl_lock_rename_workdir() for this and remove the restriction that
upperdir cannot be indexdir - because now sometimes it is.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c     | 20 +++++++++-----------
 fs/overlayfs/readdir.c |  6 +-----
 fs/overlayfs/util.c    | 12 ++----------
 3 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 340f8679b6e7..6a70faeee6fa 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -77,7 +77,6 @@ struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
 	return temp;
 }
 
-/* caller holds i_mutex on workdir */
 static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 {
 	int err;
@@ -85,6 +84,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 	struct dentry *workdir = ofs->workdir;
 	struct inode *wdir = workdir->d_inode;
 
+	inode_lock_nested(wdir, I_MUTEX_PARENT);
 	if (!ofs->whiteout) {
 		whiteout = ovl_lookup_temp(ofs, workdir);
 		if (IS_ERR(whiteout))
@@ -118,14 +118,13 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 	whiteout = ofs->whiteout;
 	ofs->whiteout = NULL;
 out:
+	inode_unlock(wdir);
 	return whiteout;
 }
 
-/* Caller must hold i_mutex on both workdir and dir */
 int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
 			     struct dentry *dentry)
 {
-	struct inode *wdir = ofs->workdir->d_inode;
 	struct dentry *whiteout;
 	int err;
 	int flags = 0;
@@ -138,18 +137,22 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
 	if (d_is_dir(dentry))
 		flags = RENAME_EXCHANGE;
 
-	err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
+	err = ovl_lock_rename_workdir(ofs->workdir, whiteout, dir, dentry);
+	if (!err) {
+		err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
+		unlock_rename(ofs->workdir, dir);
+	}
 	if (err)
 		goto kill_whiteout;
 	if (flags)
-		ovl_cleanup(ofs, wdir, dentry);
+		ovl_cleanup_unlocked(ofs, ofs->workdir, dentry);
 
 out:
 	dput(whiteout);
 	return err;
 
 kill_whiteout:
-	ovl_cleanup(ofs, wdir, whiteout);
+	ovl_cleanup_unlocked(ofs, ofs->workdir, whiteout);
 	goto out;
 }
 
@@ -783,16 +786,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 		goto out_dput_upper;
 	}
 
-	err = ovl_lock_rename_workdir(workdir, NULL, upperdir, upper);
-	if (err)
-		goto out_dput_upper;
-
 	err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
 	if (!err)
 		ovl_dir_modified(dentry->d_parent, true);
 
 	d_drop(dentry);
-	unlock_rename(workdir, upperdir);
 out_dput_upper:
 	dput(upper);
 out_dput:
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index e2d0c314df6c..ecbf39a49d57 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1230,11 +1230,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			 * Whiteout orphan index to block future open by
 			 * handle after overlay nlink dropped to zero.
 			 */
-			err = ovl_parent_lock(indexdir, index);
-			if (!err) {
-				err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
-				ovl_parent_unlock(indexdir);
-			}
+			err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
 		} else {
 			/* Cleanup orphan index entries */
 			err = ovl_cleanup_unlocked(ofs, indexdir, index);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index b06136bbe170..3df0f3efe592 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1112,12 +1112,8 @@ static void ovl_cleanup_index(struct dentry *dentry)
 		index = NULL;
 	} else if (ovl_index_all(dentry->d_sb)) {
 		/* Whiteout orphan index to block future open by handle */
-		err = ovl_parent_lock(indexdir, index);
-		if (!err) {
-			err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
-						       indexdir, index);
-			ovl_parent_unlock(indexdir);
-		}
+		err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
+					       indexdir, index);
 	} else {
 		/* Cleanup orphan index entries */
 		err = ovl_cleanup_unlocked(ofs, indexdir, index);
@@ -1225,10 +1221,6 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
 {
 	struct dentry *trap;
 
-	/* Workdir should not be the same as upperdir */
-	if (workdir == upperdir)
-		goto err;
-
 	/* Workdir should not be subdir of upperdir and vice versa */
 	trap = lock_rename(workdir, upperdir);
 	if (IS_ERR(trap))
-- 
2.49.0

[PATCH v3 18/21] ovl: narrow locking in ovl_whiteout()

[NeilBrown]

ovl_whiteout() relies on the workdir i_rwsem to provide exclusive access
to ofs->whiteout which it manipulates.  Rather than depending on this,
add a new mutex, "whiteout_lock" to explicitly provide the required
locking.  Use guard(mutex) for this so that we can return without
needing to explicitly unlock.

Then take the lock on workdir only when needed - to lookup the temp name
and to do the whiteout or link.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c       | 44 ++++++++++++++++++++++------------------
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/params.c    |  2 ++
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 6a70faeee6fa..7eb806a4e5f8 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -84,41 +84,45 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 	struct dentry *workdir = ofs->workdir;
 	struct inode *wdir = workdir->d_inode;
 
-	inode_lock_nested(wdir, I_MUTEX_PARENT);
+	guard(mutex)(&ofs->whiteout_lock);
+
 	if (!ofs->whiteout) {
+		inode_lock_nested(wdir, I_MUTEX_PARENT);
 		whiteout = ovl_lookup_temp(ofs, workdir);
-		if (IS_ERR(whiteout))
-			goto out;
-
-		err = ovl_do_whiteout(ofs, wdir, whiteout);
-		if (err) {
-			dput(whiteout);
-			whiteout = ERR_PTR(err);
-			goto out;
+		if (!IS_ERR(whiteout)) {
+			err = ovl_do_whiteout(ofs, wdir, whiteout);
+			if (err) {
+				dput(whiteout);
+				whiteout = ERR_PTR(err);
+			}
 		}
+		inode_unlock(wdir);
+		if (IS_ERR(whiteout))
+			return whiteout;
 		ofs->whiteout = whiteout;
 	}
 
 	if (!ofs->no_shared_whiteout) {
+		inode_lock_nested(wdir, I_MUTEX_PARENT);
 		whiteout = ovl_lookup_temp(ofs, workdir);
-		if (IS_ERR(whiteout))
-			goto out;
-
-		err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
-		if (!err)
-			goto out;
-
-		if (err != -EMLINK) {
+		if (!IS_ERR(whiteout)) {
+			err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
+			if (err) {
+				dput(whiteout);
+				whiteout = ERR_PTR(err);
+			}
+		}
+		inode_unlock(wdir);
+		if (!IS_ERR(whiteout))
+			return whiteout;
+		if (PTR_ERR(whiteout) != -EMLINK) {
 			pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
 				ofs->whiteout->d_inode->i_nlink, err);
 			ofs->no_shared_whiteout = true;
 		}
-		dput(whiteout);
 	}
 	whiteout = ofs->whiteout;
 	ofs->whiteout = NULL;
-out:
-	inode_unlock(wdir);
 	return whiteout;
 }
 
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index afb7762f873f..4c1bae935ced 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -88,6 +88,7 @@ struct ovl_fs {
 	/* Shared whiteout cache */
 	struct dentry *whiteout;
 	bool no_shared_whiteout;
+	struct mutex whiteout_lock;
 	/* r/o snapshot of upperdir sb's only taken on volatile mounts */
 	errseq_t errseq;
 };
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index f42488c01957..cb1a17c066cd 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -797,6 +797,8 @@ int ovl_init_fs_context(struct fs_context *fc)
 	fc->s_fs_info		= ofs;
 	fc->fs_private		= ctx;
 	fc->ops			= &ovl_context_ops;
+
+	mutex_init(&ofs->whiteout_lock);
 	return 0;
 
 out_err:
-- 
2.49.0

[PATCH v3 19/21] ovl: narrow locking in ovl_check_rename_whiteout()

[NeilBrown]

ovl_check_rename_whiteout() now only holds the directory lock when
needed, and takes it again if necessary.

This makes way for future changes where locks are taken on individual
dentries rather than the whole directory.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/super.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4c3736bf2db4..0d765aa66bd2 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -556,7 +556,6 @@ static int ovl_get_upper(struct super_block *sb, struct ovl_fs *ofs,
 static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 {
 	struct dentry *workdir = ofs->workdir;
-	struct inode *dir = d_inode(workdir);
 	struct dentry *temp;
 	struct dentry *dest;
 	struct dentry *whiteout;
@@ -577,19 +576,22 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 	err = PTR_ERR(dest);
 	if (IS_ERR(dest)) {
 		dput(temp);
-		goto out_unlock;
+		ovl_parent_unlock(workdir);
+		return err;
 	}
 
 	/* Name is inline and stable - using snapshot as a copy helper */
 	take_dentry_name_snapshot(&name, temp);
 	err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
+	ovl_parent_unlock(workdir);
 	if (err) {
 		if (err == -EINVAL)
 			err = 0;
 		goto cleanup_temp;
 	}
 
-	whiteout = ovl_lookup_upper(ofs, name.name.name, workdir, name.name.len);
+	whiteout = ovl_lookup_upper_unlocked(ofs, name.name.name,
+					     workdir, name.name.len);
 	err = PTR_ERR(whiteout);
 	if (IS_ERR(whiteout))
 		goto cleanup_temp;
@@ -598,18 +600,15 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 
 	/* Best effort cleanup of whiteout and temp file */
 	if (err)
-		ovl_cleanup(ofs, dir, whiteout);
+		ovl_cleanup_unlocked(ofs, workdir, whiteout);
 	dput(whiteout);
 
 cleanup_temp:
-	ovl_cleanup(ofs, dir, temp);
+	ovl_cleanup_unlocked(ofs, workdir, temp);
 	release_dentry_name_snapshot(&name);
 	dput(temp);
 	dput(dest);
 
-out_unlock:
-	ovl_parent_unlock(workdir);
-
 	return err;
 }
 
-- 
2.49.0

[PATCH v3 20/21] ovl: change ovl_create_real() to receive dentry parent

[NeilBrown]

Instead of passing an inode *dir, pass a dentry *parent.  This makes the
calling slightly cleaner.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c       | 7 ++++---
 fs/overlayfs/overlayfs.h | 2 +-
 fs/overlayfs/super.c     | 3 +--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 7eb806a4e5f8..dedf89546e5f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -160,9 +160,10 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
 	goto out;
 }
 
-struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
+struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
 			       struct dentry *newdentry, struct ovl_cattr *attr)
 {
+	struct inode *dir = parent->d_inode;
 	int err;
 
 	if (IS_ERR(newdentry))
@@ -223,7 +224,7 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
 {
 	struct dentry *ret;
 	inode_lock(workdir->d_inode);
-	ret = ovl_create_real(ofs, d_inode(workdir),
+	ret = ovl_create_real(ofs, workdir,
 			      ovl_lookup_temp(ofs, workdir), attr);
 	inode_unlock(workdir->d_inode);
 	return ret;
@@ -329,7 +330,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	int err;
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
-	newdentry = ovl_create_real(ofs, udir,
+	newdentry = ovl_create_real(ofs, upperdir,
 				    ovl_lookup_upper(ofs, dentry->d_name.name,
 						     upperdir, dentry->d_name.len),
 				    attr);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f6023442a45c..b1e31e060157 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -855,7 +855,7 @@ struct ovl_cattr {
 #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
 
 struct dentry *ovl_create_real(struct ovl_fs *ofs,
-			       struct inode *dir, struct dentry *newdentry,
+			       struct dentry *parent, struct dentry *newdentry,
 			       struct ovl_cattr *attr);
 int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
 int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0d765aa66bd2..9fd0b3acd1a4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -622,8 +622,7 @@ static struct dentry *ovl_lookup_or_create(struct ovl_fs *ofs,
 	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
 	child = ovl_lookup_upper(ofs, name, parent, len);
 	if (!IS_ERR(child) && !child->d_inode)
-		child = ovl_create_real(ofs, parent->d_inode, child,
-					OVL_CATTR(mode));
+		child = ovl_create_real(ofs, parent, child, OVL_CATTR(mode));
 	inode_unlock(parent->d_inode);
 	dput(parent);
 
-- 
2.49.0

[PATCH v3 21/21] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()

[NeilBrown]

The only remaining user of ovl_cleanup() is ovl_cleanup_locked(), so we
no longer need both.

This patch renames ovl_cleanup() to ovl_cleanup_locked() and makes it
static.
ovl_cleanup_unlocked() is renamed to ovl_cleanup().

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/copy_up.c   |  4 ++--
 fs/overlayfs/dir.c       | 27 ++++++++++++++-------------
 fs/overlayfs/overlayfs.h |  3 +--
 fs/overlayfs/readdir.c   | 10 +++++-----
 fs/overlayfs/super.c     |  4 ++--
 fs/overlayfs/util.c      |  2 +-
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8f8dbe8a1d54..c4d7c281d473 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -569,7 +569,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	ovl_parent_unlock(indexdir);
 out:
 	if (err)
-		ovl_cleanup_unlocked(ofs, indexdir, temp);
+		ovl_cleanup(ofs, indexdir, temp);
 	dput(temp);
 free_name:
 	kfree(name.name);
@@ -854,7 +854,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 cleanup:
 	unlock_rename(c->workdir, c->destdir);
 cleanup_unlocked:
-	ovl_cleanup_unlocked(ofs, c->workdir, temp);
+	ovl_cleanup(ofs, c->workdir, temp);
 	dput(temp);
 	goto out;
 }
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index dedf89546e5f..30619777f0f6 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -24,7 +24,8 @@ MODULE_PARM_DESC(redirect_max,
 
 static int ovl_set_redirect(struct dentry *dentry, bool samedir);
 
-int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
+static int ovl_cleanup_locked(struct ovl_fs *ofs, struct inode *wdir,
+			      struct dentry *wdentry)
 {
 	int err;
 
@@ -43,8 +44,8 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
 	return err;
 }
 
-int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
-			 struct dentry *wdentry)
+int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
+		struct dentry *wdentry)
 {
 	int err;
 
@@ -52,7 +53,7 @@ int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
 	if (err)
 		return err;
 
-	ovl_cleanup(ofs, workdir->d_inode, wdentry);
+	ovl_cleanup_locked(ofs, workdir->d_inode, wdentry);
 	ovl_parent_unlock(workdir);
 
 	return 0;
@@ -149,14 +150,14 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
 	if (err)
 		goto kill_whiteout;
 	if (flags)
-		ovl_cleanup_unlocked(ofs, ofs->workdir, dentry);
+		ovl_cleanup(ofs, ofs->workdir, dentry);
 
 out:
 	dput(whiteout);
 	return err;
 
 kill_whiteout:
-	ovl_cleanup_unlocked(ofs, ofs->workdir, whiteout);
+	ovl_cleanup(ofs, ofs->workdir, whiteout);
 	goto out;
 }
 
@@ -351,7 +352,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	return 0;
 
 out_cleanup:
-	ovl_cleanup_unlocked(ofs, upperdir, newdentry);
+	ovl_cleanup(ofs, upperdir, newdentry);
 	dput(newdentry);
 	return err;
 }
@@ -411,7 +412,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 		goto out_cleanup_unlocked;
 
 	ovl_cleanup_whiteouts(ofs, upper, list);
-	ovl_cleanup_unlocked(ofs, workdir, upper);
+	ovl_cleanup(ofs, workdir, upper);
 
 	/* dentry's upper doesn't match now, get rid of it */
 	d_drop(dentry);
@@ -421,7 +422,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 out_cleanup:
 	unlock_rename(workdir, upperdir);
 out_cleanup_unlocked:
-	ovl_cleanup_unlocked(ofs, workdir, opaquedir);
+	ovl_cleanup(ofs, workdir, opaquedir);
 	dput(opaquedir);
 out:
 	return ERR_PTR(err);
@@ -516,7 +517,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup_unlocked;
 
-		ovl_cleanup_unlocked(ofs, workdir, upper);
+		ovl_cleanup(ofs, workdir, upper);
 	} else {
 		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
 		unlock_rename(workdir, upperdir);
@@ -526,7 +527,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	ovl_dir_modified(dentry->d_parent, false);
 	err = ovl_instantiate(dentry, inode, newdentry, hardlink, NULL);
 	if (err) {
-		ovl_cleanup_unlocked(ofs, upperdir, newdentry);
+		ovl_cleanup(ofs, upperdir, newdentry);
 		dput(newdentry);
 	}
 out_dput:
@@ -541,7 +542,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 out_cleanup:
 	unlock_rename(workdir, upperdir);
 out_cleanup_unlocked:
-	ovl_cleanup_unlocked(ofs, workdir, newdentry);
+	ovl_cleanup(ofs, workdir, newdentry);
 	dput(newdentry);
 	goto out_dput;
 }
@@ -1268,7 +1269,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 		goto out_revert_creds;
 
 	if (cleanup_whiteout)
-		ovl_cleanup_unlocked(ofs, old_upperdir, newdentry);
+		ovl_cleanup(ofs, old_upperdir, newdentry);
 
 	if (overwrite && d_inode(new)) {
 		if (new_is_dir)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index b1e31e060157..56de6e83d24e 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -857,8 +857,7 @@ struct ovl_cattr {
 struct dentry *ovl_create_real(struct ovl_fs *ofs,
 			       struct dentry *parent, struct dentry *newdentry,
 			       struct ovl_cattr *attr);
-int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
-int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
+int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
 struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
 			       struct ovl_cattr *attr);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index ecbf39a49d57..b65cdfce31ce 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1048,7 +1048,7 @@ void ovl_cleanup_whiteouts(struct ovl_fs *ofs, struct dentry *upper,
 			continue;
 		}
 		if (dentry->d_inode)
-			ovl_cleanup_unlocked(ofs, upper, dentry);
+			ovl_cleanup(ofs, upper, dentry);
 		dput(dentry);
 	}
 }
@@ -1156,7 +1156,7 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
 	int err;
 
 	if (!d_is_dir(dentry) || level > 1)
-		return ovl_cleanup_unlocked(ofs, parent, dentry);
+		return ovl_cleanup(ofs, parent, dentry);
 
 	err = ovl_parent_lock(parent, dentry);
 	if (err)
@@ -1168,7 +1168,7 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
 
 		err = ovl_workdir_cleanup_recurse(ofs, &path, level + 1);
 		if (!err)
-			err = ovl_cleanup_unlocked(ofs, parent, dentry);
+			err = ovl_cleanup(ofs, parent, dentry);
 	}
 
 	return err;
@@ -1217,7 +1217,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			goto next;
 		} else if (err == -ESTALE) {
 			/* Cleanup stale index entries */
-			err = ovl_cleanup_unlocked(ofs, indexdir, index);
+			err = ovl_cleanup(ofs, indexdir, index);
 		} else if (err != -ENOENT) {
 			/*
 			 * Abort mount to avoid corrupting the index if
@@ -1233,7 +1233,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
 		} else {
 			/* Cleanup orphan index entries */
-			err = ovl_cleanup_unlocked(ofs, indexdir, index);
+			err = ovl_cleanup(ofs, indexdir, index);
 		}
 
 		if (err)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 9fd0b3acd1a4..4afa91882075 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -600,11 +600,11 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 
 	/* Best effort cleanup of whiteout and temp file */
 	if (err)
-		ovl_cleanup_unlocked(ofs, workdir, whiteout);
+		ovl_cleanup(ofs, workdir, whiteout);
 	dput(whiteout);
 
 cleanup_temp:
-	ovl_cleanup_unlocked(ofs, workdir, temp);
+	ovl_cleanup(ofs, workdir, temp);
 	release_dentry_name_snapshot(&name);
 	dput(temp);
 	dput(dest);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 3df0f3efe592..62f809cf8ba9 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1116,7 +1116,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
 					       indexdir, index);
 	} else {
 		/* Cleanup orphan index entries */
-		err = ovl_cleanup_unlocked(ofs, indexdir, index);
+		err = ovl_cleanup(ofs, indexdir, index);
 	}
 	if (err)
 		goto fail;
-- 
2.49.0

Re: [PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> More excellent review feedback - more patches :-)
>
> I've chosen to use ovl_parent_lock() here as a temporary and leave the
> debate over naming for the VFS version of the function until all the new
> names are introduced later.

Perfect.

Please push v3 patches to branch pdirops, or to a clean branch
based on vfs-6.17.file, so I can test them.

Thanks,
Amir.


>
>
> Original description:
>
> This series of patches for overlayfs is primarily focussed on preparing
> for some proposed changes to directory locking.  In the new scheme we
> will lock individual dentries in a directory rather than the whole
> directory.
>
> ovl currently will sometimes lock a directory on the upper filesystem
> and do a few different things while holding the lock.  This is
> incompatible with the new scheme.
>
> This series narrows the region of code protected by the directory lock,
> taking it multiple times when necessary.  This theoretically open up the
> possibilty of other changes happening on the upper filesytem between the
> unlock and the lock.  To some extent the patches guard against that by
> checking the dentries still have the expect parent after retaking the
> lock.  In general, I think ovl would have trouble if upperfs were being
> changed independantly, and I don't think the changes here increase the
> problem in any important way.
>
> After this series (with any needed changes) lands I will resubmit my
> change to vfs_rmdir() behaviour to have it drop the lock on error.  ovl
> will be much better positioned to handle that change.  It will come with
> the new "lookup_and_lock" API that I am proposing.
>
> Thanks,
> NeilBrown
>
>  [PATCH v3 01/21] ovl: simplify an error path in ovl_copy_up_workdir()
>  [PATCH v3 02/21] ovl: change ovl_create_index() to take dir locks
>  [PATCH v3 03/21] ovl: Call ovl_create_temp() without lock held.
>  [PATCH v3 04/21] ovl: narrow the locked region in
>  [PATCH v3 05/21] ovl: narrow locking in ovl_create_upper()
>  [PATCH v3 06/21] ovl: narrow locking in ovl_clear_empty()
>  [PATCH v3 07/21] ovl: narrow locking in ovl_create_over_whiteout()
>  [PATCH v3 08/21] ovl: simplify gotos in ovl_rename()
>  [PATCH v3 09/21] ovl: narrow locking in ovl_rename()
>  [PATCH v3 10/21] ovl: narrow locking in ovl_cleanup_whiteouts()
>  [PATCH v3 11/21] ovl: narrow locking in ovl_cleanup_index()
>  [PATCH v3 12/21] ovl: narrow locking in ovl_workdir_create()
>  [PATCH v3 13/21] ovl: narrow locking in ovl_indexdir_cleanup()
>  [PATCH v3 14/21] ovl: narrow locking in ovl_workdir_cleanup_recurse()
>  [PATCH v3 15/21] ovl: change ovl_workdir_cleanup() to take dir lock
>  [PATCH v3 16/21] ovl: narrow locking on ovl_remove_and_whiteout()
>  [PATCH v3 17/21] ovl: change ovl_cleanup_and_whiteout() to take
>  [PATCH v3 18/21] ovl: narrow locking in ovl_whiteout()
>  [PATCH v3 19/21] ovl: narrow locking in ovl_check_rename_whiteout()
>  [PATCH v3 20/21] ovl: change ovl_create_real() to receive dentry
>  [PATCH v3 21/21] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()

Re: [PATCH v3 01/21] ovl: simplify an error path in ovl_copy_up_workdir()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> If ovl_copy_up_data() fails the error is not immediately handled but the
> code continues on to call ovl_start_write() and lock_rename(),
> presumably because both of these locks are needed for the cleanup.
> Only then (if the lock was successful) is the error checked.
>
> This makes the code a little hard to follow and could be fragile.
>
> This patch changes to handle the error after the ovl_start_write()
> (which cannot fail, so there aren't multiple errors to deail with).  A
> new ovl_cleanup_unlocked() is created which takes the required directory
> lock.  This will be used extensively in later patches.
>
> In general we need to check the parent is still correct after taking the
> lock (as ovl_copy_up_workdir() does after a successful lock_rename()) so
> that is included in ovl_cleanup_unlocked() using new ovl_parent_lock()
> and ovl_parent_unlock() calls (it is planned to move this API into VFS code
> eventually, though in a slightly different form).
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/copy_up.c   | 20 +++++++++++---------
>  fs/overlayfs/dir.c       | 15 +++++++++++++++
>  fs/overlayfs/overlayfs.h |  6 ++++++
>  fs/overlayfs/util.c      | 10 ++++++++++
>  4 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8a3c0d18ec2e..79f41ef6ffa7 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -794,23 +794,24 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>          */
>         path.dentry = temp;
>         err = ovl_copy_up_data(c, &path);
> +       ovl_start_write(c->dentry);
> +       if (err)
> +               goto cleanup_unlocked;
> +
>         /*
>          * We cannot hold lock_rename() throughout this helper, because of
>          * lock ordering with sb_writers, which shouldn't be held when calling
>          * ovl_copy_up_data(), so lock workdir and destdir and make sure that
>          * temp wasn't moved before copy up completion or cleanup.
>          */
> -       ovl_start_write(c->dentry);
>         trap = lock_rename(c->workdir, c->destdir);
>         if (trap || temp->d_parent != c->workdir) {
>                 /* temp or workdir moved underneath us? abort without cleanup */
>                 dput(temp);
>                 err = -EIO;
> -               if (IS_ERR(trap))
> -                       goto out;
> -               goto unlock;
> -       } else if (err) {
> -               goto cleanup;
> +               if (!IS_ERR(trap))
> +                       unlock_rename(c->workdir, c->destdir);
> +               goto out;
>         }
>
>         err = ovl_copy_up_metadata(c, temp);
> @@ -846,7 +847,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>         ovl_inode_update(inode, temp);
>         if (S_ISDIR(inode->i_mode))
>                 ovl_set_flag(OVL_WHITEOUTS, inode);
> -unlock:
>         unlock_rename(c->workdir, c->destdir);
>  out:
>         ovl_end_write(c->dentry);
> @@ -854,9 +854,11 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>         return err;
>
>  cleanup:
> -       ovl_cleanup(ofs, wdir, temp);
> +       unlock_rename(c->workdir, c->destdir);
> +cleanup_unlocked:
> +       ovl_cleanup_unlocked(ofs, c->workdir, temp);
>         dput(temp);
> -       goto unlock;
> +       goto out;
>  }
>
>  /* Copyup using O_TMPFILE which does not require cross dir locking */
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 4fc221ea6480..67cad3dba8ad 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -43,6 +43,21 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
>         return err;
>  }
>
> +int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
> +                        struct dentry *wdentry)
> +{
> +       int err;
> +
> +       err = ovl_parent_lock(workdir, wdentry);
> +       if (err)
> +               return err;
> +
> +       ovl_cleanup(ofs, workdir->d_inode, wdentry);
> +       ovl_parent_unlock(workdir);
> +
> +       return 0;
> +}
> +
>  struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
>  {
>         struct dentry *temp;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 42228d10f6b9..6737a4692eb2 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -416,6 +416,11 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
>  }
>
>  /* util.c */
> +int ovl_parent_lock(struct dentry *parent, struct dentry *child);
> +static inline void ovl_parent_unlock(struct dentry *parent)
> +{
> +       inode_unlock(parent->d_inode);
> +}
>  int ovl_get_write_access(struct dentry *dentry);
>  void ovl_put_write_access(struct dentry *dentry);
>  void ovl_start_write(struct dentry *dentry);
> @@ -843,6 +848,7 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs,
>                                struct inode *dir, struct dentry *newdentry,
>                                struct ovl_cattr *attr);
>  int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
> +int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
>  struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
>  struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
>                                struct ovl_cattr *attr);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 2b4754c645ee..f4944f11d5eb 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1544,3 +1544,13 @@ void ovl_copyattr(struct inode *inode)
>         i_size_write(inode, i_size_read(realinode));
>         spin_unlock(&inode->i_lock);
>  }
> +
> +int ovl_parent_lock(struct dentry *parent, struct dentry *child)
> +{
> +       inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> +       if (!child || child->d_parent == parent)
> +               return 0;
> +
> +       inode_unlock(parent->d_inode);
> +       return -EINVAL;
> +}
> --
> 2.49.0
>

Re: [PATCH v3 04/21] ovl: narrow the locked region in ovl_copy_up_workdir()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> In ovl_copy_up_workdir() unlock immediately after the rename.  There is
> nothing else in the function that needs the lock.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/copy_up.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index fef873d18b2d..8f8dbe8a1d54 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -829,9 +829,10 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>                 goto cleanup;
>
>         err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
> +       unlock_rename(c->workdir, c->destdir);
>         dput(upper);
>         if (err)
> -               goto cleanup;
> +               goto cleanup_unlocked;
>
>         inode = d_inode(c->dentry);
>         if (c->metacopy_digest)
> @@ -845,7 +846,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>         ovl_inode_update(inode, temp);
>         if (S_ISDIR(inode->i_mode))
>                 ovl_set_flag(OVL_WHITEOUTS, inode);
> -       unlock_rename(c->workdir, c->destdir);
>  out:
>         ovl_end_write(c->dentry);
>
> --
> 2.49.0
>

Re: [PATCH v3 08/21] ovl: simplify gotos in ovl_rename()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> Rather than having three separate goto label: out_unlock, out_dput_old,
> and out_dput, make use of that fact that dput() happily accepts a NULL
> pointer to reduce this to just one goto label: out_unlock.
>
> olddentry and newdentry are initialised to NULL and only set once a
> value dentry is found.  They are then dput() late in the function.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/dir.c | 54 +++++++++++++++++++++++-----------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 7c92ffb6e312..138dd85d2242 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1082,9 +1082,9 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         int err;
>         struct dentry *old_upperdir;
>         struct dentry *new_upperdir;
> -       struct dentry *olddentry;
> -       struct dentry *newdentry;
> -       struct dentry *trap;
> +       struct dentry *olddentry = NULL;
> +       struct dentry *newdentry = NULL;
> +       struct dentry *trap, *de;
>         bool old_opaque;
>         bool new_opaque;
>         bool cleanup_whiteout = false;
> @@ -1197,21 +1197,23 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>                 goto out_revert_creds;
>         }
>
> -       olddentry = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
> -                                    old->d_name.len);
> -       err = PTR_ERR(olddentry);
> -       if (IS_ERR(olddentry))
> +       de = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
> +                             old->d_name.len);
> +       err = PTR_ERR(de);
> +       if (IS_ERR(de))
>                 goto out_unlock;
> +       olddentry = de;
>
>         err = -ESTALE;
>         if (!ovl_matches_upper(old, olddentry))
> -               goto out_dput_old;
> +               goto out_unlock;
>
> -       newdentry = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
> -                                    new->d_name.len);
> -       err = PTR_ERR(newdentry);
> -       if (IS_ERR(newdentry))
> -               goto out_dput_old;
> +       de = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
> +                             new->d_name.len);
> +       err = PTR_ERR(de);
> +       if (IS_ERR(de))
> +               goto out_unlock;
> +       newdentry = de;
>
>         old_opaque = ovl_dentry_is_opaque(old);
>         new_opaque = ovl_dentry_is_opaque(new);
> @@ -1220,28 +1222,28 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         if (d_inode(new) && ovl_dentry_upper(new)) {
>                 if (opaquedir) {
>                         if (newdentry != opaquedir)
> -                               goto out_dput;
> +                               goto out_unlock;
>                 } else {
>                         if (!ovl_matches_upper(new, newdentry))
> -                               goto out_dput;
> +                               goto out_unlock;
>                 }
>         } else {
>                 if (!d_is_negative(newdentry)) {
>                         if (!new_opaque || !ovl_upper_is_whiteout(ofs, newdentry))
> -                               goto out_dput;
> +                               goto out_unlock;
>                 } else {
>                         if (flags & RENAME_EXCHANGE)
> -                               goto out_dput;
> +                               goto out_unlock;
>                 }
>         }
>
>         if (olddentry == trap)
> -               goto out_dput;
> +               goto out_unlock;
>         if (newdentry == trap)
> -               goto out_dput;
> +               goto out_unlock;
>
>         if (olddentry->d_inode == newdentry->d_inode)
> -               goto out_dput;
> +               goto out_unlock;
>
>         err = 0;
>         if (ovl_type_merge_or_lower(old))
> @@ -1249,7 +1251,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         else if (is_dir && !old_opaque && ovl_type_merge(new->d_parent))
>                 err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
>         if (err)
> -               goto out_dput;
> +               goto out_unlock;
>
>         if (!overwrite && ovl_type_merge_or_lower(new))
>                 err = ovl_set_redirect(new, samedir);
> @@ -1257,12 +1259,12 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>                  ovl_type_merge(old->d_parent))
>                 err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
>         if (err)
> -               goto out_dput;
> +               goto out_unlock;
>
>         err = ovl_do_rename(ofs, old_upperdir, olddentry,
>                             new_upperdir, newdentry, flags);
>         if (err)
> -               goto out_dput;
> +               goto out_unlock;
>
>         if (cleanup_whiteout)
>                 ovl_cleanup(ofs, old_upperdir->d_inode, newdentry);
> @@ -1284,10 +1286,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         if (d_inode(new) && ovl_dentry_upper(new))
>                 ovl_copyattr(d_inode(new));
>
> -out_dput:
> -       dput(newdentry);
> -out_dput_old:
> -       dput(olddentry);
>  out_unlock:
>         unlock_rename(new_upperdir, old_upperdir);
>  out_revert_creds:
> @@ -1297,6 +1295,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         else
>                 ovl_drop_write(old);
>  out:
> +       dput(newdentry);
> +       dput(olddentry);
>         dput(opaquedir);
>         ovl_cache_free(&list);
>         return err;
> --
> 2.49.0
>

Re: [PATCH v3 09/21] ovl: narrow locking in ovl_rename()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> Drop the rename lock immediately after the rename, and use
> ovl_cleanup_unlocked() for cleanup.
>
> This makes way for future changes where locks are taken on individual
> dentries rather than the whole directory.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/dir.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 138dd85d2242..e81be60f1125 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1263,11 +1263,12 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>
>         err = ovl_do_rename(ofs, old_upperdir, olddentry,
>                             new_upperdir, newdentry, flags);
> +       unlock_rename(new_upperdir, old_upperdir);
>         if (err)
> -               goto out_unlock;
> +               goto out_revert_creds;
>
>         if (cleanup_whiteout)
> -               ovl_cleanup(ofs, old_upperdir->d_inode, newdentry);
> +               ovl_cleanup_unlocked(ofs, old_upperdir, newdentry);
>
>         if (overwrite && d_inode(new)) {
>                 if (new_is_dir)
> @@ -1286,8 +1287,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         if (d_inode(new) && ovl_dentry_upper(new))
>                 ovl_copyattr(d_inode(new));
>
> -out_unlock:
> -       unlock_rename(new_upperdir, old_upperdir);
>  out_revert_creds:
>         ovl_revert_creds(old_cred);
>         if (update_nlink)
> @@ -1300,6 +1299,10 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         dput(opaquedir);
>         ovl_cache_free(&list);
>         return err;
> +
> +out_unlock:
> +       unlock_rename(new_upperdir, old_upperdir);
> +       goto out_revert_creds;
>  }
>
>  static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
> --
> 2.49.0
>

Re: [PATCH v3 11/21] ovl: narrow locking in ovl_cleanup_index()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> ovl_cleanup_index() takes a lock on the directory and then does a lookup
> and possibly one of two different cleanups.
> This patch narrows the locking to use the _unlocked() versions of the
> lookup and one cleanup, and just takes the lock for the other cleanup.
>
> A subsequent patch will take the lock into the cleanup.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/util.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index fc229f5fb4e9..b06136bbe170 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1071,7 +1071,6 @@ static void ovl_cleanup_index(struct dentry *dentry)
>  {
>         struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>         struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
> -       struct inode *dir = indexdir->d_inode;
>         struct dentry *lowerdentry = ovl_dentry_lower(dentry);
>         struct dentry *upperdentry = ovl_dentry_upper(dentry);
>         struct dentry *index = NULL;
> @@ -1107,21 +1106,22 @@ static void ovl_cleanup_index(struct dentry *dentry)
>                 goto out;
>         }
>
> -       inode_lock_nested(dir, I_MUTEX_PARENT);
> -       index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> +       index = ovl_lookup_upper_unlocked(ofs, name.name, indexdir, name.len);
>         err = PTR_ERR(index);
>         if (IS_ERR(index)) {
>                 index = NULL;
>         } else if (ovl_index_all(dentry->d_sb)) {
>                 /* Whiteout orphan index to block future open by handle */
> -               err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> -                                              indexdir, index);
> +               err = ovl_parent_lock(indexdir, index);
> +               if (!err) {
> +                       err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> +                                                      indexdir, index);
> +                       ovl_parent_unlock(indexdir);
> +               }
>         } else {
>                 /* Cleanup orphan index entries */
> -               err = ovl_cleanup(ofs, dir, index);
> +               err = ovl_cleanup_unlocked(ofs, indexdir, index);
>         }
> -
> -       inode_unlock(dir);
>         if (err)
>                 goto fail;
>
> --
> 2.49.0
>

Re: [PATCH v3 21/21] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> The only remaining user of ovl_cleanup() is ovl_cleanup_locked(), so we
> no longer need both.
>
> This patch renames ovl_cleanup() to ovl_cleanup_locked() and makes it
> static.
> ovl_cleanup_unlocked() is renamed to ovl_cleanup().
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/copy_up.c   |  4 ++--
>  fs/overlayfs/dir.c       | 27 ++++++++++++++-------------
>  fs/overlayfs/overlayfs.h |  3 +--
>  fs/overlayfs/readdir.c   | 10 +++++-----
>  fs/overlayfs/super.c     |  4 ++--
>  fs/overlayfs/util.c      |  2 +-
>  6 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8f8dbe8a1d54..c4d7c281d473 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -569,7 +569,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
>         ovl_parent_unlock(indexdir);
>  out:
>         if (err)
> -               ovl_cleanup_unlocked(ofs, indexdir, temp);
> +               ovl_cleanup(ofs, indexdir, temp);
>         dput(temp);
>  free_name:
>         kfree(name.name);
> @@ -854,7 +854,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>  cleanup:
>         unlock_rename(c->workdir, c->destdir);
>  cleanup_unlocked:
> -       ovl_cleanup_unlocked(ofs, c->workdir, temp);
> +       ovl_cleanup(ofs, c->workdir, temp);
>         dput(temp);
>         goto out;
>  }
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index dedf89546e5f..30619777f0f6 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -24,7 +24,8 @@ MODULE_PARM_DESC(redirect_max,
>
>  static int ovl_set_redirect(struct dentry *dentry, bool samedir);
>
> -int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
> +static int ovl_cleanup_locked(struct ovl_fs *ofs, struct inode *wdir,
> +                             struct dentry *wdentry)
>  {
>         int err;
>
> @@ -43,8 +44,8 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
>         return err;
>  }
>
> -int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
> -                        struct dentry *wdentry)
> +int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
> +               struct dentry *wdentry)
>  {
>         int err;
>
> @@ -52,7 +53,7 @@ int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
>         if (err)
>                 return err;
>
> -       ovl_cleanup(ofs, workdir->d_inode, wdentry);
> +       ovl_cleanup_locked(ofs, workdir->d_inode, wdentry);
>         ovl_parent_unlock(workdir);
>
>         return 0;
> @@ -149,14 +150,14 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
>         if (err)
>                 goto kill_whiteout;
>         if (flags)
> -               ovl_cleanup_unlocked(ofs, ofs->workdir, dentry);
> +               ovl_cleanup(ofs, ofs->workdir, dentry);
>
>  out:
>         dput(whiteout);
>         return err;
>
>  kill_whiteout:
> -       ovl_cleanup_unlocked(ofs, ofs->workdir, whiteout);
> +       ovl_cleanup(ofs, ofs->workdir, whiteout);
>         goto out;
>  }
>
> @@ -351,7 +352,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>         return 0;
>
>  out_cleanup:
> -       ovl_cleanup_unlocked(ofs, upperdir, newdentry);
> +       ovl_cleanup(ofs, upperdir, newdentry);
>         dput(newdentry);
>         return err;
>  }
> @@ -411,7 +412,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>                 goto out_cleanup_unlocked;
>
>         ovl_cleanup_whiteouts(ofs, upper, list);
> -       ovl_cleanup_unlocked(ofs, workdir, upper);
> +       ovl_cleanup(ofs, workdir, upper);
>
>         /* dentry's upper doesn't match now, get rid of it */
>         d_drop(dentry);
> @@ -421,7 +422,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>  out_cleanup:
>         unlock_rename(workdir, upperdir);
>  out_cleanup_unlocked:
> -       ovl_cleanup_unlocked(ofs, workdir, opaquedir);
> +       ovl_cleanup(ofs, workdir, opaquedir);
>         dput(opaquedir);
>  out:
>         return ERR_PTR(err);
> @@ -516,7 +517,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>                 if (err)
>                         goto out_cleanup_unlocked;
>
> -               ovl_cleanup_unlocked(ofs, workdir, upper);
> +               ovl_cleanup(ofs, workdir, upper);
>         } else {
>                 err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
>                 unlock_rename(workdir, upperdir);
> @@ -526,7 +527,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         ovl_dir_modified(dentry->d_parent, false);
>         err = ovl_instantiate(dentry, inode, newdentry, hardlink, NULL);
>         if (err) {
> -               ovl_cleanup_unlocked(ofs, upperdir, newdentry);
> +               ovl_cleanup(ofs, upperdir, newdentry);
>                 dput(newdentry);
>         }
>  out_dput:
> @@ -541,7 +542,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  out_cleanup:
>         unlock_rename(workdir, upperdir);
>  out_cleanup_unlocked:
> -       ovl_cleanup_unlocked(ofs, workdir, newdentry);
> +       ovl_cleanup(ofs, workdir, newdentry);
>         dput(newdentry);
>         goto out_dput;
>  }
> @@ -1268,7 +1269,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>                 goto out_revert_creds;
>
>         if (cleanup_whiteout)
> -               ovl_cleanup_unlocked(ofs, old_upperdir, newdentry);
> +               ovl_cleanup(ofs, old_upperdir, newdentry);
>
>         if (overwrite && d_inode(new)) {
>                 if (new_is_dir)
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index b1e31e060157..56de6e83d24e 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -857,8 +857,7 @@ struct ovl_cattr {
>  struct dentry *ovl_create_real(struct ovl_fs *ofs,
>                                struct dentry *parent, struct dentry *newdentry,
>                                struct ovl_cattr *attr);
> -int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
> -int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
> +int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
>  struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
>  struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
>                                struct ovl_cattr *attr);
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index ecbf39a49d57..b65cdfce31ce 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1048,7 +1048,7 @@ void ovl_cleanup_whiteouts(struct ovl_fs *ofs, struct dentry *upper,
>                         continue;
>                 }
>                 if (dentry->d_inode)
> -                       ovl_cleanup_unlocked(ofs, upper, dentry);
> +                       ovl_cleanup(ofs, upper, dentry);
>                 dput(dentry);
>         }
>  }
> @@ -1156,7 +1156,7 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
>         int err;
>
>         if (!d_is_dir(dentry) || level > 1)
> -               return ovl_cleanup_unlocked(ofs, parent, dentry);
> +               return ovl_cleanup(ofs, parent, dentry);
>
>         err = ovl_parent_lock(parent, dentry);
>         if (err)
> @@ -1168,7 +1168,7 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
>
>                 err = ovl_workdir_cleanup_recurse(ofs, &path, level + 1);
>                 if (!err)
> -                       err = ovl_cleanup_unlocked(ofs, parent, dentry);
> +                       err = ovl_cleanup(ofs, parent, dentry);
>         }
>
>         return err;
> @@ -1217,7 +1217,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                         goto next;
>                 } else if (err == -ESTALE) {
>                         /* Cleanup stale index entries */
> -                       err = ovl_cleanup_unlocked(ofs, indexdir, index);
> +                       err = ovl_cleanup(ofs, indexdir, index);
>                 } else if (err != -ENOENT) {
>                         /*
>                          * Abort mount to avoid corrupting the index if
> @@ -1233,7 +1233,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                         err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
>                 } else {
>                         /* Cleanup orphan index entries */
> -                       err = ovl_cleanup_unlocked(ofs, indexdir, index);
> +                       err = ovl_cleanup(ofs, indexdir, index);
>                 }
>
>                 if (err)
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 9fd0b3acd1a4..4afa91882075 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -600,11 +600,11 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>
>         /* Best effort cleanup of whiteout and temp file */
>         if (err)
> -               ovl_cleanup_unlocked(ofs, workdir, whiteout);
> +               ovl_cleanup(ofs, workdir, whiteout);
>         dput(whiteout);
>
>  cleanup_temp:
> -       ovl_cleanup_unlocked(ofs, workdir, temp);
> +       ovl_cleanup(ofs, workdir, temp);
>         release_dentry_name_snapshot(&name);
>         dput(temp);
>         dput(dest);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 3df0f3efe592..62f809cf8ba9 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1116,7 +1116,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
>                                                indexdir, index);
>         } else {
>                 /* Cleanup orphan index entries */
> -               err = ovl_cleanup_unlocked(ofs, indexdir, index);
> +               err = ovl_cleanup(ofs, indexdir, index);
>         }
>         if (err)
>                 goto fail;
> --
> 2.49.0
>

Re: [PATCH v3 18/21] ovl: narrow locking in ovl_whiteout()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> ovl_whiteout() relies on the workdir i_rwsem to provide exclusive access
> to ofs->whiteout which it manipulates.  Rather than depending on this,
> add a new mutex, "whiteout_lock" to explicitly provide the required
> locking.  Use guard(mutex) for this so that we can return without
> needing to explicitly unlock.
>
> Then take the lock on workdir only when needed - to lookup the temp name
> and to do the whiteout or link.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/dir.c       | 44 ++++++++++++++++++++++------------------
>  fs/overlayfs/ovl_entry.h |  1 +
>  fs/overlayfs/params.c    |  2 ++
>  3 files changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 6a70faeee6fa..7eb806a4e5f8 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -84,41 +84,45 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
>         struct dentry *workdir = ofs->workdir;
>         struct inode *wdir = workdir->d_inode;
>
> -       inode_lock_nested(wdir, I_MUTEX_PARENT);
> +       guard(mutex)(&ofs->whiteout_lock);
> +
>         if (!ofs->whiteout) {
> +               inode_lock_nested(wdir, I_MUTEX_PARENT);
>                 whiteout = ovl_lookup_temp(ofs, workdir);
> -               if (IS_ERR(whiteout))
> -                       goto out;
> -
> -               err = ovl_do_whiteout(ofs, wdir, whiteout);
> -               if (err) {
> -                       dput(whiteout);
> -                       whiteout = ERR_PTR(err);
> -                       goto out;
> +               if (!IS_ERR(whiteout)) {
> +                       err = ovl_do_whiteout(ofs, wdir, whiteout);
> +                       if (err) {
> +                               dput(whiteout);
> +                               whiteout = ERR_PTR(err);
> +                       }
>                 }
> +               inode_unlock(wdir);
> +               if (IS_ERR(whiteout))
> +                       return whiteout;
>                 ofs->whiteout = whiteout;
>         }
>
>         if (!ofs->no_shared_whiteout) {
> +               inode_lock_nested(wdir, I_MUTEX_PARENT);
>                 whiteout = ovl_lookup_temp(ofs, workdir);
> -               if (IS_ERR(whiteout))
> -                       goto out;
> -
> -               err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
> -               if (!err)
> -                       goto out;
> -
> -               if (err != -EMLINK) {
> +               if (!IS_ERR(whiteout)) {
> +                       err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
> +                       if (err) {
> +                               dput(whiteout);
> +                               whiteout = ERR_PTR(err);
> +                       }
> +               }
> +               inode_unlock(wdir);
> +               if (!IS_ERR(whiteout))
> +                       return whiteout;
> +               if (PTR_ERR(whiteout) != -EMLINK) {
>                         pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
>                                 ofs->whiteout->d_inode->i_nlink, err);
>                         ofs->no_shared_whiteout = true;
>                 }
> -               dput(whiteout);
>         }
>         whiteout = ofs->whiteout;
>         ofs->whiteout = NULL;
> -out:
> -       inode_unlock(wdir);
>         return whiteout;
>  }
>
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index afb7762f873f..4c1bae935ced 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -88,6 +88,7 @@ struct ovl_fs {
>         /* Shared whiteout cache */
>         struct dentry *whiteout;
>         bool no_shared_whiteout;
> +       struct mutex whiteout_lock;
>         /* r/o snapshot of upperdir sb's only taken on volatile mounts */
>         errseq_t errseq;
>  };
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index f42488c01957..cb1a17c066cd 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -797,6 +797,8 @@ int ovl_init_fs_context(struct fs_context *fc)
>         fc->s_fs_info           = ofs;
>         fc->fs_private          = ctx;
>         fc->ops                 = &ovl_context_ops;
> +
> +       mutex_init(&ofs->whiteout_lock);
>         return 0;
>
>  out_err:
> --
> 2.49.0
>

Re: [PATCH v3 12/21] ovl: narrow locking in ovl_workdir_create()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> In ovl_workdir_create() don't hold the dir lock for the whole time, but
> only take it when needed.
>
> It now gets taken separately for ovl_workdir_cleanup().  A subsequent
> patch will move the locking into that function.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/super.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 2e6b25bde83f..cb2551a155d8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -299,8 +299,8 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>         int err;
>         bool retried = false;
>
> -       inode_lock_nested(dir, I_MUTEX_PARENT);
>  retry:
> +       inode_lock_nested(dir, I_MUTEX_PARENT);
>         work = ovl_lookup_upper(ofs, name, ofs->workbasedir, strlen(name));
>
>         if (!IS_ERR(work)) {
> @@ -311,23 +311,28 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>
>                 if (work->d_inode) {
>                         err = -EEXIST;
> +                       inode_unlock(dir);
>                         if (retried)
>                                 goto out_dput;
>
>                         if (persist)
> -                               goto out_unlock;
> +                               return work;
>
>                         retried = true;
> -                       err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
> -                       dput(work);
> -                       if (err == -EINVAL) {
> -                               work = ERR_PTR(err);
> -                               goto out_unlock;
> +                       err = ovl_parent_lock(ofs->workbasedir, work);
> +                       if (!err) {
> +                               err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
> +                               ovl_parent_unlock(ofs->workbasedir);
>                         }
> +                       dput(work);
> +                       if (err == -EINVAL)
> +                               return ERR_PTR(err);
> +
>                         goto retry;
>                 }
>
>                 work = ovl_do_mkdir(ofs, dir, work, attr.ia_mode);
> +               inode_unlock(dir);
>                 err = PTR_ERR(work);
>                 if (IS_ERR(work))
>                         goto out_err;
> @@ -365,11 +370,10 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>                 if (err)
>                         goto out_dput;
>         } else {
> +               inode_unlock(dir);
>                 err = PTR_ERR(work);
>                 goto out_err;
>         }
> -out_unlock:
> -       inode_unlock(dir);
>         return work;
>
>  out_dput:
> @@ -377,8 +381,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>  out_err:
>         pr_warn("failed to create directory %s/%s (errno: %i); mounting read-only\n",
>                 ofs->config.workdir, name, -err);
> -       work = NULL;
> -       goto out_unlock;
> +       return NULL;
>  }
>
>  static int ovl_check_namelen(const struct path *path, struct ovl_fs *ofs,
> --
> 2.49.0
>

Re: [PATCH v3 13/21] ovl: narrow locking in ovl_indexdir_cleanup()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> Instead of taking the directory lock for the whole cleanup, only take it
> when needed.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/readdir.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 2a222b8185a3..95d5284daf8d 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1194,7 +1194,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>         if (err)
>                 goto out;
>
> -       inode_lock_nested(dir, I_MUTEX_PARENT);
>         list_for_each_entry(p, &list, l_node) {
>                 if (p->name[0] == '.') {
>                         if (p->len == 1)
> @@ -1202,7 +1201,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                         if (p->len == 2 && p->name[1] == '.')
>                                 continue;
>                 }
> -               index = ovl_lookup_upper(ofs, p->name, indexdir, p->len);
> +               index = ovl_lookup_upper_unlocked(ofs, p->name, indexdir, p->len);
>                 if (IS_ERR(index)) {
>                         err = PTR_ERR(index);
>                         index = NULL;
> @@ -1210,7 +1209,11 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                 }
>                 /* Cleanup leftover from index create/cleanup attempt */
>                 if (index->d_name.name[0] == '#') {
> -                       err = ovl_workdir_cleanup(ofs, dir, path.mnt, index, 1);
> +                       err = ovl_parent_lock(indexdir, index);
> +                       if (!err) {
> +                               err = ovl_workdir_cleanup(ofs, dir, path.mnt, index, 1);
> +                               ovl_parent_unlock(indexdir);
> +                       }
>                         if (err)
>                                 break;
>                         goto next;
> @@ -1220,7 +1223,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                         goto next;
>                 } else if (err == -ESTALE) {
>                         /* Cleanup stale index entries */
> -                       err = ovl_cleanup(ofs, dir, index);
> +                       err = ovl_cleanup_unlocked(ofs, indexdir, index);
>                 } else if (err != -ENOENT) {
>                         /*
>                          * Abort mount to avoid corrupting the index if
> @@ -1233,10 +1236,14 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                          * Whiteout orphan index to block future open by
>                          * handle after overlay nlink dropped to zero.
>                          */
> -                       err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> +                       err = ovl_parent_lock(indexdir, index);
> +                       if (!err) {
> +                               err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> +                               ovl_parent_unlock(indexdir);
> +                       }
>                 } else {
>                         /* Cleanup orphan index entries */
> -                       err = ovl_cleanup(ofs, dir, index);
> +                       err = ovl_cleanup_unlocked(ofs, indexdir, index);
>                 }
>
>                 if (err)
> @@ -1247,7 +1254,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                 index = NULL;
>         }
>         dput(index);
> -       inode_unlock(dir);
>  out:
>         ovl_cache_free(&list);
>         if (err)
> --
> 2.49.0
>

Re: [PATCH v3 14/21] ovl: narrow locking in ovl_workdir_cleanup_recurse()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> Only take the dir lock when needed, rather than for the whole loop.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/readdir.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 95d5284daf8d..b0f9e5a00c1a 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1122,7 +1122,6 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
>         if (err)
>                 goto out;
>
> -       inode_lock_nested(dir, I_MUTEX_PARENT);
>         list_for_each_entry(p, &list, l_node) {
>                 struct dentry *dentry;
>
> @@ -1137,16 +1136,21 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
>                         err = -EINVAL;
>                         break;
>                 }
> -               dentry = ovl_lookup_upper(ofs, p->name, path->dentry, p->len);
> +               dentry = ovl_lookup_upper_unlocked(ofs, p->name, path->dentry, p->len);
>                 if (IS_ERR(dentry))
>                         continue;
> -               if (dentry->d_inode)
> -                       err = ovl_workdir_cleanup(ofs, dir, path->mnt, dentry, level);
> +               if (dentry->d_inode) {
> +                       err = ovl_parent_lock(path->dentry, dentry);
> +                       if (!err) {
> +                               err = ovl_workdir_cleanup(ofs, dir, path->mnt,
> +                                                         dentry, level);
> +                               ovl_parent_unlock(path->dentry);
> +                       }
> +               }
>                 dput(dentry);
>                 if (err)
>                         break;
>         }
> -       inode_unlock(dir);
>  out:
>         ovl_cache_free(&list);
>         return err;
> --
> 2.49.0
>

Re: [PATCH v3 15/21] ovl: change ovl_workdir_cleanup() to take dir lock as needed.

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> Rather than calling ovl_workdir_cleanup() with the dir already locked,
> change it to take the dir lock only when needed.
>
> Also change ovl_workdir_cleanup() to take a dentry for the parent rather
> than an inode.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/overlayfs.h |  2 +-
>  fs/overlayfs/readdir.c   | 36 +++++++++++++-----------------------
>  fs/overlayfs/super.c     |  6 +-----
>  3 files changed, 15 insertions(+), 29 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index cff5bb625e9d..f6023442a45c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -738,7 +738,7 @@ void ovl_cleanup_whiteouts(struct ovl_fs *ofs, struct dentry *upper,
>  void ovl_cache_free(struct list_head *list);
>  void ovl_dir_cache_free(struct inode *inode);
>  int ovl_check_d_type_supported(const struct path *realpath);
> -int ovl_workdir_cleanup(struct ovl_fs *ofs, struct inode *dir,
> +int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
>                         struct vfsmount *mnt, struct dentry *dentry, int level);
>  int ovl_indexdir_cleanup(struct ovl_fs *ofs);
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index b0f9e5a00c1a..e2d0c314df6c 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1096,7 +1096,6 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
>                                        int level)
>  {
>         int err;
> -       struct inode *dir = path->dentry->d_inode;
>         LIST_HEAD(list);
>         struct ovl_cache_entry *p;
>         struct ovl_readdir_data rdd = {
> @@ -1139,14 +1138,9 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
>                 dentry = ovl_lookup_upper_unlocked(ofs, p->name, path->dentry, p->len);
>                 if (IS_ERR(dentry))
>                         continue;
> -               if (dentry->d_inode) {
> -                       err = ovl_parent_lock(path->dentry, dentry);
> -                       if (!err) {
> -                               err = ovl_workdir_cleanup(ofs, dir, path->mnt,
> -                                                         dentry, level);
> -                               ovl_parent_unlock(path->dentry);
> -                       }
> -               }
> +               if (dentry->d_inode)
> +                       err = ovl_workdir_cleanup(ofs, path->dentry, path->mnt,
> +                                                 dentry, level);
>                 dput(dentry);
>                 if (err)
>                         break;
> @@ -1156,24 +1150,25 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
>         return err;
>  }
>
> -int ovl_workdir_cleanup(struct ovl_fs *ofs, struct inode *dir,
> +int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
>                         struct vfsmount *mnt, struct dentry *dentry, int level)
>  {
>         int err;
>
> -       if (!d_is_dir(dentry) || level > 1) {
> -               return ovl_cleanup(ofs, dir, dentry);
> -       }
> +       if (!d_is_dir(dentry) || level > 1)
> +               return ovl_cleanup_unlocked(ofs, parent, dentry);
>
> -       err = ovl_do_rmdir(ofs, dir, dentry);
> +       err = ovl_parent_lock(parent, dentry);
> +       if (err)
> +               return err;
> +       err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
> +       ovl_parent_unlock(parent);
>         if (err) {
>                 struct path path = { .mnt = mnt, .dentry = dentry };
>
> -               inode_unlock(dir);
>                 err = ovl_workdir_cleanup_recurse(ofs, &path, level + 1);
> -               inode_lock_nested(dir, I_MUTEX_PARENT);
>                 if (!err)
> -                       err = ovl_cleanup(ofs, dir, dentry);
> +                       err = ovl_cleanup_unlocked(ofs, parent, dentry);
>         }
>
>         return err;
> @@ -1184,7 +1179,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>         int err;
>         struct dentry *indexdir = ofs->workdir;
>         struct dentry *index = NULL;
> -       struct inode *dir = indexdir->d_inode;
>         struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = indexdir };
>         LIST_HEAD(list);
>         struct ovl_cache_entry *p;
> @@ -1213,11 +1207,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                 }
>                 /* Cleanup leftover from index create/cleanup attempt */
>                 if (index->d_name.name[0] == '#') {
> -                       err = ovl_parent_lock(indexdir, index);
> -                       if (!err) {
> -                               err = ovl_workdir_cleanup(ofs, dir, path.mnt, index, 1);
> -                               ovl_parent_unlock(indexdir);
> -                       }
> +                       err = ovl_workdir_cleanup(ofs, indexdir, path.mnt, index, 1);
>                         if (err)
>                                 break;
>                         goto next;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index cb2551a155d8..4c3736bf2db4 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -319,11 +319,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>                                 return work;
>
>                         retried = true;
> -                       err = ovl_parent_lock(ofs->workbasedir, work);
> -                       if (!err) {
> -                               err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
> -                               ovl_parent_unlock(ofs->workbasedir);
> -                       }
> +                       err = ovl_workdir_cleanup(ofs, ofs->workbasedir, mnt, work, 0);
>                         dput(work);
>                         if (err == -EINVAL)
>                                 return ERR_PTR(err);
> --
> 2.49.0
>

Re: [PATCH v3 16/21] ovl: narrow locking on ovl_remove_and_whiteout()

[Amir Goldstein]

On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
>
> This code:
>   performs a lookup_upper
>   creates a whiteout object
>   renames the whiteout over the result of the lookup
>
> The create and the rename must be locked separately for proposed
> directory locking changes.  This patch takes a first step of moving the
> lookup out of the locked region.  A subsequent patch will separate the
> create from the rename.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/dir.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index e81be60f1125..340f8679b6e7 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -770,15 +770,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
>                         goto out;
>         }
>
> -       err = ovl_lock_rename_workdir(workdir, NULL, upperdir, NULL);
> -       if (err)
> -               goto out_dput;
> -
> -       upper = ovl_lookup_upper(ofs, dentry->d_name.name, upperdir,
> -                                dentry->d_name.len);
> +       upper = ovl_lookup_upper_unlocked(ofs, dentry->d_name.name, upperdir,
> +                                         dentry->d_name.len);
>         err = PTR_ERR(upper);
>         if (IS_ERR(upper))
> -               goto out_unlock;
> +               goto out_dput;
>
>         err = -ESTALE;
>         if ((opaquedir && upper != opaquedir) ||
> @@ -787,17 +783,18 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
>                 goto out_dput_upper;
>         }
>
> -       err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> +       err = ovl_lock_rename_workdir(workdir, NULL, upperdir, upper);
>         if (err)
> -               goto out_d_drop;
> +               goto out_dput_upper;
> +
> +       err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> +       if (!err)
> +               ovl_dir_modified(dentry->d_parent, true);
>
> -       ovl_dir_modified(dentry->d_parent, true);
> -out_d_drop:
>         d_drop(dentry);
> +       unlock_rename(workdir, upperdir);
>  out_dput_upper:
>         dput(upper);
> -out_unlock:
> -       unlock_rename(workdir, upperdir);
>  out_dput:
>         dput(opaquedir);
>  out:
> --
> 2.49.0
>

Re: [PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[NeilBrown]

On Wed, 16 Jul 2025, Amir Goldstein wrote:
> On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
> >
> > More excellent review feedback - more patches :-)
> >
> > I've chosen to use ovl_parent_lock() here as a temporary and leave the
> > debate over naming for the VFS version of the function until all the new
> > names are introduced later.
> 
> Perfect.
> 
> Please push v3 patches to branch pdirops, or to a clean branch
> based on vfs-6.17.file, so I can test them.

There is a branch "ovl" which is based on vfs.all as I depend on a
couple of other vfs changes.

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 
> 
> >
> >
> > Original description:
> >
> > This series of patches for overlayfs is primarily focussed on preparing
> > for some proposed changes to directory locking.  In the new scheme we
> > will lock individual dentries in a directory rather than the whole
> > directory.
> >
> > ovl currently will sometimes lock a directory on the upper filesystem
> > and do a few different things while holding the lock.  This is
> > incompatible with the new scheme.
> >
> > This series narrows the region of code protected by the directory lock,
> > taking it multiple times when necessary.  This theoretically open up the
> > possibilty of other changes happening on the upper filesytem between the
> > unlock and the lock.  To some extent the patches guard against that by
> > checking the dentries still have the expect parent after retaking the
> > lock.  In general, I think ovl would have trouble if upperfs were being
> > changed independantly, and I don't think the changes here increase the
> > problem in any important way.
> >
> > After this series (with any needed changes) lands I will resubmit my
> > change to vfs_rmdir() behaviour to have it drop the lock on error.  ovl
> > will be much better positioned to handle that change.  It will come with
> > the new "lookup_and_lock" API that I am proposing.
> >
> > Thanks,
> > NeilBrown
> >
> >  [PATCH v3 01/21] ovl: simplify an error path in ovl_copy_up_workdir()
> >  [PATCH v3 02/21] ovl: change ovl_create_index() to take dir locks
> >  [PATCH v3 03/21] ovl: Call ovl_create_temp() without lock held.
> >  [PATCH v3 04/21] ovl: narrow the locked region in
> >  [PATCH v3 05/21] ovl: narrow locking in ovl_create_upper()
> >  [PATCH v3 06/21] ovl: narrow locking in ovl_clear_empty()
> >  [PATCH v3 07/21] ovl: narrow locking in ovl_create_over_whiteout()
> >  [PATCH v3 08/21] ovl: simplify gotos in ovl_rename()
> >  [PATCH v3 09/21] ovl: narrow locking in ovl_rename()
> >  [PATCH v3 10/21] ovl: narrow locking in ovl_cleanup_whiteouts()
> >  [PATCH v3 11/21] ovl: narrow locking in ovl_cleanup_index()
> >  [PATCH v3 12/21] ovl: narrow locking in ovl_workdir_create()
> >  [PATCH v3 13/21] ovl: narrow locking in ovl_indexdir_cleanup()
> >  [PATCH v3 14/21] ovl: narrow locking in ovl_workdir_cleanup_recurse()
> >  [PATCH v3 15/21] ovl: change ovl_workdir_cleanup() to take dir lock
> >  [PATCH v3 16/21] ovl: narrow locking on ovl_remove_and_whiteout()
> >  [PATCH v3 17/21] ovl: change ovl_cleanup_and_whiteout() to take
> >  [PATCH v3 18/21] ovl: narrow locking in ovl_whiteout()
> >  [PATCH v3 19/21] ovl: narrow locking in ovl_check_rename_whiteout()
> >  [PATCH v3 20/21] ovl: change ovl_create_real() to receive dentry
> >  [PATCH v3 21/21] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()
> 

Re: [PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[Amir Goldstein]

On Wed, Jul 16, 2025 at 9:19 AM NeilBrown <neil@brown.name> wrote:
>
> On Wed, 16 Jul 2025, Amir Goldstein wrote:
> > On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
> > >
> > > More excellent review feedback - more patches :-)
> > >
> > > I've chosen to use ovl_parent_lock() here as a temporary and leave the
> > > debate over naming for the VFS version of the function until all the new
> > > names are introduced later.
> >
> > Perfect.
> >
> > Please push v3 patches to branch pdirops, or to a clean branch
> > based on vfs-6.17.file, so I can test them.
>
> There is a branch "ovl" which is based on vfs.all as I depend on a
> couple of other vfs changes.

ok I will test this one.

Do you mean that ovl branch depends on other vfs changes or that pdirops
which is based on ovl branch depends on other vfs changes?

Just asking because I did not notice any other dependencies in ovl branch,
so wanted to know which are they.

Thanks,
Amir.

Re: [PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[NeilBrown]

On Wed, 16 Jul 2025, Amir Goldstein wrote:
> On Wed, Jul 16, 2025 at 9:19 AM NeilBrown <neil@brown.name> wrote:
> >
> > On Wed, 16 Jul 2025, Amir Goldstein wrote:
> > > On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
> > > >
> > > > More excellent review feedback - more patches :-)
> > > >
> > > > I've chosen to use ovl_parent_lock() here as a temporary and leave the
> > > > debate over naming for the VFS version of the function until all the new
> > > > names are introduced later.
> > >
> > > Perfect.
> > >
> > > Please push v3 patches to branch pdirops, or to a clean branch
> > > based on vfs-6.17.file, so I can test them.
> >
> > There is a branch "ovl" which is based on vfs.all as I depend on a
> > couple of other vfs changes.
> 
> ok I will test this one.
> 
> Do you mean that ovl branch depends on other vfs changes or that pdirops
> which is based on ovl branch depends on other vfs changes?

ovl branch depends on

Commit bc9241367aac ("VFS: change old_dir and new_dir in struct renamedata to dentrys")

Thanks,
NeilBrown

> 
> Just asking because I did not notice any other dependencies in ovl branch,
> so wanted to know which are they.
> 
> Thanks,
> Amir.
> 

Re: [PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[Amir Goldstein]

On Wed, Jul 16, 2025 at 10:05 AM NeilBrown <neil@brown.name> wrote:
>
> On Wed, 16 Jul 2025, Amir Goldstein wrote:
> > On Wed, Jul 16, 2025 at 9:19 AM NeilBrown <neil@brown.name> wrote:
> > >
> > > On Wed, 16 Jul 2025, Amir Goldstein wrote:
> > > > On Wed, Jul 16, 2025 at 2:47 AM NeilBrown <neil@brown.name> wrote:
> > > > >
> > > > > More excellent review feedback - more patches :-)
> > > > >
> > > > > I've chosen to use ovl_parent_lock() here as a temporary and leave the
> > > > > debate over naming for the VFS version of the function until all the new
> > > > > names are introduced later.
> > > >
> > > > Perfect.
> > > >
> > > > Please push v3 patches to branch pdirops, or to a clean branch
> > > > based on vfs-6.17.file, so I can test them.
> > >
> > > There is a branch "ovl" which is based on vfs.all as I depend on a
> > > couple of other vfs changes.
> >
> > ok I will test this one.
> >
> > Do you mean that ovl branch depends on other vfs changes or that pdirops
> > which is based on ovl branch depends on other vfs changes?
>
> ovl branch depends on
>
> Commit bc9241367aac ("VFS: change old_dir and new_dir in struct renamedata to dentrys")

I see.

Anyway, testing looks good on your branch.

Christian,

From eyeballing the changes on vfs.all, I do not see any apparent dependency
of the commit above with any of the commits in other vfs branches.

Since vfs-6.17.file currently has two ovl patches and one vfs patch which is a
prep patch for ovl, may I propose to collect all ovl patches and ovl
prep patches
on a single branch for 6.17:

1. Rename vfs-6.17.file to vfs-6.17.ovl (or not up to you)
2. Move commit bc9241367aac from vfs-6.17.misc to vfs-6.17.ovl
3. Apply Neil's patches from this series (all have my RVB)

Logically, these changes could be broken up to more than 1 PR,
but that's up to you. I can also do the ovl-only PR myself if we agree on
a stable vfs branch and its content.

WDYT?

Thanks,
Amir.

Re: [PATCH v3 00/21] ovl: narrow regions protected by i_rw_sem

[Christian Brauner]

On Wed, 16 Jul 2025 10:44:11 +1000, NeilBrown wrote:
> More excellent review feedback - more patches :-)
> 
> I've chosen to use ovl_parent_lock() here as a temporary and leave the
> debate over naming for the VFS version of the function until all the new
> names are introduced later.
> 
> 
> [...]

Applied to the vfs-6.17.file branch of the vfs/vfs.git tree.
Patches in the vfs-6.17.file branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.17.file

[01/21] ovl: simplify an error path in ovl_copy_up_workdir()
        https://git.kernel.org/vfs/vfs/c/9d23967b18c6
[02/21] ovl: change ovl_create_index() to take dir locks
        https://git.kernel.org/vfs/vfs/c/c4f8f862b31c
[03/21] ovl: Call ovl_create_temp() without lock held.
        https://git.kernel.org/vfs/vfs/c/d2c995581c7c
[04/21] ovl: narrow the locked region in ovl_copy_up_workdir()
        https://git.kernel.org/vfs/vfs/c/a735bdf0b785
[05/21] ovl: narrow locking in ovl_create_upper()
        https://git.kernel.org/vfs/vfs/c/a07052e07b67
[06/21] ovl: narrow locking in ovl_clear_empty()
        https://git.kernel.org/vfs/vfs/c/4f622bd9f3e5
[07/21] ovl: narrow locking in ovl_create_over_whiteout()
        https://git.kernel.org/vfs/vfs/c/e460bc4d012c
[08/21] ovl: simplify gotos in ovl_rename()
        https://git.kernel.org/vfs/vfs/c/76342c9eb8e2
[09/21] ovl: narrow locking in ovl_rename()
        https://git.kernel.org/vfs/vfs/c/05468498cd2f
[10/21] ovl: narrow locking in ovl_cleanup_whiteouts()
        https://git.kernel.org/vfs/vfs/c/7dfb0722ad07
[11/21] ovl: narrow locking in ovl_cleanup_index()
        https://git.kernel.org/vfs/vfs/c/8290fb412d2f
[12/21] ovl: narrow locking in ovl_workdir_create()
        https://git.kernel.org/vfs/vfs/c/61eb7fec9e79
[13/21] ovl: narrow locking in ovl_indexdir_cleanup()
        https://git.kernel.org/vfs/vfs/c/d56c6feb69cb
[14/21] ovl: narrow locking in ovl_workdir_cleanup_recurse()
        https://git.kernel.org/vfs/vfs/c/a45ee87ded78
[15/21] ovl: change ovl_workdir_cleanup() to take dir lock as needed.
        https://git.kernel.org/vfs/vfs/c/241062ae5d87
[16/21] ovl: narrow locking on ovl_remove_and_whiteout()
        https://git.kernel.org/vfs/vfs/c/c69566b1d11d
[17/21] ovl: change ovl_cleanup_and_whiteout() to take rename lock as needed
        https://git.kernel.org/vfs/vfs/c/2fa14cf2dca1
[18/21] ovl: narrow locking in ovl_whiteout()
        https://git.kernel.org/vfs/vfs/c/8afa0a736713
[19/21] ovl: narrow locking in ovl_check_rename_whiteout()
        https://git.kernel.org/vfs/vfs/c/09d56cc88c24
[20/21] ovl: change ovl_create_real() to receive dentry parent
        https://git.kernel.org/vfs/vfs/c/ee37c3cfc5df
[21/21] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()
        https://git.kernel.org/vfs/vfs/c/fe4d3360f9cb

Re: [PATCH v3 01/21] ovl: simplify an error path in ovl_copy_up_workdir()

[Miklos Szeredi]

On Wed, 16 Jul 2025 at 02:47, NeilBrown <neil@brown.name> wrote:

> +int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
> +                        struct dentry *wdentry)
> +{
> +       int err;
> +
> +       err = ovl_parent_lock(workdir, wdentry);
> +       if (err)
> +               return err;

We get a pr_err() if the cleanup failed for some reason.  But in this
case it's just an -EINVAL return, which  most callers of ovl_cleanup()
ignore.

I guess pr_err_ratelimited() in this case wouldn't hurt, since it
either indicates a bug in the code, which we want to know about, or
mischief, in which case the one making the mischief shouldn't be
surprised.

Thanks,
Miklos