Re: [PATCH 05/10] xfs: mount-api - add xfs_get_tree()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jun 24, 2019 at 10:58:50AM +0800, Ian Kent wrote:
> Add the fs_context_operations method .get_tree that validates
> mount options and fills the super block as previously done
> by the file_system_type .mount method.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 139 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index cf8efb465969..0ec0142b94e1 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1629,6 +1629,98 @@ xfs_fs_remount(
>  	return 0;
>  }
>  
> +STATIC int
> +xfs_validate_params(
> +	struct xfs_mount *mp,
> +	struct xfs_fs_context *ctx,
> +	enum fs_context_purpose purpose)

Tabs here, please:

	struct xfs_mount	*mp,
	struct xfs_fs_context	*ctx,
	enum fs_context_purpose	purpose)

> +{
> +	/*
> +	 * no recovery flag requires a read-only mount
> +	 */
> +	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> +	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> +		xfs_warn(mp, "no-recovery mounts must be read-only.");
> +		return -EINVAL;
> +	}
> +
> +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> +	    (ctx->dsunit || ctx->dswidth)) {
> +		xfs_warn(mp,
> +	"sunit and swidth options incompatible with the noalign option");
> +		return -EINVAL;
> +	}
> +
> +#ifndef CONFIG_XFS_QUOTA
> +	if (XFS_IS_QUOTA_RUNNING(mp)) {
> +		xfs_warn(mp, "quota support not available in this kernel.");
> +		return -EINVAL;
> +	}
> +#endif
> +
> +	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
> +		xfs_warn(mp, "sunit and swidth must be specified together");
> +		return -EINVAL;
> +	}
> +
> +	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> +		xfs_warn(mp,
> +	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> +			ctx->dswidth, ctx->dsunit);
> +		return -EINVAL;
> +	}
> +
> +	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> +		/*
> +		 * At this point the superblock has not been read
> +		 * in, therefore we do not know the block size.
> +		 * Before the mount call ends we will convert
> +		 * these to FSBs.
> +		 */
> +		if (purpose == FS_CONTEXT_FOR_MOUNT) {
> +			mp->m_dalign = ctx->dsunit;
> +			mp->m_swidth = ctx->dswidth;
> +		}
> +	}
> +
> +	if (mp->m_logbufs != -1 &&
> +	    mp->m_logbufs != 0 &&
> +	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
> +	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
> +		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
> +			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
> +		return -EINVAL;
> +	}
> +	if (mp->m_logbsize != -1 &&
> +	    mp->m_logbsize !=  0 &&
> +	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
> +	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
> +	     !is_power_of_2(mp->m_logbsize))) {
> +		xfs_warn(mp,
> +			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
> +			mp->m_logbsize);
> +		return -EINVAL;
> +	}
> +
> +	if (ctx->iosizelog) {
> +		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> +		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> +			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
> +				ctx->iosizelog, XFS_MIN_IO_LOG,
> +				XFS_MAX_IO_LOG);
> +			return -EINVAL;
> +		}
> +
> +		if (purpose == FS_CONTEXT_FOR_MOUNT) {
> +			mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> +			mp->m_readio_log = ctx->iosizelog;
> +			mp->m_writeio_log = ctx->iosizelog;
> +		}
> +	}

Ugggh, I don't wanna have to compare the old xfs_parseargs code with
this new xfs_validate_params code to make sure nothing got screwed up.
:)

Can this code be broken out of the existing parseargs (instead of added
further down in the file) to minimize the diff?  You're getting rid of
the old option processing code at the end of the series anyway so I
don't mind creating temporary struct xfs_fs_context structures in
xfs_parseargs if that makes it much more obvious that the validation
code itself isn't changing.

--D

> +
> +	return 0;
> +}
> +
>  /*
>   * Second stage of a freeze. The data is already frozen so we only
>   * need to take care of the metadata. Once that's done sync the superblock
> @@ -2035,6 +2127,52 @@ xfs_fs_fill_super(
>  	return error;
>  }
>  
> +STATIC int
> +xfs_fill_super(
> +	struct super_block	*sb,
> +	struct fs_context	*fc)
> +{
> +	struct xfs_fs_context	*ctx = fc->fs_private;
> +	struct xfs_mount	*mp = sb->s_fs_info;
> +	int			silent = fc->sb_flags & SB_SILENT;
> +	int			error = -ENOMEM;
> +
> +	mp->m_super = sb;
> +
> +	/*
> +	 * set up the mount name first so all the errors will refer to the
> +	 * correct device.
> +	 */
> +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> +	if (!mp->m_fsname)
> +		return -ENOMEM;
> +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> +
> +	error = xfs_validate_params(mp, ctx, fc->purpose);
> +	if (error)
> +		goto out_free_fsname;
> +
> +	error = __xfs_fs_fill_super(mp, silent);
> +	if (error)
> +		goto out_free_fsname;
> +
> +	return 0;
> +
> + out_free_fsname:
> +	sb->s_fs_info = NULL;
> +	xfs_free_fsname(mp);
> +	kfree(mp);
> +
> +	return error;
> +}
> +
> +STATIC int
> +xfs_get_tree(
> +	struct fs_context	*fc)
> +{
> +	return vfs_get_block_super(fc, xfs_fill_super);
> +}
> +
>  STATIC void
>  xfs_fs_put_super(
>  	struct super_block	*sb)
> @@ -2107,6 +2245,7 @@ static const struct super_operations xfs_super_operations = {
>  
>  static const struct fs_context_operations xfs_context_ops = {
>  	.parse_param = xfs_parse_param,
> +	.get_tree    = xfs_get_tree,
>  };
>  
>  static struct file_system_type xfs_fs_type = {
>