[09/10] btrfs: return a btrfs_bio from btrfs_bio_alloc

Message ID	20230301134244.1378533-10-hch@lst.de (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-btrfs-owner@vger.kernel.org> From: Christoph Hellwig <hch@lst.de> To: Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>, David Sterba <dsterba@suse.com> Cc: linux-btrfs@vger.kernel.org Subject: [PATCH 09/10] btrfs: return a btrfs_bio from btrfs_bio_alloc Date: Wed, 1 Mar 2023 06:42:42 -0700 Message-Id: <20230301134244.1378533-10-hch@lst.de> In-Reply-To: <20230301134244.1378533-1-hch@lst.de> References: <20230301134244.1378533-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	[01/10] btrfs: remove unused members from struct btrfs_encoded_read_private \| expand [01/10] btrfs: remove unused members from struct btrfs_encoded_read_private [02/10] btrfs: cleanup btrfs_encoded_read_regular_fill_pages [03/10] btrfs: move zero filling of compressed read bios into common code [04/10] btrfs: pass a btrfs_bio to btrfs_submit_bio [05/10] btrfs: pass a btrfs_bio to btrfs_submit_compressed_read [06/10] btrfs: store a pointer to the original btrfs_bio in struct compressed_bio [07/10] btrfs: simplify finding the inode in submit_one_bio [08/10] btrfs: store a pointer to a btrfs_bio in struct btrfs_bio_ctrl [09/10] btrfs: return a btrfs_bio from btrfs_bio_alloc [10/10] btrfs: make btrfs_split_bio work on struct btrfs_bio

Christoph Hellwig March 1, 2023, 1:42 p.m. UTC

Return the conaining struct btrfs_bio instead of the less type safe
struct bio from btrfs_bio_alloc.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/bio.c       | 12 +++++++-----
 fs/btrfs/bio.h       |  6 +++---
 fs/btrfs/extent_io.c | 18 +++++++++---------
 fs/btrfs/inode.c     | 18 +++++++++---------
 4 files changed, 28 insertions(+), 26 deletions(-)

Johannes Thumshirn March 2, 2023, 2:22 p.m. UTC | #1

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Qu Wenruo March 2, 2023, 11:31 p.m. UTC | #2

On 2023/3/1 21:42, Christoph Hellwig wrote:
> Return the conaining struct btrfs_bio instead of the less type safe
> struct bio from btrfs_bio_alloc.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/bio.c       | 12 +++++++-----
>   fs/btrfs/bio.h       |  6 +++---
>   fs/btrfs/extent_io.c | 18 +++++++++---------
>   fs/btrfs/inode.c     | 18 +++++++++---------
>   4 files changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index c04e103f876853..527081abca026f 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -48,15 +48,17 @@ void btrfs_bio_init(struct btrfs_bio *bbio, struct btrfs_inode *inode,
>    * Just like the underlying bio_alloc_bioset it will not fail as it is backed by
>    * a mempool.
>    */
> -struct bio *btrfs_bio_alloc(unsigned int nr_vecs, blk_opf_t opf,
> -			    struct btrfs_inode *inode,
> -			    btrfs_bio_end_io_t end_io, void *private)
> +struct btrfs_bio *btrfs_bio_alloc(unsigned int nr_vecs, blk_opf_t opf,
> +				  struct btrfs_inode *inode,
> +				  btrfs_bio_end_io_t end_io, void *private)
>   {
> +	struct btrfs_bio *bbio;
>   	struct bio *bio;
>   
>   	bio = bio_alloc_bioset(NULL, nr_vecs, opf, GFP_NOFS, &btrfs_bioset);
> -	btrfs_bio_init(btrfs_bio(bio), inode, end_io, private);
> -	return bio;
> +	bbio = btrfs_bio(bio);
> +	btrfs_bio_init(bbio, inode, end_io, private);
> +	return bbio;
>   }
>   
>   static struct bio *btrfs_split_bio(struct btrfs_fs_info *fs_info,
> diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h
> index b4e7d5ab7d236d..dbf125f6fa336d 100644
> --- a/fs/btrfs/bio.h
> +++ b/fs/btrfs/bio.h
> @@ -75,9 +75,9 @@ void __cold btrfs_bioset_exit(void);
>   
>   void btrfs_bio_init(struct btrfs_bio *bbio, struct btrfs_inode *inode,
>   		    btrfs_bio_end_io_t end_io, void *private);
> -struct bio *btrfs_bio_alloc(unsigned int nr_vecs, blk_opf_t opf,
> -			    struct btrfs_inode *inode,
> -			    btrfs_bio_end_io_t end_io, void *private);
> +struct btrfs_bio *btrfs_bio_alloc(unsigned int nr_vecs, blk_opf_t opf,
> +				  struct btrfs_inode *inode,
> +				  btrfs_bio_end_io_t end_io, void *private);
>   
>   static inline void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
>   {
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index df143c5267e61b..2d5e4df3419b0f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -896,13 +896,13 @@ static void alloc_new_bio(struct btrfs_inode *inode,
>   			  u64 disk_bytenr, u64 file_offset)
>   {
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> -	struct bio *bio;
> +	struct btrfs_bio *bbio;
>   
> -	bio = btrfs_bio_alloc(BIO_MAX_VECS, bio_ctrl->opf, inode,
> -			      bio_ctrl->end_io_func, NULL);
> -	bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
> -	btrfs_bio(bio)->file_offset = file_offset;
> -	bio_ctrl->bbio = btrfs_bio(bio);
> +	bbio = btrfs_bio_alloc(BIO_MAX_VECS, bio_ctrl->opf, inode,
> +			       bio_ctrl->end_io_func, NULL);
> +	bbio->bio.bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
> +	bbio->file_offset = file_offset;
> +	bio_ctrl->bbio = bbio;
>   	bio_ctrl->len_to_oe_boundary = U32_MAX;
>   
>   	/*
> @@ -911,7 +911,7 @@ static void alloc_new_bio(struct btrfs_inode *inode,
>   	 * them.
>   	 */
>   	if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE &&
> -	    btrfs_use_zone_append(btrfs_bio(bio))) {
> +	    btrfs_use_zone_append(bbio)) {
>   		struct btrfs_ordered_extent *ordered;
>   
>   		ordered = btrfs_lookup_ordered_extent(inode, file_offset);
> @@ -930,8 +930,8 @@ static void alloc_new_bio(struct btrfs_inode *inode,
>   		 * to always be set on the last added/replaced device.
>   		 * This is a bit odd but has been like that for a long time.
>   		 */
> -		bio_set_dev(bio, fs_info->fs_devices->latest_dev->bdev);
> -		wbc_init_bio(bio_ctrl->wbc, bio);
> +		bio_set_dev(&bbio->bio, fs_info->fs_devices->latest_dev->bdev);
> +		wbc_init_bio(bio_ctrl->wbc, &bbio->bio);
>   	}
>   }
>   
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ed96c84f5be71d..7e691bab72dffa 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9959,24 +9959,24 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
>   		.pending = ATOMIC_INIT(1),
>   	};
>   	unsigned long i = 0;
> -	struct bio *bio;
> +	struct btrfs_bio *bbio;
>   
>   	init_waitqueue_head(&priv.wait);
>   
> -	bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode,
> +	bbio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode,
>   			      btrfs_encoded_read_endio, &priv);
> -	bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
> +	bbio->bio.bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
>   
>   	do {
>   		size_t bytes = min_t(u64, disk_io_size, PAGE_SIZE);
>   
> -		if (bio_add_page(bio, pages[i], bytes, 0) < bytes) {
> +		if (bio_add_page(&bbio->bio, pages[i], bytes, 0) < bytes) {
>   			atomic_inc(&priv.pending);
> -			btrfs_submit_bio(btrfs_bio(bio), 0);
> +			btrfs_submit_bio(bbio, 0);
>   
> -			bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode,
> -					      btrfs_encoded_read_endio, &priv);
> -			bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
> +			bbio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode,
> +					       btrfs_encoded_read_endio, &priv);
> +			bbio->bio.bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
>   			continue;
>   		}
>   
> @@ -9986,7 +9986,7 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
>   	} while (disk_io_size);
>   
>   	atomic_inc(&priv.pending);
> -	btrfs_submit_bio(btrfs_bio(bio), 0);
> +	btrfs_submit_bio(bbio, 0);
>   
>   	if (atomic_dec_return(&priv.pending))
>   		io_wait_event(priv.wait, !atomic_read(&priv.pending));

Anand Jain March 3, 2023, 10:48 p.m. UTC | #3

On 01/03/2023 21:42, Christoph Hellwig wrote:
> Return the conaining struct btrfs_bio instead of the less type safe
> struct bio from btrfs_bio_alloc.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>


Reviewed-by: Anand Jain <anand.jain@oracle.com>


-	struct bio *bio;
+	struct btrfs_bio *bbio;

Here, dereferenced for bio from %bbio appears more than once.
I am curious why you didn't choose to initialize the bio instead.

Anand Jain March 4, 2023, 3:38 a.m. UTC | #4

On 04/03/2023 06:48, Anand Jain wrote:
> On 01/03/2023 21:42, Christoph Hellwig wrote:
>> Return the conaining struct btrfs_bio instead of the less type safe

Nit:
s/conaining/containing

>> struct bio from btrfs_bio_alloc.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> 
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
> 
> 
> -    struct bio *bio;
> +    struct btrfs_bio *bbio;
> 
> Here, dereferenced for bio from %bbio appears more than once.
> I am curious why you didn't choose to initialize the bio instead.

Christoph Hellwig March 6, 2023, 4:57 p.m. UTC | #5

On Sat, Mar 04, 2023 at 06:48:54AM +0800, Anand Jain wrote:
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
>
>
> -	struct bio *bio;
> +	struct btrfs_bio *bbio;
>
> Here, dereferenced for bio from %bbio appears more than once.
> I am curious why you didn't choose to initialize the bio instead.

As this is a bit of a theme here:  I prefer to not add a local variable
if there's less than about half a dozen dereference in the code, and
the dereferences aren't too long.  If there is a general consensus to
add a more local variables I can do that, but it doesn't feel helpful
to me.

[09/10] btrfs: return a btrfs_bio from btrfs_bio_alloc

Commit Message

Comments

Patch