Re: [PATCH 23/40] btrfs: store an inode pointer in struct btrfs_bio

[Qu Wenruo <quwenruo.btrfs@xxxxxxx>]

On 2022/3/22 23:55, Christoph Hellwig wrote:
> All the I/O going through the btrfs_bio based path are associated with an
> inode.  Add a pointer to it to simplify a few things soon.  Also pass the
> bio operation to btrfs_bio_alloc given that we have to touch it anyway.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Something I want to avoid is to futher increasing the size of btrfs_bio.

For buffered uncompressed IO, we can grab the inode from the first page.
For direct IO we have bio->bi_private (btrfs_dio_private).
For compressed IO, it's bio->bi_private again (compressed_bio).

Do the saved code lines really validate the memory usage for all bios?

Thanks,
Qu

> ---
>   fs/btrfs/compression.c |  4 +---
>   fs/btrfs/extent_io.c   | 23 ++++++++++++-----------
>   fs/btrfs/extent_io.h   |  6 ++++--
>   fs/btrfs/inode.c       |  3 ++-
>   fs/btrfs/volumes.h     |  2 ++
>   5 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 71e5b2e9a1ba8..419a09d924290 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -464,10 +464,8 @@ static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_byte
>   	struct bio *bio;
>   	int ret;
>
> -	bio = btrfs_bio_alloc(BIO_MAX_VECS);
> -
> +	bio = btrfs_bio_alloc(cb->inode, BIO_MAX_VECS, opf);
>   	bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
> -	bio->bi_opf = opf;
>   	bio->bi_private = cb;
>   	bio->bi_end_io = endio_func;
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 58ef0f4fca361..116a65787e314 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2657,10 +2657,9 @@ int btrfs_repair_one_sector(struct inode *inode,
>   		return -EIO;
>   	}
>
> -	repair_bio = btrfs_bio_alloc(1);
> +	repair_bio = btrfs_bio_alloc(inode, 1, REQ_OP_READ);
>   	repair_bbio = btrfs_bio(repair_bio);
>   	repair_bbio->file_offset = start;
> -	repair_bio->bi_opf = REQ_OP_READ;
>   	repair_bio->bi_end_io = failed_bio->bi_end_io;
>   	repair_bio->bi_iter.bi_sector = failrec->logical >> 9;
>   	repair_bio->bi_private = failed_bio->bi_private;
> @@ -3128,9 +3127,10 @@ static void end_bio_extent_readpage(struct bio *bio)
>    * new bio by bio_alloc_bioset as it does not initialize the bytes outside of
>    * 'bio' because use of __GFP_ZERO is not supported.
>    */
> -static inline void btrfs_bio_init(struct btrfs_bio *bbio)
> +static inline void btrfs_bio_init(struct btrfs_bio *bbio, struct inode *inode)
>   {
>   	memset(bbio, 0, offsetof(struct btrfs_bio, bio));
> +	bbio->inode = inode;
>   }
>
>   /*
> @@ -3138,13 +3138,14 @@ static inline void btrfs_bio_init(struct btrfs_bio *bbio)
>    *
>    * The bio allocation is backed by bioset and does not fail.
>    */
> -struct bio *btrfs_bio_alloc(unsigned int nr_iovecs)
> +struct bio *btrfs_bio_alloc(struct inode *inode, unsigned int nr_iovecs,
> +		unsigned int opf)
>   {
>   	struct bio *bio;
>
>   	ASSERT(0 < nr_iovecs && nr_iovecs <= BIO_MAX_VECS);
> -	bio = bio_alloc_bioset(NULL, nr_iovecs, 0, GFP_NOFS, &btrfs_bioset);
> -	btrfs_bio_init(btrfs_bio(bio));
> +	bio = bio_alloc_bioset(NULL, nr_iovecs, opf, GFP_NOFS, &btrfs_bioset);
> +	btrfs_bio_init(btrfs_bio(bio), inode);
>   	return bio;
>   }
>
> @@ -3156,12 +3157,13 @@ struct bio *btrfs_bio_clone(struct block_device *bdev, struct bio *bio)
>   	/* 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);
> +	btrfs_bio_init(btrfs_bio(new), btrfs_bio(bio)->inode);
>   	bbio->iter = bio->bi_iter;
>   	return new;
>   }
>
> -struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size)
> +struct bio *btrfs_bio_clone_partial(struct inode *inode, struct bio *orig,
> +		u64 offset, u64 size)
>   {
>   	struct bio *bio;
>   	struct btrfs_bio *bbio;
> @@ -3173,7 +3175,7 @@ struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size)
>   	ASSERT(bio);
>
>   	bbio = btrfs_bio(bio);
> -	btrfs_bio_init(bbio);
> +	btrfs_bio_init(btrfs_bio(bio), inode);
>
>   	bio_trim(bio, offset >> 9, size >> 9);
>   	bbio->iter = bio->bi_iter;
> @@ -3308,7 +3310,7 @@ static int alloc_new_bio(struct btrfs_inode *inode,
>   	struct bio *bio;
>   	int ret;
>
> -	bio = btrfs_bio_alloc(BIO_MAX_VECS);
> +	bio = btrfs_bio_alloc(&inode->vfs_inode, BIO_MAX_VECS, opf);
>   	/*
>   	 * For compressed page range, its disk_bytenr is always @disk_bytenr
>   	 * passed in, no matter if we have added any range into previous bio.
> @@ -3321,7 +3323,6 @@ static int alloc_new_bio(struct btrfs_inode *inode,
>   	bio_ctrl->bio_flags = bio_flags;
>   	bio->bi_end_io = end_io_func;
>   	bio->bi_private = &inode->io_tree;
> -	bio->bi_opf = opf;
>   	ret = calc_bio_boundaries(bio_ctrl, inode, file_offset);
>   	if (ret < 0)
>   		goto error;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 72d86f228c56e..d5f3d9692ea29 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -277,9 +277,11 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end);
>   void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
>   				  struct page *locked_page,
>   				  u32 bits_to_clear, unsigned long page_ops);
> -struct bio *btrfs_bio_alloc(unsigned int nr_iovecs);
> +struct bio *btrfs_bio_alloc(struct inode *inode, unsigned int nr_iovecs,
> +		unsigned int opf);
>   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);
> +struct bio *btrfs_bio_clone_partial(struct inode *inode, struct bio *orig,
> +		u64 offset, u64 size);
>
>   void end_extent_writepage(struct page *page, int err, u64 start, u64 end);
>   int btrfs_repair_eb_io_failure(const struct extent_buffer *eb, int mirror_num);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5c9d8e8a98466..18d54cfedf829 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7987,7 +7987,8 @@ static void btrfs_submit_direct(const struct iomap_iter *iter,
>   		 * This will never fail as it's passing GPF_NOFS and
>   		 * the allocation is backed by btrfs_bioset.
>   		 */
> -		bio = btrfs_bio_clone_partial(dio_bio, clone_offset, clone_len);
> +		bio = btrfs_bio_clone_partial(inode, dio_bio, clone_offset,
> +					      clone_len);
>   		bio->bi_private = dip;
>   		bio->bi_end_io = btrfs_end_dio_bio;
>   		btrfs_bio(bio)->file_offset = file_offset;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index c22148bebc2f5..a4f942547002e 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -321,6 +321,8 @@ struct btrfs_fs_devices {
>    * Mostly for btrfs specific features like csum and mirror_num.
>    */
>   struct btrfs_bio {
> +	struct inode *inode;
> +
>   	unsigned int mirror_num;
>
>   	/* for direct I/O */