Re: [PATCH v3 6/9] btrfs: introduce alloc_submit_compressed_bio() for compression

[David Sterba <dsterba@suse.cz>]

On Tue, Jun 15, 2021 at 08:18:33PM +0800, Qu Wenruo wrote:
> Just aggregate the bio allocation code into one helper, so that we can
> replace 4 call sites.
> 
> There is one special note for zoned write.
> 
> Currently btrfs_submit_compressed_write() will only allocate the first
> bio using ZONE_APPEND.
> If we have to submit current bio due to stripe boundary, the new bio
> allocated will not use ZONE_APPEND.
> 
> In theory this should be a bug, but considering zoned mode currently
> only support SINGLE profile, which doesn't have any stripe boundary
> limit, it should never be a problem.
> 
> This function will provide a good entrance for any work which needs to be
> done at bio allocation time. Like determining the stripe boundary.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/compression.c | 91 +++++++++++++++++++++++++++++-------------
>  1 file changed, 63 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 608e9325b7c8..a00f8f8d7512 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -440,6 +440,41 @@ static blk_status_t submit_compressed_bio(struct btrfs_fs_info *fs_info,
>  	return ret;
>  }
>  
> +/*
> + * To allocate a compressed_bio, which will be used to read/write on-disk data.
> + *
> + * @disk_bytenr:	Disk bytenr of the IO
> + * @opf:		Bio opf
> + * @endio_func:		Endio function

Variable comments that basically repeat the name are useless. Followup
patches add some other parameters that are not that trivial but still.

> + */
> +static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_bytenr,
> +					unsigned int opf, bio_end_io_t endio_func)
> +{
> +	struct bio *bio;
> +
> +	bio = btrfs_bio_alloc(disk_bytenr);
> +	/* bioset allocation should not fail */
> +	ASSERT(bio);

Please drop the comment and assert, that it does not fail is documented
next to btrfs_bio_alloc so it's not necessary to repeat it at each call
site.

> +
> +	bio->bi_opf = opf;
> +	bio->bi_private = cb;
> +	bio->bi_end_io = endio_func;
> +
> +	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
> +		struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
> +		struct btrfs_device *device;
> +
> +		device = btrfs_zoned_get_device(fs_info, disk_bytenr,
> +						fs_info->sectorsize);
> +		if (IS_ERR(device)) {
> +			bio_put(bio);
> +			return ERR_CAST(device);
> +		}
> +		bio_set_dev(bio, device->bdev);
> +	}
> +	return bio;
> +}
> +
>  /*
>   * worker function to build and submit bios for previously compressed pages.
>   * The corresponding pages in the inode should be marked for writeback
> @@ -486,22 +521,11 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>  	cb->orig_bio = NULL;
>  	cb->nr_pages = nr_pages;
>  
> -	bio = btrfs_bio_alloc(first_byte);
> -	bio->bi_opf = bio_op | write_flags;
> -	bio->bi_private = cb;
> -	bio->bi_end_io = end_compressed_bio_write;
> -
> -	if (use_append) {
> -		struct btrfs_device *device;
> -
> -		device = btrfs_zoned_get_device(fs_info, disk_start, PAGE_SIZE);
> -		if (IS_ERR(device)) {
> -			kfree(cb);
> -			bio_put(bio);
> -			return BLK_STS_NOTSUPP;
> -		}
> -
> -		bio_set_dev(bio, device->bdev);
> +	bio = alloc_compressed_bio(cb, first_byte, bio_op | write_flags,
> +				   end_compressed_bio_write);
> +	if (IS_ERR(bio)) {
> +		kfree(cb);
> +		return errno_to_blk_status(PTR_ERR(bio));
>  	}
>  
>  	if (blkcg_css) {
> @@ -545,10 +569,14 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>  			if (ret)
>  				goto finish_cb;
>  
> -			bio = btrfs_bio_alloc(first_byte);
> -			bio->bi_opf = bio_op | write_flags;
> -			bio->bi_private = cb;
> -			bio->bi_end_io = end_compressed_bio_write;
> +			bio = alloc_compressed_bio(cb, first_byte,
> +					bio_op | write_flags,
> +					end_compressed_bio_write);
> +			if (IS_ERR(bio)) {
> +				ret = errno_to_blk_status(PTR_ERR(bio));
> +				bio = NULL;
> +				goto finish_cb;
> +			}
>  			if (blkcg_css)
>  				bio->bi_opf |= REQ_CGROUP_PUNT;
>  			/*
> @@ -812,10 +840,13 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	/* include any pages we added in add_ra-bio_pages */
>  	cb->len = bio->bi_iter.bi_size;
>  
> -	comp_bio = btrfs_bio_alloc(cur_disk_byte);
> -	comp_bio->bi_opf = REQ_OP_READ;
> -	comp_bio->bi_private = cb;
> -	comp_bio->bi_end_io = end_compressed_bio_read;
> +	comp_bio = alloc_compressed_bio(cb, cur_disk_byte, REQ_OP_READ,
> +					end_compressed_bio_read);
> +	if (IS_ERR(comp_bio)) {
> +		ret = errno_to_blk_status(PTR_ERR(comp_bio));
> +		comp_bio = NULL;
> +		goto fail2;
> +	}
>  
>  	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
>  		u32 pg_len = PAGE_SIZE;
> @@ -856,10 +887,14 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  			if (ret)
>  				goto finish_cb;
>  
> -			comp_bio = btrfs_bio_alloc(cur_disk_byte);
> -			comp_bio->bi_opf = REQ_OP_READ;
> -			comp_bio->bi_private = cb;
> -			comp_bio->bi_end_io = end_compressed_bio_read;
> +			comp_bio = alloc_compressed_bio(cb, cur_disk_byte,
> +					REQ_OP_READ,
> +					end_compressed_bio_read);
> +			if (IS_ERR(comp_bio)) {
> +				ret = errno_to_blk_status(PTR_ERR(comp_bio));
> +				comp_bio = NULL;
> +				goto finish_cb;
> +			}
>  
>  			bio_add_page(comp_bio, page, pg_len, 0);
>  		}
> -- 
> 2.32.0