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/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.

>
>> +	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