[PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem

[PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem

[NeilBrown]

This series of patches for overlayfs is primarily focussed on preparing
for some proposed changes to directory locking.  In the new scheme we
wil 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.

The first patch in this series doesn't exactly match the above, but it
does relate to directory locking and I think it is a sensible
simplificaiton.

I have tested this with fstests, both generic and unionfs tests.  I
wouldn't be surprised if I missed something though, so please review
carefully.

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 01/12] ovl: use is_subdir() for testing if one thing is a
 [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index()
 [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir()
 [PATCH 04/12] ovl: narrow locking in ovl_create_upper()
 [PATCH 05/12] ovl: narrow locking in ovl_clear_empty()
 [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout()
 [PATCH 07/12] ovl: narrow locking in ovl_rename()
 [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts()
 [PATCH 09/12] ovl: whiteout locking changes
 [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()
 [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent
 [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()

[PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another

[NeilBrown]

Rather than using lock_rename(), use the more obvious is_subdir() for
ensuring that neither upper nor workdir contain the other.
Also be explicit in the comment that the two directories cannot be the
same.

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index cf99b276fdfb..db046b0d6a68 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -438,18 +438,12 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	return 0;
 }
 
-/* Workdir should not be subdir of upperdir and vice versa */
+/* Workdir should not be subdir of upperdir and vice versa, and
+ * they should not be the same.
+ */
 static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
 {
-	bool ok = false;
-
-	if (workdir != upperdir) {
-		struct dentry *trap = lock_rename(workdir, upperdir);
-		if (!IS_ERR(trap))
-			unlock_rename(workdir, upperdir);
-		ok = (trap == NULL);
-	}
-	return ok;
+	return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
 }
 
 static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,
-- 
2.49.0

Re: [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> Rather than using lock_rename(), use the more obvious is_subdir() for
> ensuring that neither upper nor workdir contain the other.
> Also be explicit in the comment that the two directories cannot be the
> same.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/super.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index cf99b276fdfb..db046b0d6a68 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -438,18 +438,12 @@ static int ovl_lower_dir(const char *name, struct path *path,
>         return 0;
>  }
>
> -/* Workdir should not be subdir of upperdir and vice versa */
> +/* Workdir should not be subdir of upperdir and vice versa, and
> + * they should not be the same.
> + */
>  static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
>  {
> -       bool ok = false;
> -
> -       if (workdir != upperdir) {
> -               struct dentry *trap = lock_rename(workdir, upperdir);
> -               if (!IS_ERR(trap))
> -                       unlock_rename(workdir, upperdir);
> -               ok = (trap == NULL);
> -       }
> -       return ok;
> +       return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);

I am not at ease with this simplification to an unlocked test.
In the cover letter you wrote that this patch is not like the other patches.
Is this really necessary for your work?
If not, please leave it out.

Thanks,
Amir.

Re: [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another

[NeilBrown]

On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > Rather than using lock_rename(), use the more obvious is_subdir() for
> > ensuring that neither upper nor workdir contain the other.
> > Also be explicit in the comment that the two directories cannot be the
> > same.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/super.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index cf99b276fdfb..db046b0d6a68 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -438,18 +438,12 @@ static int ovl_lower_dir(const char *name, struct path *path,
> >         return 0;
> >  }
> >
> > -/* Workdir should not be subdir of upperdir and vice versa */
> > +/* Workdir should not be subdir of upperdir and vice versa, and
> > + * they should not be the same.
> > + */
> >  static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
> >  {
> > -       bool ok = false;
> > -
> > -       if (workdir != upperdir) {
> > -               struct dentry *trap = lock_rename(workdir, upperdir);
> > -               if (!IS_ERR(trap))
> > -                       unlock_rename(workdir, upperdir);
> > -               ok = (trap == NULL);
> > -       }
> > -       return ok;
> > +       return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
> 
> I am not at ease with this simplification to an unlocked test.
> In the cover letter you wrote that this patch is not like the other patches.
> Is this really necessary for your work?
> If not, please leave it out.

I assume that by "unlocked" you mean that there are two separate calls
to is_subdir() which are not guaranteed to be coherent.  I don't see how
that could be a problem.  The directory hierarchy could certainly change
between the calls, but it could equally change immediately after a fully
locked call, and thereby invalidate the result.  It is not possible to
provide a permanent guarantee that there is never a subdir relationship
between the two.

I don't absolutely need the patch.  I plan for lock_rename() to go away.
It will be replaced by lookup_and_lock_rename() which will lock two
dentries and protect the paths from them to their common ancestor from
any renames.  lookup_and_lock_rename() can be given the two dentries
rather than having it look them up given parents and names.  So it could
be used for this test.  It just seems to me to be unnecessary
complexity.  I will drop it (for now) if you like.

NeilBrown.

[PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() 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() and ovl_create_index() out
of the locked region and has them take and release the relevant lock
themselves.

The lock that was taken before these functions are called is now taken
after.  This means that any code between where the lock was taken and
these calls is now unlocked.  This necessitates the creation of
_unlocked() versions of ovl_cleanup() and ovl_lookup_upper().  These
will be used more widely in future patches.

ovl_cleanup_unlocked() takes a dentry for the directory rather than an
inode as this simplifies calling slightly.

Note that when we move a lookup or create out of a locked region in
which the dentry is acted on, we need to ensure after taking the lock
that the dentry is still in the directory we expect it to be in.  It is
conceivable that it was moved.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/copy_up.c   | 37 +++++++++++-------
 fs/overlayfs/dir.c       | 84 +++++++++++++++++++++++++---------------
 fs/overlayfs/overlayfs.h | 10 +++++
 fs/overlayfs/super.c     |  7 ++--
 4 files changed, 88 insertions(+), 50 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8a3c0d18ec2e..7a21ad1f2b6e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -517,15 +517,12 @@ 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)
 {
 	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 = { };
@@ -558,17 +555,21 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	err = ovl_set_upper_fh(ofs, upper, temp);
 	if (err)
 		goto out;
-
+	lock_rename(indexdir, indexdir);
 	index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
 	if (IS_ERR(index)) {
 		err = PTR_ERR(index);
+	} else if (temp->d_parent != indexdir) {
+		err = -EINVAL;
+		dput(index);
 	} else {
 		err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
 		dput(index);
 	}
+	unlock_rename(indexdir, indexdir);
 out:
 	if (err)
-		ovl_cleanup(ofs, dir, temp);
+		ovl_cleanup_unlocked(ofs, indexdir, temp);
 	dput(temp);
 free_name:
 	kfree(name.name);
@@ -779,9 +780,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);
 
@@ -794,6 +793,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	 */
 	path.dentry = temp;
 	err = ovl_copy_up_data(c, &path);
+	if (err)
+		goto cleanup_write_unlocked;
 	/*
 	 * We cannot hold lock_rename() throughout this helper, because of
 	 * lock ordering with sb_writers, which shouldn't be held when calling
@@ -801,6 +802,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	 * temp wasn't moved before copy up completion or cleanup.
 	 */
 	ovl_start_write(c->dentry);
+
+	if (S_ISDIR(c->stat.mode) && c->indexed) {
+		err = ovl_create_index(c->dentry, c->origin_fh, temp);
+		if (err)
+			goto cleanup_unlocked;
+	}
+
 	trap = lock_rename(c->workdir, c->destdir);
 	if (trap || temp->d_parent != c->workdir) {
 		/* temp or workdir moved underneath us? abort without cleanup */
@@ -809,20 +817,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 		if (IS_ERR(trap))
 			goto out;
 		goto unlock;
-	} else if (err) {
-		goto cleanup;
 	}
 
 	err = ovl_copy_up_metadata(c, temp);
 	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);
@@ -857,6 +857,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	ovl_cleanup(ofs, wdir, temp);
 	dput(temp);
 	goto unlock;
+
+cleanup_write_unlocked:
+	ovl_start_write(c->dentry);
+cleanup_unlocked:
+	ovl_cleanup_unlocked(ofs, c->workdir, temp);
+	dput(temp);
+	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..a51a3dc02bf5 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;
+
+	inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
+	if (wdentry->d_parent == workdir)
+		ovl_cleanup(ofs, workdir->d_inode, wdentry);
+	else
+		err = -EINVAL;
+	inode_unlock(workdir->d_inode);
+
+	return err;
+}
+
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
 {
 	struct dentry *temp;
@@ -199,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,
@@ -348,28 +367,30 @@ 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;
+	/* This test is racey but we re-test under the lock */
+	if (upper->d_parent != upperdir)
+		goto out;
 
 	opaquedir = ovl_create_temp(ofs, workdir, OVL_CATTR(stat.mode));
 	err = PTR_ERR(opaquedir);
 	if (IS_ERR(opaquedir))
-		goto out_unlock;
-
+		/* workdir was unlocked, no upperdir */
+		goto out;
+	err = ovl_lock_rename_workdir(workdir, upperdir);
+	if (err)
+		goto out_cleanup_unlocked;
+	if (upper->d_parent->d_inode != udir)
+		goto out_cleanup;
 	err = ovl_copy_xattr(dentry->d_sb, &upperpath, opaquedir);
 	if (err)
 		goto out_cleanup;
@@ -398,10 +419,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);
 }
@@ -439,15 +460,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))
@@ -458,6 +475,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, upperdir);
+	if (err)
+		goto out_cleanup;
+
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
 	 */
@@ -472,35 +493,35 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		err = ovl_do_notify_change(ofs, newdentry, &attr);
 		inode_unlock(newdentry->d_inode);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_locked;
 	}
 	if (!hardlink) {
 		err = ovl_set_upper_acl(ofs, newdentry,
 					XATTR_NAME_POSIX_ACL_ACCESS, acl);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_locked;
 
 		err = ovl_set_upper_acl(ofs, newdentry,
 					XATTR_NAME_POSIX_ACL_DEFAULT, default_acl);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_locked;
 	}
 
 	if (!hardlink && S_ISDIR(cattr->mode)) {
 		err = ovl_set_opaque(dentry, newdentry);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_locked;
 
 		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
 				    RENAME_EXCHANGE);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_locked;
 
 		ovl_cleanup(ofs, wdir, upper);
 	} else {
 		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
 		if (err)
-			goto out_cleanup;
+			goto out_cleanup_locked;
 	}
 	ovl_dir_modified(dentry->d_parent, false);
 	err = ovl_instantiate(dentry, inode, newdentry, hardlink, NULL);
@@ -508,10 +529,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);
@@ -519,8 +539,10 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	}
 	return err;
 
+out_cleanup_locked:
+	unlock_rename(workdir, upperdir);
 out_cleanup:
-	ovl_cleanup(ofs, wdir, newdentry);
+	ovl_cleanup_unlocked(ofs, workdir, newdentry);
 	dput(newdentry);
 	goto out_dput;
 }
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 42228d10f6b9..508003e26e08 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)
@@ -843,6 +852,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/super.c b/fs/overlayfs/super.c
index db046b0d6a68..576b5c3b537c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -558,13 +558,12 @@ 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;
 
