Re: [PATCH RFC 2/3] fs: add vfs_cmd_create()

[Jan Kara <jack@suse.cz>]

On Tue 01-08-23 15:09:01, Christian Brauner wrote:
> Split the steps to create a superblock into a tiny helper. This will
> make the next patch easier to follow.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

I agree with Christoph that the error handling in vfs_fsconfig_locked() is
confusing - in particular the fact that if you 'break' out of the switch
statement it causes the fs context to be marked as failed is probably handy
but too subtle to my taste.

Also I think this patch does cause a behavioral change because before if we
bailed e.g. due to:

if (fc->phase != FS_CONTEXT_CREATE_PARAMS)

we returned -EBUSY but didn't set fc->phase = FS_CONTEXT_FAILED. After your
patch we 'break' on any error and thus fc->phase is set on any error...

								Honza

> ---
>  fs/fsopen.c | 45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index fc9d2d9fd234..af2ff05dcee5 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -209,6 +209,36 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
>  	return ret;
>  }
>  
> +static int vfs_cmd_create(struct fs_context *fc)
> +{
> +	struct super_block *sb;
> +	int ret;
> +
> +	if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> +		return -EBUSY;
> +
> +	if (!mount_capable(fc))
> +		return -EPERM;
> +
> +	fc->phase = FS_CONTEXT_CREATING;
> +
> +	ret = vfs_get_tree(fc);
> +	if (ret)
> +		return ret;
> +
> +	sb = fc->root->d_sb;
> +	ret = security_sb_kern_mount(sb);
> +	if (unlikely(ret)) {
> +		fc_drop_locked(fc);
> +		return ret;
> +	}
> +
> +	/* vfs_get_tree() callchains will have grabbed @s_umount */
> +	up_write(&sb->s_umount);
> +	fc->phase = FS_CONTEXT_AWAITING_MOUNT;
> +	return 0;
> +}
> +
>  /*
>   * Check the state and apply the configuration.  Note that this function is
>   * allowed to 'steal' the value by setting param->xxx to NULL before returning.
> @@ -224,22 +254,9 @@ static int vfs_fsconfig_locked(struct fs_context *fc, int cmd,
>  		return ret;
>  	switch (cmd) {
>  	case FSCONFIG_CMD_CREATE:
> -		if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> -			return -EBUSY;
> -		if (!mount_capable(fc))
> -			return -EPERM;
> -		fc->phase = FS_CONTEXT_CREATING;
> -		ret = vfs_get_tree(fc);
> +		ret = vfs_cmd_create(fc);
>  		if (ret)
>  			break;
> -		sb = fc->root->d_sb;
> -		ret = security_sb_kern_mount(sb);
> -		if (unlikely(ret)) {
> -			fc_drop_locked(fc);
> -			break;
> -		}
> -		up_write(&sb->s_umount);
> -		fc->phase = FS_CONTEXT_AWAITING_MOUNT;
>  		return 0;
>  	case FSCONFIG_CMD_RECONFIGURE:
>  		if (fc->phase != FS_CONTEXT_RECONF_PARAMS)
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR