* [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem
@ 2025-06-24 22:54 NeilBrown
2025-06-24 22:54 ` [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another NeilBrown
` (12 more replies)
0 siblings, 13 replies; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:54 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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()
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
@ 2025-06-24 22:54 ` NeilBrown
2025-06-25 14:54 ` Amir Goldstein
2025-06-24 22:54 ` [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held NeilBrown
` (11 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:54 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
2025-06-24 22:54 ` [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another NeilBrown
@ 2025-06-24 22:54 ` NeilBrown
2025-06-25 15:44 ` Amir Goldstein
2025-06-28 3:08 ` Dan Carpenter
2025-06-24 22:54 ` [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir() NeilBrown
` (10 subsequent siblings)
12 siblings, 2 replies; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:54 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
2025-06-24 22:54 ` [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another NeilBrown
2025-06-24 22:54 ` [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held NeilBrown
@ 2025-06-24 22:54 ` NeilBrown
2025-06-25 19:07 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 04/12] ovl: narrow locking in ovl_create_upper() NeilBrown
` (9 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:54 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 04/12] ovl: narrow locking in ovl_create_upper()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (2 preceding siblings ...)
2025-06-24 22:54 ` [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 17:55 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 05/12] ovl: narrow locking in ovl_clear_empty() NeilBrown
` (8 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 05/12] ovl: narrow locking in ovl_clear_empty()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (3 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 04/12] ovl: narrow locking in ovl_create_upper() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 18:22 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout() NeilBrown
` (7 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (4 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 05/12] ovl: narrow locking in ovl_clear_empty() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 19:08 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 07/12] ovl: narrow locking in ovl_rename() NeilBrown
` (6 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 07/12] ovl: narrow locking in ovl_rename()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (5 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 18:30 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts() NeilBrown
` (5 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (6 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 07/12] ovl: narrow locking in ovl_rename() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 18:35 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 09/12] ovl: whiteout locking changes NeilBrown
` (4 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 09/12] ovl: whiteout locking changes
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (7 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 18:54 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout() NeilBrown
` (3 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (8 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 09/12] ovl: whiteout locking changes NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 19:04 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent NeilBrown
` (2 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (9 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout() NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 19:05 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup() NeilBrown
2025-06-25 14:56 ` [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem Amir Goldstein
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (10 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent NeilBrown
@ 2025-06-24 22:55 ` NeilBrown
2025-06-25 18:57 ` Amir Goldstein
2025-06-25 14:56 ` [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem Amir Goldstein
12 siblings, 1 reply; 36+ messages in thread
From: NeilBrown @ 2025-06-24 22:55 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein
Cc: linux-unionfs, Alexander Viro, Christian Brauner, Jan Kara,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another
2025-06-24 22:54 ` [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another NeilBrown
@ 2025-06-25 14:54 ` Amir Goldstein
2025-06-25 21:45 ` NeilBrown
0 siblings, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 14:54 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
` (11 preceding siblings ...)
2025-06-24 22:55 ` [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup() NeilBrown
@ 2025-06-25 14:56 ` Amir Goldstein
2025-06-25 21:35 ` NeilBrown
12 siblings, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 14:56 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.
2025-06-24 22:54 ` [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held NeilBrown
@ 2025-06-25 15:44 ` Amir Goldstein
2025-06-25 16:02 ` Amir Goldstein
2025-06-28 3:08 ` Dan Carpenter
1 sibling, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 15:44 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.
2025-06-25 15:44 ` Amir Goldstein
@ 2025-06-25 16:02 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 16:02 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/12] ovl: narrow locking in ovl_create_upper()
2025-06-24 22:55 ` [PATCH 04/12] ovl: narrow locking in ovl_create_upper() NeilBrown
@ 2025-06-25 17:55 ` Amir Goldstein
2025-06-25 18:17 ` Amir Goldstein
0 siblings, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 17:55 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/12] ovl: narrow locking in ovl_create_upper()
2025-06-25 17:55 ` Amir Goldstein
@ 2025-06-25 18:17 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 18:17 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 05/12] ovl: narrow locking in ovl_clear_empty()
2025-06-24 22:55 ` [PATCH 05/12] ovl: narrow locking in ovl_clear_empty() NeilBrown
@ 2025-06-25 18:22 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 18:22 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 07/12] ovl: narrow locking in ovl_rename()
2025-06-24 22:55 ` [PATCH 07/12] ovl: narrow locking in ovl_rename() NeilBrown
@ 2025-06-25 18:30 ` Amir Goldstein
2025-07-02 2:16 ` NeilBrown
0 siblings, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 18:30 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts()
2025-06-24 22:55 ` [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts() NeilBrown
@ 2025-06-25 18:35 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 18:35 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 09/12] ovl: whiteout locking changes
2025-06-24 22:55 ` [PATCH 09/12] ovl: whiteout locking changes NeilBrown
@ 2025-06-25 18:54 ` Amir Goldstein
2025-07-02 2:21 ` NeilBrown
0 siblings, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 18:54 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()
2025-06-24 22:55 ` [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup() NeilBrown
@ 2025-06-25 18:57 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 18:57 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()
2025-06-24 22:55 ` [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout() NeilBrown
@ 2025-06-25 19:04 ` Amir Goldstein
2025-07-02 2:41 ` NeilBrown
0 siblings, 1 reply; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 19:04 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent
2025-06-24 22:55 ` [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent NeilBrown
@ 2025-06-25 19:05 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 19:05 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir()
2025-06-24 22:54 ` [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir() NeilBrown
@ 2025-06-25 19:07 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 19:07 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout()
2025-06-24 22:55 ` [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout() NeilBrown
@ 2025-06-25 19:08 ` Amir Goldstein
0 siblings, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-06-25 19:08 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem
2025-06-25 14:56 ` [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem Amir Goldstein
@ 2025-06-25 21:35 ` NeilBrown
0 siblings, 0 replies; 36+ messages in thread
From: NeilBrown @ 2025-06-25 21:35 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another
2025-06-25 14:54 ` Amir Goldstein
@ 2025-06-25 21:45 ` NeilBrown
0 siblings, 0 replies; 36+ messages in thread
From: NeilBrown @ 2025-06-25 21:45 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.
2025-06-24 22:54 ` [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held NeilBrown
2025-06-25 15:44 ` Amir Goldstein
@ 2025-06-28 3:08 ` Dan Carpenter
1 sibling, 0 replies; 36+ messages in thread
From: Dan Carpenter @ 2025-06-28 3:08 UTC (permalink / raw)
To: oe-kbuild, NeilBrown, Miklos Szeredi, Amir Goldstein
Cc: lkp, oe-kbuild-all, linux-unionfs, Alexander Viro,
Christian Brauner, Jan Kara, linux-fsdevel
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 07/12] ovl: narrow locking in ovl_rename()
2025-06-25 18:30 ` Amir Goldstein
@ 2025-07-02 2:16 ` NeilBrown
0 siblings, 0 replies; 36+ messages in thread
From: NeilBrown @ 2025-07-02 2:16 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 09/12] ovl: whiteout locking changes
2025-06-25 18:54 ` Amir Goldstein
@ 2025-07-02 2:21 ` NeilBrown
0 siblings, 0 replies; 36+ messages in thread
From: NeilBrown @ 2025-07-02 2:21 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()
2025-06-25 19:04 ` Amir Goldstein
@ 2025-07-02 2:41 ` NeilBrown
2025-07-02 10:04 ` Amir Goldstein
2025-07-02 10:23 ` Amir Goldstein
0 siblings, 2 replies; 36+ messages in thread
From: NeilBrown @ 2025-07-02 2:41 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()
2025-07-02 2:41 ` NeilBrown
@ 2025-07-02 10:04 ` Amir Goldstein
2025-07-02 10:23 ` Amir Goldstein
1 sibling, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-07-02 10:04 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout()
2025-07-02 2:41 ` NeilBrown
2025-07-02 10:04 ` Amir Goldstein
@ 2025-07-02 10:23 ` Amir Goldstein
1 sibling, 0 replies; 36+ messages in thread
From: Amir Goldstein @ 2025-07-02 10:23 UTC (permalink / raw)
To: NeilBrown
Cc: Miklos Szeredi, linux-unionfs, Alexander Viro, Christian Brauner,
Jan Kara, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2025-07-02 10:23 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-24 22:54 [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem NeilBrown
2025-06-24 22:54 ` [PATCH 01/12] ovl: use is_subdir() for testing if one thing is a subdir of another NeilBrown
2025-06-25 14:54 ` Amir Goldstein
2025-06-25 21:45 ` NeilBrown
2025-06-24 22:54 ` [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held NeilBrown
2025-06-25 15:44 ` Amir Goldstein
2025-06-25 16:02 ` Amir Goldstein
2025-06-28 3:08 ` Dan Carpenter
2025-06-24 22:54 ` [PATCH 03/12] ovl: narrow the locked region in ovl_copy_up_workdir() NeilBrown
2025-06-25 19:07 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 04/12] ovl: narrow locking in ovl_create_upper() NeilBrown
2025-06-25 17:55 ` Amir Goldstein
2025-06-25 18:17 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 05/12] ovl: narrow locking in ovl_clear_empty() NeilBrown
2025-06-25 18:22 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 06/12] ovl: narrow locking in ovl_create_over_whiteout() NeilBrown
2025-06-25 19:08 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 07/12] ovl: narrow locking in ovl_rename() NeilBrown
2025-06-25 18:30 ` Amir Goldstein
2025-07-02 2:16 ` NeilBrown
2025-06-24 22:55 ` [PATCH 08/12] ovl: narrow locking in ovl_cleanup_whiteouts() NeilBrown
2025-06-25 18:35 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 09/12] ovl: whiteout locking changes NeilBrown
2025-06-25 18:54 ` Amir Goldstein
2025-07-02 2:21 ` NeilBrown
2025-06-24 22:55 ` [PATCH 10/12] ovl: narrow locking in ovl_check_rename_whiteout() NeilBrown
2025-06-25 19:04 ` Amir Goldstein
2025-07-02 2:41 ` NeilBrown
2025-07-02 10:04 ` Amir Goldstein
2025-07-02 10:23 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 11/12] ovl: change ovl_create_real() to receive dentry parent NeilBrown
2025-06-25 19:05 ` Amir Goldstein
2025-06-24 22:55 ` [PATCH 12/12] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup() NeilBrown
2025-06-25 18:57 ` Amir Goldstein
2025-06-25 14:56 ` [PATCH 00/12] ovl: narrow regions protected by directory i_rw_sem Amir Goldstein
2025-06-25 21:35 ` NeilBrown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).