+	lock_rename(workdir, workdir);
 	dest = ovl_lookup_temp(ofs, workdir);
 	err = PTR_ERR(dest);
 	if (IS_ERR(dest)) {
@@ -600,7 +599,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 	dput(dest);
 
 out_unlock:
-	inode_unlock(dir);
+	unlock_rename(workdir, workdir);
 
 	return err;
 }
-- 
2.49.0

Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> 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() and ovl_create_index() out
> of the locked region and has them take and release the relevant lock
> themselves.
>
> The lock that was taken before these functions are called is now taken
> after.  This means that any code between where the lock was taken and
> these calls is now unlocked.  This necessitates the creation of
> _unlocked() versions of ovl_cleanup() and ovl_lookup_upper().  These
> will be used more widely in future patches.
>
> ovl_cleanup_unlocked() takes a dentry for the directory rather than an
> inode as this simplifies calling slightly.
>
> Note that when we move a lookup or create out of a locked region in
> which the dentry is acted on, we need to ensure after taking the lock
> that the dentry is still in the directory we expect it to be in.  It is
> conceivable that it was moved.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/copy_up.c   | 37 +++++++++++-------
>  fs/overlayfs/dir.c       | 84 +++++++++++++++++++++++++---------------
>  fs/overlayfs/overlayfs.h | 10 +++++
>  fs/overlayfs/super.c     |  7 ++--
>  4 files changed, 88 insertions(+), 50 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8a3c0d18ec2e..7a21ad1f2b6e 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -517,15 +517,12 @@ 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)
>  {
>         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 = { };
> @@ -558,17 +555,21 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
>         err = ovl_set_upper_fh(ofs, upper, temp);
>         if (err)
>                 goto out;
> -
> +       lock_rename(indexdir, indexdir);

This is a really strange hack.
I assume your next patch set is going to remove this call, but I do not wish
to merge this hack as is for an unknown period of time.

Please introduce helpers {un,}lock_parent()

>         index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
>         if (IS_ERR(index)) {
>                 err = PTR_ERR(index);
> +       } else if (temp->d_parent != indexdir) {
> +               err = -EINVAL;
> +               dput(index);

This can be inside lock_parent(parent, child)
where child is an optional arg.

err = lock_parent(indexdir, temp);
if (err)
       goto out;

Because we should be checking this right after lock and
not after ovl_lookup_upper().

>         } else {
>                 err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
>                 dput(index);
>         }
> +       unlock_rename(indexdir, indexdir);
>  out:
>         if (err)
> -               ovl_cleanup(ofs, dir, temp);
> +               ovl_cleanup_unlocked(ofs, indexdir, temp);
>         dput(temp);
>  free_name:
>         kfree(name.name);
> @@ -779,9 +780,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);
>
> @@ -794,6 +793,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>          */
>         path.dentry = temp;
>         err = ovl_copy_up_data(c, &path);
> +       if (err)
> +               goto cleanup_write_unlocked;
>         /*
>          * We cannot hold lock_rename() throughout this helper, because of
>          * lock ordering with sb_writers, which shouldn't be held when calling
> @@ -801,6 +802,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>          * temp wasn't moved before copy up completion or cleanup.
>          */
>         ovl_start_write(c->dentry);
> +
> +       if (S_ISDIR(c->stat.mode) && c->indexed) {
> +               err = ovl_create_index(c->dentry, c->origin_fh, temp);
> +               if (err)
> +                       goto cleanup_unlocked;
> +       }
> +
>         trap = lock_rename(c->workdir, c->destdir);
>         if (trap || temp->d_parent != c->workdir) {
>                 /* temp or workdir moved underneath us? abort without cleanup */
> @@ -809,20 +817,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>                 if (IS_ERR(trap))
>                         goto out;
>                 goto unlock;
> -       } else if (err) {
> -               goto cleanup;
>         }
>
>         err = ovl_copy_up_metadata(c, temp);
>         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);
> @@ -857,6 +857,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>         ovl_cleanup(ofs, wdir, temp);
>         dput(temp);
>         goto unlock;
> +
> +cleanup_write_unlocked:
> +       ovl_start_write(c->dentry);
> +cleanup_unlocked:
> +       ovl_cleanup_unlocked(ofs, c->workdir, temp);
> +       dput(temp);
> +       goto out;
>  }

Wow these various cleanup flows are quite hard to follow.
This is a massive patch set which is very hard for me to review
and it will also be hard for me to maintain the code as it is.
We need to figure out a way to simplify the code flow
either more re-factoring or using some scoped cleanup hooks.
I am open to suggestions.

Thanks,
Amir.

>
>  /* 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..a51a3dc02bf5 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;
> +
> +       inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
> +       if (wdentry->d_parent == workdir)
> +               ovl_cleanup(ofs, workdir->d_inode, wdentry);
> +       else
> +               err = -EINVAL;
> +       inode_unlock(workdir->d_inode);
> +
> +       return err;
> +}
> +
>  struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
>  {
>         struct dentry *temp;
> @@ -199,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,
> @@ -348,28 +367,30 @@ 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;
> +       /* This test is racey but we re-test under the lock */
> +       if (upper->d_parent != upperdir)
> +               goto out;
>
>         opaquedir = ovl_create_temp(ofs, workdir, OVL_CATTR(stat.mode));
>         err = PTR_ERR(opaquedir);
>         if (IS_ERR(opaquedir))
> -               goto out_unlock;
> -
> +               /* workdir was unlocked, no upperdir */
> +               goto out;
> +       err = ovl_lock_rename_workdir(workdir, upperdir);
> +       if (err)
> +               goto out_cleanup_unlocked;
> +       if (upper->d_parent->d_inode != udir)
> +               goto out_cleanup;
>         err = ovl_copy_xattr(dentry->d_sb, &upperpath, opaquedir);
>         if (err)
>                 goto out_cleanup;
> @@ -398,10 +419,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);
>  }
> @@ -439,15 +460,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))
> @@ -458,6 +475,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, upperdir);
> +       if (err)
> +               goto out_cleanup;
> +
>         /*
>          * mode could have been mutilated due to umask (e.g. sgid directory)
>          */
> @@ -472,35 +493,35 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>                 err = ovl_do_notify_change(ofs, newdentry, &attr);
>                 inode_unlock(newdentry->d_inode);
>                 if (err)
> -                       goto out_cleanup;
> +                       goto out_cleanup_locked;
>         }
>         if (!hardlink) {
>                 err = ovl_set_upper_acl(ofs, newdentry,
>                                         XATTR_NAME_POSIX_ACL_ACCESS, acl);
>                 if (err)
> -                       goto out_cleanup;
> +                       goto out_cleanup_locked;
>
>                 err = ovl_set_upper_acl(ofs, newdentry,
>                                         XATTR_NAME_POSIX_ACL_DEFAULT, default_acl);
>                 if (err)
> -                       goto out_cleanup;
> +                       goto out_cleanup_locked;
>         }
>
>         if (!hardlink && S_ISDIR(cattr->mode)) {
>                 err = ovl_set_opaque(dentry, newdentry);
>                 if (err)
> -                       goto out_cleanup;
> +                       goto out_cleanup_locked;
>
>                 err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
>                                     RENAME_EXCHANGE);
>                 if (err)
> -                       goto out_cleanup;
> +                       goto out_cleanup_locked;
>
>                 ovl_cleanup(ofs, wdir, upper);
>         } else {
>                 err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
>                 if (err)
> -                       goto out_cleanup;
> +                       goto out_cleanup_locked;
>         }
>         ovl_dir_modified(dentry->d_parent, false);
>         err = ovl_instantiate(dentry, inode, newdentry, hardlink, NULL);
> @@ -508,10 +529,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);
> @@ -519,8 +539,10 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         }
>         return err;
>
> +out_cleanup_locked:
> +       unlock_rename(workdir, upperdir);
>  out_cleanup:
> -       ovl_cleanup(ofs, wdir, newdentry);
> +       ovl_cleanup_unlocked(ofs, workdir, newdentry);
>         dput(newdentry);
>         goto out_dput;
>  }
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 42228d10f6b9..508003e26e08 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)
> @@ -843,6 +852,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/super.c b/fs/overlayfs/super.c
> index db046b0d6a68..576b5c3b537c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -558,13 +558,12 @@ 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;
>
> +       lock_rename(workdir, workdir);
>         dest = ovl_lookup_temp(ofs, workdir);
>         err = PTR_ERR(dest);
>         if (IS_ERR(dest)) {
> @@ -600,7 +599,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>         dput(dest);
>
>  out_unlock:
> -       inode_unlock(dir);
> +       unlock_rename(workdir, workdir);
>
>         return err;
>  }
> --
> 2.49.0
>

Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.

