[PATCH 0/6] block: improvement on bioset & bvec allocation

[PATCH 0/6] block: improvement on bioset & bvec allocation

[Ming Lei]

Hello,

All are bioset / bvec improvement, and most of them are quite
straightforward.


Ming Lei (6):
  block: manage bio slab cache by xarray
  block: don't pass BIOSET_NEED_BVECS for q->bio_split
  block: don't allocate inline bvecs if this bioset needn't bvecs
  block: set .bi_max_vecs as actual allocated vector number
  block: move three bvec helpers declaration into private helper
  bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in
    'cache_set'

 block/bio.c               | 116 ++++++++++++++++----------------------
 block/blk-core.c          |   2 +-
 block/blk.h               |   4 ++
 drivers/md/bcache/super.c |   2 +-
 include/linux/bio.h       |   4 +-
 5 files changed, 57 insertions(+), 71 deletions(-)

-- 
2.28.0

[PATCH 1/6] block: manage bio slab cache by xarray

[Ming Lei]

Managing bio slab cache via xarray by using slab cache size as xarray index, and
storing 'struct bio_slab' instance into xarray.

So code is simplified a lot, meantime is is more readable than before.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio.c | 104 +++++++++++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 62 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 1f2cc1fbe283..aa657cdd7c8c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -19,6 +19,7 @@
 #include <linux/highmem.h>
 #include <linux/sched/sysctl.h>
 #include <linux/blk-crypto.h>
+#include <linux/xarray.h>
 
 #include <trace/events/block.h>
 #include "blk.h"
@@ -58,61 +59,49 @@ struct bio_slab {
 	char name[8];
 };
 static DEFINE_MUTEX(bio_slab_lock);
