diff mbox series

[01/23] block: factor out a bvec_set_page helper

Message ID 20230130092157.1759539-2-hch@lst.de
State New
Headers show
Series [01/23] block: factor out a bvec_set_page helper | expand

Commit Message

Christoph Hellwig Jan. 30, 2023, 9:21 a.m. UTC
Add a helper to initialize a bvec based of a page pointer.  This will help
removing various open code bvec initializations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c |  7 +------
 block/bio.c           | 12 ++----------
 include/linux/bvec.h  | 15 +++++++++++++++
 3 files changed, 18 insertions(+), 16 deletions(-)

Comments

David Howells Jan. 30, 2023, 10:33 a.m. UTC | #1
Christoph Hellwig <hch@lst.de> wrote:

> +static inline void bvec_set_page(struct bio_vec *bv, struct page *page,
> +		unsigned int len, unsigned int offset)

Could you swap len and offset around?  It reads better offset first.  You move
offset into the page and then do something with len bytes.

David
Christoph Hellwig Jan. 30, 2023, 10:36 a.m. UTC | #2
On Mon, Jan 30, 2023 at 10:33:36AM +0000, David Howells wrote:
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > +static inline void bvec_set_page(struct bio_vec *bv, struct page *page,
> > +		unsigned int len, unsigned int offset)
> 
> Could you swap len and offset around?  It reads better offset first.  You move
> offset into the page and then do something with len bytes.

This matches bio_add_page and the order inside bio_vec itself.  willy
wanted to switch it around for bio_add_folio but Jens didn't like it,
so I'll stick to the current convention in this area as well.
Johannes Thumshirn Jan. 30, 2023, 11:55 a.m. UTC | #3
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Bart Van Assche Jan. 30, 2023, 5:09 p.m. UTC | #4
On 1/30/23 01:21, Christoph Hellwig wrote:
> Add a helper to initialize a bvec based of a page pointer.  This will help
> removing various open code bvec initializations.

Why do you want to remove the open-coded bvec initializations? What is 
wrong with open-coding bvec initialization? This patch series modifies a 
lot of code but does not improve code readability. Anyone who encounters 
code that uses the new function bvec_set_page() has to look up the 
definition of that function to figure out what it does.

> -	iv = bip->bip_vec + bip->bip_vcnt;
> -
>   	if (bip->bip_vcnt &&
>   	    bvec_gap_to_prev(&bdev_get_queue(bio->bi_bdev)->limits,
>   			     &bip->bip_vec[bip->bip_vcnt - 1], offset))
>   		return 0;
>   
> -	iv->bv_page = page;
> -	iv->bv_len = len;
> -	iv->bv_offset = offset;
> +	bvec_set_page(&bip->bip_vec[bip->bip_vcnt], page, len, offset);
>   	bip->bip_vcnt++;

Has it been considered to use structure assignment instead of 
introducing bvec_set_page(), e.g. as follows?

bip->bip_vec[bip->bip_vcnt] = (struct bio_vec) {
       .bv_page = page, .bv_len = len, .bv_offset = offset };

Thanks,

Bart.
Ilya Dryomov Jan. 30, 2023, 6:35 p.m. UTC | #5
On Mon, Jan 30, 2023 at 11:36 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Jan 30, 2023 at 10:33:36AM +0000, David Howells wrote:
> > Christoph Hellwig <hch@lst.de> wrote:
> >
> > > +static inline void bvec_set_page(struct bio_vec *bv, struct page *page,
> > > +           unsigned int len, unsigned int offset)
> >
> > Could you swap len and offset around?  It reads better offset first.  You move
> > offset into the page and then do something with len bytes.
>
> This matches bio_add_page and the order inside bio_vec itself.  willy
> wanted to switch it around for bio_add_folio but Jens didn't like it,
> so I'll stick to the current convention in this area as well.

This also matches sg_set_page() so sticking to the current convention
is definitely a good idea!

Thanks,

                Ilya
Bart Van Assche Jan. 30, 2023, 7:24 p.m. UTC | #6
On 1/30/23 09:09, Bart Van Assche wrote:
> On 1/30/23 01:21, Christoph Hellwig wrote:
>> Add a helper to initialize a bvec based of a page pointer.  This will 
>> help
>> removing various open code bvec initializations.
> 
> Why do you want to remove the open-coded bvec initializations? What is 
> wrong with open-coding bvec initialization? This patch series modifies a 
> lot of code but does not improve code readability. Anyone who encounters 
> code that uses the new function bvec_set_page() has to look up the 
> definition of that function to figure out what it does.

