Re: [PATCH v5 03/14] VFS: tidy up do_unlinkat()

Jeff Layton
2025-11-12T14:47:27Z

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> The simplification of locking in the previous patch opens up some room
> for tidying up do_unlinkat()
>
> - change all "exit" labels to describe what will happen at the label.
> - always goto an exit label on an error - unwrap the "if (!IS_ERR())" branch.
> - Move the "slashes" handing inline, but mark it as unlikely()
> - simplify use of the "inode" variable - we no longer need to test for NULL.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/namei.c | 55 ++++++++++++++++++++++++++----------------------------
> 1 file changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 231e1ffd4b8d..93c5fce2d814 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4755,65 +4755,62 @@ int do_unlinkat(int dfd, struct filename *name)
> struct path path;
> struct qstr last;
> int type;
> - struct inode *inode = NULL;
> + struct inode *inode;
> struct inode *delegated_inode = NULL;
> unsigned int lookup_flags = 0;
> retry:
> error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
> if (error)
> - goto exit1;
> + goto exit_putname;
>
> error = -EISDIR;
> if (type != LAST_NORM)
> - goto exit2;
> + goto exit_path_put;
>
> error = mnt_want_write(path.mnt);
> if (error)
> - goto exit2;
> + goto exit_path_put;
> retry_deleg:
> dentry = start_dirop(path.dentry, &last, lookup_flags);
> error = PTR_ERR(dentry);
> - if (!IS_ERR(dentry)) {
> + if (IS_ERR(dentry))
> + goto exit_drop_write;
>
> - /* Why not before? Because we want correct error value */
> - if (last.name[last.len])
> - goto slashes;
> - inode = dentry->d_inode;
> - ihold(inode);
> - error = security_path_unlink(&path, dentry);
> - if (error)
> - goto exit3;
> - error = vfs_unlink(mnt_idmap(path.mnt), path.dentry->d_inode,
> - dentry, &delegated_inode);
> -exit3:
> + /* Why not before? Because we want correct error value */
> + if (unlikely(last.name[last.len])) {
> + if (d_is_dir(dentry))
> + error = -EISDIR;
> + else
> + error = -ENOTDIR;
> end_dirop(dentry);
> + goto exit_drop_write;
> }
> - if (inode)
> - iput(inode); /* truncate the inode here */
> - inode = NULL;
> + inode = dentry->d_inode;
> + ihold(inode);
> + error = security_path_unlink(&path, dentry);
> + if (error)
> + goto exit_end_dirop;
> + error = vfs_unlink(mnt_idmap(path.mnt), path.dentry->d_inode,
> + dentry, &delegated_inode);
> +exit_end_dirop:
> + end_dirop(dentry);
> + iput(inode); /* truncate the inode here */
> if (delegated_inode) {
> error = break_deleg_wait(&delegated_inode);
> if (!error)
> goto retry_deleg;
> }
> +exit_drop_write:
> mnt_drop_write(path.mnt);
> -exit2:
> +exit_path_put:
> path_put(&path);
> if (retry_estale(error, lookup_flags)) {
> lookup_flags |= LOOKUP_REVAL;
> - inode = NULL;
> goto retry;
> }
> -exit1:
> +exit_putname:
> putname(name);
> return error;
> -
> -slashes:
> - if (d_is_dir(dentry))
> - error = -EISDIR;
> - else
> - error = -ENOTDIR;
> - goto exit3;
> }
>
> SYSCALL_DEFINE3(unlinkat, int, dfd, const char __user *, pathname, int, flag)

Reviewed-by: Jeff Layton <jlayton@kernel.org>