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@xxxxxxxx>
Thanks,
Qu
