Re: [PATCH v2] btrfs: raid56: fix scrub read assembly submitting no reads

[Qu Wenruo]

在 2026/7/19 09:07, 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. (Scrub reports
>      no errors either way: there is no counter for P/Q corruption by
>      design, so the bug here is purely the failure to read and repair.)
>
> 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 --
> roughly 1/10 runs on x86-64 KVM and up to 7/8 on a UML build whose
> timing favors page reuse.
>
> Since the bio-list check can never be true for a parity-scrub rbio --
> raid56_parity_alloc_scrub_rbio() adds a single empty completion bio
> (asserting bi_size == 0), bio_paddrs[] is only populated by
> index_rbio_pages() which is never called for BTRFS_RBIO_PARITY_SCRUB,
> and rbio_can_merge() refuses to merge rbios of different operations --
> remove the dead check entirely and assert the invariant instead, as
> suggested by Qu Wenruo.
>
> After this fix the injected corruption is read, detected and repaired
> in every run (8/8 UML, 10/10 KVM), and the new assertion never fires
> across the full fstests raid group.
>
> Fixes: 5387bd958180 ("btrfs: raid56: remove sector_ptr structure")
> CC: stable@xxxxxxxxxxxxxxx # 7.1+
> Suggested-by: Qu Wenruo <quwenruo.btrfs@xxxxxxx>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Mykola Lysenko <nickolay.lysenko@xxxxxxxxx>

Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>

And pushed into for-next branch.

Thanks,
Qu
> ---
> v2: per Qu Wenruo's review -- the membership check is dead code in
> either polarity for a scrub rbio, so remove it and ASSERT the
> invariant instead of restoring the original polarity; clarify in the
> message that the absence of P/Q error reporting is by design; move the
> AI-usage disclosure to an Assisted-by tag per
> Documentation/process/coding-assistants.rst.
>
> v1: https://lore.kernel.org/linux-btrfs/20260716174511.8738-1-nickolay.lysenko@xxxxxxxxx/
>
> Note: UML (User-Mode Linux) results above come from
> https://github.com/mykola-lysenko/btrfs-uml-fstests/ -- for reference
> only.
>
>   fs/btrfs/raid56.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2909,13 +2909,12 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
>   			continue;
>
>   		/*
> -		 * 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.
> +		 * A parity-scrub rbio carries no data in its bio list: the
> +		 * only bio there is the empty completion bio added by
> +		 * raid56_parity_alloc_scrub_rbio().  Every sector is read
> +		 * from the stripe, so only assert that invariant here.
>   		 */
> -		paddrs = sector_paddrs_in_rbio(rbio, stripe, sectornr, 1);
> -		if (paddrs == NULL)
> -			continue;
> +		ASSERT(!sector_paddrs_in_rbio(rbio, stripe, sectornr, 1));
>
>   		paddrs = rbio_stripe_paddrs(rbio, stripe, sectornr);
>   		/*
> --
> 2.43.0