Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio

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

On 2022/6/3 15:11, Christoph Hellwig wrote:
> submit_one_bio always works on the bio and compression flags from a
> btrfs_bio_ctrl structure.  Pass the explicitly and clean up the the
> calling conventions by handling a NULL bio in submit_one_bio, and
> using the btrfs_bio_ctrl to pass the mirror number as well.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

In fact the only call sites really caring num_mirror is the metadata
read path (as it doesn't rely on the read-repair code, since metadata
has inline csum and it has a different validation condition).

It may be a good idea to make a union for btrfs_bio, to contain all the
needed info for metadata verification (btrfs_key, transid, level), so
that we can get rid of the num_mirror parameter for submit_extent_page()
completely.

Thanks,
Qu
> ---
>   fs/btrfs/extent_io.c | 82 ++++++++++++++++++++------------------------
>   1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 72a258fa27947..facca7feb9a22 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -144,6 +144,7 @@ struct tree_entry {
>    */
>   struct btrfs_bio_ctrl {
>   	struct bio *bio;
> +	int mirror_num;
>   	enum btrfs_compression_type compress_type;
>   	u32 len_to_stripe_boundary;
>   	u32 len_to_oe_boundary;
> @@ -178,10 +179,11 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
>   	return ret;
>   }
>
> -static void submit_one_bio(struct bio *bio, int mirror_num,
> -			   enum btrfs_compression_type compress_type)
> +static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
>   {
> +	struct bio *bio = bio_ctrl->bio;
>   	struct inode *inode = bio_first_page_all(bio)->mapping->host;
> +	int mirror_num = bio_ctrl->mirror_num;
>
>   	/* Caller should ensure the bio has at least some range added */
>   	ASSERT(bio->bi_iter.bi_size);
> @@ -191,14 +193,17 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
>   	else if (btrfs_op(bio) == BTRFS_MAP_WRITE)
>   		btrfs_submit_data_write_bio(inode, bio, mirror_num);
>   	else
> -		btrfs_submit_data_read_bio(inode, bio, mirror_num, compress_type);
> +		btrfs_submit_data_read_bio(inode, bio, mirror_num,
> +					   bio_ctrl->compress_type);
>
> -	/*
> -	 * Above submission hooks will handle the error by ending the bio,
> -	 * which will do the cleanup properly.  So here we should not return
> -	 * any error, or the caller of submit_extent_page() will do cleanup
> -	 * again, causing problems.
> -	 */
> +	/* The bio is owned by the bi_end_io handler now */
> +	bio_ctrl->bio = NULL;
> +}
> +
> +static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
> +{
> +	if (bio_ctrl->bio)
> +		__submit_one_bio(bio_ctrl);
>   }
>
>   /*
> @@ -215,12 +220,11 @@ static void submit_write_bio(struct extent_page_data *epd, int ret)
>   		ASSERT(ret < 0);
>   		bio->bi_status = errno_to_blk_status(ret);
>   		bio_endio(bio);
> +		/* The bio is owned by the bi_end_io handler now */
> +		epd->bio_ctrl.bio = NULL;
>   	} else {
> -		submit_one_bio(bio, 0, 0);
> +		__submit_one_bio(&epd->bio_ctrl);
>   	}
> -
> -	/* The bio is owned by the bi_end_io handler now */
> -	epd->bio_ctrl.bio = NULL;
>   }
>
>   int __init extent_state_cache_init(void)
> @@ -3408,7 +3412,6 @@ static int submit_extent_page(unsigned int opf,
>   			      struct page *page, u64 disk_bytenr,
>   			      size_t size, unsigned long pg_offset,
>   			      bio_end_io_t end_io_func,
> -			      int mirror_num,
>   			      enum btrfs_compression_type compress_type,
>   			      bool force_bio_submit)
>   {
> @@ -3420,10 +3423,8 @@ static int submit_extent_page(unsigned int opf,
>
>   	ASSERT(pg_offset < PAGE_SIZE && size <= PAGE_SIZE &&
>   	       pg_offset + size <= PAGE_SIZE);
> -	if (force_bio_submit && bio_ctrl->bio) {
> -		submit_one_bio(bio_ctrl->bio, mirror_num, bio_ctrl->compress_type);
> -		bio_ctrl->bio = NULL;
> -	}
> +	if (force_bio_submit)
> +		submit_one_bio(bio_ctrl);
>
>   	while (cur < pg_offset + size) {
>   		u32 offset = cur - pg_offset;
> @@ -3463,8 +3464,7 @@ static int submit_extent_page(unsigned int opf,
>   		if (added < size - offset) {
>   			/* The bio should contain some page(s) */
>   			ASSERT(bio_ctrl->bio->bi_iter.bi_size);
> -			submit_one_bio(bio_ctrl->bio, mirror_num, bio_ctrl->compress_type);
> -			bio_ctrl->bio = NULL;
> +			submit_one_bio(bio_ctrl);
>   		}
>   		cur += added;
>   	}
> @@ -3741,10 +3741,8 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
>
>   		ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
>   					 bio_ctrl, page, disk_bytenr, iosize,
> -					 pg_offset,
> -					 end_bio_extent_readpage, 0,
> -					 this_bio_flag,
> -					 force_bio_submit);
> +					 pg_offset, end_bio_extent_readpage,
> +					 this_bio_flag, force_bio_submit);
>   		if (ret) {
>   			/*
>   			 * We have to unlock the remaining range, or the page
> @@ -3776,8 +3774,7 @@ int btrfs_readpage(struct file *file, struct page *page)
>   	 * If btrfs_do_readpage() failed we will want to submit the assembled
>   	 * bio to do the cleanup.
>   	 */
> -	if (bio_ctrl.bio)
> -		submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.compress_type);
> +	submit_one_bio(&bio_ctrl);
>   	return ret;
>   }
>
> @@ -4060,7 +4057,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>   					 disk_bytenr, iosize,
>   					 cur - page_offset(page),
>   					 end_bio_extent_writepage,
> -					 0, 0, false);
> +					 0, false);
>   		if (ret) {
>   			has_error = true;
>   			if (!saved_ret)
> @@ -4553,7 +4550,7 @@ static int write_one_subpage_eb(struct extent_buffer *eb,
>   	ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc,
>   			&epd->bio_ctrl, page, eb->start, eb->len,
>   			eb->start - page_offset(page),
> -			end_bio_subpage_eb_writepage, 0, 0, false);
> +			end_bio_subpage_eb_writepage, 0, false);
>   	if (ret) {
>   		btrfs_subpage_clear_writeback(fs_info, page, eb->start, eb->len);
>   		set_btree_ioerr(page, eb);
> @@ -4594,7 +4591,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
>   					 &epd->bio_ctrl, p, disk_bytenr,
>   					 PAGE_SIZE, 0,
>   					 end_bio_extent_buffer_writepage,
> -					 0, 0, false);
> +					 0, false);
>   		if (ret) {
>   			set_btree_ioerr(p, eb);
>   			if (PageWriteback(p))
> @@ -5207,9 +5204,7 @@ void extent_readahead(struct readahead_control *rac)
>
>   	if (em_cached)
>   		free_extent_map(em_cached);
> -
> -	if (bio_ctrl.bio)
> -		submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.compress_type);
> +	submit_one_bio(&bio_ctrl);
>   }
>
>   /*
> @@ -6536,7 +6531,9 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>   	struct btrfs_fs_info *fs_info = eb->fs_info;
>   	struct extent_io_tree *io_tree;
>   	struct page *page = eb->pages[0];
> -	struct btrfs_bio_ctrl bio_ctrl = { 0 };
> +	struct btrfs_bio_ctrl bio_ctrl = {
> +		.mirror_num = mirror_num,
> +	};
>   	int ret = 0;
>
>   	ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
> @@ -6571,8 +6568,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>   	ret = submit_extent_page(REQ_OP_READ, NULL, &bio_ctrl,
>   				 page, eb->start, eb->len,
>   				 eb->start - page_offset(page),
> -				 end_bio_extent_readpage, mirror_num, 0,
> -				 true);
> +				 end_bio_extent_readpage, 0, true);
>   	if (ret) {
>   		/*
>   		 * In the endio function, if we hit something wrong we will
> @@ -6581,10 +6577,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>   		 */
>   		atomic_dec(&eb->io_pages);
>   	}
> -	if (bio_ctrl.bio) {
> -		submit_one_bio(bio_ctrl.bio, mirror_num, 0);
> -		bio_ctrl.bio = NULL;
> -	}
> +	submit_one_bio(&bio_ctrl);
>   	if (ret || wait != WAIT_COMPLETE)
>   		return ret;
>
> @@ -6604,7 +6597,9 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>   	int all_uptodate = 1;
>   	int num_pages;
>   	unsigned long num_reads = 0;
> -	struct btrfs_bio_ctrl bio_ctrl = { 0 };
> +	struct btrfs_bio_ctrl bio_ctrl = {
> +		.mirror_num = mirror_num,
> +	};
>
>   	if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
>   		return 0;
> @@ -6678,7 +6673,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>   			err = submit_extent_page(REQ_OP_READ, NULL,
>   					 &bio_ctrl, page, page_offset(page),
>   					 PAGE_SIZE, 0, end_bio_extent_readpage,
> -					 mirror_num, 0, false);
> +					 0, false);
>   			if (err) {
>   				/*
>   				 * We failed to submit the bio so it's the
> @@ -6695,10 +6690,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>   		}
>   	}
>
> -	if (bio_ctrl.bio) {
> -		submit_one_bio(bio_ctrl.bio, mirror_num, bio_ctrl.compress_type);
> -		bio_ctrl.bio = NULL;
> -	}
> +	submit_one_bio(&bio_ctrl);
>
>   	if (ret || wait != WAIT_COMPLETE)
>   		return ret;