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

[Nikolay Borisov <nborisov@suse.com>]

On 24.05.22 г. 16:13 ч., Qu Wenruo wrote:
> 
> 
> On 2022/5/24 20:08, Christoph Hellwig wrote:
>> On Tue, May 24, 2022 at 04:21:38PM +0800, Qu Wenruo wrote:
>>>> The things like resetting initial_mirror, making the naming "initial"
>>>> meaningless.
>>>> And the reset on the length part is also very quirky.
>>>
>>> In fact, if you didn't do the initial_mirror and length change (which is
>>> a big disaster of readability, to change iterator in a loop, at least to
>>> me),
>>
>> So what is the big problem there?  Do I need more extensive documentation
>> or as there anything in this concept that is just too confusing.
> 
> Modifying the iterator inside a loop is the biggest problem to me.

What iterator gets modified? The code defines a local one, which gets 
initialized by copying the state from the bbio. Then the loop works as 
usual, that's not confusing and is in line with how the other bio vec 
iterator macros work.

> 
> Yes, extra comments can help, but that doesn't help the readability.
> 
> And that's why most of guys here prefer for () loop if we can.
> 
> Thanks,
> Qu
>>
>>> and rely on the VFS re-read behavior to fall back to sector by
>>> secot read, I would call it better readability...
>>
>> I don't think relying on undocumented VFS behavior is a good idea.  It
>> will also not work for direct I/O if any single direct I/O bio has
>> ever more than a single page, which is something btrfs badly needs
>> if it wants to get any kind of performance out of direct I/O.
>