Re: [PATCH 4/7] btrfs: scrub: remove unnecessary dev/physical lookup for scrub_stripe_report_errors()

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

在 2024/3/19 02:46, Filipe Manana 写道:
> On Thu, Mar 14, 2024 at 8:28 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2024/3/15 03:56, Filipe Manana 写道:
>>> On Thu, Mar 14, 2024 at 9:50 AM Qu Wenruo <wqu@suse.com> wrote:
>>>>
>>>> The @stripe passed into scrub_stripe_report_errors() either has
>>>> stripe->dev and stripe->physical properly populated (regular data
>>>> stripes) or stripe->flags would have SCRUB_STRIPE_FLAG_NO_REPORT
>>>> (RAID56 P/Q stripes).
>>>>
>>>> Thus there is no need to go with btrfs_map_block() to get the
>>>> dev/physical.
>>>>
>>>> Just add an extra ASSERT() to make sure we get stripe->dev populated
>>>> correctly.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    fs/btrfs/scrub.c | 59 ++++++------------------------------------------
>>>>    1 file changed, 7 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>>>> index 5e371ffdb37b..277583464371 100644
>>>> --- a/fs/btrfs/scrub.c
>>>> +++ b/fs/btrfs/scrub.c
>>>> @@ -860,10 +860,8 @@ static void scrub_stripe_submit_repair_read(struct scrub_stripe *stripe,
>>>>    static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>>>>                                          struct scrub_stripe *stripe)
>>>>    {
>>>> -       static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
>>>> -                                     DEFAULT_RATELIMIT_BURST);
>>>>           struct btrfs_fs_info *fs_info = sctx->fs_info;
>>>> -       struct btrfs_device *dev = NULL;
>>>> +       struct btrfs_device *dev = stripe->dev;
>>>>           u64 stripe_physical = stripe->physical;
>>>>           int nr_data_sectors = 0;
>>>>           int nr_meta_sectors = 0;
>>>> @@ -874,35 +872,7 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>>>>           if (test_bit(SCRUB_STRIPE_FLAG_NO_REPORT, &stripe->state))
>>>>                   return;
>>>>
>>>> -       /*
>>>> -        * Init needed infos for error reporting.
>>>> -        *
>>>> -        * Although our scrub_stripe infrastructure is mostly based on btrfs_submit_bio()
>>>> -        * thus no need for dev/physical, error reporting still needs dev and physical.
>>>> -        */
>>>> -       if (!bitmap_empty(&stripe->init_error_bitmap, stripe->nr_sectors)) {
>>>> -               u64 mapped_len = fs_info->sectorsize;
>>>> -               struct btrfs_io_context *bioc = NULL;
>>>> -               int stripe_index = stripe->mirror_num - 1;
>>>> -               int ret;
>>>> -
>>>> -               /* For scrub, our mirror_num should always start at 1. */
>>>> -               ASSERT(stripe->mirror_num >= 1);
>>>> -               ret = btrfs_map_block(fs_info, BTRFS_MAP_GET_READ_MIRRORS,
>>>> -                                     stripe->logical, &mapped_len, &bioc,
>>>> -                                     NULL, NULL);
>>>> -               /*
>>>> -                * If we failed, dev will be NULL, and later detailed reports
>>>> -                * will just be skipped.
>>>> -                */
>>>> -               if (ret < 0)
>>>> -                       goto skip;
>>>> -               stripe_physical = bioc->stripes[stripe_index].physical;
>>>> -               dev = bioc->stripes[stripe_index].dev;
>>>> -               btrfs_put_bioc(bioc);
>>>> -       }
>>>> -
>>>> -skip:
>>>> +       ASSERT(dev);
>>>>           for_each_set_bit(sector_nr, &stripe->extent_sector_bitmap, stripe->nr_sectors) {
>>>>                   u64 logical = stripe->logical +
>>>>                                 (sector_nr << fs_info->sectorsize_bits);
>>>> @@ -933,42 +903,27 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>>>>                    * output the message of repaired message.
>>>>                    */
>>>>                   if (repaired) {
>>>> -                       if (dev) {
>>>> -                               btrfs_err_rl_in_rcu(fs_info,
>>>> +                       btrfs_err_rl_in_rcu(fs_info,
>>>>                           "fixed up error at logical %llu on dev %s physical %llu",
>>>>                                               logical, btrfs_dev_name(dev),
>>>>                                               physical);
>>>> -                       } else {
>>>> -                               btrfs_err_rl_in_rcu(fs_info,
>>>> -                       "fixed up error at logical %llu on mirror %u",
>>>> -                                           logical, stripe->mirror_num);
>>>> -                       }
>>>>                           continue;
>>>>                   }
>>>>
>>>>                   /* The remaining are all for unrepaired. */
>>>> -               if (dev) {
>>>> -                       btrfs_err_rl_in_rcu(fs_info,
>>>> +               btrfs_err_rl_in_rcu(fs_info,
>>>>           "unable to fixup (regular) error at logical %llu on dev %s physical %llu",
>>>>                                               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",
>>>> -                                           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,
>>>> +                       scrub_print_common_warning("i/o error", dev,
>>>>                                                        logical, physical);
>>>>                   if (test_bit(sector_nr, &stripe->csum_error_bitmap))
>>>> -                       if (__ratelimit(&rs) && dev)
>>>> -                               scrub_print_common_warning("checksum error", dev,
>>>> +                       scrub_print_common_warning("checksum error", dev,
>>>>                                                        logical, physical);
>>>>                   if (test_bit(sector_nr, &stripe->meta_error_bitmap))
>>>> -                       if (__ratelimit(&rs) && dev)
>>>> -                               scrub_print_common_warning("header error", dev,
>>>> +                       scrub_print_common_warning("header error", dev,
>>>
>>> Why are we removing the rate limiting?
>>> This seems like an unrelated change.
>>
>> Because the ratelimit is not consistent.
>>
>> E.g. the "fixed up error" line is not rated limited by @rs, but by
>> btrfs_err_rl_in_rcu().
>
> That I don't understand.
>
> What I see is that scrub_print_common_warning() calls
> btrfs_warn_in_rcu() or btrfs_warn() or scrub_print_warning_inode()
> (which calls btrfs_warn_in_rcu()).
> I don't see where btrfs_err_rl_in_rcu() is called. So to me it seems
> @rs is doing the rate limiting.
>
>>
>> And we would have scrub_print_common_warning() to go btrfs_*_rl() helper
>> in the coming patches.
>
> Ok, but if @rs is indeed doing the rate limiting here, then from an
> organization point of view, it should be removed in the later patch
> that makes scrub_print_common_warning() use the btrfs_*_rl() helpers.

OK, I'll rearrange the patches so that @rs would only be removed after
we have migrate all the messages are migrated to rate limited versions.

Thanks,
Qu
>
> Thanks.
>
>>
>> Thanks,
>> Qu
>>>
>>> Everything else looks ok.
>>>
>>> Thanks.
>>>
>>>>                                                        logical, physical);
>>>>           }
>>>>
>>>> --
>>>> 2.44.0
>>>>
>>>>
>>>