[PATCH 0/4] dm: reduce memory overhead of DM devices

[PATCH 0/4] dm: reduce memory overhead of DM devices

[Junichi Nomura]

This series of patches reduce the memory overhead of device-mapper
device mainly by removing unused mempool for bio vecs.

DM creates per-device bioset to ensure forward progress under
low memory situation and bioset always includes mempool for bvec.

However, with the introduction of immutable bvec by Kent Overstreet,
dm core now uses bio_clone_fast for creating a clone bio, and no longer
needs bvecs for it.

For example, when you create 10,000 bio-based DM devices and 1,000
request-based DM devices, memory usage of biovec under no load is:
  # grep biovec /proc/slabinfo
  biovec-256        418068 418068   4096  ...
  biovec-128             0      0   2048  ...
  biovec-64              0      0   1024  ...
  biovec-16              0      0    256  ...

With this patch series applied, the usage becomes:
  # grep biovec /proc/slabinfo
  biovec-256           116    116   4096  ...
  biovec-128             0      0   2048  ...
  biovec-64              0      0   1024  ...
  biovec-16              0      0    256  ...

So 4096 * (418068 - 116) = 1.6GB of memory is saved in this example.

Jun'ichi Nomura (4):
  dm: remove nr_iovecs parameter from alloc_tio()
  block: use bio_clone_fast() in blk_rq_prep_clone()
  block: add bioset_nobvec_create()
  dm: use bioset_nobvec_create()

 block/bio.c         | 61 ++++++++++++++++++++++++++++++++++++++---------------
 block/blk-core.c    |  2 +-
 drivers/md/dm.c     | 15 +++++--------
 include/linux/bio.h |  1 +
 4 files changed, 51 insertions(+), 28 deletions(-)

-- 
1.9.3

--
Jun'ichi Nomura, NEC Corporation

[PATCH 1/4] dm: remove nr_iovecs parameter from alloc_tio()

[Junichi Nomura]

alloc_tio() allocates a bio for clone. It takes the number of
bvecs to allocate for the clone-bio.
However, with the introduction of bio_clone_fast() in v3.14,
we no longer need to allocate bvecs and nr_iovecs is always 0.

__clone_and_map_simple_bio() looks like passing non-zero
nr_iovecs, but its value is always within the range of
inline bvecs and no allocation actually happens.
If allocation happened, BUG_ON() in __bio_clone_fast() would
trigger.

