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

Qu Wenruo
2022-03-15T07:03:07Z

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(-)
>