Re: [PATCHv2 1/2] zram: introduce compressing backend abstraction

[Minchan Kim]

On Thu, Feb 06, 2014 at 02:16:28PM +0900, Minchan Kim wrote:
> Hello Sergey,
> 
> Sorry for late response.
> 
> On Thu, Jan 30, 2014 at 10:28:07PM +0300, Sergey Senozhatsky wrote:
> > ZRAM performs direct LZO compression algorithm calls, making it the one
> > and only option. Introduce struct zram_comp in order to support multiple
> > compression algorithms. struct zram_comp defines the following set of
> > compressing backend operations:
> > 	.create
> > 	.destroy
> > 	.compress
> > 	.decompress
> > 	.workmem_get
> > 	.workmem_put
> > 
> > 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 new object
> 
>                                                           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() will 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. Besides, zram holds buffer_lock almost through the whole write()
> > function, making parallel compression impossible. To remove requirement
> > a) new struct zcomp_workmem (workmem is a common term used by lzo, lz4
> > and zlib) contains buffers required by compression algorithm, while new
> > struct zcomp_wm_policy implements workmem handling and locking by means
> > of get() and put() semantics and removes requirement b) from zram meta.
> > In general, workmem_get() turns caller into exclusive user of workmem
> > and workem_put() makes a particular workmem available.
> > 
> > Each compressing backend may use a default workmem policy or provide
> > custom implementation. Default workmem policy (struct zcomp_wm_policy)
> > has a list of idle workmem buffers (at least 1 workmem), spinlock to
> > protect idle list and wait queue, making it possible to have parallel
> > compressions. Each time zram issues a workmem_get() call, the following
> > set of operations performed:
> 
> I'm still really not sure why backend should know workmem policy.
> I think it's matter of upper layer, not backend.
> Yeb, surely, you have a reason but it's very hard for me to know it
> by this patchset so I'd like to divide the patchset.
> (You don't need to explain it in here and I expect it would be clear
> if you separate it like I suggested below).
> Pz, see below.
> 
> > - spin lock buffer_lock
> > - if idle list is not empty, remove workmem from idle list, spin
> >   unlock and return workmem pointer
> > - if idle list is empty, current adds itself to wait queue. it will be
> >   awaken by workmem_put() caller.
> > 
> > workmem_put():
> > - spin lock buffer_lock
> > - add workmem to idle list
> > - spin unlock, wake up sleeper (if any)
> 
> Good.
> 
> > 
> > zram_comp file contains array of supported compressing backends, which
> > can be altered according to user selection.
> > 
> > Usage examples. To initialize compressing backend:
> > 	comp = zcomp_create(NAME) /* NAME e.g. lzo, lz4 */
> > 
> > which performs NAME lookup in array of supported compressing backends
> > and initialises compressing backend if requested algorithm is supported.
> > Compressing:
> > 	wm = comp->workmem_get(comp)
> > 	comp->compress(...)
> > 	[..] /* copy compressed data */
> > 	comp->workmem_put(comp, wm)
> > 
> > Free compessing backend and all ot its workmem buffers:
> > 	zcomp_destroy(comp)
> > 
> > The patch implements LZO and LZ4 backends. At this point zcomp_wm_policy
> > keeps only one workmem in the idle list, support for multi workmem buffers
> > will be introduced later.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> > ---
> >  drivers/block/zram/zcomp_lz4.c |  49 ++++++++++
> >  drivers/block/zram/zcomp_lz4.h |  18 ++++
> 
> Please don't include lz4 in this patch. It should be separated and
> description of the patch surely should include the number to prove
> lz4 is better than lzo in *what* workload so it should make
> everybody easy to convince.
> 
> And let's separate this patchset following as
> 
> 1. abstract compressor with zram_comp.
> 2. Support configurable compressor
> 3. support zcomp multi buffers
> 4. support lz4
> 
> Please don't add workmem policy in patch 1 because we still use only
> a buffer until 3 so patch 1, 2 would be very simple.
> Patch 3 might introduce wm policy. Then, it would be very clear
> why we need it for zomp_multi so that it would make review easy.
> 
> If 1,2,3 have no problem and apparenlty lz4 has a benefit, patch 4
> will be merged easily but If lz4 were rejected by some reason,
> we could support another compression easily since patch 1,2,3 is
> merged.

Hello Sergey,

If I don't show my brain to you, I guess we need more iterations
to discuss and it's really waste our time so I send prototype patch
to show the concept I am thinking.

Surely, It wouldn't work but it's enough to show my concept.

It's really simple.
Backend could just focus comp/decomp with own algorithm buffer.
(ie, backend don't need to know compress_buffer and workmem policy
because it's caller's matter, not compression backed).

I hope you complete this patch to work well with your SOB/Copyright
if you don't object the direction because most of concept and code
are from your idea and implementation.

Thanks.