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

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Nikolay Borisov <nborisov@suse.com>, Christoph Hellwig <hch@lst.de>
Cc: Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 8/8] btrfs: use btrfs_bio_for_each_sector in btrfs_check_read_dio_bio
Date: Thu, 26 May 2022 06:46:44 +0800	[thread overview]
Message-ID: <e77c1b3d-558d-fe08-fc00-5aedff972ca0@gmx.com> (raw)
In-Reply-To: <bc9b6b97-ee10-e5eb-f8ab-b533399dc772@suse.com>

On 2022/5/25 23:12, Nikolay Borisov wrote:
>
>
> 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?

Mirror number.

Thanks,
Qu

> 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.
>>