On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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 687d5e12289c..d01e83f9d800 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1262,9 +1262,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)
> @@ -1283,12 +1284,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)
> @@ -1299,6 +1296,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
>
I think we get end up with fewer and clearer to understand goto labels
with a relatively simple trick:
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index fe493f3ed6b6..7cddaa7b263e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1069,8 +1069,8 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
int err;
struct dentry *old_upperdir;
struct dentry *new_upperdir;
- struct dentry *olddentry;
- struct dentry *newdentry;
+ struct dentry *olddentry = NULL;
+ struct dentry *newdentry = NULL;
struct dentry *trap;
bool old_opaque;
bool new_opaque;
@@ -1187,18 +1187,22 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
olddentry = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
old->d_name.len);
err = PTR_ERR(olddentry);
- if (IS_ERR(olddentry))
+ if (IS_ERR(olddentry)) {
+ olddentry = NULL;
goto out_unlock;
+ }
err = -ESTALE;
if (!ovl_matches_upper(old, olddentry))
- goto out_dput_old;
+ goto out_unlock;
newdentry = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
new->d_name.len);
err = PTR_ERR(newdentry);
- if (IS_ERR(newdentry))
- goto out_dput_old;
+ if (IS_ERR(newdentry)) {
+ newdentry = NULL;
+ goto out_unlock;
+ }
old_opaque = ovl_dentry_is_opaque(old);
new_opaque = ovl_dentry_is_opaque(new);
@@ -1207,28 +1211,28 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
if (d_inode(new) && ovl_dentry_upper(new)) {
if (opaquedir) {
if (newdentry != opaquedir)
- goto out_dput;
+ goto out_unlock;
} else {
if (!ovl_matches_upper(new, newdentry))
- goto out_dput;
+ goto out_unlock;
}
} else {
if (!d_is_negative(newdentry)) {
if (!new_opaque || !ovl_upper_is_whiteout(ofs,
newdentry))
- goto out_dput;
+ goto out_unlock;
} else {
if (flags & RENAME_EXCHANGE)
- goto out_dput;
+ goto out_unlock;
}
}
if (olddentry == trap)
- goto out_dput;
+ goto out_unlock;
if (newdentry == trap)
- goto out_dput;
+ goto out_unlock;
if (olddentry->d_inode == newdentry->d_inode)
- goto out_dput;
+ goto out_unlock;
err = 0;
if (ovl_type_merge_or_lower(old))
@@ -1236,7 +1240,7 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
else if (is_dir && !old_opaque && ovl_type_merge(new->d_parent))
err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
if (err)
- goto out_dput;
+ goto out_unlock;
if (!overwrite && ovl_type_merge_or_lower(new))
err = ovl_set_redirect(new, samedir);
@@ -1244,15 +1248,16 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
ovl_type_merge(old->d_parent))
err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
if (err)
- goto out_dput;
+ goto out_unlock;
err = ovl_do_rename(ofs, old_upperdir->d_inode, olddentry,
new_upperdir->d_inode, newdentry, flags);
if (err)
- goto out_dput;
+ goto out_unlock;
+ unlock_rename(new_upperdir, old_upperdir);
if (cleanup_whiteout)
- ovl_cleanup(ofs, old_upperdir->d_inode, newdentry);
+ ovl_cleanup_unlocked(ofs, old_upperdir->d_inode, newdentry);
if (overwrite && d_inode(new)) {
if (new_is_dir)
@@ -1271,12 +1276,6 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
if (d_inode(new) && ovl_dentry_upper(new))
ovl_copyattr(d_inode(new));
-out_dput:
- dput(newdentry);
-out_dput_old:
- dput(olddentry);
-out_unlock:
- unlock_rename(new_upperdir, old_upperdir);
out_revert_creds:
ovl_revert_creds(old_cred);
if (update_nlink)
@@ -1284,9 +1283,15 @@ static int ovl_rename(struct mnt_idmap *idmap,
struct inode *olddir,
else
ovl_drop_write(old);
out:
+ dput(newdentry);
+ dput(olddentry);
dput(opaquedir);
ovl_cache_free(&list);
return err;
+
+out_unlock:
+ unlock_rename(new_upperdir, old_upperdir);
+ goto out_revert_creds;
}
