On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
