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

Qu Wenruo
2022-05-24T13:13:48Z

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.

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.