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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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