| Message ID | 20240319021547.3483050-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: fix discontiguous metadata block recovery | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Mar 19, 2024 at 01:15:20PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We detect when a buffer log format type and the magic number in the > buffer do not match. We issue a warning, but do not return an error > nor do we write back the recovered buffer. If no further recover > action is performed on that buffer, then recovery has left the > buffer in an inconsistent (out of date) state on disk. i.e. the > structure is corrupt on disk. > > If this mismatch occurs, return a -EFSCORRUPTED error and cause > recovery to abort instead of letting recovery corrupt the filesystem > and continue onwards. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_buf_item_recover.c | 51 +++++++++++++++++------------------ > 1 file changed, 24 insertions(+), 27 deletions(-) > > diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c > index d74bf7bb7794..dba57ee6fa6d 100644 > --- a/fs/xfs/xfs_buf_item_recover.c > +++ b/fs/xfs/xfs_buf_item_recover.c > @@ -207,7 +207,7 @@ xlog_recover_buf_commit_pass1( > * the first 32 bits of the buffer (most blocks), > * inside a struct xfs_da_blkinfo at the start of the buffer. > */ > -static void > +static int > xlog_recover_validate_buf_type( > struct xfs_mount *mp, > struct xfs_buf *bp, > @@ -407,11 +407,12 @@ xlog_recover_validate_buf_type( > * skipped. > */ > if (current_lsn == NULLCOMMITLSN) > - return; > + return 0;; Unnecessary double-semicolon. With that fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > if (warnmsg) { > xfs_warn(mp, warnmsg); > - ASSERT(0); > + xfs_buf_corruption_error(bp, __this_address); > + return -EFSCORRUPTED; > } > > /* > @@ -425,14 +426,11 @@ xlog_recover_validate_buf_type( > * the buffer. Therefore, initialize a bli purely to carry the LSN to > * the verifier. > */ > - if (bp->b_ops) { > - struct xfs_buf_log_item *bip; > - > - bp->b_flags |= _XBF_LOGRECOVERY; > - xfs_buf_item_init(bp, mp); > - bip = bp->b_log_item; > - bip->bli_item.li_lsn = current_lsn; > - } > + ASSERT(bp->b_ops); > + bp->b_flags |= _XBF_LOGRECOVERY; > + xfs_buf_item_init(bp, mp); > + bp->b_log_item->bli_item.li_lsn = current_lsn; > + return 0; > } > > /* > @@ -441,7 +439,7 @@ xlog_recover_validate_buf_type( > * given buffer. The bitmap in the buf log format structure indicates > * where to place the logged data. > */ > -STATIC void > +static int > xlog_recover_do_reg_buffer( > struct xfs_mount *mp, > struct xlog_recover_item *item, > @@ -523,20 +521,20 @@ xlog_recover_do_reg_buffer( > /* Shouldn't be any more regions */ > ASSERT(i == item->ri_total); > > - xlog_recover_validate_buf_type(mp, bp, buf_f, current_lsn); > + return xlog_recover_validate_buf_type(mp, bp, buf_f, current_lsn); > } > > /* > - * Perform a dquot buffer recovery. > + * Test if this dquot buffer item should be recovered. > * Simple algorithm: if we have found a QUOTAOFF log item of the same type > * (ie. USR or GRP), then just toss this buffer away; don't recover it. > * Else, treat it as a regular buffer and do recovery. > * > - * Return false if the buffer was tossed and true if we recovered the buffer to > - * indicate to the caller if the buffer needs writing. > + * Return false if the buffer should be tossed and true if the buffer needs > + * to be recovered. > */ > -STATIC bool > -xlog_recover_do_dquot_buffer( > +static bool > +xlog_recover_this_dquot_buffer( > struct xfs_mount *mp, > struct xlog *log, > struct xlog_recover_item *item, > @@ -565,8 +563,6 @@ xlog_recover_do_dquot_buffer( > */ > if (log->l_quotaoffs_flag & type) > return false; > - > - xlog_recover_do_reg_buffer(mp, item, bp, buf_f, NULLCOMMITLSN); > return true; > } > > @@ -952,18 +948,19 @@ xlog_recover_buf_commit_pass2( > > if (buf_f->blf_flags & XFS_BLF_INODE_BUF) { > error = xlog_recover_do_inode_buffer(mp, item, bp, buf_f); > - if (error) > - goto out_release; > } else if (buf_f->blf_flags & > (XFS_BLF_UDQUOT_BUF|XFS_BLF_PDQUOT_BUF|XFS_BLF_GDQUOT_BUF)) { > - bool dirty; > - > - dirty = xlog_recover_do_dquot_buffer(mp, log, item, bp, buf_f); > - if (!dirty) > + if (!xlog_recover_this_dquot_buffer(mp, log, item, bp, buf_f)) > goto out_release; > + > + error = xlog_recover_do_reg_buffer(mp, item, bp, buf_f, > + NULLCOMMITLSN); > } else { > - xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); > + error = xlog_recover_do_reg_buffer(mp, item, bp, buf_f, > + current_lsn); > } > + if (error) > + goto out_release; > > /* > * Perform delayed write on the buffer. Asynchronous writes will be > -- > 2.43.0 > >
diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c index d74bf7bb7794..dba57ee6fa6d 100644 --- a/fs/xfs/xfs_buf_item_recover.c +++ b/fs/xfs/xfs_buf_item_recover.c @@ -207,7 +207,7 @@ xlog_recover_buf_commit_pass1( * the first 32 bits of the buffer (most blocks), * inside a struct xfs_da_blkinfo at the start of the buffer. */ -static void +static int xlog_recover_validate_buf_type( struct xfs_mount *mp, struct xfs_buf *bp, @@ -407,11 +407,12 @@ xlog_recover_validate_buf_type( * skipped. */ if (current_lsn == NULLCOMMITLSN) - return; + return 0;; if (warnmsg) { xfs_warn(mp, warnmsg); - ASSERT(0); + xfs_buf_corruption_error(bp, __this_address); + return -EFSCORRUPTED; } /* @@ -425,14 +426,11 @@ xlog_recover_validate_buf_type( * the buffer. Therefore, initialize a bli purely to carry the LSN to * the verifier. */ - if (bp->b_ops) { - struct xfs_buf_log_item *bip; - - bp->b_flags |= _XBF_LOGRECOVERY; - xfs_buf_item_init(bp, mp); - bip = bp->b_log_item; - bip->bli_item.li_lsn = current_lsn; - } + ASSERT(bp->b_ops); + bp->b_flags |= _XBF_LOGRECOVERY; + xfs_buf_item_init(bp, mp); + bp->b_log_item->bli_item.li_lsn = current_lsn; + return 0; } /* @@ -441,7 +439,7 @@ xlog_recover_validate_buf_type( * given buffer. The bitmap in the buf log format structure indicates * where to place the logged data. */ -STATIC void +static int xlog_recover_do_reg_buffer( struct xfs_mount *mp, struct xlog_recover_item *item, @@ -523,20 +521,20 @@ xlog_recover_do_reg_buffer( /* Shouldn't be any more regions */ ASSERT(i == item->ri_total); - xlog_recover_validate_buf_type(mp, bp, buf_f, current_lsn); + return xlog_recover_validate_buf_type(mp, bp, buf_f, current_lsn); } /* - * Perform a dquot buffer recovery. + * Test if this dquot buffer item should be recovered. * Simple algorithm: if we have found a QUOTAOFF log item of the same type * (ie. USR or GRP), then just toss this buffer away; don't recover it. * Else, treat it as a regular buffer and do recovery. * - * Return false if the buffer was tossed and true if we recovered the buffer to - * indicate to the caller if the buffer needs writing. + * Return false if the buffer should be tossed and true if the buffer needs + * to be recovered. */ -STATIC bool -xlog_recover_do_dquot_buffer( +static bool +xlog_recover_this_dquot_buffer( struct xfs_mount *mp, struct xlog *log, struct xlog_recover_item *item, @@ -565,8 +563,6 @@ xlog_recover_do_dquot_buffer( */ if (log->l_quotaoffs_flag & type) return false; - - xlog_recover_do_reg_buffer(mp, item, bp, buf_f, NULLCOMMITLSN); return true; } @@ -952,18 +948,19 @@ xlog_recover_buf_commit_pass2( if (buf_f->blf_flags & XFS_BLF_INODE_BUF) { error = xlog_recover_do_inode_buffer(mp, item, bp, buf_f); - if (error) - goto out_release; } else if (buf_f->blf_flags & (XFS_BLF_UDQUOT_BUF|XFS_BLF_PDQUOT_BUF|XFS_BLF_GDQUOT_BUF)) { - bool dirty; - - dirty = xlog_recover_do_dquot_buffer(mp, log, item, bp, buf_f); - if (!dirty) + if (!xlog_recover_this_dquot_buffer(mp, log, item, bp, buf_f)) goto out_release; + + error = xlog_recover_do_reg_buffer(mp, item, bp, buf_f, + NULLCOMMITLSN); } else { - xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn); + error = xlog_recover_do_reg_buffer(mp, item, bp, buf_f, + current_lsn); } + if (error) + goto out_release; /* * Perform delayed write on the buffer. Asynchronous writes will be