On Fri, 11 Jul 2025, Amir Goldstein wrote:
> 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;
> }
>
I decided to make the goto changed into a separate patch as follows. My
version is slightly different to yours (see new var "de").
Thanks,
NeilBrown
From: NeilBrown <neil@xxxxxxxxxx>
Date: Mon, 14 Jul 2025 10:44:03 +1000
Subject: [PATCH] ovl: simplify gotos in ovl_rename()
Rather than having three separate goto label: out_unlock, out_dput_old,
and out_dput, make use of that fact that dput() happily accepts a NULL
point to reduce this to just one goto label: out_unlock.
olddentry and newdentry are initialised to NULL and only set once a
value dentry is found. They are then put late in the function.
Suggested-by: Amir Goldstein <amir73il@xxxxxxxxx>
Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
---
fs/overlayfs/dir.c | 54 +++++++++++++++++++++++-----------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index e094adf9d169..63460bdd71cf 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1082,9 +1082,9 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
int err;
struct dentry *old_upperdir;
struct dentry *new_upperdir;
- struct dentry *olddentry;
- struct dentry *newdentry;
- struct dentry *trap;
+ struct dentry *olddentry = NULL;
+ struct dentry *newdentry = NULL;
+ struct dentry *trap, *de;
bool old_opaque;
bool new_opaque;
bool cleanup_whiteout = false;
@@ -1197,21 +1197,23 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
goto out_revert_creds;
}
- olddentry = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
- old->d_name.len);
- err = PTR_ERR(olddentry);
- if (IS_ERR(olddentry))
+ de = ovl_lookup_upper(ofs, old->d_name.name, old_upperdir,
+ old->d_name.len);
+ err = PTR_ERR(de);
+ if (IS_ERR(de))
goto out_unlock;
+ olddentry = de;
err = -ESTALE;
if (!ovl_matches_upper(old, olddentry))
- goto out_dput_old;
+ goto out_unlock;
- newdentry = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
- new->d_name.len);
- err = PTR_ERR(newdentry);
- if (IS_ERR(newdentry))
- goto out_dput_old;
+ de = ovl_lookup_upper(ofs, new->d_name.name, new_upperdir,
+ new->d_name.len);
+ err = PTR_ERR(de);
+ if (IS_ERR(de))
+ goto out_unlock;
+ newdentry = de;
old_opaque = ovl_dentry_is_opaque(old);
new_opaque = ovl_dentry_is_opaque(new);
@@ -1220,28 +1222,28 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
if (d_inode(new) && ovl_dentry_upper(new)) {
if (opaquedir) {
if (newdentry != opaquedir)
- goto out_dput;
+ goto out_unlock;
} else {
if (!ovl_matches_upper(new, newdentry))
- goto out_dput;
+ goto out_unlock;
}
} else {
if (!d_is_negative(newdentry)) {
if (!new_opaque || !ovl_upper_is_whiteout(ofs, newdentry))
- goto out_dput;
+ goto out_unlock;
} else {
if (flags & RENAME_EXCHANGE)
- goto out_dput;
+ goto out_unlock;
}
}
if (olddentry == trap)
- goto out_dput;
+ goto out_unlock;
if (newdentry == trap)
- goto out_dput;
+ goto out_unlock;
if (olddentry->d_inode == newdentry->d_inode)
- goto out_dput;
+ goto out_unlock;
err = 0;
if (ovl_type_merge_or_lower(old))
@@ -1249,7 +1251,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
else if (is_dir && !old_opaque && ovl_type_merge(new->d_parent))
err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
if (err)
- goto out_dput;
+ goto out_unlock;
if (!overwrite && ovl_type_merge_or_lower(new))
err = ovl_set_redirect(new, samedir);
@@ -1257,12 +1259,12 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
ovl_type_merge(old->d_parent))
err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
if (err)
- goto out_dput;
+ goto out_unlock;
err = ovl_do_rename(ofs, old_upperdir, olddentry,
new_upperdir, newdentry, flags);
if (err)
- goto out_dput;
+ goto out_unlock;
if (cleanup_whiteout)
ovl_cleanup(ofs, old_upperdir->d_inode, newdentry);
@@ -1284,10 +1286,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
if (d_inode(new) && ovl_dentry_upper(new))
ovl_copyattr(d_inode(new));
-out_dput:
- dput(newdentry);
-out_dput_old:
- dput(olddentry);
out_unlock:
unlock_rename(new_upperdir, old_upperdir);
out_revert_creds:
@@ -1297,6 +1295,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
else
ovl_drop_write(old);
out:
+ dput(newdentry);
+ dput(olddentry);
dput(opaquedir);
ovl_cache_free(&list);
return err;
--
2.49.0
