On Mon, Jul 30, 2018 at 06:06:51PM -0500, Eric Sandeen wrote: > On 7/30/18 12:30 AM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > Split the superblock verifier into the common checks, the read-time > > checks, and the write-time check functions. No functional changes, but > > we're setting up to add more write-only checks. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > Couple nitpicks below,change them on the way in if you like. > > Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx> > > > --- > > fs/xfs/libxfs/xfs_sb.c | 205 ++++++++++++++++++++++++++---------------------- > > 1 file changed, 112 insertions(+), 93 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > index b3ad15956366..516bef7b0f50 100644 > > --- a/fs/xfs/libxfs/xfs_sb.c > > +++ b/fs/xfs/libxfs/xfs_sb.c > > @@ -96,80 +96,96 @@ xfs_perag_put( > > trace_xfs_perag_put(pag->pag_mount, pag->pag_agno, ref, _RET_IP_); > > } > > > > -/* > > - * Check the validity of the SB found. > > - */ > > +/* Check all the superblock fields we care about when reading one in. */ > > STATIC int > > -xfs_mount_validate_sb( > > - xfs_mount_t *mp, > > - xfs_sb_t *sbp, > > - bool check_inprogress, > > - bool check_version) > > +xfs_validate_sb_read( > > + struct xfs_mount *mp, > > + struct xfs_sb *sbp) > > { > > - uint32_t agcount = 0; > > - uint32_t rem; > > - > > - if (sbp->sb_magicnum != XFS_SB_MAGIC) { > > - xfs_warn(mp, "bad magic number"); > > - return -EWRONGFS; > > - } > > - > > - > > - if (!xfs_sb_good_version(sbp)) { > > - xfs_warn(mp, "bad version"); > > - return -EWRONGFS; > > - } > > + if (!xfs_sb_version_hascrc(sbp)) > > + return 0; > > Can we make this > > if (!(XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)) ? > > nitpicky but it always bugs me to see "do we have metadata crcs" as a standin > for other available features (like feature flags). Yes, they go together, > but bleah. > > (and the original code tested superblock version, right?) One of them did, the other one used the helper. It was weird. > > /* > > - * Version 5 superblock feature mask validation. Reject combinations the > > - * kernel cannot support up front before checking anything else. For > > - * write validation, we don't need to check feature masks. > > + * Version 5 superblock feature mask validation. Reject combinations > > + * the kernel cannot support up front before checking anything else. > > + * For write validation, we don't need to check feature masks. > > Huh, I wonder why not. But ok, keeping the same behavior. > > > */ > > - if (check_version && XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) { > > - if (xfs_sb_has_compat_feature(sbp, > > - XFS_SB_FEAT_COMPAT_UNKNOWN)) { > > - xfs_warn(mp, > > + if (xfs_sb_has_compat_feature(sbp, XFS_SB_FEAT_COMPAT_UNKNOWN)) { > > + xfs_warn(mp, > > "Superblock has unknown compatible features (0x%x) enabled.", > > - (sbp->sb_features_compat & > > - XFS_SB_FEAT_COMPAT_UNKNOWN)); > > - xfs_warn(mp, > > + (sbp->sb_features_compat & XFS_SB_FEAT_COMPAT_UNKNOWN)); > > + xfs_warn(mp, > > "Using a more recent kernel is recommended."); > > - } > > + } > > > > - if (xfs_sb_has_ro_compat_feature(sbp, > > - XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) { > > - xfs_alert(mp, > > + if (xfs_sb_has_ro_compat_feature(sbp, > > + XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) { > > + xfs_alert(mp, > > "Superblock has unknown read-only compatible features (0x%x) enabled.", > > - (sbp->sb_features_ro_compat & > > - XFS_SB_FEAT_RO_COMPAT_UNKNOWN)); > > - if (!(mp->m_flags & XFS_MOUNT_RDONLY)) { > > - xfs_warn(mp, > > + (sbp->sb_features_ro_compat & > > + XFS_SB_FEAT_RO_COMPAT_UNKNOWN)); > > + if (!(mp->m_flags & XFS_MOUNT_RDONLY)) { > > + xfs_warn(mp, > > "Attempted to mount read-only compatible filesystem read-write."); > > - xfs_warn(mp, > > + xfs_warn(mp, > > "Filesystem can only be safely mounted read only."); > > > > - return -EINVAL; > > - } > > + return -EINVAL; > > } > > - if (xfs_sb_has_incompat_feature(sbp, > > - XFS_SB_FEAT_INCOMPAT_UNKNOWN)) { > > - xfs_warn(mp, > > + } > > + if (xfs_sb_has_incompat_feature(sbp, > > + XFS_SB_FEAT_INCOMPAT_UNKNOWN)) { > > + xfs_warn(mp, > > "Superblock has unknown incompatible features (0x%x) enabled.", > > - (sbp->sb_features_incompat & > > - XFS_SB_FEAT_INCOMPAT_UNKNOWN)); > > - xfs_warn(mp, > > + (sbp->sb_features_incompat & > > + XFS_SB_FEAT_INCOMPAT_UNKNOWN)); > > + xfs_warn(mp, > > "Filesystem can not be safely mounted by this kernel."); > > - return -EINVAL; > > - } > > - } else if (xfs_sb_version_hascrc(sbp)) { > > - /* > > - * We can't read verify the sb LSN because the read verifier is > > - * called before the log is allocated and processed. We know the > > - * log is set up before write verifier (!check_version) calls, > > - * so just check it here. > > - */ > > - if (!xfs_log_check_lsn(mp, sbp->sb_lsn)) > > - return -EFSCORRUPTED; > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +/* Check all the superblock fields we care about when writing one out. */ > > +STATIC int > > +xfs_validate_sb_write( > > + struct xfs_mount *mp, > > + struct xfs_sb *sbp) > > +{ > > + if (!xfs_sb_version_hascrc(sbp)) > > + return 0; > > + > > + /* > > + * We can't read verify the sb LSN because the read verifier is called > > + * before the log is allocated and processed. We know the log is set up > > + * before write verifier (!check_version) calls, so just check it here. > > + */ > > + if (!xfs_log_check_lsn(mp, sbp->sb_lsn)) > > + return -EFSCORRUPTED; > > + > > + return 0; > > +} > > + > > +/* Check the validity of the SB. */ > > +STATIC int > > +xfs_validate_sb_common( > > + struct xfs_mount *mp, > > + struct xfs_buf *bp, > > + struct xfs_sb *sbp) > > +{ > > + uint32_t agcount = 0; > > + uint32_t rem; > > + > > + if (sbp->sb_magicnum != XFS_SB_MAGIC) { > > + xfs_warn(mp, "bad magic number"); > > + return -EWRONGFS; > > + } > > + > > + > > just one newline pls ;) (even if it was there before) Ok, I'll fix those things on the way in. --D > > + if (!xfs_sb_good_version(sbp)) { > > + xfs_warn(mp, "bad version"); > > + return -EWRONGFS; > > } > > > > if (xfs_sb_version_has_pquotino(sbp)) { > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html