xfs: log proper length of btree block in scrub/repair
diff mbox series

Message ID f66b01bb-b4ce-8713-c3db-fbbd39703737@redhat.com
State Accepted
Headers show
Series
  • xfs: log proper length of btree block in scrub/repair
Related show

Commit Message

Eric Sandeen Aug. 27, 2019, 7:17 p.m. UTC
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...

Comments

Dave Chinner Aug. 27, 2019, 11:29 p.m. UTC | #1
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.
Christoph Hellwig Aug. 29, 2019, 8:26 a.m. UTC | #2
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>
Darrick J. Wong Aug. 29, 2019, 9:31 p.m. UTC | #3
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;
>  
>
Eric Sandeen Aug. 30, 2019, 2:36 p.m. UTC | #4
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>
>

Patch
diff mbox series

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;