Please ignore the above question - I just noticed that this question has 
been answered in the cover letter.

Bart.
Jakub Kicinski Jan. 31, 2023, 4:47 a.m. UTC | #7
On Mon, 30 Jan 2023 10:21:35 +0100 Christoph Hellwig wrote:
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index 35c25dff651a5e..9e3dac51eb26b6 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -35,6 +35,21 @@ struct bio_vec {
>  	unsigned int	bv_offset;
>  };
>  
> +/**
> + * bvec_set_page - initialize a bvec based off a struct page
> + * @bv:		bvec to initialize
> + * @page:	page the bvec should point to
> + * @len:	length of the bvec
> + * @offset:	offset into the page
> + */
> +static inline void bvec_set_page(struct bio_vec *bv, struct page *page,
> +		unsigned int len, unsigned int offset)
> +{
> +	bv->bv_page = page;
> +	bv->bv_len = len;
> +	bv->bv_offset = offset;
> +}

kinda random thought but since we're touching this area - could we
perhaps move the definition of struct bio_vec and trivial helpers 
like this into a new header? bvec.h pulls in mm.h which is a right
behemoth :S
Matthew Wilcox Jan. 31, 2023, 5 a.m. UTC | #8
On Mon, Jan 30, 2023 at 08:47:58PM -0800, Jakub Kicinski wrote:
> kinda random thought but since we're touching this area - could we
> perhaps move the definition of struct bio_vec and trivial helpers 
> like this into a new header? bvec.h pulls in mm.h which is a right
> behemoth :S

I bet we can drop mm.h now.  It was originally added for nth_page()
in 3d75ca0adef4 but those were all removed by b8753433fc61.

A quick smoke test on my default testing config doesn't find any
problems.  Let me send a patch and see if the build bots complain.
Matthew Wilcox Jan. 31, 2023, 5:28 a.m. UTC | #9
On Tue, Jan 31, 2023 at 05:00:32AM +0000, Matthew Wilcox wrote:
> On Mon, Jan 30, 2023 at 08:47:58PM -0800, Jakub Kicinski wrote:
> > kinda random thought but since we're touching this area - could we
> > perhaps move the definition of struct bio_vec and trivial helpers 
> > like this into a new header? bvec.h pulls in mm.h which is a right
> > behemoth :S
> 
> I bet we can drop mm.h now.  It was originally added for nth_page()
> in 3d75ca0adef4 but those were all removed by b8753433fc61.
> 
> A quick smoke test on my default testing config doesn't find any
> problems.  Let me send a patch and see if the build bots complain.

Disappointingly, it doesn't really change anything.  1134 files
depend on mm.h both before and after [1].  Looks like it's due to
arch/x86/include/asm/cacheflush.h pulling in linux/mm.h, judging by the
contents of .build_test_kernel-x86_64/net/ipv6/.inet6_hashtables.o.cmd.
But *lots* of header files pull in mm.h, including scatterlist.h,
vt_kern.h, net.h, nfs_fs.h, sunrpc/svc.h and security.h.

I suppose it may cut down on include loops to drop it here, so I'm
still in favour of the patch I posted, but this illustrates how
deeply entangled our headers still are.

[1] find .build_test_kernel-x86_64/ -name '.*.cmd' |xargs grep 'include/linux/mm.h' |wc -l
Jakub Kicinski Jan. 31, 2023, 5:52 a.m. UTC | #10
On Tue, 31 Jan 2023 05:28:19 +0000 Matthew Wilcox wrote:
> > I bet we can drop mm.h now.  It was originally added for nth_page()
> > in 3d75ca0adef4 but those were all removed by b8753433fc61.
> > 
> > A quick smoke test on my default testing config doesn't find any
> > problems.  Let me send a patch and see if the build bots complain.  
> 
> Disappointingly, it doesn't really change anything.  1134 files
> depend on mm.h both before and after [1].  Looks like it's due to
> arch/x86/include/asm/cacheflush.h pulling in linux/mm.h, judging by the
> contents of .build_test_kernel-x86_64/net/ipv6/.inet6_hashtables.o.cmd.
> But *lots* of header files pull in mm.h, including scatterlist.h,
> vt_kern.h, net.h, nfs_fs.h, sunrpc/svc.h and security.h.
> 
> I suppose it may cut down on include loops to drop it here, so I'm
> still in favour of the patch I posted, but this illustrates how
> deeply entangled our headers still are.