[Amir Goldstein]

On Wed, Jun 25, 2025 at 5:44 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > 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() and ovl_create_index() out
> > of the locked region and has them take and release the relevant lock
> > themselves.
> >
> > The lock that was taken before these functions are called is now taken
> > after.  This means that any code between where the lock was taken and
> > these calls is now unlocked.  This necessitates the creation of
> > _unlocked() versions of ovl_cleanup() and ovl_lookup_upper().  These
> > will be used more widely in future patches.
> >
> > ovl_cleanup_unlocked() takes a dentry for the directory rather than an
> > inode as this simplifies calling slightly.
> >
> > Note that when we move a lookup or create out of a locked region in
> > which the dentry is acted on, we need to ensure after taking the lock
> > that the dentry is still in the directory we expect it to be in.  It is
> > conceivable that it was moved.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/copy_up.c   | 37 +++++++++++-------
> >  fs/overlayfs/dir.c       | 84 +++++++++++++++++++++++++---------------
> >  fs/overlayfs/overlayfs.h | 10 +++++
> >  fs/overlayfs/super.c     |  7 ++--
> >  4 files changed, 88 insertions(+), 50 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 8a3c0d18ec2e..7a21ad1f2b6e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -517,15 +517,12 @@ 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)
> >  {
> >         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 = { };
> > @@ -558,17 +555,21 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> >         err = ovl_set_upper_fh(ofs, upper, temp);
> >         if (err)
> >                 goto out;
> > -
> > +       lock_rename(indexdir, indexdir);
>
> This is a really strange hack.
> I assume your next patch set is going to remove this call, but I do not wish
> to merge this hack as is for an unknown period of time.
>
> Please introduce helpers {un,}lock_parent()
>
> >         index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> >         if (IS_ERR(index)) {
> >                 err = PTR_ERR(index);
> > +       } else if (temp->d_parent != indexdir) {
> > +               err = -EINVAL;
> > +               dput(index);
>
> This can be inside lock_parent(parent, child)
> where child is an optional arg.
>
> err = lock_parent(indexdir, temp);
> if (err)
>        goto out;
>
> Because we should be checking this right after lock and
> not after ovl_lookup_upper().
>
> >         } else {
> >                 err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> >                 dput(index);
> >         }
> > +       unlock_rename(indexdir, indexdir);
> >  out:
> >         if (err)
> > -               ovl_cleanup(ofs, dir, temp);
> > +               ovl_cleanup_unlocked(ofs, indexdir, temp);
> >         dput(temp);
> >  free_name:
> >         kfree(name.name);
> > @@ -779,9 +780,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);
> >
> > @@ -794,6 +793,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >          */
> >         path.dentry = temp;
> >         err = ovl_copy_up_data(c, &path);
> > +       if (err)
> > +               goto cleanup_write_unlocked;
> >         /*
> >          * We cannot hold lock_rename() throughout this helper, because of
> >          * lock ordering with sb_writers, which shouldn't be held when calling
> > @@ -801,6 +802,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >          * temp wasn't moved before copy up completion or cleanup.
> >          */
> >         ovl_start_write(c->dentry);
> > +
> > +       if (S_ISDIR(c->stat.mode) && c->indexed) {
> > +               err = ovl_create_index(c->dentry, c->origin_fh, temp);
> > +               if (err)
> > +                       goto cleanup_unlocked;
> > +       }
> > +
> >         trap = lock_rename(c->workdir, c->destdir);
> >         if (trap || temp->d_parent != c->workdir) {
> >                 /* temp or workdir moved underneath us? abort without cleanup */
> > @@ -809,20 +817,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >                 if (IS_ERR(trap))
> >                         goto out;
> >                 goto unlock;
> > -       } else if (err) {
> > -               goto cleanup;
> >         }
> >
> >         err = ovl_copy_up_metadata(c, temp);
> >         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);
> > @@ -857,6 +857,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >         ovl_cleanup(ofs, wdir, temp);
> >         dput(temp);
> >         goto unlock;
> > +
> > +cleanup_write_unlocked:
> > +       ovl_start_write(c->dentry);
> > +cleanup_unlocked:
> > +       ovl_cleanup_unlocked(ofs, c->workdir, temp);
> > +       dput(temp);
> > +       goto out;
> >  }
>
> Wow these various cleanup flows are quite hard to follow.
> This is a massive patch set which is very hard for me to review
> and it will also be hard for me to maintain the code as it is.
> We need to figure out a way to simplify the code flow
> either more re-factoring or using some scoped cleanup hooks.
> I am open to suggestions.
>
> Thanks,
> Amir.
>
> >
> >  /* 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..a51a3dc02bf5 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;
> > +
> > +       inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
> > +       if (wdentry->d_parent == workdir)
> > +               ovl_cleanup(ofs, workdir->d_inode, wdentry);
> > +       else
> > +               err = -EINVAL;
> > +       inode_unlock(workdir->d_inode);
> > +
> > +       return err;
> > +}
> > +

Just to illustrate what I meant and how the flow of ovl_cleanup_unlocked()
and later on ovl_cleanup() would look simpler:

int lock_parent(struct dentry *parent, struct dentry *child)
{
       int err;

       inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
       if (!child || child->d_parent == parent)
              return 0;

       inode_unlock(parent->d_inode);
       return -EINVAL;
}

int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
                        struct dentry *wdentry)
{
       int err;

       err = parent_lock(workdir, wdentry);
       if (err)
               return err;

       ovl_cleanup(ofs, workdir->d_inode, wdentry);
       parent_unlock(workdir);
       return 0;
}

Thanks,
Amir.

Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.

[Dan Carpenter]

Hi NeilBrown,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/NeilBrown/ovl-use-is_subdir-for-testing-if-one-thing-is-a-subdir-of-another/20250625-070919
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20250624230636.3233059-3-neil%40brown.name
patch subject: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.
config: x86_64-randconfig-161-20250627 (https://download.01.org/0day-ci/archive/20250628/202506281017.jeQF1pnr-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202506281017.jeQF1pnr-lkp@intel.com/

New smatch warnings:
fs/overlayfs/dir.c:427 ovl_clear_empty() warn: passing zero to 'ERR_PTR'

vim +/ERR_PTR +427 fs/overlayfs/dir.c

e9be9d5e76e348 Miklos Szeredi    2014-10-24  353  static struct dentry *ovl_clear_empty(struct dentry *dentry,
e9be9d5e76e348 Miklos Szeredi    2014-10-24  354  				      struct list_head *list)
e9be9d5e76e348 Miklos Szeredi    2014-10-24  355  {
576bb263450bbb Christian Brauner 2022-04-04  356  	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  357  	struct dentry *workdir = ovl_workdir(dentry);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  358  	struct inode *wdir = workdir->d_inode;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  359  	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  360  	struct inode *udir = upperdir->d_inode;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  361  	struct path upperpath;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  362  	struct dentry *upper;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  363  	struct dentry *opaquedir;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  364  	struct kstat stat;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  365  	int err;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  366  
cc6f67bcafcb6b Miklos Szeredi    2015-05-19  367  	if (WARN_ON(!workdir))
cc6f67bcafcb6b Miklos Szeredi    2015-05-19  368  		return ERR_PTR(-EROFS);
cc6f67bcafcb6b Miklos Szeredi    2015-05-19  369  
e9be9d5e76e348 Miklos Szeredi    2014-10-24  370  	ovl_path_upper(dentry, &upperpath);
a528d35e8bfcc5 David Howells     2017-01-31  371  	err = vfs_getattr(&upperpath, &stat,
a528d35e8bfcc5 David Howells     2017-01-31  372  			  STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  373  	if (err)
fb1b87daadb6ed NeilBrown         2025-06-25  374  		goto out;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  375  
e9be9d5e76e348 Miklos Szeredi    2014-10-24  376  	err = -ESTALE;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  377  	if (!S_ISDIR(stat.mode))
fb1b87daadb6ed NeilBrown         2025-06-25  378  		goto out;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  379  	upper = upperpath.dentry;
fb1b87daadb6ed NeilBrown         2025-06-25  380  	/* This test is racey but we re-test under the lock */
fb1b87daadb6ed NeilBrown         2025-06-25  381  	if (upper->d_parent != upperdir)
fb1b87daadb6ed NeilBrown         2025-06-25  382  		goto out;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  383  
576bb263450bbb Christian Brauner 2022-04-04  384  	opaquedir = ovl_create_temp(ofs, workdir, OVL_CATTR(stat.mode));
e9be9d5e76e348 Miklos Szeredi    2014-10-24  385  	err = PTR_ERR(opaquedir);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  386  	if (IS_ERR(opaquedir))
fb1b87daadb6ed NeilBrown         2025-06-25  387  		/* workdir was unlocked, no upperdir */
fb1b87daadb6ed NeilBrown         2025-06-25  388  		goto out;
fb1b87daadb6ed NeilBrown         2025-06-25  389  	err = ovl_lock_rename_workdir(workdir, upperdir);
fb1b87daadb6ed NeilBrown         2025-06-25  390  	if (err)
fb1b87daadb6ed NeilBrown         2025-06-25  391  		goto out_cleanup_unlocked;
fb1b87daadb6ed NeilBrown         2025-06-25  392  	if (upper->d_parent->d_inode != udir)
fb1b87daadb6ed NeilBrown         2025-06-25  393  		goto out_cleanup;

