Re: [PATCH v2] btrfs: scrub: avoid unnecessary extent tree search for striped profiles

[Qu Wenruo <wqu@suse.com>]

On 2023/8/17 19:47, David Sterba wrote:
> On Tue, Aug 15, 2023 at 07:07:19PM +0800, Qu Wenruo wrote:
>> [PROBLEM]
>> Since commit 8557635ed2b0 ("btrfs: scrub: introduce dedicated helper to
>> scrub simple-stripe based range"), the scrub speed of striped profiles
>> (RAID0/RAID10/RAID5/RAID6) are degraded, if the block group is mostly
>> empty or fragmented.
>>
>> [CAUSE]
>> In scrub_simple_stripe(), which is the responsible for RAID0/RAID10
>> profiles, we just call scrub_simple_mirror() and increase our
>> @cur_logical and @cur_physical.
>>
>> The problem is, if there are no more extents inside the block group, or
>> the next extent is far away from our current logical, we would call
>> scrub_simple_mirror() for the empty ranges again and again, until we
>> reach the next next.
>>
>> This is completely a waste of CPU time, thus it greatly degrade the
>> scrub performance for stripped profiles.
>>
>> This is also affecting RAID56, as we rely on scrub_simple_mirror() for
>> data stripes of RAID56.
>>
>> [FIX]
>> - Introduce scrub_ctx::found_next to record the next extent we found
>>    This member would be updated by find_first_extent_item() calls inside
>>    scrub_find_fill_first_stripe().
>>
>> - Skip to the next stripe directly in scrub_simple_stripe()
>>    If we detect sctx->found_next is beyond our current stripe, we just
>>    skip to the full stripe which covers the target bytenr.
>>
>> - Skip to the next full stripe covering sctx->found_next
>>    Unlike RAID0/RAID10, we can not easily skip to the next stripe due to
>>    rotation.
>>    But we can still skip to the next full stripe, which can still save us
>>    a lot of time.
>>
>> Fixes: 8557635ed2b0 ("btrfs: scrub: introduce dedicated helper to scrub simple-stripe based range")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Fix a u64/u32 division not using the div_u64() helper
>>
>> - Slightly change the advancement of logical/physical for RAID0 and
>>    RAID56
>>    Now logical/physical is always increased first, this removes one
>>    if () branch.
>>
>> This patch is based on the scrub_testing branch (which is misc-next +
>> scrub performance fixes).
>>
>> Thus there would be quite some conflicts for stable branches and would
>> need manual backport.
> 
> Added to misc-next, thanks.

BTW, did you hit any compiling errors on 32bits systems?

Some bots reported linkage error due to div_u64(), but I see the same 
div_u64() calls inside scrub.c thus I'm not sure what's really going on 
here.

Thanks,
Qu