* [PATCH 1/6] block: manage bio slab cache by xarray
2020-12-30 0:32 [PATCH 0/6] block: improvement on bioset & bvec allocation Ming Lei
@ 2020-12-30 0:32 ` Ming Lei
2021-01-04 8:54 ` Christoph Hellwig
2020-12-30 0:32 ` [PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split Ming Lei
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2020-12-30 0:32 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, linux-kernel, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/6] block: manage bio slab cache by xarray
2020-12-30 0:32 ` [PATCH 1/6] block: manage bio slab cache by xarray Ming Lei
@ 2021-01-04 8:54 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-01-04 8:54 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig, linux-kernel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split
2020-12-30 0:32 [PATCH 0/6] block: improvement on bioset & bvec allocation Ming Lei
2020-12-30 0:32 ` [PATCH 1/6] block: manage bio slab cache by xarray Ming Lei
@ 2020-12-30 0:32 ` Ming Lei
2021-01-04 18:13 ` Christoph Hellwig
2020-12-30 0:32 ` [PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs Ming Lei
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2020-12-30 0:32 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, linux-kernel, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split
2020-12-30 0:32 ` [PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split Ming Lei
@ 2021-01-04 18:13 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-01-04 18:13 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig, linux-kernel
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs
2020-12-30 0:32 [PATCH 0/6] block: improvement on bioset & bvec allocation Ming Lei
2020-12-30 0:32 ` [PATCH 1/6] block: manage bio slab cache by xarray Ming Lei
2020-12-30 0:32 ` [PATCH 2/6] block: don't pass BIOSET_NEED_BVECS for q->bio_split Ming Lei
@ 2020-12-30 0:32 ` Ming Lei
2021-01-04 8:55 ` Christoph Hellwig
2020-12-30 0:32 ` [PATCH 4/6] block: set .bi_max_vecs as actual allocated vector number Ming Lei
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2020-12-30 0:32 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, linux-kernel, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs
2020-12-30 0:32 ` [PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs Ming Lei
@ 2021-01-04 8:55 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-01-04 8:55 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig, linux-kernel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/6] block: set .bi_max_vecs as actual allocated vector number
2020-12-30 0:32 [PATCH 0/6] block: improvement on bioset & bvec allocation Ming Lei
` (2 preceding siblings ...)
2020-12-30 0:32 ` [PATCH 3/6] block: don't allocate inline bvecs if this bioset needn't bvecs Ming Lei
@ 2020-12-30 0:32 ` Ming Lei
2021-01-04 8:55 ` Christoph Hellwig
2020-12-30 0:32 ` [PATCH 5/6] block: move three bvec helpers declaration into private helper Ming Lei
2020-12-30 0:32 ` [PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set' Ming Lei
5 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2020-12-30 0:32 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, linux-kernel, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 5/6] block: move three bvec helpers declaration into private helper
2020-12-30 0:32 [PATCH 0/6] block: improvement on bioset & bvec allocation Ming Lei
` (3 preceding siblings ...)
2020-12-30 0:32 ` [PATCH 4/6] block: set .bi_max_vecs as actual allocated vector number Ming Lei
@ 2020-12-30 0:32 ` Ming Lei
2021-01-04 8:56 ` Christoph Hellwig
2020-12-30 0:32 ` [PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set' Ming Lei
5 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2020-12-30 0:32 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, linux-kernel, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set'
2020-12-30 0:32 [PATCH 0/6] block: improvement on bioset & bvec allocation Ming Lei
` (4 preceding siblings ...)
2020-12-30 0:32 ` [PATCH 5/6] block: move three bvec helpers declaration into private helper Ming Lei
@ 2020-12-30 0:32 ` Ming Lei
2021-01-04 8:57 ` Christoph Hellwig
5 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2020-12-30 0:32 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, linux-kernel, Ming Lei,
linux-bcache, Coly Li
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set'
2020-12-30 0:32 ` [PATCH 6/6] bcache: don't pass BIOSET_NEED_BVECS for the 'bio_set' embedded in 'cache_set' Ming Lei
@ 2021-01-04 8:57 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-01-04 8:57 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, Christoph Hellwig, linux-kernel,
linux-bcache, Coly Li
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>
^ permalink raw reply [flat|nested] 13+ messages in thread