On Thu, 26 Jun 2025, Amir Goldstein wrote:
> 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
The lock was taken for the purpose of doing a rename. So using
lock_rename and unlock_rename documents that. I can use the less
informative "inode_lock" if you prefer.
> and why not return err?
Some people like to only have a single "return" in a function. Some are
comfortable with more. I guess I wasn't sure where you stood.
>
> > }
> >
> > /* 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.
I'll get rid of them then.
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
