Re: [PATCH] btrfs: raid56: fix inverted bio-list check in scrub read assembly

Hi Qu! Thanks a lot for your review!

On Thu, Jul 16, 2026 at 3:29 PM Qu Wenruo <[email protected]> wrote:
>
>
>
> 在 2026/7/17 03:15, Mykola Lysenko 写道:
> > Commit 5387bd958180 ("btrfs: raid56: remove sector_ptr structure")
> > converted the bio-list membership checks from sector pointers to
> > physical addresses.  The two conversions in rmw_assemble_write_bios()
> > kept their polarity (skip the sector when it is NOT in the bio list,
> > i.e. when there is nothing to write), but scrub_assemble_read_bios()
> > has the opposite polarity -- skip the sector when it IS in the bio
> > list, because then there is nothing to read -- and the conversion
> > flipped it:
> >
> >       -       sector = sector_in_rbio(rbio, stripe, sectornr, 1);
> >       -       if (sector)
> >       +       paddr = sector_paddr_in_rbio(rbio, stripe, sectornr, 1);
> >       +       if (paddr == INVALID_PADDR)
> >                       continue;
> >
> > Since a parity-scrub rbio's bio list only holds the empty completion
> > bio, the result is that scrub_assemble_read_bios() submits no reads at
> > all.
>
> Then you should remove the check completely, and replace it with an
> ASSERT() to make sure scrub should not have any bio sectors, aka,
> rbio->bio_paddrs[] are all INVALID_PADDR, or all bios (should be one) in
> the bio_list are empty.
>
> Otherwise the analyze looks good to me.

Sounds good, will be addressed in v2.

>
> > finish_parity_scrub() then compares the parity it computes from
> > the (cached, correct) data stripes against whatever happens to be in
> > the freshly allocated, uninitialized stripe pages:
> >
> >    - if the garbage differs from the computed parity, the sector is
> >      "repaired" and written back -- accidentally producing the correct
> >      on-disk result;
> >    - if a recycled page happens to still hold the old (correct) parity
> >      content, the sector is deemed clean, dropped from dbitmap, and the
> >      actually-corrupt on-disk parity is left in place -- silently, with
> >      every scrub error counter reading zero.
>
> Unfortunately that's by design, as scrub never reports P/Q corruptions
> since there is no counter for them, and there is no space left for
> expansion either.

Got it, will reframe.

>
> >
> > The second case is intermittent because it depends on page-allocator
> > recycling.  Observed with fstests btrfs/297 (raid5, 2 devices): the
> > corrupted P stripe intermittently stays corrupt after a scrub that
> > reports no errors -- roughly 1/10 runs on x86-64 KVM and up to 7/8 on
> > a UML build whose timing favors page reuse. Instrumentation of
> > verify_one_parity_step() showed the "on disk" bytes never matching the
> > device content (stale 0xaa / zeroed pages instead of the injected
> > 0xff), and after this fix the injected corruption is read, detected
> > and repaired in every run (8/8 UML, 10/10 KVM).
> >
> > Fixes: 5387bd958180 ("btrfs: raid56: remove sector_ptr structure")
> > CC: [email protected] # 7.1+
> > Signed-off-by: Mykola Lysenko <[email protected]>
> > ---
> > Note 1: I found, reproduced and created a fix for this problem using AI
> > tools
>
> Not sure if we still require the disclosure of AI usage using
> Assisted-by: tag, but I'd prefer that to be extra clear.

Sounds good.

>
> Thanks,
> Qu

Thank you!

>
> >
> > Note 2: I am referencing UML (User-Mode Linux) above which is coming
> > from the project https://github.com/mykola-lysenko/btrfs-uml-fstests/ to
> > run xfstests in the UML. For reference only.
> >
> >   fs/btrfs/raid56.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> > index 00a01b97cc..93de764d70 100644
> > --- a/fs/btrfs/raid56.c
> > +++ b/fs/btrfs/raid56.c
> > @@ -2910,11 +2910,11 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
> >
> >               /*
> >                * We want to find all the sectors missing from the rbio and
> > -              * read them from the disk. If sector_paddr_in_rbio() finds a sector
> > -              * in the bio list we don't need to read it off the stripe.
> > +              * read them from the disk. If sector_paddrs_in_rbio() finds a
> > +              * sector in the bio list we don't need to read it off the
> > +              * stripe.
> >                */
> > -             paddrs = sector_paddrs_in_rbio(rbio, stripe, sectornr, 1);
> > -             if (paddrs == NULL)
> > +             if (sector_paddrs_in_rbio(rbio, stripe, sectornr, 1))
> >                       continue;
> >
> >               paddrs = rbio_stripe_paddrs(rbio, stripe, sectornr);
>