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

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

[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. 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.

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: stable@vger.kernel.org # 7.1+
Signed-off-by: Mykola Lysenko <nickolay.lysenko@gmail.com>
---
Note 1: I found, reproduced and created a fix for this problem using AI
tools

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);
-- 
2.43.0

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

[Qu Wenruo]

在 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.

> 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.

> 
> 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: stable@vger.kernel.org # 7.1+
> Signed-off-by: Mykola Lysenko <nickolay.lysenko@gmail.com>
> ---
> 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.

Thanks,
Qu

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

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

[Mykola Lysenko]

Hi Qu! Thanks a lot for your review!

On Thu, Jul 16, 2026 at 3:29 PM Qu Wenruo <quwenruo.btrfs@gmx.com> 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: stable@vger.kernel.org # 7.1+
> > Signed-off-by: Mykola Lysenko <nickolay.lysenko@gmail.com>
> > ---
> > 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);
>

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

[David Sterba]

On Fri, Jul 17, 2026 at 07:59:26AM +0930, Qu Wenruo wrote:
> > 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.

Right, unfortunately scrub was not designed with future extensions of
the stats. We can add v2 of the scrub progress ioctl as this does the
reporting part, and not the main scrub ioctl.