Re: [PATCH 1/7] namei: clean up do_rmdir retry logic

[Christian Brauner <christian.brauner@ubuntu.com>]

On Mon, Jul 12, 2021 at 07:36:43PM +0700, Dmitry Kadashev wrote:
> Moving the main logic to a helper function makes the whole thing much
> easier to follow.
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/
> Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
> ---
>  fs/namei.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index b5adfd4f7de6..ae6cde7dc91e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3947,7 +3947,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir,
>  }
>  EXPORT_SYMBOL(vfs_rmdir);
>  
> -int do_rmdir(int dfd, struct filename *name)
> +static int rmdir_helper(int dfd, struct filename *name,
> +			unsigned int lookup_flags)
>  {
>  	struct user_namespace *mnt_userns;
>  	int error;
> @@ -3955,54 +3956,59 @@ int do_rmdir(int dfd, struct filename *name)
>  	struct path path;
>  	struct qstr last;
>  	int type;
> -	unsigned int lookup_flags = 0;
> -retry:
> +
>  	error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
>  	if (error)
> -		goto exit1;
> +		return error;
>  
>  	switch (type) {
>  	case LAST_DOTDOT:
>  		error = -ENOTEMPTY;
> -		goto exit2;
> +		goto exit1;
>  	case LAST_DOT:
>  		error = -EINVAL;
> -		goto exit2;
> +		goto exit1;
>  	case LAST_ROOT:
>  		error = -EBUSY;
> -		goto exit2;
> +		goto exit1;
>  	}
>  
>  	error = mnt_want_write(path.mnt);
>  	if (error)
> -		goto exit2;
> +		goto exit1;
>  
>  	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
>  	dentry = __lookup_hash(&last, path.dentry, lookup_flags);
>  	error = PTR_ERR(dentry);
>  	if (IS_ERR(dentry))
> -		goto exit3;
> +		goto exit2;
>  	if (!dentry->d_inode) {
>  		error = -ENOENT;
> -		goto exit4;
> +		goto exit3;
>  	}
>  	error = security_path_rmdir(&path, dentry);
>  	if (error)
> -		goto exit4;
> +		goto exit3;
>  	mnt_userns = mnt_user_ns(path.mnt);
>  	error = vfs_rmdir(mnt_userns, path.dentry->d_inode, dentry);
> -exit4:
> -	dput(dentry);
>  exit3:
> +	dput(dentry);
> +exit2:
>  	inode_unlock(path.dentry->d_inode);
>  	mnt_drop_write(path.mnt);
> -exit2:
> -	path_put(&path);
> -	if (retry_estale(error, lookup_flags)) {
> -		lookup_flags |= LOOKUP_REVAL;
> -		goto retry;
> -	}
>  exit1:
> +	path_put(&path);
> +	return error;
> +}
> +
> +int do_rmdir(int dfd, struct filename *name)
> +{
> +	int error;
> +
> +	error = rmdir_helper(dfd, name, 0);
> +	if (retry_estale(error, 0))
> +		error = rmdir_helper(dfd, name, LOOKUP_REVAL);

Instead of naming all these $something_helper I would follow the
underscore naming pattern we usually do, i.e. instead of e.g.
rmdir_helper do __rmdir() or __do_rmdir().