Date:	Fri, 28 Feb 2014 20:40:00 +0300
From:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Minchan Kim <minchan@...nel.org>,
	Jerome Marchand <jmarchan@...hat.com>,
	Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv8 1/6] zram: introduce compressing backend abstraction

Hello Andrew,

On (02/27/14 15:18), Andrew Morton wrote:
> On Wed, 26 Feb 2014 15:27:54 +0300 Sergey Senozhatsky <sergey.senozhatsky@...il.com> wrote:
> 
> > ZRAM performs direct LZO compression algorithm calls, making it the one and
> > only option. Introduce compressing backend abstraction zcomp in order to
> > support multiple compression algorithms with the following set of operations:
> 
> Well...  why?  Presumably because other compression schemes have
> compelling advantages over LZO, otherwise there's no point in the
> patch!
> 
> Please describe this in some detail, so that others can judge the
> desirability of the patch.
> 

thanks for review.
all of your notes and concerns were addressed in v9 (will send shortly).

	-ss

> >         .create
> >         .destroy
> >         .compress
> >         .decompress
> > 
> > Schematically zram write() usually contains the following steps:
> > 0) preparation (decompression of partioal IO, etc.)
> > 1) lock buffer_lock mutex (protects meta compress buffers)
> > 2) compress (using meta compress buffers)
> > 3) alloc and map zs_pool object
> > 4) copy compressed data (from meta compress buffers) to object allocated by 3)
> > 5) free previous pool page, assign a new one
> > 6) unlock buffer_lock mutex
> > 
> > As we can see, compressing buffers must remain untouched from 1) to 4),
> > because, otherwise, concurrent write() can overwrite data. At the same time,
> > zram_meta must be aware of a) specific compression algorithm memory requirements
> > and b) necessary locking to protect compression buffers. To remove requirement
> > a) new struct zcomp_strm introduced, which contains a compress/decompress
> > `buffer' and compression algorithm `private' part. While struct zcomp implements
> > zcomp_strm stream handling and locking by means of get() and put() semantics and
> > removes requirement b) from zram meta. zcomp ->create() and ->destroy(),
> > respectively, allocate and deallocate algorithm specific zcomp_strm `private'
> > part.
> > 
> > Every zcomp has zcomp stream and mutex to protect its compression stream. Stream
> > usage semantics remains the same -- only one write can hold stream lock and use
> > its buffers. zcomp_strm_get() turns caller into exclusive user of a stream
> > (holding stream mutex until zram put stream), and zcomp_strm_put() makes zcomp
> > stream available (unlock the stream mutex). Hence no concurrent write
> > (compression) operations possible at the moment.
> > 
> > iozone -t 3 -R -r 16K -s 60M -I +Z
> > 
> >        test            base           patched
> > --------------------------------------------------
> >   Initial write      597992.91       591660.58
> >         Rewrite      609674.34       616054.97
> >            Read     2404771.75      2452909.12
> >         Re-read     2459216.81      2470074.44
> >    Reverse Read     1652769.66      1589128.66
> >     Stride read     2202441.81      2202173.31
> >     Random read     2236311.47      2276565.31
> >  Mixed workload     1423760.41      1709760.06
> >    Random write      579584.08       615933.86
> >          Pwrite      597550.02       594933.70
> >           Pread     1703672.53      1718126.72
> >          Fwrite     1330497.06      1461054.00
> >           Fread     3922851.00      3957242.62
> > 
> > Usage examples:
> > 
> > 	comp = zcomp_create(NAME) /* NAME e.g. "lzo" */
> > 
> > which initialises compressing backend if requested algorithm is supported.
> > 
> > Compress:
> > 	zstrm = zcomp_strm_get(comp)
> > 	zcomp_compress(comp, zstrm, src, &dst_len)
> > 	[..] /* copy compressed data */
> > 	zcomp_strm_put(comp, zstrm)
> > 
> > Decompress:
> > 	zcomp_decompress(comp, src, src_len, dst);
> > 
> > Free compessing backend and its zcomp stream:
> > 	zcomp_destroy(comp)
> > 
> > ...
> >
> > --- /dev/null
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Copyright (C) 2014 Sergey Senozhatsky.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include <linux/wait.h>
> > +#include <linux/sched.h>
> > +
> > +#include "zcomp.h"
> > +
> > +extern struct zcomp_backend zcomp_lzo;
> 
> This should be in a header file please.  So we know that the definition
> site and all users agree on the type.
> 
> > +static struct zcomp_backend *find_backend(const char *compress)
> > +{
> > +	if (strncmp(compress, "lzo", 3) == 0)
> > +		return &zcomp_lzo;
> > +	return NULL;
> > +}
> > +
> > +static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > +	if (zstrm->private)
> > +		comp->backend->destroy(zstrm->private);
> > +	free_pages((unsigned long)zstrm->buffer, 1);
> > +	kfree(zstrm);
> > +}
> > +
> > +/*
> > + * allocate new zcomp_strm structure with ->private initialized by
> > + * backend, return NULL on error
> > + */
> > +static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> > +{
> > +	struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_KERNEL);
> > +	if (!zstrm)
> > +		return NULL;
> > +
> > +	zstrm->private = comp->backend->create();
> > +	/*
> > +	 * allocate 2 pages. 1 for compressed data, plus 1 extra for the
> > +	 * case when compressed size is larger than the original one
> > +	 */
> > +	zstrm->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> > +	if (!zstrm->private || !zstrm->buffer) {
> > +		zcomp_strm_free(comp, zstrm);
> > +		zstrm = NULL;
> > +	}
> > +	return zstrm;
> > +}
> > +
> > +struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> > +{
> > +	mutex_lock(&comp->strm_lock);
> > +	return comp->zstrm;
> > +}
> > +
> > +void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > +	mutex_unlock(&comp->strm_lock);
> > +}
> 
> These are poorly named.  When we see "get" and "put" we expect that
> they will increment-refcount and decrement-refcount-free-on-zero.  But
> these function do not implement such a pattern.
> 
> > +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > +		const unsigned char *src, size_t *dst_len)
> > +{
> > +	return comp->backend->compress(src, zstrm->buffer, dst_len,
> > +			zstrm->private);
> > +}
> > +
> > +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > +		size_t src_len, unsigned char *dst)
> > +{
> > +	return comp->backend->decompress(src, src_len, dst);
> > +}
> > +
> > +void zcomp_destroy(struct zcomp *comp)
> > +{
> > +	zcomp_strm_free(comp, comp->zstrm);
> > +	kfree(comp);
> > +}
> > +
> > +/*
> > + * search available compressors for requested algorithm.
> > + * allocate new zcomp and initialize it. return NULL
> > + * if requested algorithm is not supported or in case
> > + * of init error
> > + */
> > +struct zcomp *zcomp_create(const char *compress)
> > +{
> > +	struct zcomp *comp;
> > +	struct zcomp_backend *backend;
> > +
> > +	backend = find_backend(compress);
> > +	if (!backend)
> > +		return NULL;
> > +
> > +	comp = kzalloc(sizeof(struct zcomp), GFP_KERNEL);
> > +	if (!comp)
> > +		return NULL;
> > +
> > +	comp->backend = backend;
> > +	mutex_init(&comp->strm_lock);
> > +
> > +	comp->zstrm = zcomp_strm_alloc(comp);
> > +	if (!comp->zstrm) {
> > +		kfree(comp);
> > +		return NULL;
> > +	}
> > +	return comp;
> > +}
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > new file mode 100644
> > index 0000000..5106f8e
> > --- /dev/null
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Copyright (C) 2014 Sergey Senozhatsky.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#ifndef _ZCOMP_H_
> > +#define _ZCOMP_H_
> > +
> > +#include <linux/mutex.h>
> > +
> > +struct zcomp_strm {
> > +	void *buffer;     /* compression/decompression buffer */
> > +	void *private;
> 
> Document this?
> 
> > +	struct list_head list;
> 
> What does this do and what lock protects it?
> 
> > +};
> > +
> > +/* static compression backend */
> > +struct zcomp_backend {
> > +	int (*compress)(const unsigned char *src, unsigned char *dst,
> > +			size_t *dst_len, void *private);
> > +
> > +	int (*decompress)(const unsigned char *src, size_t src_len,
> > +			unsigned char *dst);
> > +
> > +	void *(*create)(void);
> > +	void (*destroy)(void *private);
> > +
> > +	const char *name;
> > +};
> > +
> > +/* dynamic per-device compression frontend */
> > +struct zcomp {
> > +	struct mutex strm_lock;
> > +	struct zcomp_strm *zstrm;
> > +	struct zcomp_backend *backend;
> > +};
> > +
> > +struct zcomp *zcomp_create(const char *comp);
> > +void zcomp_destroy(struct zcomp *comp);
> > +
> > +struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
> > +void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm);
> > +
> > +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > +		const unsigned char *src, size_t *dst_len);
> > +
> > +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > +		size_t src_len, unsigned char *dst);
> > +#endif /* _ZCOMP_H_ */
> > diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
> > new file mode 100644
> > index 0000000..b7722a2
> > --- /dev/null
> > +++ b/drivers/block/zram/zcomp_lzo.c
> > @@ -0,0 +1,48 @@
> > +/*
> > + * Copyright (C) 2014 Sergey Senozhatsky.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/lzo.h>
> > +
> > +#include "zcomp.h"
> > +
> > +static void *lzo_create(void)
> > +{
> > +	return kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> > +}
> > +
> > +static void lzo_destroy(void *private)
> > +{
> > +	kfree(private);
> > +}
> > +
> > +static int lzo_compress(const unsigned char *src, unsigned char *dst,
> > +		size_t *dst_len, void *private)
> > +{
> > +	int ret = lzo1x_1_compress(src, PAGE_SIZE, dst, dst_len, private);
> > +	return ret == LZO_E_OK ? 0 : ret;
> > +}
> > +
> > +static int lzo_decompress(const unsigned char *src, size_t src_len,
> > +		unsigned char *dst)
> > +{
> > +	size_t dst_len = PAGE_SIZE;
> > +	int ret = lzo1x_decompress_safe(src, src_len, dst, &dst_len);
> > +	return ret == LZO_E_OK ? 0 : ret;
> > +}
> > +
> > +extern struct zcomp_backend zcomp_lzo;
> 
> This is unneeded, should be in the header file.
> 
> > +struct zcomp_backend zcomp_lzo = {
> > +	.compress = lzo_compress,
> > +	.decompress = lzo_decompress,
> > +	.create = lzo_create,
> > +	.destroy = lzo_destroy,
> > +	.name = "lzo",
> > +};
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists