Re: [PATCH 1/5] btrfs: don't drop the error of the last split bio

[Zygo Blaxell <ce3g8jdj@umail.furryterror.org>]

On Fri, Oct 30, 2026 at 02:38:07PM +0000, Josef Bacik wrote:
> When a btrfs_bio had to be split, btrfs_bio_end_io() saves the first error
> of any of its parts in the status field of the original btrfs_bio. The
> part that completes last is supposed to load it into bi_status of the
> original bio before calling the end_io handler, but that is only done if
> that last part was successful:
> 
> 	if (atomic_dec_and_test(&bbio->pending_ios)) {
> 		/* Load split bio's error which might be set above. */
> 		if (status == BLK_STS_OK)
> 			bbio->bio.bi_status = READ_ONCE(bbio->status);
> 
> If the last part failed, the assumption is that bi_status was set at the
> top of the function. That is true if the last part is what is left of the
> original bio. If it is a split bio, the assignment was done to the split
> bio, which was freed right after. If the original bio has completed before
> without an error, its bi_status is BLK_STS_OK and stays like that:
> 
>   original bio (tail)                 split bio (head)
> 
>   btrfs_bio_end_io(orig, OK)
>     orig->bio.bi_status = OK
>     pending_ios 2 -> 1
>                                       btrfs_bio_end_io(split, IOERR)
>                                         split->bio.bi_status = IOERR
>                                         bio_put(split)
>                                         cmpxchg(&orig->status, OK, IOERR)
>                                         pending_ios 1 -> 0
>                                         orig->end_io(), bi_status is OK
> 
> The end_io handler covers the whole range, so for a buffered read all the
> folios are marked uptodate, including the ones that were never filled, and
> read(2) returns their content instead of -EIO. For a write the ordered
> extent is completed as if all the data had reached the device.
> 
> This can be reproduced with raid0 data on two devices, one of them slower
> than the other and with a bad block, by reading a file whose last stripe
> is on the fast device.

I've been running an identical patch because the same bug causes
undetected data corruption on raid56.

A read that fails csum verification completes only after read repair has
tried every mirror.  This takes far longer than the healthy splits of the
same bio, so the failing split completes last almost every time.

On a degraded raid56 read, the result is corruption that is not reported
to userspace.  raid56 reads are split at stripe boundaries, so any read
that crosses a boundary can return a sector that failed its csum check
to userspace as zeros, with no error.  The csum failure is logged in
dmesg because the device stats are updated (so the corruption is not
strictly silent), but the read succeeds.  A 4K read of the same sector
is not split, and correctly returns EIO.

The data in that sector is unrecoverable either way.  The change only
affects what userspace is told when it reads the data, i.e. EIO instead
of a read that returns junk while claiming success.

Reproducer: on a raid5 filesystem, corrupt one stripe row's parity,
leaving the data intact, then withhold one data member of that row and
mount degraded.  Reconstructing the missing column then yields wrong
bytes for that column.  Every other column still works.  Reads of 64K or
more over that range return zeros and succeed, while 4K reads return EIO.

Tested-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>

> The saved status has the first error of all the parts, or BLK_STS_OK if
> there was none, so always use it.
> 
> Fixes: d48e1dea3931 ("btrfs: fix error propagation of split bios")
> CC: stable@vger.kernel.org # 6.12+
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/bio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 2c6234f6182f..fc0b021cdefb 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -127,9 +127,12 @@ void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
>  		cmpxchg(&bbio->status, BLK_STS_OK, status);
>  
>  	if (atomic_dec_and_test(&bbio->pending_ios)) {
> -		/* Load split bio's error which might be set above. */
> -		if (status == BLK_STS_OK)
> -			bbio->bio.bi_status = READ_ONCE(bbio->status);
> +		/*
> +		 * Load the first error, if any. It may come from a split bio,
> +		 * including the one we were called for, in which case nothing
> +		 * stored it in the original bio yet.
> +		 */
> +		bbio->bio.bi_status = READ_ONCE(bbio->status);
>  
>  		if (bbio_has_ordered_extent(bbio)) {
>  			struct btrfs_ordered_extent *ordered = bbio->ordered;
> 
> -- 
> 2.55.0
> 
>