Re: [PATCH v2 4/7] btrfs: introduce new read-repair infrastructure

Qu Wenruo
2022-05-26T09:13:44Z

On 2022/5/26 16:54, Christoph Hellwig wrote:
> On Thu, May 26, 2022 at 04:49:15PM +0800, Qu Wenruo wrote:
>>>>> Because that really sucks for the case where the whole I/O fails.
>>>>> Which is the common failure scenario.
>>>>
>>>> But it's just a performance problem, which is not that critical.
>>>
>>> I'm officially lost now.
>>
>> Why? If you care so much about the code simplicity, sector-by-sector is
>> the best.
>> If you care so much about the performance, the latest bitmap is the
>> best, no matter if it's the worst checker patter or not.
>
> Because you tell me that handling the most common and important case
> in read repair is just a performance issue, which you keep arguing for
> micro-optimizing a corner case.

Because I'm fine either way, but not fine with the middle ground.

I purposed both versions, to fulfill the different requirements.
The tradeoff is not avoidable, we have to choose the poison.

If we want to go code simplicity, then my argument to support
sector-by-sector is, the repair is already a cold path (the same
argument you go on the checker pattern), thus overall the performance
drop is not that critical.

If we want to go the best perf (even corruption is already a corner
case), then there is the bitmap version, handling all cases, at the cost
of complex code.

If you can find a simpler version, and still handle the checker pattern
sanely (aka, without reading the same bad mirror again and again), sure,
I'm always fine to go that version.

Otherwise sector-by-sector or bitmap seems a more sane choice to do between.

Thanks,
Qu

> And not, for the case of failing a
> large bio (which arguably can only happen for buffered I/O at the
> moment, but that is another thing to look into) the bitmaps will only
> help you for up to 64 sectors. Way better than just doing a single
> sector synchronous I/O but not exactly nice while still being a fair
> amount of code compared to just doing variable sized synchronous
> I/O.