Re: [PATCH v2 11/15] xfs: mount api - add xfs_reconfigure()

[Brian Foster <bfoster@redhat.com>]

On Fri, Aug 23, 2019 at 09:00:16AM +0800, Ian Kent wrote:
> Add the fs_context_operations method .reconfigure that performs
> remount validation as previously done by the super_operations
> .remount_fs method.
> 
> An attempt has also been made to update the comment about options
> handling problems with mount(8) to reflect the current situation.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 76374d602257..aae0098fecab 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1522,6 +1522,89 @@ xfs_fs_remount(
>  	return 0;
>  }
>  
> +/*
> + * There have been problems in the past with options passed from mount(8).
> + *
> + * The problem being that options passed by mount(8) in the case where only
> + * the the mount point path is given would consist of the existing fstab
> + * options with the options from mtab for the current mount merged in and
> + * the options given on the command line last. But the result couldn't be
> + * relied upon to accurately reflect the current mount options so that
> + * rejecting options that can't be changed on reconfigure could erronously
> + * cause mount failure.
> + *
> + * The mount-api uses a legacy mount options handler in the VFS to handle
> + * mount(8) so these options will continue to be passed. Even if mount(8)
> + * is updated to use fsopen()/fsconfig()/fsmount() it's likely to continue
> + * to set the existing options so options problems with reconfigure could
> + * continue.
> + *
> + * For the longest time mtab locking was a problem and this could have been
> + * one possible cause. It's also possible there could have been options
> + * order problems.
> + *
> + * That has changed now as mtab is a link to the proc file system mount
> + * table so mtab options should be always accurate.
> + *
> + * Consulting the util-linux maintainer (Karel Zak) he is confident that,
> + * in this case, the options passed by mount(8) will be those of the current
> + * mount and the options order should be a correct merge of fstab and mtab
> + * options, and new options given on the command line.
> + *
> + * So, in theory, it should be possible to compare incoming options and
> + * return an error for options that differ from the current mount and can't
> + * be changed on reconfigure to prevent users from believing they might have
> + * changed mount options using remount which can't be changed.
> + *
> + * But for now continue to return success for every reconfigure request, and
> + * silently ignore all options that can't actually be changed.
> + */

This seems like all good information for a commit log description or
perhaps to land in a common header where some of these fs context bits
are declared, but overly broad for a function header comment for an XFS
callback. I'd more expect some information around the fundamental
difference between 'mp' and 'new_mp,' where those come from, what they
mean, etc. From the code, it seems like new_mp is transient and reflects
changes that we need to incorporate in the original mp..?

Brian

> +STATIC int
> +xfs_reconfigure(
> +	struct fs_context *fc)
> +{
> +	struct xfs_fs_context	*ctx = fc->fs_private;
> +	struct xfs_mount	*mp = XFS_M(fc->root->d_sb);
> +	struct xfs_mount        *new_mp = fc->s_fs_info;
> +	xfs_sb_t		*sbp = &mp->m_sb;
> +	int			flags = fc->sb_flags;
> +	int			error;
> +
> +	error = xfs_validate_params(new_mp, ctx, false);
> +	if (error)
> +		return error;
> +
> +	/* inode32 -> inode64 */
> +	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> +	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> +		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
> +	}
> +
> +	/* inode64 -> inode32 */
> +	if (!(mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> +	    (new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> +		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
> +	}
> +
> +	/* ro -> rw */
> +	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
> +		error = xfs_remount_rw(mp);
> +		if (error)
> +			return error;
> +	}
> +
> +	/* rw -> ro */
> +	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
> +		error = xfs_remount_ro(mp);
> +		if (error)
> +			return error;
> +	}
> +
> +	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
> @@ -2049,6 +2132,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,
> +	.reconfigure = xfs_reconfigure,
>  };
>  
>  static struct file_system_type xfs_fs_type = {
>