[V6,15/30] block: introduce bio_clone_chunk_bioset()

Commit Message

Christoph Hellwig June 13, 2018, 2:56 p.m. UTC

On Sat, Jun 09, 2018 at 08:29:59PM +0800, Ming Lei wrote:
> There is one use case(DM) which requires to clone bio chunk by
> chunk, so introduce this API.

I don't think DM is the special case here.  The special case is the
bounce code that only wants single page bios.  Between that, and the
fact that we only have two callers and one of them is inside the
block layer I would suggest to fold in the following patch to make
bio_clone_bioset clone in multi-page bvecs and make the bounce code
use the low-level interface directly:

Comments

Ming Lei June 14, 2018, 2:01 a.m. UTC | #1

On Wed, Jun 13, 2018 at 07:56:54AM -0700, Christoph Hellwig wrote:
> On Sat, Jun 09, 2018 at 08:29:59PM +0800, Ming Lei wrote:
> > There is one use case(DM) which requires to clone bio chunk by
> > chunk, so introduce this API.
> 
> I don't think DM is the special case here.  The special case is the
> bounce code that only wants single page bios.  Between that, and the
> fact that we only have two callers and one of them is inside the
> block layer I would suggest to fold in the following patch to make
> bio_clone_bioset clone in multi-page bvecs and make the bounce code
> use the low-level interface directly:

Bounce limits the max pages as 256 will do bio splitting, so won't need
this change.


Thanks,
Ming

Christoph Hellwig June 14, 2018, 6:39 a.m. UTC | #2

On Thu, Jun 14, 2018 at 10:01:38AM +0800, Ming Lei wrote:
> Bounce limits the max pages as 256 will do bio splitting, so won't need
> this change.

Behavior for the bounce code does not change with my patch.

The important points are:

 - the default interface (bio_clone_bioset in this case) should always
   operate on full biosets
 - if the bounce code needs bioves limited to single pages it should
   be treated as the special case
 - given that the bounce code is inside the block layer using the
   __-prefixed internal interface is perfectly fine
 - last but not least I think the parameter switching the behavior
   needs a much more descriptive name as suggested in my patch

Ming Lei June 14, 2018, 7:28 a.m. UTC | #3

On Wed, Jun 13, 2018 at 11:39:20PM -0700, Christoph Hellwig wrote:
> On Thu, Jun 14, 2018 at 10:01:38AM +0800, Ming Lei wrote:
> > Bounce limits the max pages as 256 will do bio splitting, so won't need
> > this change.
> 
> Behavior for the bounce code does not change with my patch.
> 
> The important points are:
> 
>  - the default interface (bio_clone_bioset in this case) should always
>    operate on full biosets
>  - if the bounce code needs bioves limited to single pages it should
>    be treated as the special case
>  - given that the bounce code is inside the block layer using the
>    __-prefixed internal interface is perfectly fine
>  - last but not least I think the parameter switching the behavior
>    needs a much more descriptive name as suggested in my patch

Fair enough, will switch to this way and avoid DM's change, even though
it is a dying interface.

Thanks,
Ming

Patch

diff --git a/block/bio.c b/block/bio.c
index 284085ab97e7..cef45c8d0a19 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -644,13 +644,14 @@  struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
 }
 EXPORT_SYMBOL(bio_clone_fast);
 
-static struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
-				      struct bio_set *bs, bool seg)
+struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
+		struct bio_set *bs, bool single_page_only)
 {
 	struct bvec_iter iter;
 	struct bio_vec bv;
 	struct bio *bio;
-	int nr_vecs = seg ? bio_segments(bio_src) : bio_chunks(bio_src);
+	int nr_vecs = single_page_only ?
+		bio_segments(bio_src) : bio_chunks(bio_src);
 
 	/*
 	 * Pre immutable biovecs, __bio_clone() used to just do a memcpy from
@@ -692,7 +693,7 @@  static struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 		bio->bi_io_vec[bio->bi_vcnt++] = bio_src->bi_io_vec[0];
 		break;
 	default:
-		if (seg) {
+		if (single_page_only) {
 			bio_for_each_segment(bv, bio_src, iter)
 				bio->bi_io_vec[bio->bi_vcnt++] = bv;
 		} else {
@@ -728,26 +729,10 @@  static struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
  */
 struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 			     struct bio_set *bs)
-{
-	return __bio_clone_bioset(bio_src, gfp_mask, bs, true);
-}
-EXPORT_SYMBOL(bio_clone_bioset);
-
-/**
- * 	bio_clone_seg_bioset - clone a bio segment by segment
- * 	@bio_src: bio to clone
- *	@gfp_mask: allocation priority
- *	@bs: bio_set to allocate from
- *
- *	Clone bio. Caller will own the returned bio, but not the actual data it
- *	points to. Reference count of returned bio will be one.
- */
-struct bio *bio_clone_chunk_bioset(struct bio *bio_src, gfp_t gfp_mask,
-				   struct bio_set *bs)
 {
 	return __bio_clone_bioset(bio_src, gfp_mask, bs, false);
 }
-EXPORT_SYMBOL(bio_clone_chunk_bioset);
+EXPORT_SYMBOL(bio_clone_bioset);
 
 /**
  *	bio_add_pc_page	-	attempt to add page to bio
diff --git a/block/bounce.c b/block/bounce.c
index c6af0bd29ec9..62dab528dc1b 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -224,8 +224,8 @@  static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
 		generic_make_request(*bio_orig);
 		*bio_orig = bio;
 	}
-	bio = bio_clone_bioset(*bio_orig, GFP_NOIO, passthrough ? NULL :
-			&bounce_bio_set);
+	bio = __bio_clone_bioset(*bio_orig, GFP_NOIO, passthrough ? NULL :
+			&bounce_bio_set, true);
 
 	bio_for_each_chunk_segment_all(to, bio, i, citer) {
 		struct page *page = to->bv_page;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 13ca3574d972..98dff36b89a3 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1582,8 +1582,8 @@  static blk_qc_t __split_and_process_bio(struct mapped_device *md,
 				 * the usage of io->orig_bio in dm_remap_zone_report()
 				 * won't be affected by this reassignment.
 				 */
-				struct bio *b = bio_clone_chunk_bioset(bio, GFP_NOIO,
-								       &md->queue->bio_split);
+				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
+								 &md->queue->bio_split);
 				ci.io->orig_bio = b;
 				bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9);
 				bio_chain(b, bio);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 58838dc12d69..5ccafeadbe95 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -486,7 +486,8 @@  extern void bio_put(struct bio *);
 extern void __bio_clone_fast(struct bio *, struct bio *);
 extern struct bio *bio_clone_fast(struct bio *, gfp_t, struct bio_set *);
 extern struct bio *bio_clone_bioset(struct bio *, gfp_t, struct bio_set *bs);
-extern struct bio *bio_clone_chunk_bioset(struct bio *, gfp_t, struct bio_set *bs);
+extern struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
+		struct bio_set *bs, bool single_page_only);
 
 extern struct bio_set fs_bio_set;