Re: [PATCH 10/10] btrfs: do not allocate a btrfs_bio for low-level bios

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

On 2022/5/4 20:25, Christoph Hellwig wrote:
> The bios submitted from btrfs_map_bio don't really interact with the
> rest of btrfs and the only btrfs_bio member actually used in the
> low-level bios is the pointer to the btrfs_io_contex used for endio
> handler.
>
> Use a union in struct btrfs_io_stripe that allows the endio handler to
> find the btrfs_io_context and remove the spurious ->device assignment
> so that a plain fs_bio_set bio can be used for the low-level bios
> allocated inside btrfs_map_bio.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Although such union is not that reader friendly, it should be safe enough.

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


Although in the future, I may add a new bioset for such low-level bios
for other purposes (like read-repair at per-stripe level), thus may
allow a more readable solution.
But that would be another story in the future anyway.

Thanks,
Qu
> ---
>   fs/btrfs/extent_io.c | 13 -------------
>   fs/btrfs/extent_io.h |  1 -
>   fs/btrfs/volumes.c   | 20 ++++++++++----------
>   fs/btrfs/volumes.h   |  5 ++++-
>   4 files changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 93191771e5bf1..7efbc964585d3 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3210,19 +3210,6 @@ struct bio *btrfs_bio_alloc(unsigned int nr_iovecs)
>   	return bio;
>   }
>
> -struct bio *btrfs_bio_clone(struct block_device *bdev, struct bio *bio)
> -{
> -	struct btrfs_bio *bbio;
> -	struct bio *new;
> -
> -	/* Bio allocation backed by a bioset does not fail */
> -	new = bio_alloc_clone(bdev, bio, GFP_NOFS, &btrfs_bioset);
> -	bbio = btrfs_bio(new);
> -	btrfs_bio_init(bbio);
> -	bbio->iter = bio->bi_iter;
> -	return new;
> -}
> -
>   struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size)
>   {
>   	struct bio *bio;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 17674b7e699c6..ce69a4732f718 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -248,7 +248,6 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
>
>   int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array);
>   struct bio *btrfs_bio_alloc(unsigned int nr_iovecs);
> -struct bio *btrfs_bio_clone(struct block_device *bdev, struct bio *bio);
>   struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size);
>
>   void end_extent_writepage(struct page *page, int err, u64 start, u64 end);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 28ac2bfb5d296..8577893ccb4b9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6666,23 +6666,21 @@ static void btrfs_end_bioc(struct btrfs_io_context *bioc, bool async)
>
>   static void btrfs_end_bio(struct bio *bio)
>   {
> -	struct btrfs_io_context *bioc = bio->bi_private;
> +	struct btrfs_io_stripe *stripe = bio->bi_private;
> +	struct btrfs_io_context *bioc = stripe->bioc;
>
>   	if (bio->bi_status) {
>   		atomic_inc(&bioc->error);
>   		if (bio->bi_status == BLK_STS_IOERR ||
>   		    bio->bi_status == BLK_STS_TARGET) {
> -			struct btrfs_device *dev = btrfs_bio(bio)->device;
> -
> -			ASSERT(dev->bdev);
>   			if (btrfs_op(bio) == BTRFS_MAP_WRITE)
> -				btrfs_dev_stat_inc_and_print(dev,
> +				btrfs_dev_stat_inc_and_print(stripe->dev,
>   						BTRFS_DEV_STAT_WRITE_ERRS);
>   			else if (!(bio->bi_opf & REQ_RAHEAD))
> -				btrfs_dev_stat_inc_and_print(dev,
> +				btrfs_dev_stat_inc_and_print(stripe->dev,
>   						BTRFS_DEV_STAT_READ_ERRS);
>   			if (bio->bi_opf & REQ_PREFLUSH)
> -				btrfs_dev_stat_inc_and_print(dev,
> +				btrfs_dev_stat_inc_and_print(stripe->dev,
>   						BTRFS_DEV_STAT_FLUSH_ERRS);
>   		}
>   	}
> @@ -6714,14 +6712,16 @@ static void submit_stripe_bio(struct btrfs_io_context *bioc,
>   	}
>
>   	if (clone) {
> -		bio = btrfs_bio_clone(dev->bdev, orig_bio);
> +		bio = bio_alloc_clone(dev->bdev, orig_bio, GFP_NOFS,
> +				      &fs_bio_set);
>   	} else {
>   		bio = orig_bio;
>   		bio_set_dev(bio, dev->bdev);
> +		btrfs_bio(bio)->device = dev;
>   	}
>
> -	bio->bi_private = bioc;
> -	btrfs_bio(bio)->device = dev;
> +	bioc->stripes[dev_nr].bioc = bioc;
> +	bio->bi_private = &bioc->stripes[dev_nr];
>   	bio->bi_end_io = btrfs_end_bio;
>   	bio->bi_iter.bi_sector = physical >> 9;
>   	/*
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 28e28b7c48649..825e44c82f2b0 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -396,7 +396,10 @@ static inline void btrfs_bio_free_csum(struct btrfs_bio *bbio)
>
>   struct btrfs_io_stripe {
>   	struct btrfs_device *dev;
> -	u64 physical;
> +	union {
> +		u64 physical;			/* block mapping */
> +		struct btrfs_io_context *bioc;	/* for the endio handler */
> +	};
>   	u64 length; /* only used for discard mappings */
>   };
>