Re: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Minchan Kim <minchan@kernel.org>]

Hello Sergey,

I reviewed this patchset and I suggest somethings.
Please have a look and feedback to me. :)

1. Let's define new file zram_comp.c
2. zram_comp includes following field
   .create
   .compress
   .decompress.
   .destroy
   .name

1) create/destroy
Will set up necessary things like allocating buffer, lock init or other things
might happen in future when initialization time.
I have a plan to support multiple buffer to do compression/decompression in
parallel so we could optimize write VS write path, too.

2) compress/decompress

It's very clear.

3) name field will be used for tell to user what's kinds of compression
algorithm zram support.

On Fri, Jan 17, 2014 at 02:04:16PM +0300, Sergey Senozhatsky wrote:
> This is preparation patch to add LZ4 compression support.
> 
> struct zram_compress_ops defines common compress and decompress
> prototypes. Use these ops->compress and ops->decompress callbacks
> instead of direct LZO lzo1x_1_compress() and lzo1x_decompress_safe()
> calls.
> 
> Compressor ops should be defined before zram meta allocation,
> because the size of meta->compress_workmem depends on selected
> compression algorithm.
> 
> Define LZO compressor ops and set it as the only one available
> zram compressor at the moment.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 20 +++++++++++++-------
>  drivers/block/zram/zram_drv.h | 10 ++++++++++
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 7124042..4f2c748 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -28,10 +28,9 @@
>  #include <linux/device.h>
>  #include <linux/genhd.h>
>  #include <linux/highmem.h>
> -#include <linux/slab.h>
> -#include <linux/lzo.h>
>  #include <linux/string.h>
>  #include <linux/vmalloc.h>
> +#include <linux/lzo.h>

Let's include zram_comps.h only and zram_comps.h includes other compression
alrogithms header. If user don't want to support some compression
alrogithm, it shouldn't be included.
Of course, user can select kinds of compressor in configuration step.

>  
>  #include "zram_drv.h"
>  
> @@ -42,6 +41,12 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +static struct zram_compress_ops lzo_compressor = {
> +	.workmem_sz = LZO1X_MEM_COMPRESS,

workmem_sz should be part of compressor internal.

> +	.compress = lzo1x_1_compress,
> +	.decompress = lzo1x_decompress_safe,
> +};
> +
>  #define ZRAM_ATTR_RO(name)						\
>  static ssize_t zram_attr_##name##_show(struct device *d,		\
>  				struct device_attribute *attr, char *b)	\
> @@ -166,14 +171,14 @@ static void zram_meta_free(struct zram_meta *meta)
>  	kfree(meta);
>  }
>  
> -static struct zram_meta *zram_meta_alloc(u64 disksize)
> +static struct zram_meta *zram_meta_alloc(int workmem_sz, u64 disksize)
>  {
>  	size_t num_pages;
>  	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
>  	if (!meta)
>  		goto out;
>  
> -	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +	meta->compress_workmem = kzalloc(workmem_sz, GFP_KERNEL);

Instead of exposing compression internal stuff, let's call comp->create.

>  	if (!meta->compress_workmem)
>  		goto free_meta;
>  
> @@ -301,7 +306,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  	if (size == PAGE_SIZE)
>  		copy_page(mem, cmem);
>  	else
> -		ret = lzo1x_decompress_safe(cmem, size,	mem, &clen);
> +		ret = zram->ops->decompress(cmem, size, mem, &clen);
>  	zs_unmap_object(meta->mem_pool, handle);
>  	read_unlock(&meta->tb_lock);
>  
> @@ -420,7 +425,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		goto out;
>  	}
>  
> -	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
> +	ret = zram->ops->compress(uncmem, PAGE_SIZE, src, &clen,
>  			       meta->compress_workmem);

Ditto. compress_workmem should be part of compressor itself.


>  	if (!is_partial_io(bvec)) {
>  		kunmap_atomic(user_mem);
> @@ -551,7 +556,7 @@ static ssize_t disksize_store(struct device *dev,
>  	}
>  
>  	disksize = PAGE_ALIGN(disksize);
> -	zram->meta = zram_meta_alloc(disksize);
> +	zram->meta = zram_meta_alloc(zram->ops->workmem_sz, disksize);

So, we don't need zram->ops->workmem_sz.

>  
>  	zram->disksize = disksize;
>  	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> @@ -785,6 +790,7 @@ static int create_device(struct zram *zram, int device_id)
>  		goto out_free_disk;
>  	}
>  
> +	zram->ops = &lzo_compressor;

Let's define choose_compressor function with some argument
so the result is following as

zram->comp = choose_compressor("lzo");

>  	zram->meta = NULL;
>  	return 0;
>  
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 1d5b1f5..5488682 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -88,8 +88,18 @@ struct zram_meta {
>  	struct mutex buffer_lock; /* protect compress buffers */
>  };
>  
> +/* compression/decompression functions and algorithm-specific workmem size */
> +struct zram_compress_ops {
> +	int (*compress)(const unsigned char *src, size_t src_len,
> +			unsigned char *dst, size_t *dst_len, void *wrkmem);
> +	int (*decompress)(const unsigned char *src, size_t src_len,
> +			unsigned char *dst, size_t *dst_len);
> +	long workmem_sz;
> +};
> +
>  struct zram {
>  	struct zram_meta *meta;
> +	struct zram_compress_ops *ops;
>  	struct request_queue *queue;
>  	struct gendisk *disk;
>  	/* Prevent concurrent execution of device init, reset and R/W request */
> -- 
> 1.8.5.3.493.gb139ac2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim