Re: [PATCH v4 0/9] btrfs: refactor scrub entrances for each profile

[Qu Wenruo <wqu@suse.com>]

On 2022/3/11 15:38, Qu Wenruo wrote:
> This patchset is cherry-picked from my github repo:
> https://github.com/adam900710/linux/tree/refactor_scrub

Just to mention, the branch get an update as misc-next now merged the 
renaming part.

The good news is, this entrance refactor can be applied without 
conflicts. So no update on this patchset.

Thanks,
Qu
> 
> [Changelog]
> v2:
> - Rebased to latest misc-next
> 
> - Fix several uninitialized variables in the 2nd and 3rd patch
>    This is because @physical, @physical_end and @offset are also used for
>    zoned device sync.
> 
>    Initial those values early to fix the problem.
> 
> v3:
> - Add two patches to better split cleanups from refactors
>    One to change the timing of initialization of @physical and
>    @physical_end
> 
>    One to remove dead non-RAID56 branches after making scrub_stripe() to
>    work on RAID56 only.
> 
> - Fix an unfinished comment in scrub_simple_mirror()
> 
> v4:
> - Rebased after scrub renaming patchset
>    Only minor conflicts.
> 
> - Fix uninitialized variable in patch 6 and 7
>    Now there should be no uninitialized even only patches 1~6 are
>    applied.
> 
> [CRAP-BUT-IT-WORKS(TM)]
> 
> Scrub is one of the area we seldom touch because:
> 
> - It's a mess
>    Just check scrub_stripe() function.
>    It's a function scrubbing a stripe for *all* profiles.
> 
>    It's near 400 lines for a single complex function, with double while()
>    loop and several different jumps inside the loop.
> 
>    Not to mention the lack of comments for various structures.
> 
>    This should and will never happen under our current code standard.
> 
> - It just works
>    I have hit more than 10 bugs during development, and I just want to
>    give up the refactor, as even the code is crap, it works, passing the
>    existing scrub/replace group.
>    While no matter how small code change I'm doing, it always fails to pass
>    the same tests.
> 
> [REFACTOR-IDEA]
> 
> The core idea here, is to get rid of one-fit-all solution for
> scrub_stripe().
> 
> Instead, we explicitly separate the scrub into 3 groups (the idea is
> from my btrfs-fuse project):
> 
> - Simple-mirror based profiles
>    This includes SINGLE/DUP/RAID1/RAID1C* profiles.
>    They have no stripe, and their repair is purely mirror based.
> 
> - Simple-stripe based profiles
>    This includes RAID0/RAID10 profiles.
>    They are just simple stripe (without P/Q nor rotation), with extra
>    mirrors to support their repair.
> 
> - RAID56
>    The most complex profiles, they have extra P/Q, and have rotation.
> 
> [REFACTOR-IMPLEMENTATION]
> 
> So we have 3 entrances for all those supported profiles:
> 
> - scrub_simple_mirror()
>    For SINGLE/DUP/RAID1/RAID1C* profiles.
>    Just go through each extent and scrub the extent.
> 
> - scrub_simple_stripe()
>    For RAID0/RAID10 profiles.
>    Instead we go each data stripe first, then inside each data stripe, we
>    can call scrub_simple_mirror(), since after stripe split, RAID0 is
>    just SINGLE and RAID10 is just RAID1.
> 
> - scrub_stripe() untouched for RAID56
>    RAID56 still has all the complex things to do, but they can still be
>    split into two types (already done by the original code)
> 
>    * data stripes
>      They are no different in the verification part, RAID56 is just
>      SINGLE if we ignore the repair path.
>      It's only in repair path that our path divides.
> 
>      So we can reuse scrub_simple_mirror() again.
> 
>    * P/Q stripes
>      They already have a dedicated function handling the case.
> 
> With all these refactors, although we have several more functions, we
> get rid of:
> 
> - A double while () loop
> - Several jumps inside the double loop
> - Complex calculation to try to fit all profiles
> 
> And we get:
> 
> - Better comments
> - More dedicated functions
> - A better basis for further refactors
> 
> [FUTURE CLEANUPS]
> - Refactor scrub_pages/scrub_parity/... structures
> - Further cleanup RAID56 codes
> 
> Changelog:
> v2:
> - Rebased to latest misc-next
> 
> - Fix several uninitialized variables in the 2nd and 3rd patch
>    This is because @physical, @physical_end and @offset are also used for
>    zoned device sync.
> 
>    Initial those values early to fix the problem.
> 
> v3:
> - Add two patches to better split cleanups from refactors
>    One to change the timing of initialization of @physical and
>    @physical_end
> 
>    One to remove dead non-RAID56 branches after making scrub_stripe() to
>    work on RAID56 only.
> 
> - Fix an unfinished comment in scrub_simple_mirror()
> 
> Qu Wenruo (9):
>    btrfs: calculate @physical_end using @dev_extent_len directly in
>      scrub_stripe()
>    btrfs: introduce a helper to locate an extent item
>    btrfs: introduce dedicated helper to scrub simple-mirror based range
>    btrfs: introduce dedicated helper to scrub simple-stripe based range
>    btrfs: scrub: cleanup the non-RAID56 branches in scrub_stripe()
>    btrfs: use scrub_simple_mirror() to handle RAID56 data stripe scrub
>    btrfs: refactor scrub_raid56_parity()
>    btrfs: use find_first_extent_item() to replace the open-coded extent
>      item search
>    btrfs: move scrub_remap_extent() call into scrub_extent() with more
>      comments
> 
>   fs/btrfs/scrub.c | 1037 +++++++++++++++++++++++++---------------------
>   1 file changed, 559 insertions(+), 478 deletions(-)
>