Re: [PATCH 1/2] btrfs: fix direct I/O read repair for split bios

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

On 2022/3/30 22:43, David Sterba wrote:
> On Thu, Mar 24, 2022 at 05:06:27PM +0100, Christoph Hellwig wrote:
>> When a bio is split in btrfs_submit_direct, dip->file_offset contains
>> the file offset for the first bio.  But this means the start value used
>> in btrfs_check_read_dio_bio is incorrect for subsequent bios.  Add
>> a file_offset field to struct btrfs_bio to pass along the correct offset.
>>
>> Given that check_data_csum only uses start of an error message this
>> means problems with this miscalculation will only show up when I/O
>> fails or checksums mismatch.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Qu, you've removed the same logic in f4f39fc5dc30 ("btrfs: remove
> btrfs_bio::logical member") where it was a different name for the same
> variable. What changed in the logic that we don't need to store it along
> the btrfs_bio and that btrfs_dio_private can't provide anymore?

All my fault, I didn't realize that in btrfs_submit_direct() what we
really do is splitting the iomap bio.

Thus we still need that @logical member as dip is only allocated for the
whole iomap bio, not for each split btrfs bio.

Thus we need the fixes: tag.

>
> I'm a bit worried about your changes that remove/rewrite code, silently
> introducing bugs so it has to be reinstated. We don't have enough
> review coverage and in the amount of patches you send I'm increasingly
> worried how many bugs I've inadvertently let in.

Normally it should be caught by test cases. But test case coverage is
not that better than our review coverage, especially for read repair, as
it's a btrfs specific feature, and almost impossible to do stress tests.

The good news is, for most of my subpage related rewrite, the existing
test cases are pretty good catching the bugs.


I don't really have better way other than adding regression tests cases
until we found some regression.

Thanks,
Qu