On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 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 2b879d7c386e..5afe17cee305 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -1270,9 +1270,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)
> > @@ -1291,12 +1292,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)
> > @@ -1307,6 +1304,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;
> > }
>
> Once again, I really do not like the resulting code flow.
> This is a huge function and impossile to follow all condition.
> As a rule of thumb, I think you need to factor out the block of code under
> lock_rename() to avoid those horrible goto mazes.
I'll see what I can do to improve it.
>
> The no error case used to do dput(newdentry) and dput(olddentry)
> how come they are gone now?
> what am I missing?
Those dput()s are still there. I removed the goto labels between them
but not the dput()s themselves.
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