Should there be an error code for this?

dad7017a840d8d Christian Brauner 2022-04-04  394  	err = ovl_copy_xattr(dentry->d_sb, &upperpath, opaquedir);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  395  	if (err)
e9be9d5e76e348 Miklos Szeredi    2014-10-24  396  		goto out_cleanup;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  397  
5cf5b477f0ca33 Miklos Szeredi    2016-12-16  398  	err = ovl_set_opaque(dentry, opaquedir);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  399  	if (err)
e9be9d5e76e348 Miklos Szeredi    2014-10-24  400  		goto out_cleanup;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  401  
5955102c9984fa Al Viro           2016-01-22  402  	inode_lock(opaquedir->d_inode);
5272eaf3a56827 Christian Brauner 2022-04-04  403  	err = ovl_set_attr(ofs, opaquedir, &stat);
5955102c9984fa Al Viro           2016-01-22  404  	inode_unlock(opaquedir->d_inode);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  405  	if (err)
e9be9d5e76e348 Miklos Szeredi    2014-10-24  406  		goto out_cleanup;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  407  
bc9241367aac08 NeilBrown         2025-06-13  408  	err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  409  	if (err)
e9be9d5e76e348 Miklos Szeredi    2014-10-24  410  		goto out_cleanup;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  411  
576bb263450bbb Christian Brauner 2022-04-04  412  	ovl_cleanup_whiteouts(ofs, upper, list);
576bb263450bbb Christian Brauner 2022-04-04  413  	ovl_cleanup(ofs, wdir, upper);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  414  	unlock_rename(workdir, upperdir);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  415  
e9be9d5e76e348 Miklos Szeredi    2014-10-24  416  	/* dentry's upper doesn't match now, get rid of it */
e9be9d5e76e348 Miklos Szeredi    2014-10-24  417  	d_drop(dentry);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  418  
e9be9d5e76e348 Miklos Szeredi    2014-10-24  419  	return opaquedir;
e9be9d5e76e348 Miklos Szeredi    2014-10-24  420  
e9be9d5e76e348 Miklos Szeredi    2014-10-24  421  out_cleanup:
e9be9d5e76e348 Miklos Szeredi    2014-10-24  422  	unlock_rename(workdir, upperdir);
fb1b87daadb6ed NeilBrown         2025-06-25  423  out_cleanup_unlocked:
fb1b87daadb6ed NeilBrown         2025-06-25  424  	ovl_cleanup_unlocked(ofs, workdir, opaquedir);
fb1b87daadb6ed NeilBrown         2025-06-25  425  	dput(opaquedir);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  426  out:
e9be9d5e76e348 Miklos Szeredi    2014-10-24 @427  	return ERR_PTR(err);
e9be9d5e76e348 Miklos Szeredi    2014-10-24  428  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir()

[NeilBrown]

In ovl_copy_up_workdir() unlock immediately after the rename, and then
use ovl_cleanup_unlocked() with separate locking rather than using the
same lock to protect both.

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/copy_up.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 7a21ad1f2b6e..884c738b67ff 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -763,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;
@@ -793,8 +792,10 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	 */
 	path.dentry = temp;
 	err = ovl_copy_up_data(c, &path);
-	if (err)
-		goto cleanup_write_unlocked;
+	if (err) {
+		ovl_start_write(c->dentry);
+		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
@@ -814,9 +815,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 		/* temp or workdir moved underneath us? abort without cleanup */
 		dput(temp);
 		err = -EIO;
-		if (IS_ERR(trap))
-			goto out;
-		goto unlock;
+		if (!IS_ERR(trap))
+			unlock_rename(c->workdir, c->destdir);
+		goto out;
 	}
 
 	err = ovl_copy_up_metadata(c, temp);
@@ -830,9 +831,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)
@@ -846,20 +848,13 @@ 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);
 
 	return err;
 
 cleanup:
-	ovl_cleanup(ofs, wdir, temp);
-	dput(temp);
-	goto unlock;
-
-cleanup_write_unlocked:
-	ovl_start_write(c->dentry);
+	unlock_rename(c->workdir, c->destdir);
 cleanup_unlocked:
 	ovl_cleanup_unlocked(ofs, c->workdir, temp);
 	dput(temp);
-- 
2.49.0

Re: [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> In ovl_copy_up_workdir() unlock immediately after the rename, and then
> use ovl_cleanup_unlocked() with separate locking rather than using the
> same lock to protect both.
>
> 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>

FYI I am not reviewing this one because the code was made too much
spaghetti to my taste by patch 2, so I will wait for a better version

> ---
>  fs/overlayfs/copy_up.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 7a21ad1f2b6e..884c738b67ff 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -763,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;
> @@ -793,8 +792,10 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>          */
>         path.dentry = temp;
>         err = ovl_copy_up_data(c, &path);
> -       if (err)
> -               goto cleanup_write_unlocked;
> +       if (err) {
> +               ovl_start_write(c->dentry);
> +               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
> @@ -814,9 +815,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>                 /* temp or workdir moved underneath us? abort without cleanup */
>                 dput(temp);
>                 err = -EIO;
> -               if (IS_ERR(trap))
> -                       goto out;
> -               goto unlock;
> +               if (!IS_ERR(trap))
> +                       unlock_rename(c->workdir, c->destdir);
> +               goto out;
>         }
>
>         err = ovl_copy_up_metadata(c, temp);
> @@ -830,9 +831,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)
> @@ -846,20 +848,13 @@ 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);
>
>         return err;
>
>  cleanup:
> -       ovl_cleanup(ofs, wdir, temp);
> -       dput(temp);
> -       goto unlock;
> -
> -cleanup_write_unlocked:
> -       ovl_start_write(c->dentry);
> +       unlock_rename(c->workdir, c->destdir);
>  cleanup_unlocked:
>         ovl_cleanup_unlocked(ofs, c->workdir, temp);
>         dput(temp);
> --
> 2.49.0
>

[PATCH 04/12] 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.

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

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index a51a3dc02bf5..2d67704d641e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -326,9 +326,10 @@ 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);
+	inode_unlock(udir);
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
-		goto out_unlock;
+		goto out;
 
 	if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry) &&
 	    !ovl_allow_offline_changes(ofs)) {
@@ -340,14 +341,13 @@ 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);
+out:
 	return err;
 
 out_cleanup:
-	ovl_cleanup(ofs, udir, newdentry);
+	ovl_cleanup_unlocked(ofs, upperdir, newdentry);
 	dput(newdentry);
-	goto out_unlock;
+	goto out;
 }
 
 static struct dentry *ovl_clear_empty(struct dentry *dentry,
-- 
2.49.0

Re: [PATCH 04/12] ovl: narrow locking in ovl_create_upper()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> 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.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/dir.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index a51a3dc02bf5..2d67704d641e 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -326,9 +326,10 @@ 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);
> +       inode_unlock(udir);
>         err = PTR_ERR(newdentry);
>         if (IS_ERR(newdentry))
> -               goto out_unlock;
> +               goto out;
>
>         if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry) &&
>             !ovl_allow_offline_changes(ofs)) {
> @@ -340,14 +341,13 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,

>        ovl_dir_modified(dentry->d_parent, false);

inside ovl_dir_modified() =>ovl_dir_version_inc() there is:
   WARN_ON(!inode_is_locked(inode));

so why is this WARN_ON not triggered by this change?
either there are more changes that fix it later,
or your tests did not cover this (seems unlikely)
or you did not look in dmesg and overlay fstests do not check for it?
some other explanation?

Thanks,
Amir.

Re: [PATCH 04/12] ovl: narrow locking in ovl_create_upper()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 7:55 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > 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.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/dir.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index a51a3dc02bf5..2d67704d641e 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -326,9 +326,10 @@ 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);
> > +       inode_unlock(udir);
> >         err = PTR_ERR(newdentry);
> >         if (IS_ERR(newdentry))
> > -               goto out_unlock;
> > +               goto out;
> >
> >         if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry) &&
> >             !ovl_allow_offline_changes(ofs)) {
> > @@ -340,14 +341,13 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>
> >        ovl_dir_modified(dentry->d_parent, false);
>
> inside ovl_dir_modified() =>ovl_dir_version_inc() there is:
>    WARN_ON(!inode_is_locked(inode));
>
> so why is this WARN_ON not triggered by this change?
> either there are more changes that fix it later,
> or your tests did not cover this (seems unlikely)
> or you did not look in dmesg and overlay fstests do not check for it?
> some other explanation?
>

The latter - the assertion is on the ovl dir inode lock and you dropped
the upper dir inode lock.

Feel free to add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

[PATCH 05/12] 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.

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

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 2d67704d641e..e3ea7d02219f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -355,7 +355,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 inode *udir = upperdir->d_inode;
 	struct path upperpath;
@@ -408,10 +407,10 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
 	if (err)
 		goto out_cleanup;
+	unlock_rename(workdir, upperdir);
 
 	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

Re: [PATCH 05/12] ovl: narrow locking in ovl_clear_empty()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> 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.
>
> Signed-off-by: NeilBrown <neil@brown.name>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/dir.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 2d67704d641e..e3ea7d02219f 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -355,7 +355,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 inode *udir = upperdir->d_inode;
>         struct path upperpath;
> @@ -408,10 +407,10 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>         err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
>         if (err)
>                 goto out_cleanup;
> +       unlock_rename(workdir, upperdir);
>
>         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 06/12] 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.

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 e3ea7d02219f..2b879d7c386e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -440,9 +440,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;
@@ -513,22 +511,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_locked;
+			goto out_cleanup;
 
