| Message ID | 20230130092157.1759539-2-hch@lst.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/23] block: factor out a bvec_set_page helper | expand |
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
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.
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
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.
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
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.
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
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.
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
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.
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
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 --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 */
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(-)