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

[Qu Wenruo <wqu@suse.com>]

Hi David,

Any plan to merge this patchset?

And if you need, I can merge all these refactors into one branch for you 
to fetch.

I know this sounds terrifying especially after the autodefrag "refactor" 
disaster, but unlike autodefrag, scrub/replace has existing test case 
coverage and this time it's really refactor, no behavior change (at 
least no intentional one).

I hope to get a stable base for the incoming scrub_page/scrub_bio 
structure cleanup.

As there is some plan to make scrub to use page::index for those 
anonymous pages, so they don't need to rely on bi_private to get their 
logical bytenr, and hopefully just one structure, scrub_range, to 
replace the scrub_page/scrub_bio things.

Thanks,
Qu

On 2022/1/7 10:34, Qu Wenruo wrote:
> The branch is based on several recent submitted small cleanups, thus
> it's better to fetch the branch from github:
> 
> https://github.com/adam900710/linux/tree/refactor_scrub
> 
> [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
> 
> [INCOMING CLEANUPS]
> 
> - Use find_first_extent_item() to cleanup the RAID56 code
>    This part itself can be as large this patchset already, thus will be
>    in its own patchset.
> 
> - Refactor scrub_pages/scrub_parity/... structures
> 
> Qu Wenruo (4):
>    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: use scrub_simple_mirror() to handle RAID56 data stripe scrub
> 
>   fs/btrfs/scrub.c | 702 ++++++++++++++++++++++++++++-------------------
>   1 file changed, 422 insertions(+), 280 deletions(-)
>