Re: [PATCH 8/8] btrfs: use btrfs_bio_for_each_sector in btrfs_check_read_dio_bio

[Christoph Hellwig <hch@lst.de>]

On Mon, May 23, 2022 at 08:07:01AM +0800, Qu Wenruo wrote:
>
> I checked the code, but still find the code in patch "btrfs: add new
> read repair infrastructure" not that instinctive.
>
> - Why we bother different repair methods in btrfs_repair_one_mirror()?
>   In fact btrfs_repair_io_failure() can handle all profiles.

Becasue btrfs_repair_io_failure can't handle multiple-page I/O.  It
is also is rather cumbersome because it bypassed the normal bio
mapping.  As a follow on I'd rather move it over to btrfs_map_bio
with a special flag for the single mirror parity write rather than that
hack.

>   Then why we go back to write the whole bio?

Because the whole bio at this point is all the bad sectors.  There
is no point in writing only parts of the bio because that would leave
corruption on disk.

>   The only reason I can think of is, we're still trying to do some
>   "optimization".
>
>   But all our bio submission is already synchronous, I doubt such
>   "optimization" would make much difference.

Can you explain what you mean here?

>
> - The bio truncation
>   This really looks like a bandage to address the checker pattern
>   corruption.
>   I doubt why not just do per-sector read/write like:

Because now you wait for each I/O.

> To me, the "optimization" of batched read/write is only relevant if we
> have tons of corrupted sectors in a read bio, which I don't believe is a
> hot path in real world anyway.

It is is very easy low hanging fruit, and actually pretty common for
actual failures of components.  In other words:  single sector failures
happen some times, usually due to corruption on the transfer wire.  If
actual corruption happens on the driver it will usually fail quite
bit areas.  These are rather rare these days as a lot of device shut
down before returning bad data, but if it happens you'll rarely see
just a single sector.