-		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_locked;
+			goto out_cleanup;
 	}
 	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

Re: [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> 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.
>
> Signed-off-by: NeilBrown <neil@brown.name>

FYI I am not reviewing this one because the code was made too much
spaghetti to my taste by patch 2, so I will wait for a better version

Thanks,
Amir.

> ---
>  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 e3ea7d02219f..2b879d7c386e 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -440,9 +440,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;
> @@ -513,22 +511,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_locked;
> +                       goto out_cleanup;
>
> -               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_locked;
> +                       goto out_cleanup;
>         }
>         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 07/12] 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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 2b879d7c386e..5afe17cee305 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1270,9 +1270,10 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 			    new_upperdir, newdentry, flags);
 	if (err)
 		goto out_dput;
+	unlock_rename(new_upperdir, old_upperdir);
 
 	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)
@@ -1291,12 +1292,8 @@ 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:
 	ovl_revert_creds(old_cred);
 	if (update_nlink)
@@ -1307,6 +1304,14 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	dput(opaquedir);
 	ovl_cache_free(&list);
 	return err;
+
+out_dput:
+	dput(newdentry);
+out_dput_old:
+	dput(olddentry);
+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 07/12] ovl: narrow locking in ovl_rename()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 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>
> ---
>  fs/overlayfs/dir.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 2b879d7c386e..5afe17cee305 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1270,9 +1270,10 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>                             new_upperdir, newdentry, flags);
>         if (err)
>                 goto out_dput;
> +       unlock_rename(new_upperdir, old_upperdir);
>
>         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)
> @@ -1291,12 +1292,8 @@ 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:
>         ovl_revert_creds(old_cred);
>         if (update_nlink)
> @@ -1307,6 +1304,14 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         dput(opaquedir);
>         ovl_cache_free(&list);
>         return err;
> +
> +out_dput:
> +       dput(newdentry);
> +out_dput_old:
> +       dput(olddentry);
> +out_unlock:
> +       unlock_rename(new_upperdir, old_upperdir);
> +       goto out_revert_creds;
>  }

Once again, I really do not like the resulting code flow.
This is a huge function and impossile to follow all condition.
As a rule of thumb, I think you need to factor out the block of code under
lock_rename() to avoid those horrible goto mazes.

The no error case used to do dput(newdentry) and dput(olddentry)
how come they are gone now?
what am I missing?

Thanks,
Amir.

Re: [PATCH 07/12] ovl: narrow locking in ovl_rename()

[NeilBrown]

On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 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>
> > ---
> >  fs/overlayfs/dir.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 2b879d7c386e..5afe17cee305 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -1270,9 +1270,10 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> >                             new_upperdir, newdentry, flags);
> >         if (err)
> >                 goto out_dput;
> > +       unlock_rename(new_upperdir, old_upperdir);
> >
> >         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)
> > @@ -1291,12 +1292,8 @@ 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:
> >         ovl_revert_creds(old_cred);
> >         if (update_nlink)
> > @@ -1307,6 +1304,14 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> >         dput(opaquedir);
> >         ovl_cache_free(&list);
> >         return err;
> > +
> > +out_dput:
> > +       dput(newdentry);
> > +out_dput_old:
> > +       dput(olddentry);
> > +out_unlock:
> > +       unlock_rename(new_upperdir, old_upperdir);
> > +       goto out_revert_creds;
> >  }
> 
> Once again, I really do not like the resulting code flow.
> This is a huge function and impossile to follow all condition.
> As a rule of thumb, I think you need to factor out the block of code under
> lock_rename() to avoid those horrible goto mazes.

I'll see what I can do to improve it.

> 
> The no error case used to do dput(newdentry) and dput(olddentry)
> how come they are gone now?
> what am I missing?

Those dput()s are still there.  I removed the goto labels between them
but not the dput()s themselves.

Thanks,
NeilBrown

> 
> Thanks,
> Amir.
> 

[PATCH 08/12] 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.

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

Re: [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> 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.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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 09/12] ovl: whiteout locking changes

[NeilBrown]

ovl_writeout() relies on the workdir i_rwsem to provide exclusive access
to ofs->writeout which it manipulates.  Rather than depending on this,
add a new mutex, "writeout_lock" to explicitly provide the required
locking.

Then take the lock on workdir only when needed - to lookup the temp name
and to do the whiteout or link.  So ovl_writeout() and similarly
ovl_cleanup_and_writeout() and ovl_workdir_cleanup() are now called
without the lock held.  This requires changes to
ovl_remove_and_whiteout(), ovl_cleanup_index(), ovl_indexdir_cleanup(),
and ovl_workdir_create().

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/overlayfs/dir.c       | 71 +++++++++++++++++++++-------------------
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/params.c    |  2 ++
 fs/overlayfs/readdir.c   | 31 ++++++++++--------
 fs/overlayfs/super.c     |  9 +++--
 fs/overlayfs/util.c      |  7 ++--
 7 files changed, 67 insertions(+), 56 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 5afe17cee305..78b0d956b0ac 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,47 +84,51 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 	struct dentry *workdir = ofs->workdir;
 	struct inode *wdir = workdir->d_inode;
 
+	mutex_lock(&ofs->whiteout_lock);
 	if (!ofs->whiteout) {
+		inode_lock(wdir);
 		whiteout = ovl_lookup_temp(ofs, workdir);
+		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))
 			goto out;
-
-		err = ovl_do_whiteout(ofs, wdir, whiteout);
-		if (err) {
-			dput(whiteout);
-			whiteout = ERR_PTR(err);
-			goto out;
-		}
 		ofs->whiteout = whiteout;
 	}
 
 	if (!ofs->no_shared_whiteout) {
+		inode_lock(wdir);
 		whiteout = ovl_lookup_temp(ofs, workdir);
-		if (IS_ERR(whiteout))
-			goto out;
-
-		err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
-		if (!err)
+		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) || PTR_ERR(whiteout) != -EMLINK)
 			goto out;
 
-		if (err != -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);
+		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;
 	}
 	whiteout = ofs->whiteout;
 	ofs->whiteout = NULL;
 out:
+	mutex_unlock(&ofs->whiteout_lock);
 	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 +141,26 @@ 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, dir);
+	if (!err) {
+		if (whiteout->d_parent == ofs->workdir)
+			err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir,
+					    dentry, flags);
+		else
+			err = -EINVAL;
+		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;
 }
 
@@ -777,15 +788,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 			goto out;
 	}
 
-	err = ovl_lock_rename_workdir(workdir, upperdir);
-	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) ||
@@ -803,8 +810,6 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 	d_drop(dentry);
 out_dput_upper:
 	dput(upper);
-out_unlock:
-	unlock_rename(workdir, upperdir);
 out_dput:
 	dput(opaquedir);
 out:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 508003e26e08..25378b81251e 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -732,7 +732,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/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:
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 2a222b8185a3..fd98444dacef 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1141,7 +1141,8 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 		if (IS_ERR(dentry))
 			continue;
 		if (dentry->d_inode)
-			err = ovl_workdir_cleanup(ofs, dir, path->mnt, dentry, level);
+			err = ovl_workdir_cleanup(ofs, path->dentry, path->mnt,
+						  dentry, level);
 		dput(dentry);
 		if (err)
 			break;
@@ -1152,24 +1153,27 @@ 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);
+		return ovl_cleanup_unlocked(ofs, parent, dentry);
 	}
 
-	err = ovl_do_rmdir(ofs, dir, dentry);
+	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
+	if (dentry->d_parent == parent)
+		err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
+	else
+		err = -EINVAL;
+	inode_unlock(parent->d_inode);
 	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;
@@ -1180,7 +1184,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;
@@ -1194,7 +1197,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 +1204,8 @@ 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 +1213,8 @@ 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_workdir_cleanup(ofs, indexdir, path.mnt,
+						  index, 1);
 			if (err)
 				break;
 			goto next;
