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.
The no error case used to do dput(newdentry) and dput(olddentry)
how come they are gone now?
what am I missing?
Thanks,
Amir.
