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

[Amir Goldstein <amir73il@gmail.com>]

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.