@@ -1220,7 +1224,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
@@ -1236,7 +1240,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(ofs, dir, index);
+			err = ovl_cleanup_unlocked(ofs, indexdir, index);
 		}
 
 		if (err)
@@ -1247,7 +1251,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 		index = NULL;
 	}
 	dput(index);
-	inode_unlock(dir);
 out:
 	ovl_cache_free(&list);
 	if (err)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 576b5c3b537c..3583e359655f 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,6 +311,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 
 		if (work->d_inode) {
 			err = -EEXIST;
+			inode_unlock(dir);
 			if (retried)
 				goto out_dput;
 
@@ -318,7 +319,8 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 				goto out_unlock;
 
 			retried = true;
-			err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
+			err = ovl_workdir_cleanup(ofs, ofs->workbasedir, mnt,
+						  work, 0);
 			dput(work);
 			if (err == -EINVAL) {
 				work = ERR_PTR(err);
@@ -328,6 +330,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 		}
 
 		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 +368,11 @@ 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:
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 2b4754c645ee..565f7d8c0147 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,8 +1106,7 @@ 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;
@@ -1118,10 +1116,9 @@ static void ovl_cleanup_index(struct dentry *dentry)
 					       indexdir, index);
 	} 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 09/12] ovl: whiteout locking changes

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> ovl_writeout() relies on the workdir i_rwsem to provide exclusive access
> to ofs->writeout which it manipulates.  Rather than depending on this,

typo writeout/whiteout all over this commit message

> add a new mutex, "writeout_lock" to explicitly provide the required
> locking.
>
> Then take the lock on workdir only when needed - to lookup the temp name
> and to do the whiteout or link.  So ovl_writeout() and similarly
> ovl_cleanup_and_writeout() and ovl_workdir_cleanup() are now called
> without the lock held.  This requires changes to
> ovl_remove_and_whiteout(), ovl_cleanup_index(), ovl_indexdir_cleanup(),
> and ovl_workdir_create().
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/dir.c       | 71 +++++++++++++++++++++-------------------
>  fs/overlayfs/overlayfs.h |  2 +-
>  fs/overlayfs/ovl_entry.h |  1 +
>  fs/overlayfs/params.c    |  2 ++
>  fs/overlayfs/readdir.c   | 31 ++++++++++--------
>  fs/overlayfs/super.c     |  9 +++--
>  fs/overlayfs/util.c      |  7 ++--
>  7 files changed, 67 insertions(+), 56 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 5afe17cee305..78b0d956b0ac 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,47 +84,51 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
>         struct dentry *workdir = ofs->workdir;
>         struct inode *wdir = workdir->d_inode;
>
> +       mutex_lock(&ofs->whiteout_lock);
>         if (!ofs->whiteout) {
> +               inode_lock(wdir);
>                 whiteout = ovl_lookup_temp(ofs, workdir);
> +               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))
>                         goto out;
> -
> -               err = ovl_do_whiteout(ofs, wdir, whiteout);
> -               if (err) {
> -                       dput(whiteout);
> -                       whiteout = ERR_PTR(err);
> -                       goto out;
> -               }
>                 ofs->whiteout = whiteout;
>         }
>
>         if (!ofs->no_shared_whiteout) {
> +               inode_lock(wdir);
>                 whiteout = ovl_lookup_temp(ofs, workdir);
> -               if (IS_ERR(whiteout))
> -                       goto out;
> -
> -               err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
> -               if (!err)
> +               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) || PTR_ERR(whiteout) != -EMLINK)
>                         goto out;
>
> -               if (err != -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);

Where did this dput go?

> +               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;
>         }
>         whiteout = ofs->whiteout;
>         ofs->whiteout = NULL;
>  out:
> +       mutex_unlock(&ofs->whiteout_lock);
>         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 +141,26 @@ 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, dir);
> +       if (!err) {
> +               if (whiteout->d_parent == ofs->workdir)
> +                       err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir,
> +                                           dentry, flags);
> +               else
> +                       err = -EINVAL;
> +               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;
>  }
>
> @@ -777,15 +788,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
>                         goto out;
>         }
>
> -       err = ovl_lock_rename_workdir(workdir, upperdir);
> -       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) ||
> @@ -803,8 +810,6 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
>         d_drop(dentry);
>  out_dput_upper:
>         dput(upper);
> -out_unlock:
> -       unlock_rename(workdir, upperdir);
>  out_dput:
>         dput(opaquedir);
>  out:
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 508003e26e08..25378b81251e 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -732,7 +732,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/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:
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 2a222b8185a3..fd98444dacef 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1141,7 +1141,8 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
>                 if (IS_ERR(dentry))
>                         continue;
>                 if (dentry->d_inode)
> -                       err = ovl_workdir_cleanup(ofs, dir, path->mnt, dentry, level);
> +                       err = ovl_workdir_cleanup(ofs, path->dentry, path->mnt,
> +                                                 dentry, level);
>                 dput(dentry);
>                 if (err)
>                         break;
> @@ -1152,24 +1153,27 @@ 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);
> +               return ovl_cleanup_unlocked(ofs, parent, dentry);
>         }
>
> -       err = ovl_do_rmdir(ofs, dir, dentry);
> +       inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> +       if (dentry->d_parent == parent)
> +               err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
> +       else
> +               err = -EINVAL;
> +       inode_unlock(parent->d_inode);
>         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;
> @@ -1180,7 +1184,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;
> @@ -1194,7 +1197,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 +1204,8 @@ 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 +1213,8 @@ 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_workdir_cleanup(ofs, indexdir, path.mnt,
> +                                                 index, 1);
>                         if (err)
>                                 break;
>                         goto next;
> @@ -1220,7 +1224,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
> @@ -1236,7 +1240,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(ofs, dir, index);
> +                       err = ovl_cleanup_unlocked(ofs, indexdir, index);
>                 }
>
>                 if (err)
> @@ -1247,7 +1251,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                 index = NULL;
>         }
>         dput(index);
> -       inode_unlock(dir);
>  out:
>         ovl_cache_free(&list);
>         if (err)
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 576b5c3b537c..3583e359655f 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,6 +311,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>
>                 if (work->d_inode) {
>                         err = -EEXIST;
> +                       inode_unlock(dir);
>                         if (retried)
>                                 goto out_dput;
>
> @@ -318,7 +319,8 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>                                 goto out_unlock;
>
>                         retried = true;
> -                       err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
> +                       err = ovl_workdir_cleanup(ofs, ofs->workbasedir, mnt,
> +                                                 work, 0);
>                         dput(work);
>                         if (err == -EINVAL) {
>                                 work = ERR_PTR(err);
> @@ -328,6 +330,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>                 }
>
>                 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 +368,11 @@ 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:

This label name is now misleading

> -       inode_unlock(dir);
>         return work;
>

Thanks,
Amir.

Re: [PATCH 09/12] ovl: whiteout locking changes

[NeilBrown]

On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > ovl_writeout() relies on the workdir i_rwsem to provide exclusive access
> > to ofs->writeout which it manipulates.  Rather than depending on this,
> 
> typo writeout/whiteout all over this commit message

Fixed - thanks.

> 
> > add a new mutex, "writeout_lock" to explicitly provide the required
> > locking.
> >
> > Then take the lock on workdir only when needed - to lookup the temp name
> > and to do the whiteout or link.  So ovl_writeout() and similarly
> > ovl_cleanup_and_writeout() and ovl_workdir_cleanup() are now called
> > without the lock held.  This requires changes to
> > ovl_remove_and_whiteout(), ovl_cleanup_index(), ovl_indexdir_cleanup(),
> > and ovl_workdir_create().
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/dir.c       | 71 +++++++++++++++++++++-------------------
> >  fs/overlayfs/overlayfs.h |  2 +-
> >  fs/overlayfs/ovl_entry.h |  1 +
> >  fs/overlayfs/params.c    |  2 ++
> >  fs/overlayfs/readdir.c   | 31 ++++++++++--------
> >  fs/overlayfs/super.c     |  9 +++--
> >  fs/overlayfs/util.c      |  7 ++--
> >  7 files changed, 67 insertions(+), 56 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 5afe17cee305..78b0d956b0ac 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,47 +84,51 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> >         struct dentry *workdir = ofs->workdir;
> >         struct inode *wdir = workdir->d_inode;
> >
> > +       mutex_lock(&ofs->whiteout_lock);
> >         if (!ofs->whiteout) {
> > +               inode_lock(wdir);
> >                 whiteout = ovl_lookup_temp(ofs, workdir);
> > +               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))
> >                         goto out;
> > -
> > -               err = ovl_do_whiteout(ofs, wdir, whiteout);
> > -               if (err) {
> > -                       dput(whiteout);
> > -                       whiteout = ERR_PTR(err);
> > -                       goto out;
> > -               }
> >                 ofs->whiteout = whiteout;
> >         }
> >
> >         if (!ofs->no_shared_whiteout) {
> > +               inode_lock(wdir);
> >                 whiteout = ovl_lookup_temp(ofs, workdir);
> > -               if (IS_ERR(whiteout))
> > -                       goto out;
> > -
> > -               err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
> > -               if (!err)
> > +               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) || PTR_ERR(whiteout) != -EMLINK)
> >                         goto out;
> >
> > -               if (err != -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);
> 
> Where did this dput go?

