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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/3/14 22:54, Anand Jain 写道:
> 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+

IIRC Fixes: tag is more accurate than just CC stable, as stable has the 
correct bots catching the fixes line.

Thanks,
Qu
> 
> 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);
> 
>