-static struct bio_slab *bio_slabs;
-static unsigned int bio_slab_nr, bio_slab_max;
+static DEFINE_XARRAY(bio_slabs);
+
+static struct bio_slab *create_bio_slab(unsigned int size)
+{
+	struct bio_slab *bslab = kzalloc(sizeof(*bslab), GFP_KERNEL);
+	if (!bslab)
+		return NULL;
+
+	snprintf(bslab->name, sizeof(bslab->name), "bio-%d", size);
+	bslab->slab = kmem_cache_create(bslab->name, size,
+			ARCH_KMALLOC_MINALIGN, SLAB_HWCACHE_ALIGN, NULL);
+	if (bslab->slab) {
+		bslab->slab_ref = 1;
+		bslab->slab_size = size;
+	} else {
+		kfree(bslab);
+		bslab = NULL;
+	}
+	return bslab;
+}
 
 static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
 {
 	unsigned int sz = sizeof(struct bio) + extra_size;
 	struct kmem_cache *slab = NULL;
-	struct bio_slab *bslab, *new_bio_slabs;
-	unsigned int new_bio_slab_max;
-	unsigned int i, entry = -1;
+	struct bio_slab *bslab;
 
 	mutex_lock(&bio_slab_lock);
-
-	i = 0;
-	while (i < bio_slab_nr) {
-		bslab = &bio_slabs[i];
-
-		if (!bslab->slab && entry == -1)
-			entry = i;
-		else if (bslab->slab_size == sz) {
+	bslab = xa_load(&bio_slabs, sz);
+	if (bslab) {
+		slab = bslab->slab;
+		bslab->slab_ref++;
+	} else {
+		bslab = create_bio_slab(sz);
+		if(bslab && !xa_err(xa_store(&bio_slabs, sz, bslab,
+						GFP_KERNEL)))
 			slab = bslab->slab;
-			bslab->slab_ref++;
-			break;
+		else {
+			if (bslab)
+				kmem_cache_destroy(bslab->slab);
+			kfree(bslab);
 		}
-		i++;
-	}
-
-	if (slab)
-		goto out_unlock;
-
-	if (bio_slab_nr == bio_slab_max && entry == -1) {
-		new_bio_slab_max = bio_slab_max << 1;
-		new_bio_slabs = krealloc(bio_slabs,
-					 new_bio_slab_max * sizeof(struct bio_slab),
-					 GFP_KERNEL);
-		if (!new_bio_slabs)
-			goto out_unlock;
-		bio_slab_max = new_bio_slab_max;
-		bio_slabs = new_bio_slabs;
 	}
-	if (entry == -1)
-		entry = bio_slab_nr++;
-
-	bslab = &bio_slabs[entry];
-
-	snprintf(bslab->name, sizeof(bslab->name), "bio-%d", entry);
-	slab = kmem_cache_create(bslab->name, sz, ARCH_KMALLOC_MINALIGN,
-				 SLAB_HWCACHE_ALIGN, NULL);
-	if (!slab)
-		goto out_unlock;
-
-	bslab->slab = slab;
-	bslab->slab_ref = 1;
-	bslab->slab_size = sz;
-out_unlock:
 	mutex_unlock(&bio_slab_lock);
 	return slab;
 }
@@ -120,27 +109,26 @@ static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
 static void bio_put_slab(struct bio_set *bs)
 {
 	struct bio_slab *bslab = NULL;
-	unsigned int i;
+	unsigned int slab_size = bs->front_pad + sizeof(struct bio) +
+		BIO_INLINE_VECS * sizeof(struct bio_vec);
 
 	mutex_lock(&bio_slab_lock);
 
-	for (i = 0; i < bio_slab_nr; i++) {
-		if (bs->bio_slab == bio_slabs[i].slab) {
-			bslab = &bio_slabs[i];
-			break;
-		}
-	}
-
+	bslab = xa_load(&bio_slabs, slab_size);
 	if (WARN(!bslab, KERN_ERR "bio: unable to find slab!\n"))
 		goto out;
 
+	WARN_ON_ONCE(bslab->slab != bs->bio_slab);
+
 	WARN_ON(!bslab->slab_ref);
 
 	if (--bslab->slab_ref)
 		goto out;
 
+	xa_erase(&bio_slabs, slab_size);
+
 	kmem_cache_destroy(bslab->slab);
-	bslab->slab = NULL;
+	kfree(bslab);
 
 out:
 	mutex_unlock(&bio_slab_lock);
@@ -1651,16 +1639,8 @@ static void __init biovec_init_slabs(void)
 
 static int __init init_bio(void)
 {
-	bio_slab_max = 2;
-	bio_slab_nr = 0;
-	bio_slabs = kcalloc(bio_slab_max, sizeof(struct bio_slab),
-			    GFP_KERNEL);
-
 	BUILD_BUG_ON(BIO_FLAG_LAST > BVEC_POOL_OFFSET);
 
-	if (!bio_slabs)
-		panic("bio: can't allocate bios\n");
-
 	bio_integrity_init();
 	biovec_init_slabs();
 
-- 
2.28.0

[PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split

[Ming Lei]

q->bio_split is only used by bio_split() for fast cloning bio, and no
need to allocate bvecs, so remove this flag.

Signed-off-by: Ming Lei <ming.lei@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 96e5fcd7f071..3480ca037d3f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -530,7 +530,7 @@ struct request_queue *blk_alloc_queue(int node_id)
 	if (q->id < 0)
 		goto fail_q;
 
-	ret = bioset_init(&q->bio_split, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
+	ret = bioset_init(&q->bio_split, BIO_POOL_SIZE, 0, 0);
 	if (ret)
 		goto fail_id;
 
-- 
2.28.0

[PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs

[Ming Lei]

The inline bvecs won't be used if user needn't bvecs by not passing
BIOSET_NEED_BVECS, so don't allocate bvecs in this situation.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio.c         | 11 +++++++----
 include/linux/bio.h |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index aa657cdd7c8c..3991a5aab1bc 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -110,7 +110,7 @@ static void bio_put_slab(struct bio_set *bs)
 {
 	struct bio_slab *bslab = NULL;
 	unsigned int slab_size = bs->front_pad + sizeof(struct bio) +
-		BIO_INLINE_VECS * sizeof(struct bio_vec);
+		bs->back_pad;
 
 	mutex_lock(&bio_slab_lock);
 
@@ -1567,15 +1567,18 @@ int bioset_init(struct bio_set *bs,
 		unsigned int front_pad,
 		int flags)
 {
-	unsigned int back_pad = BIO_INLINE_VECS * sizeof(struct bio_vec);
-
 	bs->front_pad = front_pad;
 
+	if (flags & BIOSET_NEED_BVECS)
+		bs->back_pad = BIO_INLINE_VECS * sizeof(struct bio_vec);
+	else
+		bs->back_pad = 0;
+
 	spin_lock_init(&bs->rescue_lock);
 	bio_list_init(&bs->rescue_list);
 	INIT_WORK(&bs->rescue_work, bio_alloc_rescue);
 
-	bs->bio_slab = bio_find_or_create_slab(front_pad + back_pad);
+	bs->bio_slab = bio_find_or_create_slab(bs->front_pad + bs->back_pad);
 	if (!bs->bio_slab)
 		return -ENOMEM;
 
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1edda614f7ce..f606eb1e556f 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -703,6 +703,7 @@ struct bio_set {
 	mempool_t bvec_integrity_pool;
 #endif
 
+	unsigned int back_pad;
 	/*
 	 * Deadlock avoidance for stacking block drivers: see comments in
 	 * bio_alloc_bioset() for details
-- 
2.28.0

[PATCH 4/6] block: set .bi_max_vecs as actual allocated vector number

[Ming Lei]

bvec_alloc() may allocate more bio vectors than requested, so set .bi_max_vecs as
actual allocated vector number, instead of the requested number. This way can help
fs build bigger bio because new bio often won't be allocated until the current one
becomes full.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index 3991a5aab1bc..e567feb380b6 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -502,12 +502,13 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
 			goto err_free;
 
 		bio->bi_flags |= idx << BVEC_POOL_OFFSET;
+		bio->bi_max_vecs = bvec_nr_vecs(idx);
 	} else if (nr_iovecs) {
 		bvl = bio->bi_inline_vecs;
+		bio->bi_max_vecs = inline_vecs;
 	}
 
 	bio->bi_pool = bs;
-	bio->bi_max_vecs = nr_iovecs;
 	bio->bi_io_vec = bvl;
 	return bio;
 
-- 
2.28.0

[PATCH 5/6] block: move three bvec helpers declaration into private helper

[Ming Lei]

bvec_alloc(), bvec_free() and bvec_nr_vecs() are only used inside block
layer core functions, no need to declare them in public header.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk.h         | 4 ++++
 include/linux/bio.h | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/blk.h b/block/blk.h
index 7550364c326c..ac60f5024d6f 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -55,6 +55,10 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
 
 void blk_freeze_queue(struct request_queue *q);
 
+extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *);
+extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int);
+extern unsigned int bvec_nr_vecs(unsigned short idx);
+
 static inline bool biovec_phys_mergeable(struct request_queue *q,
 		struct bio_vec *vec1, struct bio_vec *vec2)
 {
diff --git a/include/linux/bio.h b/include/linux/bio.h
index f606eb1e556f..70914dd6a70d 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -478,9 +478,6 @@ static inline void zero_fill_bio(struct bio *bio)
 	zero_fill_bio_iter(bio, bio->bi_iter);
 }
 
-extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *);
-extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int);
-extern unsigned int bvec_nr_vecs(unsigned short idx);
 extern const char *bio_devname(struct bio *bio, char *buffer);
 
 #define bio_set_dev(bio, bdev) 			\
-- 
2.28.0

[PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set'

[Ming Lei]

This bioset is just for allocating bio only from bio_next_split, and it needn't
bvecs, so remove the flag.

Cc: linux-bcache@vger.kernel.org
Cc: Coly Li <colyli@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/bcache/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 0e06d721cd8e..61c8aafc0a32 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1897,7 +1897,7 @@ struct cache_set *bch_cache_set_alloc(struct cache_sb *sb)
 		goto err;
 
 	if (bioset_init(&c->bio_split, 4, offsetof(struct bbio, bio),
-			BIOSET_NEED_BVECS|BIOSET_NEED_RESCUER))
+			BIOSET_NEED_RESCUER))
 		goto err;
 
 	c->uuids = alloc_meta_bucket_pages(GFP_KERNEL, sb);
-- 
2.28.0

Re: [PATCH 1/6] block: manage bio slab cache by xarray

[Christoph Hellwig]

On Wed, Dec 30, 2020 at 08:32:50AM +0800, Ming Lei wrote:
> Managing bio slab cache via xarray by using slab cache size as xarray index, and

Overly long line in the commit log above.

> +static struct bio_slab *create_bio_slab(unsigned int size)
> +{
> +	struct bio_slab *bslab = kzalloc(sizeof(*bslab), GFP_KERNEL);
> +	if (!bslab)
> +		return NULL;

Missing whitespace after the variable declaration.

> +
> +	snprintf(bslab->name, sizeof(bslab->name), "bio-%d", size);
> +	bslab->slab = kmem_cache_create(bslab->name, size,
> +			ARCH_KMALLOC_MINALIGN, SLAB_HWCACHE_ALIGN, NULL);
> +	if (bslab->slab) {
> +		bslab->slab_ref = 1;
> +		bslab->slab_size = size;
> +	} else {
> +		kfree(bslab);
> +		bslab = NULL;
> +	}
> +	return bslab;
> +}

I'd simply this to:

	if (!bslab->slab) {
		kfree(bslab);
		return NULL;
	}

	bslab->slab_ref = 1;
	bslab->slab_size = size;
	return bslab;
}

>  
>  static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
>  {
>  	unsigned int sz = sizeof(struct bio) + extra_size;
>  	struct kmem_cache *slab = NULL;
> +	struct bio_slab *bslab;
>  
>  	mutex_lock(&bio_slab_lock);
> +	bslab = xa_load(&bio_slabs, sz);
> +	if (bslab) {
> +		slab = bslab->slab;
> +		bslab->slab_ref++;
> +	} else {
> +		bslab = create_bio_slab(sz);
> +		if(bslab && !xa_err(xa_store(&bio_slabs, sz, bslab,
> +						GFP_KERNEL)))

Missing whitespace after the "if"

But more importantly, I'd expect the xa_store to go into create_bio_slab
to make the code a little more readable and to consolidate the error
handling.  Also we really shouldn't need the slab local variable in
this function.

>  static void bio_put_slab(struct bio_set *bs)
>  {
>  	struct bio_slab *bslab = NULL;
> +	unsigned int slab_size = bs->front_pad + sizeof(struct bio) +
> +		BIO_INLINE_VECS * sizeof(struct bio_vec);

This calculation would look nice factored out into a little helper with
a comment explaining it.

Re: [PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs

[Christoph Hellwig]

On Wed, Dec 30, 2020 at 08:32:52AM +0800, Ming Lei wrote:
> The inline bvecs won't be used if user needn't bvecs by not passing
> BIOSET_NEED_BVECS, so don't allocate bvecs in this situation.

Looks sensible.

> -	bs->bio_slab = bio_find_or_create_slab(front_pad + back_pad);
> +	bs->bio_slab = bio_find_or_create_slab(bs->front_pad + bs->back_pad);

Passing the bioset to bio_find_or_create_slab and returning an errno
might be a nicer calling convention now.

Re: [PATCH 4/6] block: set .bi_max_vecs as actual allocated vector number

[Christoph Hellwig]

On Wed, Dec 30, 2020 at 08:32:53AM +0800, Ming Lei wrote:
> bvec_alloc() may allocate more bio vectors than requested, so set .bi_max_vecs as
> actual allocated vector number, instead of the requested number. This way can help
> fs build bigger bio because new bio often won't be allocated until the current one
> becomes full.

Overly long lines above.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] block: move three bvec helpers declaration into private helper

[Christoph Hellwig]

> +extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *);
> +extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int);
> +extern unsigned int bvec_nr_vecs(unsigned short idx);

Please drop the externs while you're at it.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set'

[Christoph Hellwig]

On Wed, Dec 30, 2020 at 08:32:55AM +0800, Ming Lei wrote:
> This bioset is just for allocating bio only from bio_next_split, and it needn't
> bvecs, so remove the flag.

Overly long line above.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split

[Christoph Hellwig]

On Wed, Dec 30, 2020 at 08:32:51AM +0800, Ming Lei wrote:
> q->bio_split is only used by bio_split() for fast cloning bio, and no
> need to allocate bvecs, so remove this flag.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>