This patch removes the nr_iovecs parameter from alloc_tio()
to prevent possible future mis-use of the interface.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 drivers/md/dm.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 32b958d..4210b3c 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1249,13 +1249,13 @@ static void clone_bio(struct dm_target_io *tio, struct bio *bio,
 }
 
 static struct dm_target_io *alloc_tio(struct clone_info *ci,
-				      struct dm_target *ti, int nr_iovecs,
+				      struct dm_target *ti,
 				      unsigned target_bio_nr)
 {
 	struct dm_target_io *tio;
 	struct bio *clone;
 
-	clone = bio_alloc_bioset(GFP_NOIO, nr_iovecs, ci->md->bs);
+	clone = bio_alloc_bioset(GFP_NOIO, 0, ci->md->bs);
 	tio = container_of(clone, struct dm_target_io, clone);
 
 	tio->io = ci->io;
@@ -1269,16 +1269,11 @@ static void __clone_and_map_simple_bio(struct clone_info *ci,
 				       struct dm_target *ti,
 				       unsigned target_bio_nr, unsigned *len)
 {
-	struct dm_target_io *tio = alloc_tio(ci, ti, ci->bio->bi_max_vecs, target_bio_nr);
+	struct dm_target_io *tio = alloc_tio(ci, ti, target_bio_nr);
 	struct bio *clone = &tio->clone;
 
 	tio->len_ptr = len;
 
-	/*
-	 * Discard requests require the bio's inline iovecs be initialized.
-	 * ci->bio->bi_max_vecs is BIO_INLINE_VECS anyway, for both flush
-	 * and discard, so no need for concern about wasted bvec allocations.
-	 */
 	 __bio_clone_fast(clone, ci->bio);
 	if (len)
 		bio_setup_sector(clone, ci->sector, *len);
@@ -1322,7 +1317,7 @@ static void __clone_and_map_data_bio(struct clone_info *ci, struct dm_target *ti
 		num_target_bios = ti->num_write_bios(ti, bio);
 
 	for (target_bio_nr = 0; target_bio_nr < num_target_bios; target_bio_nr++) {
-		tio = alloc_tio(ci, ti, 0, target_bio_nr);
+		tio = alloc_tio(ci, ti, target_bio_nr);
 		tio->len_ptr = len;
 		clone_bio(tio, bio, sector, *len);
 		__map_bio(tio);
-- 
1.9.3

Re: [PATCH 1/4] dm: remove nr_iovecs parameter from alloc_tio()

[Mike Snitzer]

On Fri, Oct 03 2014 at  7:55am -0400,
Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> alloc_tio() allocates a bio for clone. It takes the number of
> bvecs to allocate for the clone-bio.
> However, with the introduction of bio_clone_fast() in v3.14,
> we no longer need to allocate bvecs and nr_iovecs is always 0.
> 
> __clone_and_map_simple_bio() looks like passing non-zero
> nr_iovecs, but its value is always within the range of
> inline bvecs and no allocation actually happens.
> If allocation happened, BUG_ON() in __bio_clone_fast() would
> trigger.
> 
> This patch removes the nr_iovecs parameter from alloc_tio()
> to prevent possible future mis-use of the interface.
> 
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

I picked this patch up and staged in linux-dm.git's for-next, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=8d59090cdc0ea91ffc390f78d0406baf8f66ae38

[PATCH 2/4] block: use bio_clone_fast() in blk_rq_prep_clone()

[Junichi Nomura]

Request cloning clones bios in the request to track the completion
of each bio.
For that purpose, we can use bio_clone_fast() instead of bio_clone()
to avoid unnecessary allocation and copy of bvecs.

This patch reduces memory footprint of request-based device-mapper
(about 1-4KB for each request) and is a preparation for further
reduction of memory usage by removing unused bvec mempool.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 block/blk-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index bf930f4..a92f720 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2931,7 +2931,7 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
 	blk_rq_init(NULL, rq);
 
 	__rq_for_each_bio(bio_src, rq_src) {
-		bio = bio_clone_bioset(bio_src, gfp_mask, bs);
+		bio = bio_clone_fast(bio_src, gfp_mask, bs);
 		if (!bio)
 			goto free_and_out;
 
-- 
1.9.3

[PATCH 3/4] block: add bioset_nobvec_create()

[Junichi Nomura]

Users of bio_clone_fast() do not need its own bvecs.
Allocating bvec mempool for such users is waste of memory.

This variant allows them to create bioset without bvec mempool.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 block/bio.c         | 61 ++++++++++++++++++++++++++++++++++++++---------------
 include/linux/bio.h |  1 +
 2 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 3e6331d..98c5100 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -428,6 +428,9 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
 		front_pad = 0;
 		inline_vecs = nr_iovecs;
 	} else {
+		/* should not use nobvec bioset for nr_iovecs > 0 */
+		if (WARN_ON_ONCE(!bs->bvec_pool && nr_iovecs > 0))
+			return NULL;
 		/*
 		 * generic_make_request() converts recursion to iteration; this
 		 * means if we're running beneath it, any bios we allocate and
@@ -1900,20 +1903,9 @@ void bioset_free(struct bio_set *bs)
 }
 EXPORT_SYMBOL(bioset_free);
 
-/**
- * bioset_create  - Create a bio_set
- * @pool_size:	Number of bio and bio_vecs to cache in the mempool
- * @front_pad:	Number of bytes to allocate in front of the returned bio
- *
- * Description:
- *    Set up a bio_set to be used with @bio_alloc_bioset. Allows the caller
- *    to ask for a number of bytes to be allocated in front of the bio.
- *    Front pad allocation is useful for embedding the bio inside
- *    another structure, to avoid allocating extra data to go with the bio.
- *    Note that the bio must be embedded at the END of that structure always,
- *    or things will break badly.
- */
-struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
+static struct bio_set *__bioset_create(unsigned int pool_size,
+				       unsigned int front_pad,
+				       unsigned int create_bvec_pool)
 {
 	unsigned int back_pad = BIO_INLINE_VECS * sizeof(struct bio_vec);
 	struct bio_set *bs;
@@ -1938,9 +1930,11 @@ struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
 	if (!bs->bio_pool)
 		goto bad;
 
-	bs->bvec_pool = biovec_create_pool(pool_size);
-	if (!bs->bvec_pool)
-		goto bad;
+	if (create_bvec_pool) {
+		bs->bvec_pool = biovec_create_pool(pool_size);
+		if (!bs->bvec_pool)
+			goto bad;
+	}
 
 	bs->rescue_workqueue = alloc_workqueue("bioset", WQ_MEM_RECLAIM, 0);
 	if (!bs->rescue_workqueue)
@@ -1951,8 +1945,41 @@ bad:
 	bioset_free(bs);
 	return NULL;
 }
+
+/**
+ * bioset_create  - Create a bio_set
+ * @pool_size:	Number of bio and bio_vecs to cache in the mempool
+ * @front_pad:	Number of bytes to allocate in front of the returned bio
+ *
+ * Description:
+ *    Set up a bio_set to be used with @bio_alloc_bioset. Allows the caller
+ *    to ask for a number of bytes to be allocated in front of the bio.
+ *    Front pad allocation is useful for embedding the bio inside
+ *    another structure, to avoid allocating extra data to go with the bio.
+ *    Note that the bio must be embedded at the END of that structure always,
+ *    or things will break badly.
+ */
+struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
+{
+	return __bioset_create(pool_size, front_pad, 1);
+}
 EXPORT_SYMBOL(bioset_create);
 
+/**
+ * bioset_nobvec_create  - Create a bio_set without bio_vec mempool
+ * @pool_size:	Number of bio to cache in the mempool
+ * @front_pad:	Number of bytes to allocate in front of the returned bio
+ *
+ * Description:
+ *    Same functionality as bioset_create() except that mempool is not
+ *    created for bio_vecs. Saving some memory for bio_clone_fast() users.
+ */
+struct bio_set *bioset_nobvec_create(unsigned int pool_size, unsigned int front_pad)
+{
+	return __bioset_create(pool_size, front_pad, 0);
+}
+EXPORT_SYMBOL(bioset_nobvec_create);
+
 #ifdef CONFIG_BLK_CGROUP
 /**
  * bio_associate_current - associate a bio with %current
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b39e500..7d7b2a7 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -342,6 +342,7 @@ static inline struct bio *bio_next_split(struct bio *bio, int sectors,
 }
 
 extern struct bio_set *bioset_create(unsigned int, unsigned int);
+extern struct bio_set *bioset_nobvec_create(unsigned int, unsigned int);
 extern void bioset_free(struct bio_set *);
 extern mempool_t *biovec_create_pool(int pool_entries);
 
-- 
1.9.3

Re: [PATCH 3/4] block: add bioset_nobvec_create()

[Mike Snitzer]

On Fri, Oct 03 2014 at  7:55am -0400,
Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> Users of bio_clone_fast() do not need its own bvecs.
> Allocating bvec mempool for such users is waste of memory.
> 
> This variant allows them to create bioset without bvec mempool.
> 
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> ---
>  block/bio.c         | 61 ++++++++++++++++++++++++++++++++++++++---------------
>  include/linux/bio.h |  1 +
>  2 files changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 3e6331d..98c5100 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1900,20 +1903,9 @@ void bioset_free(struct bio_set *bs)
>  }
>  EXPORT_SYMBOL(bioset_free);
>  
> -/**
> - * bioset_create  - Create a bio_set
> - * @pool_size:	Number of bio and bio_vecs to cache in the mempool
> - * @front_pad:	Number of bytes to allocate in front of the returned bio
> - *
> - * Description:
> - *    Set up a bio_set to be used with @bio_alloc_bioset. Allows the caller
> - *    to ask for a number of bytes to be allocated in front of the bio.
> - *    Front pad allocation is useful for embedding the bio inside
> - *    another structure, to avoid allocating extra data to go with the bio.
> - *    Note that the bio must be embedded at the END of that structure always,
> - *    or things will break badly.
> - */
> -struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
> +static struct bio_set *__bioset_create(unsigned int pool_size,
> +				       unsigned int front_pad,
> +				       unsigned int create_bvec_pool)
>  {
>  	unsigned int back_pad = BIO_INLINE_VECS * sizeof(struct bio_vec);
>  	struct bio_set *bs;

Any reason not to use bool for create_bvec_pool?

> @@ -1951,8 +1945,41 @@ bad:
>  	bioset_free(bs);
>  	return NULL;
>  }
> +
> +/**
> + * bioset_create  - Create a bio_set
> + * @pool_size:	Number of bio and bio_vecs to cache in the mempool
> + * @front_pad:	Number of bytes to allocate in front of the returned bio
> + *
> + * Description:
> + *    Set up a bio_set to be used with @bio_alloc_bioset. Allows the caller
> + *    to ask for a number of bytes to be allocated in front of the bio.
> + *    Front pad allocation is useful for embedding the bio inside
> + *    another structure, to avoid allocating extra data to go with the bio.
> + *    Note that the bio must be embedded at the END of that structure always,
> + *    or things will break badly.
> + */
> +struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
> +{
> +	return __bioset_create(pool_size, front_pad, 1);
> +}
>  EXPORT_SYMBOL(bioset_create);
>  
> +/**
> + * bioset_nobvec_create  - Create a bio_set without bio_vec mempool
> + * @pool_size:	Number of bio to cache in the mempool
> + * @front_pad:	Number of bytes to allocate in front of the returned bio
> + *
> + * Description:
> + *    Same functionality as bioset_create() except that mempool is not
> + *    created for bio_vecs. Saving some memory for bio_clone_fast() users.
> + */
> +struct bio_set *bioset_nobvec_create(unsigned int pool_size, unsigned int front_pad)
> +{
> +	return __bioset_create(pool_size, front_pad, 0);
> +}
> +EXPORT_SYMBOL(bioset_nobvec_create);
> +
>  #ifdef CONFIG_BLK_CGROUP
>  /**
>   * bio_associate_current - associate a bio with %current

And use true and false accordingly...

> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index b39e500..7d7b2a7 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -342,6 +342,7 @@ static inline struct bio *bio_next_split(struct bio *bio, int sectors,
>  }
>  
>  extern struct bio_set *bioset_create(unsigned int, unsigned int);
> +extern struct bio_set *bioset_nobvec_create(unsigned int, unsigned int);
>  extern void bioset_free(struct bio_set *);
>  extern mempool_t *biovec_create_pool(int pool_entries);

Also, I prefer the function name: bioset_create_nobvec (to keep it in
the "bioset_create" family).

Otherwise, please feel free to add to v2:

Acked-by: Mike Snitzer <snitzer@redhat.com>

[PATCH 4/4] dm: use bioset_nobvec_create()

[Junichi Nomura]

Since dm core uses bio_clone_fast() for both bio-based and request-based
DM device, there is no need for bvec mempool.

With this patch, on arch with 4KB page for example, memory usage will be
reduced by 64KB for each bio-based DM device and 1MB for each
request-based DM device.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 drivers/md/dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4210b3c..7160853 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2895,7 +2895,7 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity, u
 	if (!pools->io_pool)
 		goto out;
 
-	pools->bs = bioset_create(pool_size, front_pad);
+	pools->bs = bioset_nobvec_create(pool_size, front_pad);
 	if (!pools->bs)
 		goto out;
 
-- 
1.9.3

Re: [PATCH 0/4] dm: reduce memory overhead of DM devices

[Mike Snitzer]

On Fri, Oct 03 2014 at  7:48am -0400,
Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> This series of patches reduce the memory overhead of device-mapper
> device mainly by removing unused mempool for bio vecs.
> 
> DM creates per-device bioset to ensure forward progress under
> low memory situation and bioset always includes mempool for bvec.
> 
> However, with the introduction of immutable bvec by Kent Overstreet,
> dm core now uses bio_clone_fast for creating a clone bio, and no longer
> needs bvecs for it.
> 
> For example, when you create 10,000 bio-based DM devices and 1,000
> request-based DM devices, memory usage of biovec under no load is:
>   # grep biovec /proc/slabinfo
>   biovec-256        418068 418068   4096  ...
>   biovec-128             0      0   2048  ...
>   biovec-64              0      0   1024  ...
>   biovec-16              0      0    256  ...
> 
> With this patch series applied, the usage becomes:
>   # grep biovec /proc/slabinfo
>   biovec-256           116    116   4096  ...
>   biovec-128             0      0   2048  ...
>   biovec-64              0      0   1024  ...
>   biovec-16              0      0    256  ...
> 
> So 4096 * (418068 - 116) = 1.6GB of memory is saved in this example.

Very nice Junichi!

Kent definitely left us some low hanging fruit that offers a huge win
(and fixes the really long-standing problem of excessive memory reserves
for each request-based DM device!)

Jens, this is obviously an awkward patchset given it touches both block
and DM (block more so than DM).  I'd be fine with you pulling it into
block (or vice-versa if you're OK with me staging it for 3.18 via
linux-dm.git with your Acked-by).

Either way, this is all to say, once my feedback to patch 3 is taken
into account, for the set:

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 0/4] dm: reduce memory overhead of DM devices

[Mike Snitzer]

On Fri, Oct 03 2014 at  9:23am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Oct 03 2014 at  7:48am -0400,
> Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
> 
> > This series of patches reduce the memory overhead of device-mapper
> > device mainly by removing unused mempool for bio vecs.
> > 
> > DM creates per-device bioset to ensure forward progress under
> > low memory situation and bioset always includes mempool for bvec.
> > 
> > However, with the introduction of immutable bvec by Kent Overstreet,
> > dm core now uses bio_clone_fast for creating a clone bio, and no longer
> > needs bvecs for it.
> > 
> > For example, when you create 10,000 bio-based DM devices and 1,000
> > request-based DM devices, memory usage of biovec under no load is:
> >   # grep biovec /proc/slabinfo
> >   biovec-256        418068 418068   4096  ...
> >   biovec-128             0      0   2048  ...
> >   biovec-64              0      0   1024  ...
> >   biovec-16              0      0    256  ...
> > 
> > With this patch series applied, the usage becomes:
> >   # grep biovec /proc/slabinfo
> >   biovec-256           116    116   4096  ...
> >   biovec-128             0      0   2048  ...
> >   biovec-64              0      0   1024  ...
> >   biovec-16              0      0    256  ...
> > 
> > So 4096 * (418068 - 116) = 1.6GB of memory is saved in this example.
> 
> Very nice Junichi!
> 
> Kent definitely left us some low hanging fruit that offers a huge win
> (and fixes the really long-standing problem of excessive memory reserves
> for each request-based DM device!)
> 
> Jens, this is obviously an awkward patchset given it touches both block
> and DM (block more so than DM).  I'd be fine with you pulling it into
> block (or vice-versa if you're OK with me staging it for 3.18 via
> linux-dm.git with your Acked-by).

I picked up this patchset, refactored the patches based on my earlier
suggested s/bioset_nobvec_create/bioset_create_nobvec/ rename and
tweaked headers a bit.  I've staged the result in the for-next branch of
linux-dm.git for v3.18 inclusion, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

b83bb63 dm: remove nr_iovecs parameter from alloc_tio()
b875215 block: use bio_clone_fast() in blk_rq_prep_clone()
cd595c9 block: add bioset_create_nobvec()
3611594 dm: use bioset_create_nobvec()

Jens, I can easily yield this patchset to you if you care.  Or I can
backfill your Acked-by in the above 2 block commits (I obviously won't
just send block changes upstream without your OK!).

Please advise, thanks!
Mike

Re: [PATCH 0/4] dm: reduce memory overhead of DM devices

[Jens Axboe]

On 2014-10-03 14:48, Mike Snitzer wrote:
> On Fri, Oct 03 2014 at  9:23am -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
>
>> On Fri, Oct 03 2014 at  7:48am -0400,
>> Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
>>
>>> This series of patches reduce the memory overhead of device-mapper
>>> device mainly by removing unused mempool for bio vecs.
>>>
>>> DM creates per-device bioset to ensure forward progress under
>>> low memory situation and bioset always includes mempool for bvec.
>>>
>>> However, with the introduction of immutable bvec by Kent Overstreet,
>>> dm core now uses bio_clone_fast for creating a clone bio, and no longer
>>> needs bvecs for it.
>>>
>>> For example, when you create 10,000 bio-based DM devices and 1,000
>>> request-based DM devices, memory usage of biovec under no load is:
>>>    # grep biovec /proc/slabinfo
>>>    biovec-256        418068 418068   4096  ...
>>>    biovec-128             0      0   2048  ...
>>>    biovec-64              0      0   1024  ...
>>>    biovec-16              0      0    256  ...
>>>
>>> With this patch series applied, the usage becomes:
>>>    # grep biovec /proc/slabinfo
>>>    biovec-256           116    116   4096  ...
>>>    biovec-128             0      0   2048  ...
>>>    biovec-64              0      0   1024  ...
>>>    biovec-16              0      0    256  ...
>>>
>>> So 4096 * (418068 - 116) = 1.6GB of memory is saved in this example.
>>
>> Very nice Junichi!
>>
>> Kent definitely left us some low hanging fruit that offers a huge win
>> (and fixes the really long-standing problem of excessive memory reserves
>> for each request-based DM device!)
>>
>> Jens, this is obviously an awkward patchset given it touches both block
>> and DM (block more so than DM).  I'd be fine with you pulling it into
>> block (or vice-versa if you're OK with me staging it for 3.18 via
>> linux-dm.git with your Acked-by).
>
> I picked up this patchset, refactored the patches based on my earlier
> suggested s/bioset_nobvec_create/bioset_create_nobvec/ rename and
> tweaked headers a bit.  I've staged the result in the for-next branch of
> linux-dm.git for v3.18 inclusion, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next
>
> b83bb63 dm: remove nr_iovecs parameter from alloc_tio()
> b875215 block: use bio_clone_fast() in blk_rq_prep_clone()
> cd595c9 block: add bioset_create_nobvec()
> 3611594 dm: use bioset_create_nobvec()
>
> Jens, I can easily yield this patchset to you if you care.  Or I can
> backfill your Acked-by in the above 2 block commits (I obviously won't
> just send block changes upstream without your OK!).

My only worry is the switch to fast clones, apart from that, it's pretty 
basic. But we are pretty close to the 3.18 merge window, and as such, 
I'd prefer to let it simmer for a release.

-- 
Jens Axboe

Re: [PATCH 0/4] dm: reduce memory overhead of DM devices

[Mike Snitzer]

On Fri, Oct 03 2014 at  4:57pm -0400,
Jens Axboe <axboe@kernel.dk> wrote:

> >Jens, I can easily yield this patchset to you if you care.  Or I can
> >backfill your Acked-by in the above 2 block commits (I obviously won't
> >just send block changes upstream without your OK!).
> 
> My only worry is the switch to fast clones, apart from that, it's
> pretty basic. But we are pretty close to the 3.18 merge window, and
> as such, I'd prefer to let it simmer for a release.

Fine with me.  fast clones is all we need (thanks to Kent's immutable
biovec work), but waiting until 3.19 will give us plenty of time to
verify all is well.

[PATCH v2 1/4] dm: remove nr_iovecs parameter from alloc_tio()

[Mike Snitzer]

From: Junichi Nomura <j-nomura@ce.jp.nec.com>

alloc_tio() uses bio_alloc_bioset() to allocate a clone-bio for a bio.
alloc_tio() takes the number of bvecs to allocate for the clone-bio.
However, with v3.14's immutable biovec changes DM now uses
__bio_clone_fast() and no longer needs to allocate bvecs.

In practice, the 'nr_iovecs' passed to alloc_tio() is always effectively
0.  __clone_and_map_simple_bio() looked like it was passing non-zero
nr_iovecs, but its value was always within the range of inline bvecs and
no allocation actually happened.  If allocation happened, the BUG_ON() in
__bio_clone_fast() would've triggered.

Remove the nr_iovecs parameter from alloc_tio() to prevent possible
future bio_alloc_bioset() mis-use of a new bioset interface that will no
longer allow bvecs to be allocated.

Also fix extra whitespace before the __bio_clone_fast() call in
__clone_and_map_simple_bio().

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index e1db62c..aaeed99 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1372,13 +1372,13 @@ static void clone_bio(struct dm_target_io *tio, struct bio *bio,
 }
 
 static struct dm_target_io *alloc_tio(struct clone_info *ci,
-				      struct dm_target *ti, int nr_iovecs,
+				      struct dm_target *ti,
 				      unsigned target_bio_nr)
 {
 	struct dm_target_io *tio;
 	struct bio *clone;
 
-	clone = bio_alloc_bioset(GFP_NOIO, nr_iovecs, ci->md->bs);
+	clone = bio_alloc_bioset(GFP_NOIO, 0, ci->md->bs);
 	tio = container_of(clone, struct dm_target_io, clone);
 
 	tio->io = ci->io;
@@ -1392,17 +1392,12 @@ static void __clone_and_map_simple_bio(struct clone_info *ci,
 				       struct dm_target *ti,
 				       unsigned target_bio_nr, unsigned *len)
 {
-	struct dm_target_io *tio = alloc_tio(ci, ti, ci->bio->bi_max_vecs, target_bio_nr);
+	struct dm_target_io *tio = alloc_tio(ci, ti, target_bio_nr);
 	struct bio *clone = &tio->clone;
 
 	tio->len_ptr = len;
 
-	/*
-	 * Discard requests require the bio's inline iovecs be initialized.
-	 * ci->bio->bi_max_vecs is BIO_INLINE_VECS anyway, for both flush
-	 * and discard, so no need for concern about wasted bvec allocations.
-	 */
-	 __bio_clone_fast(clone, ci->bio);
+	__bio_clone_fast(clone, ci->bio);
 	if (len)
 		bio_setup_sector(clone, ci->sector, *len);
 
@@ -1445,7 +1440,7 @@ static void __clone_and_map_data_bio(struct clone_info *ci, struct dm_target *ti
 		num_target_bios = ti->num_write_bios(ti, bio);
 
 	for (target_bio_nr = 0; target_bio_nr < num_target_bios; target_bio_nr++) {
-		tio = alloc_tio(ci, ti, 0, target_bio_nr);
+		tio = alloc_tio(ci, ti, target_bio_nr);
 		tio->len_ptr = len;
 		clone_bio(tio, bio, sector, *len);
 		__map_bio(tio);
-- 
1.9.3

[PATCH v2 2/4] block: use bio_clone_fast() in blk_rq_prep_clone()

[Mike Snitzer]

From: Junichi Nomura <j-nomura@ce.jp.nec.com>

Request cloning clones bios in the request to track the completion
of each bio.
For that purpose, we can use bio_clone_fast() instead of bio_clone()
to avoid unnecessary allocation and copy of bvecs.

This patch reduces memory footprint of request-based device-mapper
(about 1-4KB for each request) and is a preparation for further
reduction of memory usage by removing unused bvec mempool.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 block/blk-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index bf930f4..a92f720 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2931,7 +2931,7 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
 	blk_rq_init(NULL, rq);
 
 	__rq_for_each_bio(bio_src, rq_src) {
-		bio = bio_clone_bioset(bio_src, gfp_mask, bs);
+		bio = bio_clone_fast(bio_src, gfp_mask, bs);
 		if (!bio)
 			goto free_and_out;
 
-- 
1.9.3

[PATCH v2 3/4] block: add bioset_create_nobvec()

[Mike Snitzer]

From: Junichi Nomura <j-nomura@ce.jp.nec.com>

Users of bio_clone_fast() do not want bios with their own bvecs.
Allocating a bvec mempool as part of the bioset intended for such users
is a waste of memory.

bioset_create_nobvec() creates a bioset that doesn't have the bvec
mempool.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 block/bio.c         | 61 ++++++++++++++++++++++++++++++++++++++---------------
 include/linux/bio.h |  1 +
 2 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 3e6331d..3e6e198 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -428,6 +428,9 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
 		front_pad = 0;
 		inline_vecs = nr_iovecs;
 	} else {
+		/* should not use nobvec bioset for nr_iovecs > 0 */
+		if (WARN_ON_ONCE(!bs->bvec_pool && nr_iovecs > 0))
+			return NULL;
 		/*
 		 * generic_make_request() converts recursion to iteration; this
 		 * means if we're running beneath it, any bios we allocate and
@@ -1900,20 +1903,9 @@ void bioset_free(struct bio_set *bs)
 }
 EXPORT_SYMBOL(bioset_free);
 
-/**
- * bioset_create  - Create a bio_set
- * @pool_size:	Number of bio and bio_vecs to cache in the mempool
- * @front_pad:	Number of bytes to allocate in front of the returned bio
- *
- * Description:
- *    Set up a bio_set to be used with @bio_alloc_bioset. Allows the caller
- *    to ask for a number of bytes to be allocated in front of the bio.
- *    Front pad allocation is useful for embedding the bio inside
- *    another structure, to avoid allocating extra data to go with the bio.
- *    Note that the bio must be embedded at the END of that structure always,
- *    or things will break badly.
- */
-struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
+static struct bio_set *__bioset_create(unsigned int pool_size,
+				       unsigned int front_pad,
+				       bool create_bvec_pool)
 {
 	unsigned int back_pad = BIO_INLINE_VECS * sizeof(struct bio_vec);
 	struct bio_set *bs;
@@ -1938,9 +1930,11 @@ struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
 	if (!bs->bio_pool)
 		goto bad;
 
-	bs->bvec_pool = biovec_create_pool(pool_size);
-	if (!bs->bvec_pool)
-		goto bad;
+	if (create_bvec_pool) {
+		bs->bvec_pool = biovec_create_pool(pool_size);
+		if (!bs->bvec_pool)
+			goto bad;
+	}
 
 	bs->rescue_workqueue = alloc_workqueue("bioset", WQ_MEM_RECLAIM, 0);
 	if (!bs->rescue_workqueue)
@@ -1951,8 +1945,41 @@ bad:
 	bioset_free(bs);
 	return NULL;
 }
+
+/**
+ * bioset_create  - Create a bio_set
+ * @pool_size:	Number of bio and bio_vecs to cache in the mempool
+ * @front_pad:	Number of bytes to allocate in front of the returned bio
+ *
+ * Description:
+ *    Set up a bio_set to be used with @bio_alloc_bioset. Allows the caller
+ *    to ask for a number of bytes to be allocated in front of the bio.
+ *    Front pad allocation is useful for embedding the bio inside
+ *    another structure, to avoid allocating extra data to go with the bio.
+ *    Note that the bio must be embedded at the END of that structure always,
+ *    or things will break badly.
+ */
+struct bio_set *bioset_create(unsigned int pool_size, unsigned int front_pad)
+{
+	return __bioset_create(pool_size, front_pad, true);
+}
 EXPORT_SYMBOL(bioset_create);
 
+/**
+ * bioset_create_nobvec  - Create a bio_set without bio_vec mempool
+ * @pool_size:	Number of bio to cache in the mempool
+ * @front_pad:	Number of bytes to allocate in front of the returned bio
+ *
+ * Description:
+ *    Same functionality as bioset_create() except that mempool is not
+ *    created for bio_vecs. Saving some memory for bio_clone_fast() users.
+ */
+struct bio_set *bioset_create_nobvec(unsigned int pool_size, unsigned int front_pad)
+{
+	return __bioset_create(pool_size, front_pad, false);
+}
+EXPORT_SYMBOL(bioset_create_nobvec);
+
 #ifdef CONFIG_BLK_CGROUP
 /**
  * bio_associate_current - associate a bio with %current
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b39e500..0fd9c96 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -342,6 +342,7 @@ static inline struct bio *bio_next_split(struct bio *bio, int sectors,
 }
 
 extern struct bio_set *bioset_create(unsigned int, unsigned int);
+extern struct bio_set *bioset_create_nobvec(unsigned int, unsigned int);
 extern void bioset_free(struct bio_set *);
 extern mempool_t *biovec_create_pool(int pool_entries);
 
-- 
1.9.3

[PATCH v2 4/4] dm: use bioset_create_nobvec()

[Mike Snitzer]

From: Junichi Nomura <j-nomura@ce.jp.nec.com>

Since DM core uses bio_clone_fast() for both bio-based and request-based
DM devices there is no need for DM's bioset to have a bvec mempool.

With this patch, on arch with 4KB page for example, memory usage will be
reduced by 64KB for each bio-based DM device and 1MB for each
request-based DM device.

For example, when you create 10,000 bio-based DM devices and 1,000
request-based DM devices, memory usage of biovec under no load is:
  # grep biovec /proc/slabinfo

  biovec-256        418068 418068   4096  ...
  biovec-128             0      0   2048  ...
  biovec-64              0      0   1024  ...
  biovec-16              0      0    256  ...

With this patch series applied, the usage becomes:
  # grep biovec /proc/slabinfo

  biovec-256           116    116   4096  ...
  biovec-128             0      0   2048  ...
  biovec-64              0      0   1024  ...
  biovec-16              0      0    256  ...

So 4096 * (418068 - 116) = 1.6GB of memory is saved in this example.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index aaeed99..f78ac7f 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3021,7 +3021,7 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity, u
 	if (!pools->io_pool)
 		goto out;
 
-	pools->bs = bioset_create(pool_size, front_pad);
+	pools->bs = bioset_nobvec_create(pool_size, front_pad);
 	if (!pools->bs)
 		goto out;
 
-- 
1.9.3

Re: [PATCH v2 4/4] dm: use bioset_create_nobvec()

[Junichi Nomura]

On 10/04/14 06:27, Mike Snitzer wrote:
> @@ -3021,7 +3021,7 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity, u
>  	if (!pools->io_pool)
>  		goto out;
>  
> -	pools->bs = bioset_create(pool_size, front_pad);
> +	pools->bs = bioset_nobvec_create(pool_size, front_pad);
>  	if (!pools->bs)
>  		goto out;

Hi Mike,

thank you for the rename. It's better.
But we have to s/nobvec_create/create_nobvec/ here, too.

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH v2 4/4] dm: use bioset_create_nobvec()

[Mike Snitzer]

On Sun, Oct 05 2014 at  7:06pm -0400,
Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> On 10/04/14 06:27, Mike Snitzer wrote:
> > @@ -3021,7 +3021,7 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity, u
> >  	if (!pools->io_pool)
> >  		goto out;
> >  
> > -	pools->bs = bioset_create(pool_size, front_pad);
> > +	pools->bs = bioset_nobvec_create(pool_size, front_pad);
> >  	if (!pools->bs)
> >  		goto out;
> 
> Hi Mike,
> 
> thank you for the rename. It's better.
> But we have to s/nobvec_create/create_nobvec/ here, too.

Oops, thanks for letting me know.  I've fixed it and rebased
linux-dm.git's for-next branch ontop of linux-block's for-3.18/core
since Jens staged your 2 block patches.

Re: [PATCH 0/4] dm: reduce memory overhead of DM devices

[Christoph Hellwig]

On Fri, Oct 03, 2014 at 11:48:08AM +0000, Junichi Nomura wrote:
> This series of patches reduce the memory overhead of device-mapper
> device mainly by removing unused mempool for bio vecs.
> 
> DM creates per-device bioset to ensure forward progress under
> low memory situation and bioset always includes mempool for bvec.

Thanks, this looks like a good step forward.

I'd still love to go all the way and avoid any bio cloning.  I can
see two ways to do this:

 1) similar to the original block layer multipathing with a callback
    that allows the submitter to hook into the completion path at
    blk_update_request time.
 2) by not completing the io chain in req_bio_endio, similar to how
    we do it for requests that are part of a flush sequence using
    the REQ_FLUSH_SEQ.

While the first seems like the cleaner architecture it might be worth
to look into the second version given that 1) wasn't well received
last time, and 2) already works nicely for a very similar use
case.  If we end up implementing 1) it should also be able to replace
the current flag based completion hack for the flush sequence.