Re: [PATCH 00/13] btrfs: make read repair work in synchronous mode

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

On 2022/5/13 01:08, David Sterba wrote:
> On Tue, May 03, 2022 at 02:49:44PM +0800, Qu Wenruo wrote:
>
>> By this we can:
>> - Remove the re-entry behavior of endio function
>>    Now everything is handled inside end_bio_extent_readpage().
>>
>> - Remove the io_failure_tree completely
>>    As we don't need to record which mirror has failed.
>>
>> - Slightly reduced overhead on read repair
>>    Now we won't call btrfs_map_bio() for each corrupted sector, as we can
>>    merge the sectors into a much larger bio.
>
> I thake this as the summary points for the whole patchset and frankly I
> don't see it justified given all the new problems with the preallocation
> and shuffling with the structures. What we have now is not perfect and I
> would like to get rid of the io_failure_tree too, yet it works.

Indeed I have no better solution for the bitmap.

I have one very limited optimization for it, e.g. if we hit profiles
without any extra mirrors (like RAID0/SINGLE), we skip the preallocation
and just error out.

But that's all, we have to allocate the memory, and since I'm neither a
fan of mempool (nor a good fit for variable length bitmap), we really
only have two solutions:

- Allocate bitmap when we hit the first error
   The way I did in previous RFC version.
   But it also means, if the memory allocation failed, we will error out
   without reading out the correct mirror. (Christoph is strongly against
   the error path)

- Preallocate bitmap before we submit the bio
   The way I did in this version.

Now I think the first solution would be more sane, no change in
btrfs_bio (which is already big).
And the memory allocation failure will not really cause any bit damage,
VFS will re-try a read with much smaller block size.

>
> The main point is probably to stop reentering the functions, though the
> idea of having a single tree that tracks the repair state and there are
> callbacks dealing with that is not IMHO bad design. The alternative is
> to complicate data structures and endio handler. I'm not sure if it's
> worth the risk.

The point of the repair work is a first step towards less members in
btrfs_bio.

In fact, all logical read/write should not really care about read
repair/csum/mirror num etc.

Thus it can be hidden into lower layer, thus my ultimate goal is to make
logical layer to only use plain bio completely.

But the existing read repair is a blockage, thus why I'm purposing this
patchset.

Thanks,
Qu