[PATCH] xfs: log proper length of btree block in scrub/repair

[PATCH] xfs: log proper length of btree block in scrub/repair

[Eric Sandeen]

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);
 	bp->b_ops = ops;
 	*bpp = bp;
 

Re: [PATCH] xfs: log proper length of btree block in scrub/repair

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: log proper length of btree block in scrub/repair

[Christoph Hellwig]

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>

Re: [PATCH] xfs: log proper length of btree block in scrub/repair

[Darrick J. Wong]

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;
>  
> 

Re: [PATCH] xfs: log proper length of btree block in scrub/repair

[Eric Sandeen]

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>
>