Re: [PATCH 2/3] xfs: split the sunit parameter update into two parts

[Brian Foster <bfoster@redhat.com>]

On Wed, Dec 18, 2019 at 11:37:35AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If the administrator provided a sunit= mount option, we need to validate
> the raw parameter, convert the mount option units (512b blocks) into the
> internal unit (fs blocks), and then validate that the (now cooked)
> parameter doesn't screw anything up on disk.  The incore inode geometry
> computation can depend on the new sunit option, but a subsequent patch
> will make validating the cooked value depends on the computed inode
> geometry, so break the sunit update into two steps.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_mount.c |  126 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 75 insertions(+), 51 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index fca65109cf24..3750f0074c74 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -360,66 +360,79 @@ xfs_readsb(
>  }
>  
>  /*
> - * Update alignment values based on mount options and sb values
> + * If we were provided with new sunit/swidth values as mount options, make sure
> + * that they pass basic alignment and superblock feature checks, and convert
> + * them into the same units (FSB) that everything else expects.  This step
> + * /must/ be done before computing the inode geometry.
>   */
>  STATIC int
> -xfs_update_alignment(xfs_mount_t *mp)
> +xfs_validate_new_dalign(
> +	struct xfs_mount	*mp)
>  {
...
> +		} else if (mp->m_dalign) {
> +			mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
>  		} else {
>  			xfs_warn(mp,
> -	"cannot change alignment: superblock does not support data alignment");
> +		"alignment check failed: sunit(%d) less than bsize(%d)",
> +				 mp->m_dalign, mp->m_sb.sb_blocksize);
>  			return -EINVAL;
>  		}
> +	}
> +
> +	/* Update superblock with new values and log changes. */

We can probably drop the above comment since this hunk no longer does
either. ;) With that fixed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +	if (!xfs_sb_version_hasdalign(&mp->m_sb)) {
> +		xfs_warn(mp,
> +"cannot change alignment: superblock does not support data alignment");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Update alignment values based on mount options and sb values
> + */
> +STATIC int
> +xfs_update_alignment(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +
> +	if (mp->m_dalign) {
> +		if (sbp->sb_unit == mp->m_dalign &&
> +		    sbp->sb_width == mp->m_swidth)
> +			return 0;
> +
> +		sbp->sb_unit = mp->m_dalign;
> +		sbp->sb_width = mp->m_swidth;
> +		mp->m_update_sb = true;
>  	} else if ((mp->m_flags & XFS_MOUNT_NOALIGN) != XFS_MOUNT_NOALIGN &&
>  		    xfs_sb_version_hasdalign(&mp->m_sb)) {
> -			mp->m_dalign = sbp->sb_unit;
> -			mp->m_swidth = sbp->sb_width;
> +		mp->m_dalign = sbp->sb_unit;
> +		mp->m_swidth = sbp->sb_width;
>  	}
>  
>  	return 0;
> @@ -648,12 +661,12 @@ xfs_mountfs(
>  	}
>  
>  	/*
> -	 * Check if sb_agblocks is aligned at stripe boundary
> -	 * If sb_agblocks is NOT aligned turn off m_dalign since
> -	 * allocator alignment is within an ag, therefore ag has
> -	 * to be aligned at stripe boundary.
> +	 * If we were given new sunit/swidth options, do some basic validation
> +	 * checks and convert the incore dalign and swidth values to the
> +	 * same units (FSB) that everything else uses.  This /must/ happen
> +	 * before computing the inode geometry.
>  	 */
> -	error = xfs_update_alignment(mp);
> +	error = xfs_validate_new_dalign(mp);
>  	if (error)
>  		goto out;
>  
> @@ -664,6 +677,17 @@ xfs_mountfs(
>  	xfs_rmapbt_compute_maxlevels(mp);
>  	xfs_refcountbt_compute_maxlevels(mp);
>  
> +	/*
> +	 * Check if sb_agblocks is aligned at stripe boundary.  If sb_agblocks
> +	 * is NOT aligned turn off m_dalign since allocator alignment is within
> +	 * an ag, therefore ag has to be aligned at stripe boundary.  Note that
> +	 * we must compute the free space and rmap btree geometry before doing
> +	 * this.
> +	 */
> +	error = xfs_update_alignment(mp);
> +	if (error)
> +		goto out;
> +
>  	/* enable fail_at_unmount as default */
>  	mp->m_fail_unmount = true;
>  
>