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

[Hannes Reinecke <hare@suse.de>]

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