| Message ID | f66b01bb-b4ce-8713-c3db-fbbd39703737@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: log proper length of btree block in scrub/repair | expand |
On Tue, Aug 27, 2019 at 02:17:36PM -0500, Eric Sandeen wrote: > xfs_trans_log_buf() takes a final argument of the last byte to > log in the buffer; b_length is in basic blocks, so this isn't > the correct last byte. Fix it. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > just found by inspection/pattern matching, not tested TBH... > > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c > index 4cfeec57fb05..7bcc755beb40 100644 > --- a/fs/xfs/scrub/repair.c > +++ b/fs/xfs/scrub/repair.c > @@ -351,7 +351,7 @@ xrep_init_btblock( > xfs_buf_zero(bp, 0, BBTOB(bp->b_length)); > xfs_btree_init_block(mp, bp, btnum, 0, 0, sc->sa.agno); > xfs_trans_buf_set_type(tp, bp, XFS_BLFT_BTREE_BUF); > - xfs_trans_log_buf(tp, bp, 0, bp->b_length); > + xfs_trans_log_buf(tp, bp, 0, BBTOB(bp->b_length) - 1); Ok, while this is technically wrong, it would not have resulted in a bug at all. That's because the length would have been rounded up to a BLFCHUNK of 128 bytes, and that covers the entire btree block header. Hence the part of the buffer that actually matters for recovery was still logged correctly. Still, should be fixed, so consider it: Reviewed-by: Dave Chinner <dchinner@redhat.com> Cheers, Dave.
On Tue, Aug 27, 2019 at 02:17:36PM -0500, Eric Sandeen wrote: > xfs_trans_log_buf() takes a final argument of the last byte to > log in the buffer; b_length is in basic blocks, so this isn't > the correct last byte. Fix it. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > just found by inspection/pattern matching, not tested TBH... Looks good. And I wonder if we should fix the interface instead, as it seems to lead to convoluted coe in just about every caller. Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Aug 27, 2019 at 02:17:36PM -0500, Eric Sandeen wrote: > xfs_trans_log_buf() takes a final argument of the last byte to > log in the buffer; b_length is in basic blocks, so this isn't > the correct last byte. Fix it. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > just found by inspection/pattern matching, not tested TBH... > > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c > index 4cfeec57fb05..7bcc755beb40 100644 > --- a/fs/xfs/scrub/repair.c > +++ b/fs/xfs/scrub/repair.c > @@ -351,7 +351,7 @@ xrep_init_btblock( > xfs_buf_zero(bp, 0, BBTOB(bp->b_length)); > xfs_btree_init_block(mp, bp, btnum, 0, 0, sc->sa.agno); > xfs_trans_buf_set_type(tp, bp, XFS_BLFT_BTREE_BUF); > - xfs_trans_log_buf(tp, bp, 0, bp->b_length); > + xfs_trans_log_buf(tp, bp, 0, BBTOB(bp->b_length) - 1); > bp->b_ops = ops; > *bpp = bp; > >
On 8/29/19 3:26 AM, Christoph Hellwig wrote: > On Tue, Aug 27, 2019 at 02:17:36PM -0500, Eric Sandeen wrote: >> xfs_trans_log_buf() takes a final argument of the last byte to >> log in the buffer; b_length is in basic blocks, so this isn't >> the correct last byte. Fix it. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> just found by inspection/pattern matching, not tested TBH... > > Looks good. And I wonder if we should fix the interface instead, > as it seems to lead to convoluted coe in just about every caller. Yup, I had considered that too. -Eric > Reviewed-by: Christoph Hellwig <hch@lst.de> >
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c index 4cfeec57fb05..7bcc755beb40 100644 --- a/fs/xfs/scrub/repair.c +++ b/fs/xfs/scrub/repair.c @@ -351,7 +351,7 @@ xrep_init_btblock( xfs_buf_zero(bp, 0, BBTOB(bp->b_length)); xfs_btree_init_block(mp, bp, btnum, 0, 0, sc->sa.agno); xfs_trans_buf_set_type(tp, bp, XFS_BLFT_BTREE_BUF); - xfs_trans_log_buf(tp, bp, 0, bp->b_length); + xfs_trans_log_buf(tp, bp, 0, BBTOB(bp->b_length) - 1); bp->b_ops = ops; *bpp = bp;
xfs_trans_log_buf() takes a final argument of the last byte to log in the buffer; b_length is in basic blocks, so this isn't the correct last byte. Fix it. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- just found by inspection/pattern matching, not tested TBH...