Re: [PATCH RFC v2 05/12] btrfs: add a helper to queue a corrupted sector for read repair

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

On 2022/4/28 21:44, Christoph Hellwig wrote:
> On Thu, Apr 28, 2022 at 01:20:37PM +0800, Qu Wenruo wrote:
>> This function will get called very frequently, and I really want to avoid
>> doing such re-search every time from the beginning of the original bio.
>>
>> Maybe we can cache a bvec_iter and using the bi_size to check if the target
>> offset is still beyond us (then advance), or re-wind and re-search from the
>> beginning.
>>
>> I guess there is no existing helper to do the same work, right?
>
> It is basically impossible to review this because you just add a
> standalone static helper without the callers.  Please split the
> work into logical chunks that actually are reviewable.  Yes, that will
> be a pretty large patch, but that's still much better than having to
> jump around hal a dozen ones.
>
> No, there is no way to efficiently look up what bvec in a bio some
> offset falls into, because the bvecs are variable length.
>
> It seems like the caller (at least the one added a little later, not
> sure if there are more) is iterating over the bitmap and then calls
> this for every bit set.  So for that you're much better off making
> the __bio_for_each_segment the main loop, and then at the beginning of
> the loop checking the bitmap if we need to handle this sector.
>
>
>>> +	struct bio_list read_bios;
>
> I'd just calls this bios.  Obviously they are used for reading here.
>
> Also please be very careful about dead locks.  The mempool for the
> btrfs_bios is small right now, you need to size it up by the
> largerst number of bios that can be on this list.

In fact I have some version locally checking the return value from
btrfs_bio_alloc(), if we failed to alloc memory, then just mark the
btrfs_read_repair_ctrl::error bit, and mark all remaining bad sectors as
error, no more repairing.

As memory allocation failure is much more a problem than failed read repair.

Another consideration is, would it really dead lock?

We're only called for read path, not writeback path.
IIRC it's easier to hit dead lock at writeback path, as writeback can be
triggered by memory pressure.

But would the same problem happen just for read path?

Thanks,
Qu