Re: [PATCH RFC v2 04/12] btrfs: add btrfs_read_repair_ctrl to record corrupted sectors

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

On 2022/4/29 06:51, Qu Wenruo wrote:
>
>
> On 2022/4/28 21:37, Christoph Hellwig wrote:
>> On Wed, Apr 27, 2022 at 03:18:50PM +0800, Qu Wenruo wrote:
>>> Currently we only allocate two bitmaps and initialize various members,
>>> no real work done yet.
>>
>> I'm rather worried about these allocations.  These are called from
>> the I/O completion work queues, which can be rather deadlock heavy.
>> Never mind that just failing an I/O repair/recovery when we are out
>> of memory seems like a rather bad idea.
>
> That's why there is a btrfs_read_repair_ctrl::error member.
>
> If we failed the memory allocation, then we will not do any repair.
>
> To me, memory allocation is a much bigger problem.
>
>
> Although we can put the bitmap into btrfs_bio structure, and
> pre-allocate it for every bio we're going to submit.
>
> But I'm not sure if the pre-allocation way is a good idea, considering
> read-repair should be a relatively code path.

s/code/cold/

But at least for the bio submission path, we have way less strict
context, thus it's indeed a better call sites.

On the other hand, we're already allocating memory inside endio
function, for a long long time.

Check endio_readpage_release_extent(), they all need to pre-allocate
memory for extent tree update (although using mostly ATOMIC gfp flags).

I guess it's an abuse and we would like to remove it in the long run?

Thanks,
Qu

>
>>
>>> +    if (!ctrl->initialized) {
>>
>> I don't think you need the initialize field.  Just check for
>> failed_bio being non-NULL to simplify this.
>
> That's indeed simpler.
>
>>
>>> +        const u32 sectorsize = fs_info->sectorsize;
>>> +
>>> +        ASSERT(ctrl->cur_bad_bitmap == NULL &&
>>> +               ctrl->prev_bad_bitmap == NULL);
>>> +        /*
>>> +         * failed_bio->bi_iter is not reliable at endio time, thus we
>>> +         * must rely on btrfs_bio::iter to grab the original logical
>>> +         * bytenr.
>>> +         */
>>> +        ASSERT(btrfs_bio(failed_bio)->iter.bi_size);
>>
>> Also things would be lot more readable if the code inside this branch
>> just moved into a helper that you call if ->failed_bio is not set.
>
> Indeed.
>
>>
>>> +        ctrl->cur_bad_bitmap = bitmap_alloc(ctrl->bio_size >>
>>> +                    fs_info->sectorsize_bits, GFP_NOFS);
>>> +        ctrl->prev_bad_bitmap = bitmap_alloc(ctrl->bio_size >>
>>> +                    fs_info->sectorsize_bits, GFP_NOFS);
>>> +        /* Just set the error bit, so we will never try repair */
>>> +        if (!ctrl->cur_bad_bitmap || !ctrl->prev_bad_bitmap) {
>>> +            kfree(ctrl->cur_bad_bitmap);
>>> +            kfree(ctrl->prev_bad_bitmap);
>>> +            ctrl->cur_bad_bitmap = NULL;
>>> +            ctrl->prev_bad_bitmap = NULL;
>>> +            ctrl->error = true;
>>> +        }
>>
>> I don't think we need the extra error value either, you can just check
>> one of the bitmap pointers for NULL.  That being said, as mentioned
>> above I'm really worried about these huge allocations that can fail.
>> I think we need a mempool of some fixed size here and use that, and just
>> change the algorithm to work in chunks based on this upper bound.
>>
>>> +/* Strucutre for data read time repair. */
>>> +struct btrfs_read_repair_ctrl {
>>
>> Can we keep that structure private?  Based on the rest of the series
>> there actually is a fair amount of code using it, what about isolating
>> it in a new read_repair.c instead of in the giant extent_io.c and
>> inode.c files?
>
> I was considering putting it into read_repair.c, and since you're also
> mentioning that, I guess it's a good idea now.
>
> And if we're going to make that structure private, I guess we have to
> pre-allocate it in btrfs_bio as a pointer then.
>
> Thanks,
> Qu
>