| Message ID | 20230627224412.2242198-7-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: various fixes for 6.5 | expand |
> + if (sbp->sb_logblocks > XFS_MAX_LOG_BLOCKS) { > + xfs_notice(mp, > + "Log size 0x%x blocks too large, maximum size is 0x%llx blocks", > + sbp->sb_logblocks, XFS_MAX_LOG_BLOCKS); Just a little nitpick, didn't we traditionally align the overly long format strings with a single tab and not to the same line as the xfs_notice/warn/etc? Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Jun 27, 2023 at 11:08:46PM -0700, Christoph Hellwig wrote: > > + if (sbp->sb_logblocks > XFS_MAX_LOG_BLOCKS) { > > + xfs_notice(mp, > > + "Log size 0x%x blocks too large, maximum size is 0x%llx blocks", > > + sbp->sb_logblocks, XFS_MAX_LOG_BLOCKS); > > Just a little nitpick, didn't we traditionally align the overly > long format strings with a single tab and not to the same line as > the xfs_notice/warn/etc? I don't think we have a particluarly formal definition of that. I mean, just look at all the different variants in xfs_sb_validate_common: if (xfs_sb_is_v5(sbp)) { if (sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) { xfs_notice(mp, "Block size (%u bytes) too small for Version 5 superblock (minimum %d bytes)", sbp->sb_blocksize, XFS_MIN_CRC_BLOCKSIZE); return -EFSCORRUPTED; } /* V5 has a separate project quota inode */ if (sbp->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) { xfs_notice(mp, "Version 5 of Super block has XFS_OQUOTA bits."); return -EFSCORRUPTED; } /* * Full inode chunks must be aligned to inode chunk size when * sparse inodes are enabled to support the sparse chunk * allocation algorithm and prevent overlapping inode records. */ if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_SPINODES) { uint32_t align; align = XFS_INODES_PER_CHUNK * sbp->sb_inodesize >> sbp->sb_blocklog; if (sbp->sb_inoalignmt != align) { xfs_warn(mp, "Inode block alignment (%u) must match chunk size (%u) for sparse inodes.", sbp->sb_inoalignmt, align); return -EINVAL; } } } else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD | XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) { xfs_notice(mp, "Superblock earlier than Version 5 has XFS_{P|G}QUOTA_{ENFD|CHKD} bits."); return -EFSCORRUPTED; } if (unlikely( sbp->sb_logstart == 0 && mp->m_logdev_targp == mp->m_ddev_targp)) { xfs_warn(mp, "filesystem is marked as having an external log; " "specify logdev on the mount command line."); return -EINVAL; } if (unlikely( sbp->sb_logstart != 0 && mp->m_logdev_targp != mp->m_ddev_targp)) { xfs_warn(mp, "filesystem is marked as having an internal log; " "do not specify logdev on the mount command line."); return -EINVAL; } There's 4-5 different variations in indenting in 6-7 warning messages. And there are more variations in that function, too. There's no consistent rule to follow, so I just set the indenting such that the format string didn't run over 80 columns and didn't think any further.. I think that if we really care, a separate cleanup patch to make all the long format strings use consistent zero-indenting and the variables one tab like this one: xfs_notice(mp, "Block size (%u bytes) too small for Version 5 superblock (minimum %d bytes)", sbp->sb_blocksize, XFS_MIN_CRC_BLOCKSIZE); Could be done. If I'm ever lacking for something to do... > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! _dave.
On Wed, Jun 28, 2023 at 08:44:10AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > If the journal geometry results in a sector or log stripe unit > validation problem, it indicates that we cannot set the log up to > safely write to the the journal. In these cases, we must abort the > mount because the corruption needs external intervention to resolve. > Similarly, a journal that is too large cannot be written to safely, > either, so we shouldn't allow those geometries to mount, either. > > If the log is too small, we risk having transaction reservations > overruning the available log space and the system hanging waiting > for space it can never provide. This is purely a runtime hang issue, > not a corruption issue as per the first cases listed above. We abort > mounts of the log is too small for V5 filesystems, but we must allow > v4 filesystems to mount because, historically, there was no log size > validity checking and so some systems may still be out there with > undersized logs. > > The problem is that on V4 filesystems, when we discover a log > geometry problem, we skip all the remaining checks and then allow > the log to continue mounting. This mean that if one of the log size > checks fails, we skip the log stripe unit check. i.e. we allow the > mount because a "non-fatal" geometry is violated, and then fail to > check the hard fail geometries that should fail the mount. > > Move all these fatal checks to the superblock verifier, and add a > new check for the two log sector size geometry variables having the > same values. This will prevent any attempt to mount a log that has > invalid or inconsistent geometries long before we attempt to mount > the log. > > However, for the minimum log size checks, we can only do that once > we've setup up the log and calculated all the iclog sizes and > roundoffs. Hence this needs to remain in the log mount code after > the log has been initialised. It is also the only case where we > should allow a v4 filesystem to continue running, so leave that > handling in place, too. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_sb.c | 56 +++++++++++++++++++++++++++++++++++++++++- > fs/xfs/xfs_log.c | 47 +++++++++++------------------------ > 2 files changed, 70 insertions(+), 33 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index ba0f17bc1dc0..5e174685a77c 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -412,7 +412,6 @@ xfs_validate_sb_common( > sbp->sb_inodelog < XFS_DINODE_MIN_LOG || > sbp->sb_inodelog > XFS_DINODE_MAX_LOG || > sbp->sb_inodesize != (1 << sbp->sb_inodelog) || > - sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE || > sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) || > XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_MIN_AG_BYTES || > XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_MAX_AG_BYTES || > @@ -430,6 +429,61 @@ xfs_validate_sb_common( > return -EFSCORRUPTED; > } > > + /* > + * Logs that are too large are not supported at all. Reject them > + * outright. Logs that are too small are tolerated on v4 filesystems, > + * but we can only check that when mounting the log. Hence we skip > + * those checks here. > + */ > + if (sbp->sb_logblocks > XFS_MAX_LOG_BLOCKS) { > + xfs_notice(mp, > + "Log size 0x%x blocks too large, maximum size is 0x%llx blocks", > + sbp->sb_logblocks, XFS_MAX_LOG_BLOCKS); > + return -EFSCORRUPTED; > + } > + > + if (XFS_FSB_TO_B(mp, sbp->sb_logblocks) > XFS_MAX_LOG_BYTES) { > + xfs_warn(mp, > + "log size 0x%llx bytes too large, maximum size is 0x%llx bytes", > + XFS_FSB_TO_B(mp, sbp->sb_logblocks), > + XFS_MAX_LOG_BYTES); > + return -EFSCORRUPTED; > + } > + > + /* > + * Do not allow filesystems with corrupted log sector or stripe units to > + * be mounted. We cannot safely size the iclogs or write to the log if > + * the log stripe unit is not valid. > + */ > + if (sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT) { > + if (sbp->sb_logsectsize != (1U << sbp->sb_logsectlog)) { > + xfs_notice(mp, > + "log sector size in bytes/log2 (0x%x/0x%x) must match", > + sbp->sb_logsectsize, 1U << sbp->sb_logsectlog); > + return -EFSCORRUPTED; > + } > + } else if (sbp->sb_logsectsize || sbp->sb_logsectlog) { > + xfs_notice(mp, > + "log sector size in bytes/log2 (0x%x/0x%x) are not zero", > + sbp->sb_logsectsize, sbp->sb_logsectlog); > + return -EFSCORRUPTED; > + } > + > + if (sbp->sb_logsunit > 1) { > + if (sbp->sb_logsunit % sbp->sb_blocksize) { > + xfs_notice(mp, > + "log stripe unit 0x%x bytes must be a multiple of block size", > + sbp->sb_logsunit); > + return -EFSCORRUPTED; > + } > + if (sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE) { > + xfs_notice(mp, > + "log stripe unit 0x%x bytes over maximum size (0x%x bytes)", > + sbp->sb_logsunit, XLOG_MAX_RECORD_BSIZE); > + return -EFSCORRUPTED; > + } > + } > + > /* Validate the realtime geometry; stolen from xfs_repair */ > if (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE || > sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE) { > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index fc61cc024023..79004d193e54 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -639,7 +639,6 @@ xfs_log_mount( > int num_bblks) > { > struct xlog *log; > - bool fatal = xfs_has_crc(mp); > int error = 0; > int min_logfsbs; > > @@ -663,53 +662,37 @@ xfs_log_mount( > mp->m_log = log; > > /* > - * Validate the given log space and drop a critical message via syslog > - * if the log size is too small that would lead to some unexpected > - * situations in transaction log space reservation stage. > + * Now that we have set up the log and it's internal geometry > + * parameters, we can validate the given log space and drop a critical > + * message via syslog if the log size is too small. A log that is too > + * small can lead to unexpected situations in transaction log space > + * reservation stage. The superblock verifier has already validated all > + * the other log geometry constraints, so we don't have to check those > + * here. > * > - * Note: we can't just reject the mount if the validation fails. This > - * would mean that people would have to downgrade their kernel just to > - * remedy the situation as there is no way to grow the log (short of > - * black magic surgery with xfs_db). > + * Note: For v4 filesystems, we can't just reject the mount if the > + * validation fails. This would mean that people would have to > + * downgrade their kernel just to remedy the situation as there is no > + * way to grow the log (short of black magic surgery with xfs_db). > * > - * We can, however, reject mounts for CRC format filesystems, as the > + * We can, however, reject mounts for V5 format filesystems, as the > * mkfs binary being used to make the filesystem should never create a > * filesystem with a log that is too small. > */ > min_logfsbs = xfs_log_calc_minimum_size(mp); > - > if (mp->m_sb.sb_logblocks < min_logfsbs) { > xfs_warn(mp, > "Log size %d blocks too small, minimum size is %d blocks", > mp->m_sb.sb_logblocks, min_logfsbs); > - error = -EINVAL; > - } else if (mp->m_sb.sb_logblocks > XFS_MAX_LOG_BLOCKS) { > - xfs_warn(mp, > - "Log size %d blocks too large, maximum size is %lld blocks", > - mp->m_sb.sb_logblocks, XFS_MAX_LOG_BLOCKS); > - error = -EINVAL; > - } else if (XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks) > XFS_MAX_LOG_BYTES) { > - xfs_warn(mp, > - "log size %lld bytes too large, maximum size is %lld bytes", > - XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks), > - XFS_MAX_LOG_BYTES); > - error = -EINVAL; > - } else if (mp->m_sb.sb_logsunit > 1 && > - mp->m_sb.sb_logsunit % mp->m_sb.sb_blocksize) { > - xfs_warn(mp, > - "log stripe unit %u bytes must be a multiple of block size", > - mp->m_sb.sb_logsunit); > - error = -EINVAL; > - fatal = true; > - } > - if (error) { > + > /* > * Log check errors are always fatal on v5; or whenever bad > * metadata leads to a crash. > */ > - if (fatal) { > + if (xfs_has_crc(mp)) { > xfs_crit(mp, "AAIEEE! Log failed size checks. Abort!"); > ASSERT(0); > + error = -EINVAL; > goto out_free_log; > } > xfs_crit(mp, "Log size out of supported range."); > -- > 2.40.1 >
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index ba0f17bc1dc0..5e174685a77c 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -412,7 +412,6 @@ xfs_validate_sb_common( sbp->sb_inodelog < XFS_DINODE_MIN_LOG || sbp->sb_inodelog > XFS_DINODE_MAX_LOG || sbp->sb_inodesize != (1 << sbp->sb_inodelog) || - sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE || sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) || XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_MIN_AG_BYTES || XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_MAX_AG_BYTES || @@ -430,6 +429,61 @@ xfs_validate_sb_common( return -EFSCORRUPTED; } + /* + * Logs that are too large are not supported at all. Reject them + * outright. Logs that are too small are tolerated on v4 filesystems, + * but we can only check that when mounting the log. Hence we skip + * those checks here. + */ + if (sbp->sb_logblocks > XFS_MAX_LOG_BLOCKS) { + xfs_notice(mp, + "Log size 0x%x blocks too large, maximum size is 0x%llx blocks", + sbp->sb_logblocks, XFS_MAX_LOG_BLOCKS); + return -EFSCORRUPTED; + } + + if (XFS_FSB_TO_B(mp, sbp->sb_logblocks) > XFS_MAX_LOG_BYTES) { + xfs_warn(mp, + "log size 0x%llx bytes too large, maximum size is 0x%llx bytes", + XFS_FSB_TO_B(mp, sbp->sb_logblocks), + XFS_MAX_LOG_BYTES); + return -EFSCORRUPTED; + } + + /* + * Do not allow filesystems with corrupted log sector or stripe units to + * be mounted. We cannot safely size the iclogs or write to the log if + * the log stripe unit is not valid. + */ + if (sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT) { + if (sbp->sb_logsectsize != (1U << sbp->sb_logsectlog)) { + xfs_notice(mp, + "log sector size in bytes/log2 (0x%x/0x%x) must match", + sbp->sb_logsectsize, 1U << sbp->sb_logsectlog); + return -EFSCORRUPTED; + } + } else if (sbp->sb_logsectsize || sbp->sb_logsectlog) { + xfs_notice(mp, + "log sector size in bytes/log2 (0x%x/0x%x) are not zero", + sbp->sb_logsectsize, sbp->sb_logsectlog); + return -EFSCORRUPTED; + } + + if (sbp->sb_logsunit > 1) { + if (sbp->sb_logsunit % sbp->sb_blocksize) { + xfs_notice(mp, + "log stripe unit 0x%x bytes must be a multiple of block size", + sbp->sb_logsunit); + return -EFSCORRUPTED; + } + if (sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE) { + xfs_notice(mp, + "log stripe unit 0x%x bytes over maximum size (0x%x bytes)", + sbp->sb_logsunit, XLOG_MAX_RECORD_BSIZE); + return -EFSCORRUPTED; + } + } + /* Validate the realtime geometry; stolen from xfs_repair */ if (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE || sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE) { diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index fc61cc024023..79004d193e54 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -639,7 +639,6 @@ xfs_log_mount( int num_bblks) { struct xlog *log; - bool fatal = xfs_has_crc(mp); int error = 0; int min_logfsbs; @@ -663,53 +662,37 @@ xfs_log_mount( mp->m_log = log; /* - * Validate the given log space and drop a critical message via syslog - * if the log size is too small that would lead to some unexpected - * situations in transaction log space reservation stage. + * Now that we have set up the log and it's internal geometry + * parameters, we can validate the given log space and drop a critical + * message via syslog if the log size is too small. A log that is too + * small can lead to unexpected situations in transaction log space + * reservation stage. The superblock verifier has already validated all + * the other log geometry constraints, so we don't have to check those + * here. * - * Note: we can't just reject the mount if the validation fails. This - * would mean that people would have to downgrade their kernel just to - * remedy the situation as there is no way to grow the log (short of - * black magic surgery with xfs_db). + * Note: For v4 filesystems, we can't just reject the mount if the + * validation fails. This would mean that people would have to + * downgrade their kernel just to remedy the situation as there is no + * way to grow the log (short of black magic surgery with xfs_db). * - * We can, however, reject mounts for CRC format filesystems, as the + * We can, however, reject mounts for V5 format filesystems, as the * mkfs binary being used to make the filesystem should never create a * filesystem with a log that is too small. */ min_logfsbs = xfs_log_calc_minimum_size(mp); - if (mp->m_sb.sb_logblocks < min_logfsbs) { xfs_warn(mp, "Log size %d blocks too small, minimum size is %d blocks", mp->m_sb.sb_logblocks, min_logfsbs); - error = -EINVAL; - } else if (mp->m_sb.sb_logblocks > XFS_MAX_LOG_BLOCKS) { - xfs_warn(mp, - "Log size %d blocks too large, maximum size is %lld blocks", - mp->m_sb.sb_logblocks, XFS_MAX_LOG_BLOCKS); - error = -EINVAL; - } else if (XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks) > XFS_MAX_LOG_BYTES) { - xfs_warn(mp, - "log size %lld bytes too large, maximum size is %lld bytes", - XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks), - XFS_MAX_LOG_BYTES); - error = -EINVAL; - } else if (mp->m_sb.sb_logsunit > 1 && - mp->m_sb.sb_logsunit % mp->m_sb.sb_blocksize) { - xfs_warn(mp, - "log stripe unit %u bytes must be a multiple of block size", - mp->m_sb.sb_logsunit); - error = -EINVAL; - fatal = true; - } - if (error) { + /* * Log check errors are always fatal on v5; or whenever bad * metadata leads to a crash. */ - if (fatal) { + if (xfs_has_crc(mp)) { xfs_crit(mp, "AAIEEE! Log failed size checks. Abort!"); ASSERT(0); + error = -EINVAL; goto out_free_log; } xfs_crit(mp, "Log size out of supported range.");