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

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

On 2022/3/31 07:14, Qu Wenruo wrote:
>
>
> 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?

OK, all these tools (including a new md5sum_direct I crafted) are
working as expected.

The always sectorsized read is from btrfs, not those tools.

In btrfs_dio_iomap_begin(), if the operation is read, we always limit
the size to sectorsize:

	if (!write)
		len = min_t(u64, len, fs_info->sectorsize);

This behavior saved my ass, as all our direct read will only cause one
single sector, thus it will never be split, thus read repair will never
need to both the dip->file_offset mismatch.

Furthermore, write path doesn't need to bother file offset in the split
bio anyway.

So no fixes: tag is needed, I am saved by a weird behavior there.

Thanks,
Qu



>
> Thanks,
> Qu
>
>>
>> Thanks,
>> Qu