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

[Christoph Hellwig <hch@infradead.org>]

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.

> +	if (!ctrl->initialized) {

I don't think you need the initialize field.  Just check for
failed_bio being non-NULL to simplify this.

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

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