Re: [PATCH 21/40] btrfs: cleanup btrfs_submit_data_bio

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

On 2022/3/22 23:55, Christoph Hellwig wrote:
> Clean up the code flow to be straight forward.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/inode.c | 85 +++++++++++++++++++++---------------------------
>   1 file changed, 37 insertions(+), 48 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 325e773c6e880..a54b7fd4658d0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2511,67 +2511,56 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>
>   {
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	enum btrfs_wq_endio_type metadata = BTRFS_WQ_ENDIO_DATA;
> -	blk_status_t ret = 0;
> -	int skip_sum;
> -	int async = !atomic_read(&BTRFS_I(inode)->sync_writers);
> -
> -	skip_sum = (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM) ||
> -		test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state);
> -
> -	if (btrfs_is_free_space_inode(BTRFS_I(inode)))
> -		metadata = BTRFS_WQ_ENDIO_FREE_SPACE;
> +	struct btrfs_inode *bi = BTRFS_I(inode);
> +	blk_status_t ret;
>
>   	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
> -		struct page *page = bio_first_bvec_all(bio)->bv_page;
> -		loff_t file_offset = page_offset(page);
> -
> -		ret = extract_ordered_extent(BTRFS_I(inode), bio, file_offset);
> +		ret = extract_ordered_extent(bi, bio,
> +				page_offset(bio_first_bvec_all(bio)->bv_page));
>   		if (ret)
> -			goto out;
> +			return ret;
>   	}
>
> -	if (btrfs_op(bio) != BTRFS_MAP_WRITE) {
> +	if (btrfs_op(bio) == BTRFS_MAP_WRITE) {
> +		if ((bi->flags & BTRFS_INODE_NODATASUM) ||
> +		    test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state))
> +			goto mapit;
> +
> +		if (!atomic_read(&bi->sync_writers)) {
> +			/* csum items have already been cloned */
> +			if (btrfs_is_data_reloc_root(bi->root))
> +				goto mapit;
> +			return btrfs_wq_submit_bio(inode, bio, mirror_num, bio_flags,
> +						  0, btrfs_submit_bio_start);
> +		}
> +		ret = btrfs_csum_one_bio(bi, bio, 0, 0);
> +		if (ret)
> +			return ret;

Previously we would also call bio_endio() on the bio, do we miss the
endio call on it?


> +	} else {
> +		enum btrfs_wq_endio_type metadata = BTRFS_WQ_ENDIO_DATA;
> +
> +		if (btrfs_is_free_space_inode(bi))
> +			metadata = BTRFS_WQ_ENDIO_FREE_SPACE;
> +
>   		ret = btrfs_bio_wq_end_io(fs_info, bio, metadata);
>   		if (ret)
> -			goto out;
> +			return ret;
>
> -		if (bio_flags & EXTENT_BIO_COMPRESSED) {
> -			ret = btrfs_submit_compressed_read(inode, bio,
> +		if (bio_flags & EXTENT_BIO_COMPRESSED)
> +			return btrfs_submit_compressed_read(inode, bio,
>   							   mirror_num,
>   							   bio_flags);
> -			goto out;

Previously the out tag also ends the io for the bio.
Now we don't.

> -		} else {
> -			/*
> -			 * Lookup bio sums does extra checks around whether we
> -			 * need to csum or not, which is why we ignore skip_sum
> -			 * here.
> -			 */
> -			ret = btrfs_lookup_bio_sums(inode, bio, NULL);
> -			if (ret)
> -				goto out;
> -		}
> -		goto mapit;
> -	} else if (async && !skip_sum) {
> -		/* csum items have already been cloned */
> -		if (btrfs_is_data_reloc_root(root))
> -			goto mapit;
> -		/* we're doing a write, do the async checksumming */
> -		ret = btrfs_wq_submit_bio(inode, bio, mirror_num, bio_flags,
> -					  0, btrfs_submit_bio_start);
> -		goto out;
> -	} else if (!skip_sum) {
> -		ret = btrfs_csum_one_bio(BTRFS_I(inode), bio, 0, 0);
> +
> +		/*
> +		 * Lookup bio sums does extra checks around whether we need to
> +		 * csum or not, which is why we ignore skip_sum here.
> +		 */
> +		ret = btrfs_lookup_bio_sums(inode, bio, NULL);
>   		if (ret)
> -			goto out;
> +			return ret;

The same missing endio call for error path.

Thanks,
Qu
>   	}
> -
>   mapit:
> -	ret = btrfs_map_bio(fs_info, bio, mirror_num);
> -
> -out:
> -	return ret;
> +	return btrfs_map_bio(fs_info, bio, mirror_num);
>   }
>
>   /*