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

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

On 2022/3/31 06:24, Qu Wenruo wrote:
>
>
> 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.

While crafting the test case for this particular case, I found all the
existing tools, like dd (iflag=direct) or xfs_io (pwrite -d -i), are all
reading the content using 4K block, even block size is specified.

Thus unable to reproduce the bug (no split will happen).

Any good tool to cause a large direct IO read?

Thanks,
Qu

>
> Thanks,
> Qu