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

Hannes Reinecke
2021-01-11T06:58:07Z

On 1/11/21 4:05 AM, Ming Lei wrote:
> 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 it becomes more readable than before.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> block/bio.c | 116 ++++++++++++++++++++++------------------------------
> 1 file changed, 49 insertions(+), 67 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 1f2cc1fbe283..cfa0e9db30e0 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,89 +59,80 @@ 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 kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
> +static struct bio_slab *create_bio_slab(unsigned int 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 = kzalloc(sizeof(*bslab), GFP_KERNEL);
>
> - mutex_lock(&bio_slab_lock);
> + if (!bslab)
> + return NULL;
>
> - i = 0;
> - while (i < bio_slab_nr) {
> - bslab = &bio_slabs[i];
> + 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)
> + goto fail_alloc_slab;
>
> - if (!bslab->slab && entry == -1)
> - entry = i;
> - else if (bslab->slab_size == sz) {
> - slab = bslab->slab;
> - bslab->slab_ref++;
> - break;
> - }
> - i++;
> - }
> + bslab->slab_ref = 1;
> + bslab->slab_size = size;
>

Why don't you use a kref here?

Otherwise:

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer