[PATCH 0/4] md: use bio_clone_fast()

[PATCH 0/4] md: use bio_clone_fast()

[Ming Lei]

Hi,

This patches replaces bio_clone() with bio_fast_clone() in
bio_clone_mddev() because:

1) bio_clone_mddev() is used in raid normal I/O and isn't in
resync I/O path, and all the direct access to bvec table in
raid happens on resync I/O only except for write behind of raid1.
Write behind is treated specially, so the replacement is safe.

2) for write behind, bio_clone() is kept, but this patchset
introduces bio_clone_bioset_partial() to just clone one specific 
bvecs range instead of whole table. Then write behind is improved
too.


Thanks,
Ming

Ming Lei (4):
  block: introduce bio_clone_bioset_partial()
  md: introduce bio_clone_slow_mddev_partial()
  md/raid1: use bio_clone_slow_mddev_partial in case of write behind
  md: fast clone bio in bio_clone_mddev()

 block/bio.c         | 61 +++++++++++++++++++++++++++++++++++++++++------------
 drivers/md/md.c     | 24 +++++++++++++++++++--
 drivers/md/md.h     |  3 +++
 drivers/md/raid1.c  | 21 +++++++++++++-----
 include/linux/bio.h | 11 ++++++++--
 5 files changed, 98 insertions(+), 22 deletions(-)

-- 
2.7.4

[PATCH 1/4] block: introduce bio_clone_bioset_partial()

[Ming Lei]

md still need bio clone(not the fast version) for behind write,
and it is more efficient to use bio_clone_bioset_partial().

The idea is simple and just copy the bvecs range specified from
parameters.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 block/bio.c         | 61 +++++++++++++++++++++++++++++++++++++++++------------
 include/linux/bio.h | 11 ++++++++--
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 4b564d0c3e29..5eec5e08417f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -625,21 +625,20 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
 }
 EXPORT_SYMBOL(bio_clone_fast);
 
-/**
- * 	bio_clone_bioset - clone a bio
- * 	@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_bioset(struct bio *bio_src, gfp_t gfp_mask,
-			     struct bio_set *bs)
+static struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
+				      struct bio_set *bs, int offset,
+				      int size)
 {
 	struct bvec_iter iter;
 	struct bio_vec bv;
 	struct bio *bio;
+	struct bvec_iter iter_src = bio_src->bi_iter;
+
+	/* for supporting partial clone */
+	if (offset || size != bio_src->bi_iter.bi_size) {
+		bio_advance_iter(bio_src, &iter_src, offset);
+		iter_src.bi_size = size;
+	}
 
 	/*
 	 * Pre immutable biovecs, __bio_clone() used to just do a memcpy from
@@ -663,7 +662,8 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 	 *    __bio_clone_fast() anyways.
 	 */
 
-	bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
+	bio = bio_alloc_bioset(gfp_mask, __bio_segments(bio_src,
+			       &iter_src), bs);
 	if (!bio)
 		return NULL;
 	bio->bi_bdev		= bio_src->bi_bdev;
@@ -680,7 +680,7 @@ 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:
-		bio_for_each_segment(bv, bio_src, iter)
+		__bio_for_each_segment(bv, bio_src, iter, iter_src)
 			bio->bi_io_vec[bio->bi_vcnt++] = bv;
 		break;
 	}
@@ -699,9 +699,44 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 
 	return bio;
 }
+
+/**
+ * 	bio_clone_bioset - clone a bio
+ * 	@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_bioset(struct bio *bio_src, gfp_t gfp_mask,
+			     struct bio_set *bs)
+{
+	return __bio_clone_bioset(bio_src, gfp_mask, bs, 0,
+				  bio_src->bi_iter.bi_size);
+}
 EXPORT_SYMBOL(bio_clone_bioset);
 
 /**
+ * 	bio_clone_bioset_partial - clone a partial bio
+ * 	@bio_src: bio to clone
+ *	@gfp_mask: allocation priority
+ *	@bs: bio_set to allocate from
+ *	@offset: cloned starting from the offset
+ *	@size: size for the cloned bio
+ *
+ *	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_bioset_partial(struct bio *bio_src, gfp_t gfp_mask,
+				     struct bio_set *bs, int offset,
+				     int size)
+{
+	return __bio_clone_bioset(bio_src, gfp_mask, bs, offset, size);
+}
+EXPORT_SYMBOL(bio_clone_bioset_partial);
+
+/**
  *	bio_add_pc_page	-	attempt to add page to bio
  *	@q: the target queue
  *	@bio: destination bio
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 7cf8a6c70a3f..8e521194f6fc 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -183,7 +183,7 @@ static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
 
 #define bio_iter_last(bvec, iter) ((iter).bi_size == (bvec).bv_len)
 
-static inline unsigned bio_segments(struct bio *bio)
+static inline unsigned __bio_segments(struct bio *bio, struct bvec_iter *bvec)
 {
 	unsigned segs = 0;
 	struct bio_vec bv;
@@ -205,12 +205,17 @@ static inline unsigned bio_segments(struct bio *bio)
 		break;
 	}
 
-	bio_for_each_segment(bv, bio, iter)
+	__bio_for_each_segment(bv, bio, iter, *bvec)
 		segs++;
 
 	return segs;
 }
 
+static inline unsigned bio_segments(struct bio *bio)
+{
+	return __bio_segments(bio, &bio->bi_iter);
+}
+
 /*
  * get a reference to a bio, so it won't disappear. the intended use is
  * something like:
@@ -384,6 +389,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_bioset_partial(struct bio *, gfp_t,
+					    struct bio_set *, int, int);
 
 extern struct bio_set *fs_bio_set;
 
-- 
2.7.4

[PATCH 2/4] md: introduce bio_clone_slow_mddev_partial()

[Ming Lei]

In raid1/raid10, most users of bio_clone_mddev() need bio_trim() too,
that means only part of the bio is required to be cloned, and not
necessary to clone the whole bio each time, and it is just enough
to clone the specified bvecs range.

So this patch introduces bio_clone_slow_mddev_partial() to improve
the situation, and it is named as slow because the following patch
will switch to bio_clone_fast() in bio_clone_mddev().

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/md.c | 16 ++++++++++++++++
 drivers/md/md.h |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4c1b82defa78..704be11355a9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -200,6 +200,22 @@ struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
 }
 EXPORT_SYMBOL_GPL(bio_clone_mddev);
 
+struct bio *bio_clone_slow_mddev_partial(struct bio *bio, gfp_t gfp_mask,
+					 struct mddev *mddev, int offset,
+					 int size)
+{
+	struct bio_set *bs;
+
+	if (!mddev || !mddev->bio_set)
+		bs = fs_bio_set;
+	else
+		bs = mddev->bio_set;
+
+	return bio_clone_bioset_partial(bio, gfp_mask, bs, offset << 9,
+					size << 9);
+}
+EXPORT_SYMBOL_GPL(bio_clone_slow_mddev_partial);
+
 /*
  * We have a system wide 'event count' that is incremented
  * on any 'interesting' event, and readers of /proc/mdstat
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 968bbe72b237..4f4e6ded59e5 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -675,6 +675,9 @@ extern void mddev_suspend(struct mddev *mddev);
 extern void mddev_resume(struct mddev *mddev);
 extern struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
 				   struct mddev *mddev);
+extern struct bio *bio_clone_slow_mddev_partial(struct bio *bio, gfp_t gfp_mask,
+						struct mddev *mddev, int offset,
+						int size);
 extern struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
 				   struct mddev *mddev);
 
-- 
2.7.4

Re: [PATCH 2/4] md: introduce bio_clone_slow_mddev_partial()

[Christoph Hellwig]

> +struct bio *bio_clone_slow_mddev_partial(struct bio *bio, gfp_t gfp_mask,
> +					 struct mddev *mddev, int offset,
> +					 int size)
> +{
> +	struct bio_set *bs;
> +
> +	if (!mddev || !mddev->bio_set)
> +		bs = fs_bio_set;
> +	else
> +		bs = mddev->bio_set;
> +
> +	return bio_clone_bioset_partial(bio, gfp_mask, bs, offset << 9,
> +					size << 9);
> +}
> +EXPORT_SYMBOL_GPL(bio_clone_slow_mddev_partial);

As far as I can tell the caller always has a mddev, and an active
mddev always has a bio_set.  So let's just skip this wrapper.

[PATCH 3/4] md/raid1: use bio_clone_slow_mddev_partial in case of write behind

[Ming Lei]

Write behind need to replace pages in bio's bvecs, and we have
to clone a fresh bio with new bvec table, so use the introduced
bio_clone_slow_mddev_partial() for it.

For other bio_clone_mddev() cases, we will use fast clone since
they don't need to touch bvec table.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/raid1.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 830ff2b20346..e1c5639febdb 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1341,13 +1341,12 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 
 	first_clone = 1;
 	for (i = 0; i < disks; i++) {
-		struct bio *mbio;
+		struct bio *mbio = NULL;
+		int offset;
 		if (!r1_bio->bios[i])
 			continue;
 
-		mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
-		bio_trim(mbio, r1_bio->sector - bio->bi_iter.bi_sector,
-			 max_sectors);
+		offset = r1_bio->sector - bio->bi_iter.bi_sector;
 
 		if (first_clone) {
 			/* do behind I/O ?
@@ -1357,8 +1356,14 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 			if (bitmap &&
 			    (atomic_read(&bitmap->behind_writes)
 			     < mddev->bitmap_info.max_write_behind) &&
-			    !waitqueue_active(&bitmap->behind_wait))
+			    !waitqueue_active(&bitmap->behind_wait)) {
+				mbio = bio_clone_slow_mddev_partial(bio,
+								    GFP_NOIO,
+								    mddev,
+								    offset,
+								    max_sectors);
 				alloc_behind_pages(mbio, r1_bio);
+			}
 
 			bitmap_startwrite(bitmap, r1_bio->sector,
 					  r1_bio->sectors,
@@ -1366,6 +1371,12 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 						   &r1_bio->state));
 			first_clone = 0;
 		}
+
+		if (!mbio) {
+			mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+			bio_trim(mbio, offset, max_sectors);
+		}
+
 		if (r1_bio->behind_bvecs) {
 			struct bio_vec *bvec;
 			int j;
-- 
2.7.4

[PATCH 4/4] md: fast clone bio in bio_clone_mddev()

[Ming Lei]

Firstly bio_clone_mddev() is used in raid normal I/O and isn't
in resync I/O path.

Secondly all the direct access to bvec table in raid happens on
resync I/O except for write behind of raid1, in which we still
use bio_clone() for allocating new bvec table.

So this patch replaces bio_clone() with bio_clone_fast()
in bio_clone_mddev().

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/md.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 704be11355a9..7d176f025add 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -193,10 +193,14 @@ EXPORT_SYMBOL_GPL(bio_alloc_mddev);
 struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
 			    struct mddev *mddev)
 {
+	struct bio_set *bs;
+
 	if (!mddev || !mddev->bio_set)
-		return bio_clone(bio, gfp_mask);
+		bs = fs_bio_set;
+	else
+		bs = mddev->bio_set;
 
-	return bio_clone_bioset(bio, gfp_mask, mddev->bio_set);
+	return bio_clone_fast(bio, gfp_mask, bs);
 }
 EXPORT_SYMBOL_GPL(bio_clone_mddev);
 
-- 
2.7.4

Re: [PATCH 4/4] md: fast clone bio in bio_clone_mddev()

[Christoph Hellwig]

On Sun, Feb 05, 2017 at 02:22:13PM +0800, Ming Lei wrote:
> Firstly bio_clone_mddev() is used in raid normal I/O and isn't
> in resync I/O path.
> 
> Secondly all the direct access to bvec table in raid happens on
> resync I/O except for write behind of raid1, in which we still
> use bio_clone() for allocating new bvec table.
> 
> So this patch replaces bio_clone() with bio_clone_fast()
> in bio_clone_mddev().

Having the _fast in the name would be really useful for the
reader.  And as far as I can tell in the callers mddev is never
NULL and neither is ->bio_set, so replacing bio_clone_mddev with
raw calls to bio_clone_fast would be my preference.

Re: [PATCH 4/4] md: fast clone bio in bio_clone_mddev()

[Ming Lei]

On Mon, Feb 6, 2017 at 4:54 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, Feb 05, 2017 at 02:22:13PM +0800, Ming Lei wrote:
>> Firstly bio_clone_mddev() is used in raid normal I/O and isn't
>> in resync I/O path.
>>
>> Secondly all the direct access to bvec table in raid happens on
>> resync I/O except for write behind of raid1, in which we still
>> use bio_clone() for allocating new bvec table.
>>
>> So this patch replaces bio_clone() with bio_clone_fast()
>> in bio_clone_mddev().
>
> Having the _fast in the name would be really useful for the
> reader.  And as far as I can tell in the callers mddev is never
> NULL and neither is ->bio_set, so replacing bio_clone_mddev with

In theory, ->bio_set still might be NULL in case of failed memory allocation,
please see md_run().

> raw calls to bio_clone_fast would be my preference.



Thanks,
Ming Lei

Re: [PATCH 4/4] md: fast clone bio in bio_clone_mddev()

[Ming Lei]

On Mon, Feb 6, 2017 at 4:54 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, Feb 05, 2017 at 02:22:13PM +0800, Ming Lei wrote:
>> Firstly bio_clone_mddev() is used in raid normal I/O and isn't
>> in resync I/O path.
>>
>> Secondly all the direct access to bvec table in raid happens on
>> resync I/O except for write behind of raid1, in which we still
>> use bio_clone() for allocating new bvec table.
>>
>> So this patch replaces bio_clone() with bio_clone_fast()
>> in bio_clone_mddev().
>
> Having the _fast in the name would be really useful for the
> reader.  And as far as I can tell in the callers mddev is never
> NULL and neither is ->bio_set, so replacing bio_clone_mddev with

In theory, ->bio_set still might be NULL in case of failed memory allocation,
please see md_run().

> raw calls to bio_clone_fast would be my preference.



Thanks,
Ming Lei

Re: [PATCH 4/4] md: fast clone bio in bio_clone_mddev()

[Christoph Hellwig]

On Mon, Feb 06, 2017 at 06:43:32PM +0800, Ming Lei wrote:
> In theory, ->bio_set still might be NULL in case of failed memory allocation,
> please see md_run().

And that's something that should be fixed.  Silently not having mempool
is very bad behavior.

Re: [PATCH 4/4] md: fast clone bio in bio_clone_mddev()

[Christoph Hellwig]

On Mon, Feb 06, 2017 at 06:43:32PM +0800, Ming Lei wrote:
> In theory, ->bio_set still might be NULL in case of failed memory allocation,
> please see md_run().

And that's something that should be fixed.  Silently not having mempool
is very bad behavior.