On Wed, Oct 07, 2020 at 03:29:42PM -0700, Darrick J. Wong wrote: > On Wed, Oct 07, 2020 at 10:04:01PM +0800, Gao Xiang wrote: > > From: Gao Xiang <hsiangkao@xxxxxxxxxx> > > > > Introduce a common helper to consolidate > > stripe validation process. Also make kernel > > code xfs_validate_sb_common() use it first. > > Please use all 72(?) columns here. will fix. > > > > > Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx> > > --- > > libxfs/xfs_sb.c | 54 +++++++++++++++++++++++++++++++++++++++---------- > > libxfs/xfs_sb.h | 3 +++ > > These libxfs changes will have to go through the kernel first. will send another patch together with the next version. > > > 2 files changed, 46 insertions(+), 11 deletions(-) > > > > diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c > > index d37d60b39a52..bd65828c844e 100644 > > --- a/libxfs/xfs_sb.c > > +++ b/libxfs/xfs_sb.c > > @@ -357,21 +357,13 @@ xfs_validate_sb_common( > > } > > } > > > > - if (sbp->sb_unit) { > > - if (!xfs_sb_version_hasdalign(sbp) || > > - sbp->sb_unit > sbp->sb_width || > > - (sbp->sb_width % sbp->sb_unit) != 0) { > > - xfs_notice(mp, "SB stripe unit sanity check failed"); > > - return -EFSCORRUPTED; > > - } > > - } else if (xfs_sb_version_hasdalign(sbp)) { > > + if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) { > > Urgh, this logic makes my brain hurt. > > "If the zeroness of sb_unit differs from the unsetness of the dalign > feature"? This might need some kind of comment, such as: > > /* > * Either sb_unit and hasdalign are both set, or they are zero > * and not set, respectively. > */ > if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) { > Ok, yet I think the comment might describe failure condition (which causes -EFSCORRUPTED) instead directly, like, /* * Either (sb_unit and !hasdalign) or (!sb_unit and hasdalign) * would imply the image is corrupted. */ if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) { > > > xfs_notice(mp, "SB stripe alignment sanity check failed"); > > return -EFSCORRUPTED; > > - } else if (sbp->sb_width) { > > - xfs_notice(mp, "SB stripe width sanity check failed"); > > - return -EFSCORRUPTED; > > } > > > > + if (!xfs_validate_stripe_factors(mp, sbp->sb_unit, sbp->sb_width, 0)) > > + return -EFSCORRUPTED; > > > > if (xfs_sb_version_hascrc(&mp->m_sb) && > > sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) { > > @@ -1208,3 +1200,43 @@ xfs_sb_get_secondary( > > *bpp = bp; > > return 0; > > } > > + > > +/* > > + * If sectorsize is specified, sunit / swidth must be in bytes; > > + * or both can be in any kind of units (e.g. 512B sector or blocksize). > > + */ > > +bool > > +xfs_validate_stripe_factors( > > + struct xfs_mount *mp, > > + int sunit, > > + int swidth, > > + int sectorsize) > > +{ > > + if (sectorsize && sunit % sectorsize) { > > + xfs_notice(mp, > > +"stripe unit (%d) must be a multiple of the sector size (%d)", > > + sunit, sectorsize); > > + return false; > > + } > > + > > + if ((sunit && !swidth) || (!sunit && swidth)) { > > + xfs_notice(mp, > > +"stripe unit (%d) and width (%d) are partially valid", sunit, swidth); > > I would break these into separate checks and messages. Ok, will update in the next version. > > > + return false; > > + } > > + > > + if (sunit > swidth) { > > + xfs_notice(mp, > > +"stripe unit (%d) is too large of the stripe width (%d)", sunit, swidth); > > "stripe unit (%d) is larger than the stripe width..." Will update too. Thanks, Gao Xiang > > --D