Re: [PATCH 05/13] btrfs: add btrfs_read_repair_ctrl to record corrupted sectors

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

On 2022/5/4 22:05, Christoph Hellwig wrote:
> On Wed, May 04, 2022 at 09:12:50AM +0800, Qu Wenruo wrote:
>>> This is a really bad idea.  Now every read needs to pay the quite
>>> large price for corner case of a read repair.  As I mentioned last time
>>> I think a mempool with a few entries that any read repair can dip into
>>> is a much better choice here.
>>
>> The problem is, can mempool provide a variable length entry?
>
> It can't.  But you can allocate a few different bucket sizes as a
> starter or do a multi-level setup where you don't have a bitmap that
> operates on the entire bio.

That sounds way more complex than either the original method (just fail
to repair) or the current pre-allocation method.

We need to cover a pretty wide length variable, from the minimal 4K
(only need one bit), to much larger ones (observed bio over 16M, but
only for write).

It would make sense if we have a hard limit on how large a read bio can
be (I have only observed 4M as the biggest bio size for read, and if
limited to 4M, we only need 128bytes for the bitmap and can go mempool).


In fact, the original one (just error out if failed to allocate memory)
is way more robust than you think.

The point here is, if a large read bio failed, VFS will retry with much
smaller block size (normally just one page or one block), and if we even
failed to allocate memory for an u32, we're really screwed up.

Do we really need to go down the rabbit hole of using mempool for
variable length situation? Especially we're not that eager to ensure the
memory allocation.

THanks,
Qu