+1 it's a bit of a chicken and an egg problem. Until mm.h is gone 
from bvec there's no point removing other headers which pull it in 
to skbuff.h.
Chaitanya Kulkarni Jan. 31, 2023, 6:55 a.m. UTC | #11
On 1/30/23 01:21, Christoph Hellwig wrote:
> Add a helper to initialize a bvec based of a page pointer.  This will help
> removing various open code bvec initializations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck
Christoph Hellwig Jan. 31, 2023, 1:45 p.m. UTC | #12
On Mon, Jan 30, 2023 at 09:09:23AM -0800, Bart Van Assche wrote:
> Has it been considered to use structure assignment instead of introducing 
> bvec_set_page(), e.g. as follows?
>
> bip->bip_vec[bip->bip_vcnt] = (struct bio_vec) {
>       .bv_page = page, .bv_len = len, .bv_offset = offset };

Unless it's hidden behind a macro it doesn't solve the problem of
abstraction away the layout.  I'm also find it less readable.
diff mbox series

Patch

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 3f5685c00e360b..a3776064c52a16 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -124,23 +124,18 @@  int bio_integrity_add_page(struct bio *bio, struct page *page,
 			   unsigned int len, unsigned int offset)
 {
 	struct bio_integrity_payload *bip = bio_integrity(bio);
-	struct bio_vec *iv;
 
 	if (bip->bip_vcnt >= bip->bip_max_vcnt) {
 		printk(KERN_ERR "%s: bip_vec full\n", __func__);
 		return 0;
 	}
 
-	iv = bip->bip_vec + bip->bip_vcnt;
-
 	if (bip->bip_vcnt &&
 	    bvec_gap_to_prev(&bdev_get_queue(bio->bi_bdev)->limits,
 			     &bip->bip_vec[bip->bip_vcnt - 1], offset))
 		return 0;
 
-	iv->bv_page = page;
-	iv->bv_len = len;
-	iv->bv_offset = offset;
+	bvec_set_page(&bip->bip_vec[bip->bip_vcnt], page, len, offset);
 	bip->bip_vcnt++;
 
 	return len;
diff --git a/block/bio.c b/block/bio.c
index d7fbc7adfc50aa..71e411a0c12950 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1029,10 +1029,7 @@  int bio_add_hw_page(struct request_queue *q, struct bio *bio,
 	if (bio->bi_vcnt >= queue_max_segments(q))
 		return 0;
 
-	bvec = &bio->bi_io_vec[bio->bi_vcnt];
-	bvec->bv_page = page;
-	bvec->bv_len = len;
-	bvec->bv_offset = offset;
+	bvec_set_page(&bio->bi_io_vec[bio->bi_vcnt], page, len, offset);
 	bio->bi_vcnt++;
 	bio->bi_iter.bi_size += len;
 	return len;
@@ -1108,15 +1105,10 @@  EXPORT_SYMBOL_GPL(bio_add_zone_append_page);
 void __bio_add_page(struct bio *bio, struct page *page,
 		unsigned int len, unsigned int off)
 {
-	struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt];
-
 	WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED));
 	WARN_ON_ONCE(bio_full(bio, len));
 
-	bv->bv_page = page;
-	bv->bv_offset = off;
-	bv->bv_len = len;
-
+	bvec_set_page(&bio->bi_io_vec[bio->bi_vcnt], page, len, off);
 	bio->bi_iter.bi_size += len;
 	bio->bi_vcnt++;
 }
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 35c25dff651a5e..9e3dac51eb26b6 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -35,6 +35,21 @@  struct bio_vec {
 	unsigned int	bv_offset;
 };
 
+/**
+ * bvec_set_page - initialize a bvec based off a struct page
+ * @bv:		bvec to initialize
+ * @page:	page the bvec should point to
+ * @len:	length of the bvec
+ * @offset:	offset into the page
+ */
+static inline void bvec_set_page(struct bio_vec *bv, struct page *page,
+		unsigned int len, unsigned int offset)
+{
+	bv->bv_page = page;
+	bv->bv_len = len;
+	bv->bv_offset = offset;
+}
+
 struct bvec_iter {
 	sector_t		bi_sector;	/* device address in 512 byte
 						   sectors */