Message ID | 20201007140402.14295-3-hsiangkao@aol.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfsprogs: consolidate stripe validation | expand |
On Wed, Oct 07, 2020 at 10:04:01PM +0800, Gao Xiang wrote: > From: Gao Xiang <hsiangkao@redhat.com> > > 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. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > libxfs/xfs_sb.c | 54 +++++++++++++++++++++++++++++++++++++++---------- > libxfs/xfs_sb.h | 3 +++ These libxfs changes will have to go through the kernel first. > 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)) { > 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. > + 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..." --D > + return false; > + } > + > + if (sunit && (swidth % sunit)) { > + xfs_notice(mp, > +"stripe width (%d) must be a multiple of the stripe unit (%d)", > + swidth, sunit); > + return false; > + } > + return true; > +} > + > diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h > index 92465a9a5162..015b2605f587 100644 > --- a/libxfs/xfs_sb.h > +++ b/libxfs/xfs_sb.h > @@ -42,4 +42,7 @@ extern int xfs_sb_get_secondary(struct xfs_mount *mp, > struct xfs_trans *tp, xfs_agnumber_t agno, > struct xfs_buf **bpp); > > +extern bool xfs_validate_stripe_factors(struct xfs_mount *mp, > + int sunit, int swidth, int sectorsize); > + > #endif /* __XFS_SB_H__ */ > -- > 2.24.0 >
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@redhat.com> > > > > 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@redhat.com> > > --- > > 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
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)) { 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); + return false; + } + + if (sunit > swidth) { + xfs_notice(mp, +"stripe unit (%d) is too large of the stripe width (%d)", sunit, swidth); + return false; + } + + if (sunit && (swidth % sunit)) { + xfs_notice(mp, +"stripe width (%d) must be a multiple of the stripe unit (%d)", + swidth, sunit); + return false; + } + return true; +} + diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h index 92465a9a5162..015b2605f587 100644 --- a/libxfs/xfs_sb.h +++ b/libxfs/xfs_sb.h @@ -42,4 +42,7 @@ extern int xfs_sb_get_secondary(struct xfs_mount *mp, struct xfs_trans *tp, xfs_agnumber_t agno, struct xfs_buf **bpp); +extern bool xfs_validate_stripe_factors(struct xfs_mount *mp, + int sunit, int swidth, int sectorsize); + #endif /* __XFS_SB_H__ */