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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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