On Mon, Jul 14, 2025 at 3:00 AM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> 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.
Good idea.
> My version is slightly different to yours (see new var "de").
>
Looks nicer.
Thanks,
Amir.
> 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
>
