Re: [PATCH 07/17] btrfs: allow btrfs_submit_bio to split bios

Qu Wenruo
2022-09-12T22:24:16Z

On 2022/9/12 21:55, Christoph Hellwig wrote:
> On Mon, Sep 12, 2022 at 08:20:37AM +0800, Qu Wenruo wrote:
>> Sorry for the late reply, but I still have a question related the
>> chained bio way.
>>
>> Since we go the chained method, it means, if we hit an error for the
>> splitted bio, the whole bio will be marked error.
>
> The only chained bios in the sense of using bio chaining are the
> writes to the multiple legs of mirrored volumes.
>
>> Especially for read bios, that can be a problem (currently only for
>> RAID10 though), which can affect the read repair behavior.
>>
>> E.g. we have a 4-disks RAID10 looks like this:
>>
>> Disk 1 (unreliable): Mirror 1 of logical range [X, X + 64K)
>> Disk 2 (reliable): Mirror 2 of logical range [X, X + 64K)
>> Disk 3 (reliable): Mirror 1 of logical range [X + 64K, X + 128K)
>> Disk 4 (unreliable): Mirror 2 of logical range [X + 64K, X + 128K)
>>
>> And we submit a read for range [X, X + 128K)
>>
>> The first 64K will use mirror 1, thus reading from Disk 1.
>> The second 64K will also use mirror 1, thus read from Disk 2.
>>
>> But the first 64K read failed due to whatever reason, thus we mark the
>> whole range error, and needs to go repair code.
>
> With the code posted in this series that is not what happens. Instead
> the checksum validation and then repair happen when the read from
> mirror 1 / disk 1 completes, but before the results are propagated
> up. That was the prime reason why I had to move the repair code
> below btrfs_submit_bio (that it happend to removed code and consolidate
> the exact behavior is a nice side-effect).
>
>> Does the read-repair code now has something to compensate the chained
>> behavior?
>
> It doesn't compensate it, but it is invoked at a low enough level so
> that this problem does not happen.

You're completely right, it's the 4th patch putting the verification
code into the endio function, thus the verification is still done
per-splitted-bio.

I really should review the whole series in one go...

Then it looks pretty good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu