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

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

On 2022/5/24 21: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.
>
> 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.

Just allow me to do another attempt on this.

This time, still bitmap based, but no pre/runtime allocation.
Just use the on-stack structure to contain a u32 bitmap.


And we no longer use the bitmap for the whole bio, but only for the
first corrupted sector, to the next good sector.

If the bitmap is not large enough to contain the next sector, we just
finish the current batch.

Now we should have all the best things, batched submission, no memory
allocation, no modification on the iterator inside a loop.

Although the cost is that, we are still doing the bitmap way, and we
have more call sites of btrfs_read_repair_finish().

Thanks,
Qu
>
> 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.