Re: [PATCH 1/7] btrfs: scrub: fix incorrectly reported logical/physical address

[Anand Jain <anand.jain@oracle.com>]

On 3/14/24 15:20, Qu Wenruo wrote:
> [BUG]
> Scrub is not reporting the correct logical/physical address, it can be
> verified by the following script:
> 
>   # mkfs.btrfs -f $dev1
>   # mount $dev1 $mnt
>   # xfs_io -f -c "pwrite -S 0xaa 0 128k" $mnt/file1
>   # umount $mnt
>   # xfs_io -f -c "pwrite -S 0xff 13647872 4k" $dev1
>   # mount $dev1 $mnt
>   # btrfs scrub start -fB $mnt
>   # umount $mnt
> 
> Note above 13647872 is the physical address for logical 13631488 + 4K.
> 
> Scrub would report the following error:
> 
>   BTRFS error (device dm-2): unable to fixup (regular) error at logical 13631488 on dev /dev/mapper/test-scratch1 physical 13631488
>   BTRFS warning (device dm-2): checksum error at logical 13631488 on dev /dev/mapper/test-scratch1, physical 13631488, root 5, inode 257, offset 0, length 4096, links 1 (path: file1)
> 
> On the other hand, "btrfs check --check-data-csum" is reporting the
> correct logical/physical address:
> 
>   Checking filesystem on /dev/test/scratch1
>   UUID: db2eb621-b09d-4f24-8199-da17dc7b3201
>   [5/7] checking csums against data
>   mirror 1 bytenr 13647872 csum 0x13fec125 expected csum 0x656bd64e
>   ERROR: errors found in csum tree
> 
> [CAUSE]
> In the function scrub_stripe_report_errors(), we always use the
> stripe->logical and its physical address to print the error message, not
> taking the sector number into consideration at all.
> 
> [FIX]
> Fix the error reporting function by calculating logical/physical with
> the sector number.
> 
> Now the scrub report is correct:
> 
>   BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872 on dev /dev/mapper/test-scratch1 physical 13647872
>   BTRFS warning (device dm-2): checksum error at logical 13647872 on dev /dev/mapper/test-scratch1, physical 13647872, root 5, inode 257, offset 16384, length 4096, links 1 (path: file1)
> 
> Fixes: 0096580713ff ("btrfs: scrub: introduce error reporting functionality for scrub_stripe")
> Signed-off-by: Qu Wenruo <wqu@suse.com>


To help accurate debug on stable kernel, we could also add
CC: stable@vger.kernel.org #6.4+

Not too particular though.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.


> ---
>   fs/btrfs/scrub.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index fa25004ab04e..119e98797b21 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -870,7 +870,7 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>   				      DEFAULT_RATELIMIT_BURST);
>   	struct btrfs_fs_info *fs_info = sctx->fs_info;
>   	struct btrfs_device *dev = NULL;
> -	u64 physical = 0;
> +	u64 stripe_physical = stripe->physical;
>   	int nr_data_sectors = 0;
>   	int nr_meta_sectors = 0;
>   	int nr_nodatacsum_sectors = 0;
> @@ -903,13 +903,17 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>   		 */
>   		if (ret < 0)
>   			goto skip;
> -		physical = bioc->stripes[stripe_index].physical;
> +		stripe_physical = bioc->stripes[stripe_index].physical;
>   		dev = bioc->stripes[stripe_index].dev;
>   		btrfs_put_bioc(bioc);
>   	}
>   
>   skip:
>   	for_each_set_bit(sector_nr, &stripe->extent_sector_bitmap, stripe->nr_sectors) {
> +		u64 logical = stripe->logical +
> +			      (sector_nr << fs_info->sectorsize_bits);
> +		u64 physical = stripe_physical +
> +			      (sector_nr << fs_info->sectorsize_bits);
>   		bool repaired = false;
>   
>   		if (stripe->sectors[sector_nr].is_metadata) {
> @@ -938,12 +942,12 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>   			if (dev) {
>   				btrfs_err_rl_in_rcu(fs_info,
>   			"fixed up error at logical %llu on dev %s physical %llu",
> -					    stripe->logical, btrfs_dev_name(dev),
> +					    logical, btrfs_dev_name(dev),
>   					    physical);
>   			} else {
>   				btrfs_err_rl_in_rcu(fs_info,
>   			"fixed up error at logical %llu on mirror %u",
> -					    stripe->logical, stripe->mirror_num);
> +					    logical, stripe->mirror_num);
>   			}
>   			continue;
>   		}
> @@ -952,26 +956,26 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>   		if (dev) {
>   			btrfs_err_rl_in_rcu(fs_info,
>   	"unable to fixup (regular) error at logical %llu on dev %s physical %llu",
> -					    stripe->logical, btrfs_dev_name(dev),
> +					    logical, btrfs_dev_name(dev),
>   					    physical);
>   		} else {
>   			btrfs_err_rl_in_rcu(fs_info,
>   	"unable to fixup (regular) error at logical %llu on mirror %u",
> -					    stripe->logical, stripe->mirror_num);
> +					    logical, stripe->mirror_num);
>   		}
>   
>   		if (test_bit(sector_nr, &stripe->io_error_bitmap))
>   			if (__ratelimit(&rs) && dev)
>   				scrub_print_common_warning("i/o error", dev, false,
> -						     stripe->logical, physical);
> +						     logical, physical);
>   		if (test_bit(sector_nr, &stripe->csum_error_bitmap))
>   			if (__ratelimit(&rs) && dev)
>   				scrub_print_common_warning("checksum error", dev, false,
> -						     stripe->logical, physical);
> +						     logical, physical);
>   		if (test_bit(sector_nr, &stripe->meta_error_bitmap))
>   			if (__ratelimit(&rs) && dev)
>   				scrub_print_common_warning("header error", dev, false,
> -						     stripe->logical, physical);
> +						     logical, physical);
>   	}
>   
>   	spin_lock(&sctx->stat_lock);