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

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <[email protected]>
> 
> 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 <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
> ---
>  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 <[email protected]>