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

Brian Foster
2019-12-19T13:18:27Z

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;
>
>