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

Qu Wenruo
2022-05-23T00:07:18Z

On 2022/5/22 20:53, Christoph Hellwig wrote:
> On Sun, May 22, 2022 at 08:38:50PM +0800, Qu Wenruo wrote:
>>> 'cause now a lot of the bio works will depend on the read repair, and
>>> I don't want to block it on yet another series..
>>
>> Although I believe we will have to take more time on the read repair
>> code/functionality.
>>
>> Especially all our submitted version have their own problems.
>>
>> From the basic handling of checker pattern corruption, to the trade-off
>> between memory allocation and performance for read on corrupted data.
>
> I've already pushed out a new version here:
>
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-read_repair
>
> so feel free to take a look. I just don't want to spam the list with it
> quite yet with this series outstanding.

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.

Then why we go back to write the whole bio?
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.

- 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:

+ /* Init read bio to contain that corrupted sector only */
+ for (i = get_next_mirror(init_mirror); i != init_mirror; i++) {
+ int ret;
+
+ ret = btrfs_map_bio_wait(inode, read_bio, i);
+ /* Failed to submit, try next mirror */
+ if (ret < 0)
+ continue;
+
+ /* Verify the checksum */
+ if (failed_bbio->csum)
+ ret = btrfs_check_data_sector(fs_info, page,
+ pgoff, repair_bbio->csum);
+ if (ret == 0) {
+ found_good = true;
+ btrfs_repair_io_failure();
+ break;
+ }
+ }
+ if (!found_good)
+ return -EIO;

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.

Thanks,
Qu