fs/btrfs/raid56.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
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@vger.kernel.org # 7.1+
Suggested-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Mykola Lysenko <nickolay.lysenko@gmail.com>
---
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@gmail.com/
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
在 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@vger.kernel.org # 7.1+
> Suggested-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Mykola Lysenko <nickolay.lysenko@gmail.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
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@gmail.com/
>
> 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
© 2016 - 2026 Red Hat, Inc.