13 lines up in the patch.  It is called when ovl_do_link() fails.  It is
now closer to the ovl_do_link() call.

> 
> > +               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;
> >         }
> >         whiteout = ofs->whiteout;
> >         ofs->whiteout = NULL;
> >  out:
> > +       mutex_unlock(&ofs->whiteout_lock);
> >         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 +141,26 @@ 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, dir);
> > +       if (!err) {
> > +               if (whiteout->d_parent == ofs->workdir)
> > +                       err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir,
> > +                                           dentry, flags);
> > +               else
> > +                       err = -EINVAL;
> > +               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;
> >  }
> >
> > @@ -777,15 +788,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> >                         goto out;
> >         }
> >
> > -       err = ovl_lock_rename_workdir(workdir, upperdir);
> > -       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) ||
> > @@ -803,8 +810,6 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> >         d_drop(dentry);
> >  out_dput_upper:
> >         dput(upper);
> > -out_unlock:
> > -       unlock_rename(workdir, upperdir);
> >  out_dput:
> >         dput(opaquedir);
> >  out:
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 508003e26e08..25378b81251e 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -732,7 +732,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/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:
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 2a222b8185a3..fd98444dacef 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1141,7 +1141,8 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
> >                 if (IS_ERR(dentry))
> >                         continue;
> >                 if (dentry->d_inode)
> > -                       err = ovl_workdir_cleanup(ofs, dir, path->mnt, dentry, level);
> > +                       err = ovl_workdir_cleanup(ofs, path->dentry, path->mnt,
> > +                                                 dentry, level);
> >                 dput(dentry);
> >                 if (err)
> >                         break;
> > @@ -1152,24 +1153,27 @@ 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);
> > +               return ovl_cleanup_unlocked(ofs, parent, dentry);
> >         }
> >
> > -       err = ovl_do_rmdir(ofs, dir, dentry);
> > +       inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> > +       if (dentry->d_parent == parent)
> > +               err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
> > +       else
> > +               err = -EINVAL;
> > +       inode_unlock(parent->d_inode);
> >         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;
> > @@ -1180,7 +1184,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;
> > @@ -1194,7 +1197,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 +1204,8 @@ 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 +1213,8 @@ 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_workdir_cleanup(ofs, indexdir, path.mnt,
> > +                                                 index, 1);
> >                         if (err)
> >                                 break;
> >                         goto next;
> > @@ -1220,7 +1224,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
> > @@ -1236,7 +1240,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(ofs, dir, index);
> > +                       err = ovl_cleanup_unlocked(ofs, indexdir, index);
> >                 }
> >
> >                 if (err)
> > @@ -1247,7 +1251,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> >                 index = NULL;
> >         }
> >         dput(index);
> > -       inode_unlock(dir);
> >  out:
> >         ovl_cache_free(&list);
> >         if (err)
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 576b5c3b537c..3583e359655f 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,6 +311,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
> >
> >                 if (work->d_inode) {
> >                         err = -EEXIST;
> > +                       inode_unlock(dir);
> >                         if (retried)
> >                                 goto out_dput;
> >
> > @@ -318,7 +319,8 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
> >                                 goto out_unlock;
> >
> >                         retried = true;
> > -                       err = ovl_workdir_cleanup(ofs, dir, mnt, work, 0);
> > +                       err = ovl_workdir_cleanup(ofs, ofs->workbasedir, mnt,
> > +                                                 work, 0);
> >                         dput(work);
> >                         if (err == -EINVAL) {
> >                                 work = ERR_PTR(err);
> > @@ -328,6 +330,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
> >                 }
> >
> >                 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 +368,11 @@ 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:
> 
> This label name is now misleading

Yep - I'll fix it.

Thanks,
NeilBrown

> 
> > -       inode_unlock(dir);
> >         return work;
> >
> 
> Thanks,
> Amir.
> 

[PATCH 10/12] 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.

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3583e359655f..8331667b8101 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -554,7 +554,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;
@@ -571,19 +570,22 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 	err = PTR_ERR(dest);
 	if (IS_ERR(dest)) {
 		dput(temp);
-		goto out_unlock;
+		unlock_rename(workdir, workdir);
+		goto out;
 	}
 
 	/* 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);
+	unlock_rename(workdir, 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;
@@ -592,18 +594,16 @@ 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:
-	unlock_rename(workdir, workdir);
-
+out:
 	return err;
 }
 
-- 
2.49.0

Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> 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.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/super.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 3583e359655f..8331667b8101 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -554,7 +554,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;
> @@ -571,19 +570,22 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>         err = PTR_ERR(dest);
>         if (IS_ERR(dest)) {
>                 dput(temp);
> -               goto out_unlock;
> +               unlock_rename(workdir, workdir);
> +               goto out;

dont use unlock_rename hack please
and why not 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);
> +       unlock_rename(workdir, 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;
> @@ -592,18 +594,16 @@ 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:
> -       unlock_rename(workdir, workdir);
> -
> +out:
>         return err;
>  }

I dont see the point in creating those out goto targets
that just return err.
I do not mind keeping them around if they use to do something and now
they don't or when replacing goto out_unlock with goto out,
but that is not the case here.

Thanks,
Amir.

Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()

[NeilBrown]

On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > 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.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/super.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 3583e359655f..8331667b8101 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -554,7 +554,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;
> > @@ -571,19 +570,22 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
> >         err = PTR_ERR(dest);
> >         if (IS_ERR(dest)) {
> >                 dput(temp);
> > -               goto out_unlock;
> > +               unlock_rename(workdir, workdir);
> > +               goto out;
> 
> dont use unlock_rename hack please

The lock was taken for the purpose of doing a rename.  So using
lock_rename and unlock_rename documents that.  I can use the less
informative "inode_lock" if you prefer.

> and why not return err?

Some people like to only have a single "return" in a function.  Some are
comfortable with more.  I guess I wasn't sure where you stood.

> 
> >         }
> >
> >         /* 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);
> > +       unlock_rename(workdir, 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;
> > @@ -592,18 +594,16 @@ 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:
> > -       unlock_rename(workdir, workdir);
> > -
> > +out:
> >         return err;
> >  }
> 
> I dont see the point in creating those out goto targets
> that just return err.
> I do not mind keeping them around if they use to do something and now
> they don't or when replacing goto out_unlock with goto out,
> but that is not the case here.

I'll get rid of them then.

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 

Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()

[Amir Goldstein]

On Wed, Jul 2, 2025 at 4:41 AM NeilBrown <neil@brown.name> wrote:
>
> On Thu, 26 Jun 2025, Amir Goldstein wrote:
> > On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> > >
> > > 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.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  fs/overlayfs/super.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > > index 3583e359655f..8331667b8101 100644
> > > --- a/fs/overlayfs/super.c
> > > +++ b/fs/overlayfs/super.c
> > > @@ -554,7 +554,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;
> > > @@ -571,19 +570,22 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
> > >         err = PTR_ERR(dest);
> > >         if (IS_ERR(dest)) {
> > >                 dput(temp);
> > > -               goto out_unlock;
> > > +               unlock_rename(workdir, workdir);
> > > +               goto out;
> >
> > dont use unlock_rename hack please
>
> The lock was taken for the purpose of doing a rename.  So using
> lock_rename and unlock_rename documents that.  I can use the less
> informative "inode_lock" if you prefer.
>
> > and why not return err?
>
> Some people like to only have a single "return" in a function.  Some are
> comfortable with more.  I guess I wasn't sure where you stood.
>

Generally, I think we need to move toward more scoped variables and
less mult-goto-labels in ovl code, which are a common source of bugs,
but I will not require this work from you of course.

Thanks,
Amir.

Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()

[Amir Goldstein]

On Wed, Jul 2, 2025 at 4:41 AM NeilBrown <neil@brown.name> wrote:
>
> On Thu, 26 Jun 2025, Amir Goldstein wrote:
> > On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> > >
> > > 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.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  fs/overlayfs/super.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > > index 3583e359655f..8331667b8101 100644
> > > --- a/fs/overlayfs/super.c
> > > +++ b/fs/overlayfs/super.c
> > > @@ -554,7 +554,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;
> > > @@ -571,19 +570,22 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
> > >         err = PTR_ERR(dest);
> > >         if (IS_ERR(dest)) {
> > >                 dput(temp);
> > > -               goto out_unlock;
> > > +               unlock_rename(workdir, workdir);
> > > +               goto out;
> >
> > dont use unlock_rename hack please
>
> The lock was taken for the purpose of doing a rename.  So using
> lock_rename and unlock_rename documents that.

IMO this is not a good excuse for using lock_rename() here

> I can use the less informative "inode_lock" if you prefer.
>

I meant that you should use the new helper that I proposed
in review of patch #2, lock_parent(workdir, temp)
instead of the weird looking lock_rename(workdir, workdir)

BTW, I see that lock_rename_child() effectively has
lock_parent() code in its first part, so maybe factor out this code
as lock_parent().

Thanks,
Amir.

[PATCH 11/12] 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.

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 78b0d956b0ac..9a43ab23cf01 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -164,9 +164,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))
@@ -227,7 +228,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;
@@ -333,7 +334,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 25378b81251e..3d89e1c8d565 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -849,7 +849,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 8331667b8101..1ba1bffc4547 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -617,8 +617,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

Re: [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> Instead of passing an inode *dir, pass a dentry *parent.  This makes the
> calling slightly cleaner.
>
> Signed-off-by: NeilBrown <neil@brown.name>

nice

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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 78b0d956b0ac..9a43ab23cf01 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -164,9 +164,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))
> @@ -227,7 +228,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;
> @@ -333,7 +334,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 25378b81251e..3d89e1c8d565 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -849,7 +849,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 8331667b8101..1ba1bffc4547 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -617,8 +617,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 12/12] 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 moves ovl_cleanup() code into ovl_cleanup_locked(), and then
renames ovl_cleanup_locked() to ovl_cleanup().

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 884c738b67ff..baaa46d00de6 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,
 	unlock_rename(indexdir, indexdir);
 out:
 	if (err)
-		ovl_cleanup_unlocked(ofs, indexdir, temp);
+		ovl_cleanup(ofs, indexdir, temp);
 	dput(temp);
 free_name:
 	kfree(name.name);
@@ -856,7 +856,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 9a43ab23cf01..77a09b0190a2 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -24,16 +24,24 @@ 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)
+int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
+			 struct dentry *wdentry)
 {
 	int err;
 
-	dget(wdentry);
-	if (d_is_dir(wdentry))
-		err = ovl_do_rmdir(ofs, wdir, wdentry);
-	else
-		err = ovl_do_unlink(ofs, wdir, wdentry);
-	dput(wdentry);
+	inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
+	if (wdentry->d_parent == workdir) {
+		struct inode *wdir = workdir->d_inode;
+
+		dget(wdentry);
+		if (d_is_dir(wdentry))
+			err = ovl_do_rmdir(ofs, wdir, wdentry);
+		else
+			err = ovl_do_unlink(ofs, wdir, wdentry);
+		dput(wdentry);
+	} else
+		err = -EINVAL;
+	inode_unlock(workdir->d_inode);
 
 	if (err) {
 		pr_err("cleanup of '%pd2' failed (%i)\n",
@@ -43,21 +51,6 @@ 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;
-
-	inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
-	if (wdentry->d_parent == workdir)
-		ovl_cleanup(ofs, workdir->d_inode, wdentry);
-	else
-		err = -EINVAL;
-	inode_unlock(workdir->d_inode);
-
-	return err;
-}
-
 struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
 {
 	struct dentry *temp;
@@ -153,14 +146,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;
 }
 
@@ -357,7 +350,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	return err;
 
 out_cleanup:
-	ovl_cleanup_unlocked(ofs, upperdir, newdentry);
+	ovl_cleanup(ofs, upperdir, newdentry);
 	dput(newdentry);
 	goto out;
 }
@@ -422,7 +415,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	unlock_rename(workdir, upperdir);
 
 	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);
@@ -432,7 +425,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);
@@ -527,7 +520,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup;
 
-		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);
@@ -537,7 +530,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:
@@ -552,7 +545,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 out_cleanup_locked:
 	unlock_rename(workdir, upperdir);
 out_cleanup:
-	ovl_cleanup_unlocked(ofs, workdir, newdentry);
+	ovl_cleanup(ofs, workdir, newdentry);
 	dput(newdentry);
 	goto out_dput;
 }
@@ -1279,7 +1272,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	unlock_rename(new_upperdir, old_upperdir);
 
 	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 3d89e1c8d565..2f09c3c825f2 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -851,8 +851,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 fd98444dacef..9af73da04d2a 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);
 	}
 }
@@ -1159,7 +1159,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);
 	}
 
 	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
@@ -1173,7 +1173,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;
@@ -1224,7 +1224,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
@@ -1240,7 +1240,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 1ba1bffc4547..6dbfbad8aeca 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -594,11 +594,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 565f7d8c0147..b2c3e7be957b 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)
-- 
2.49.0

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

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 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 moves ovl_cleanup() code into ovl_cleanup_locked(), and then
> renames ovl_cleanup_locked() to ovl_cleanup().
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/copy_up.c   |  4 +--
>  fs/overlayfs/dir.c       | 55 ++++++++++++++++++----------------------
>  fs/overlayfs/overlayfs.h |  3 +--
>  fs/overlayfs/readdir.c   | 10 ++++----
>  fs/overlayfs/super.c     |  4 +--
>  fs/overlayfs/util.c      |  2 +-
>  6 files changed, 35 insertions(+), 43 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 884c738b67ff..baaa46d00de6 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,
>         unlock_rename(indexdir, indexdir);
>  out:
>         if (err)
> -               ovl_cleanup_unlocked(ofs, indexdir, temp);
> +               ovl_cleanup(ofs, indexdir, temp);
>         dput(temp);
>  free_name:
>         kfree(name.name);
> @@ -856,7 +856,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 9a43ab23cf01..77a09b0190a2 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -24,16 +24,24 @@ 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)
> +int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
> +                        struct dentry *wdentry)
>  {
>         int err;
>
> -       dget(wdentry);
> -       if (d_is_dir(wdentry))
> -               err = ovl_do_rmdir(ofs, wdir, wdentry);
> -       else
> -               err = ovl_do_unlink(ofs, wdir, wdentry);
> -       dput(wdentry);
> +       inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
> +       if (wdentry->d_parent == workdir) {
> +               struct inode *wdir = workdir->d_inode;
> +
> +               dget(wdentry);
> +               if (d_is_dir(wdentry))
> +                       err = ovl_do_rmdir(ofs, wdir, wdentry);
> +               else
> +                       err = ovl_do_unlink(ofs, wdir, wdentry);
> +               dput(wdentry);
> +       } else
> +               err = -EINVAL;
> +       inode_unlock(workdir->d_inode);
>

The way that it looks now, I would rather keep it as
static inline ovl_cleanup_locked() helper
but I think that with the lock_parent() that I suggested in patch 2
the unified ovl_cleanup() version would be fine.

Thanks,
Amir.

>         if (err) {
>                 pr_err("cleanup of '%pd2' failed (%i)\n",
> @@ -43,21 +51,6 @@ 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;
> -
> -       inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
> -       if (wdentry->d_parent == workdir)
> -               ovl_cleanup(ofs, workdir->d_inode, wdentry);
> -       else
> -               err = -EINVAL;
> -       inode_unlock(workdir->d_inode);
> -
> -       return err;
> -}
> -
>  struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
>  {
>         struct dentry *temp;
> @@ -153,14 +146,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;
>  }
>
> @@ -357,7 +350,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>         return err;
>
>  out_cleanup:
> -       ovl_cleanup_unlocked(ofs, upperdir, newdentry);
> +       ovl_cleanup(ofs, upperdir, newdentry);
>         dput(newdentry);
>         goto out;
>  }
> @@ -422,7 +415,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>         unlock_rename(workdir, upperdir);
>
>         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);
> @@ -432,7 +425,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);
> @@ -527,7 +520,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>                 if (err)
>                         goto out_cleanup;
>
> -               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);
> @@ -537,7 +530,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:
> @@ -552,7 +545,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  out_cleanup_locked:
>         unlock_rename(workdir, upperdir);
>  out_cleanup:
> -       ovl_cleanup_unlocked(ofs, workdir, newdentry);
> +       ovl_cleanup(ofs, workdir, newdentry);
>         dput(newdentry);
>         goto out_dput;
>  }
> @@ -1279,7 +1272,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>         unlock_rename(new_upperdir, old_upperdir);
>
>         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 3d89e1c8d565..2f09c3c825f2 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -851,8 +851,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 fd98444dacef..9af73da04d2a 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);
>         }
>  }
> @@ -1159,7 +1159,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);
>         }
>
>         inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> @@ -1173,7 +1173,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;
> @@ -1224,7 +1224,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
> @@ -1240,7 +1240,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 1ba1bffc4547..6dbfbad8aeca 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -594,11 +594,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 565f7d8c0147..b2c3e7be957b 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)
> --
> 2.49.0
>

Re: [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem

[Amir Goldstein]

On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
>
> This series of patches for overlayfs is primarily focussed on preparing
> for some proposed changes to directory locking.  In the new scheme we
> wil 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.
>
> The first patch in this series doesn't exactly match the above, but it
> does relate to directory locking and I think it is a sensible
> simplificaiton.
>
> I have tested this with fstests, both generic and unionfs tests.  I
> wouldn't be surprised if I missed something though, so please review
> carefully.

Can you share a git branch for me to pull and test?

Thanks,
Amir.

Re: [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem

[NeilBrown]

On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > This series of patches for overlayfs is primarily focussed on preparing
> > for some proposed changes to directory locking.  In the new scheme we
> > wil 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.
> >
> > The first patch in this series doesn't exactly match the above, but it
> > does relate to directory locking and I think it is a sensible
> > simplificaiton.
> >
> > I have tested this with fstests, both generic and unionfs tests.  I
> > wouldn't be surprised if I missed something though, so please review
> > carefully.
> 
> Can you share a git branch for me to pull and test?

My current work tree can be found at
  https://github.com/neilbrown/linux/tree/pdirops
or branch "pdirops" of 
  https://github.com/neilbrown/linux.git

Thanks for the thorough review - I'll work through it and respond over
coming days.

Thanks,
NeilBrown