Re: [PATCH 2/3] btrfs: merge end_write_bio and flush_write_bio

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

On 2022/6/3 15:11, Christoph Hellwig wrote:
> Merge end_write_bio and flush_write_bio into a single submit_write_bio
> helper, that either submits the bio or ends it if a negative errno was
> passed in.  This consolidates a lot of duplicated checks in the callers.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 94 ++++++++++++++------------------------------
>   1 file changed, 30 insertions(+), 64 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 025349aeec31f..72a258fa27947 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -201,39 +201,26 @@ static void submit_one_bio(struct bio *bio, int mirror_num,
>   	 */
>   }
>
> -/* Cleanup unsubmitted bios */
> -static void end_write_bio(struct extent_page_data *epd, int ret)
> -{
> -	struct bio *bio = epd->bio_ctrl.bio;
> -
> -	if (bio) {
> -		bio->bi_status = errno_to_blk_status(ret);
> -		bio_endio(bio);
> -		epd->bio_ctrl.bio = NULL;
> -	}
> -}
> -
>   /*
> - * Submit bio from extent page data via submit_one_bio
> - *
> - * Return 0 if everything is OK.
> - * Return <0 for error.
> + * Submit or fail the current bio in an extent_page_data structure.
>    */
> -static void flush_write_bio(struct extent_page_data *epd)
> +static void submit_write_bio(struct extent_page_data *epd, int ret)
>   {
>   	struct bio *bio = epd->bio_ctrl.bio;
>
> -	if (bio) {
> +	if (!bio)
> +		return;
> +
> +	if (ret) {
> +		ASSERT(ret < 0);
> +		bio->bi_status = errno_to_blk_status(ret);
> +		bio_endio(bio);
> +	} else {
>   		submit_one_bio(bio, 0, 0);
> -		/*
> -		 * Clean up of epd->bio is handled by its endio function.
> -		 * And endio is either triggered by successful bio execution
> -		 * or the error handler of submit bio hook.
> -		 * So at this point, no matter what happened, we don't need
> -		 * to clean up epd->bio.
> -		 */
> -		epd->bio_ctrl.bio = NULL;
>   	}
> +
> +	/* The bio is owned by the bi_end_io handler now */
> +	epd->bio_ctrl.bio = NULL;
>   }
>
>   int __init extent_state_cache_init(void)
> @@ -4248,7 +4235,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>   	int ret = 0;
>
>   	if (!btrfs_try_tree_write_lock(eb)) {
> -		flush_write_bio(epd);
> +		submit_write_bio(epd, 0);
>   		flush = 1;
>   		btrfs_tree_lock(eb);
>   	}
> @@ -4258,7 +4245,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>   		if (!epd->sync_io)
>   			return 0;
>   		if (!flush) {
> -			flush_write_bio(epd);
> +			submit_write_bio(epd, 0);
>   			flush = 1;
>   		}
>   		while (1) {
> @@ -4305,7 +4292,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>
>   		if (!trylock_page(p)) {
>   			if (!flush) {
> -				flush_write_bio(epd);
> +				submit_write_bio(epd, 0);
>   				flush = 1;
>   			}
>   			lock_page(p);
> @@ -4721,7 +4708,7 @@ static int submit_eb_subpage(struct page *page,
>
>   cleanup:
>   	/* We hit error, end bio for the submitted extent buffers */
> -	end_write_bio(epd, ret);
> +	submit_write_bio(epd, ret);
>   	return ret;
>   }
>
> @@ -4900,10 +4887,6 @@ int btree_write_cache_pages(struct address_space *mapping,
>   		index = 0;
>   		goto retry;
>   	}
> -	if (ret < 0) {
> -		end_write_bio(&epd, ret);
> -		goto out;
> -	}
>   	/*
>   	 * If something went wrong, don't allow any metadata write bio to be
>   	 * submitted.
> @@ -4930,21 +4913,17 @@ int btree_write_cache_pages(struct address_space *mapping,
>   	 *   Now such dirty tree block will not be cleaned by any dirty
>   	 *   extent io tree. Thus we don't want to submit such wild eb
>   	 *   if the fs already has error.
> -	 */
> -	if (!BTRFS_FS_ERROR(fs_info)) {
> -		flush_write_bio(&epd);
> -	} else {
> -		ret = -EROFS;
> -		end_write_bio(&epd, ret);
> -	}
> -out:
> -	btrfs_zoned_meta_io_unlock(fs_info);
> -	/*
> +	 *
>   	 * We can get ret > 0 from submit_extent_page() indicating how many ebs
>   	 * were submitted. Reset it to 0 to avoid false alerts for the caller.
>   	 */
>   	if (ret > 0)
>   		ret = 0;
> +	if (!ret && BTRFS_FS_ERROR(fs_info))
> +		ret = -EROFS;
> +	submit_write_bio(&epd, ret);
> +
> +	btrfs_zoned_meta_io_unlock(fs_info);
>   	return ret;
>   }
>
> @@ -5046,7 +5025,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   			 * tmpfs file mapping
>   			 */
>   			if (!trylock_page(page)) {
> -				flush_write_bio(epd);
> +				submit_write_bio(epd, 0);
>   				lock_page(page);
>   			}
>
> @@ -5057,7 +5036,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>
>   			if (wbc->sync_mode != WB_SYNC_NONE) {
>   				if (PageWriteback(page))
> -					flush_write_bio(epd);
> +					submit_write_bio(epd, 0);
>   				wait_on_page_writeback(page);
>   			}
>
> @@ -5097,7 +5076,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   		 * page in our current bio, and thus deadlock, so flush the
>   		 * write bio here.
>   		 */
> -		flush_write_bio(epd);
> +		submit_write_bio(epd, 0);
>   		goto retry;
>   	}
>
> @@ -5118,13 +5097,7 @@ int extent_write_full_page(struct page *page, struct writeback_control *wbc)
>   	};
>
>   	ret = __extent_writepage(page, wbc, &epd);
> -	ASSERT(ret <= 0);
> -	if (ret < 0) {
> -		end_write_bio(&epd, ret);
> -		return ret;
> -	}
> -
> -	flush_write_bio(&epd);
> +	submit_write_bio(&epd, ret);
>   	return ret;
>   }
>
> @@ -5185,10 +5158,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end)
>   		cur = cur_end + 1;
>   	}
>
> -	if (!found_error)
> -		flush_write_bio(&epd);
> -	else
> -		end_write_bio(&epd, ret);
> +	submit_write_bio(&epd, found_error ? ret : 0);
>
>   	wbc_detach_inode(&wbc_writepages);
>   	if (found_error)
> @@ -5214,12 +5184,8 @@ int extent_writepages(struct address_space *mapping,
>   	btrfs_zoned_data_reloc_lock(BTRFS_I(inode));
>   	ret = extent_write_cache_pages(mapping, wbc, &epd);
>   	btrfs_zoned_data_reloc_unlock(BTRFS_I(inode));
> -	ASSERT(ret <= 0);
> -	if (ret < 0) {
> -		end_write_bio(&epd, ret);
> -		return ret;
> -	}
> -	flush_write_bio(&epd);
> +
> +	submit_write_bio(&epd, ret);
>   	return ret;
>   }
>