Re: zswap: use PAGE_SIZE * 2 for compression dst buffer size when calling crypto compression API

["Um, Taeil" <taeilum@amazon.com>]

We can tell whether compressed size is greater than PAGE_SIZE by looking at the returned *dlen value from crypto_comp_compress. This should be fairly easy.
This is actually what zram is doing today. zram looks for *dlen and not take the compressed result if *dlen is greater than certain size.
I think errors from crypto_comp_compress should be real errors. 

Today in kernel compression drivers such as lzo and lz4, they do not stop just because compression result size is greater than source size.
Also, some H/W accelerators would not have the option of stopping compression just because of the result size is greater than source size.

Thank you,
Taeil

On 9/18/18, 2:44 PM, "Dan Streetman" <ddstreet@ieee.org> wrote:

    On Tue, Sep 18, 2018 at 2:52 PM Um, Taeil <taeilum@amazon.com> wrote:
    >
    > Problem statement:
    > "compressed data are not fully copied to destination buffer when compressed data size is greater than source data"
    >
    > Why:
    > 5th argument of crypto_comp_compress function is *dlen, which tell the compression driver how many bytes the destination buffer space is allocated (allowed to write data).
    > This *dlen is important especially for H/W accelerator based compression driver because it is dangerous if we allow the H/W accelerator to access memory beyond *dst + *dlen.
    > Note that buffer location would be passed as physical address.
    > Due to the above reason, H/W accelerator based compression driver need to honor *dlen value when it serves crypto_comp_compress API.
    
    and that's exactly what zswap wants to happen - any compressor (hw or
    sw) should fail with an error code (ENOSPC makes the most sense, but
    zswap doesn't actually care) if the compressed data size is larger
    than the provided data buffer.
    
    > Today, we pass slen = PAGE_SIZE and *dlen=PAGE_SIZE to crypto_comp_compress in zswap.c.
    > If compressed data size is greater than source (uncompressed) data size,  H/W accelerator cannot copy (deliver) the entire compressed data.
    
    If the "compressed" data is larger than 1 page, then there is no point
    in storing the page in zswap.
    
    remember that zswap is different than zram; in zram, there's no other
    place to store the data.  However, with zswap, if compression fails or
    isn't good, we can just pass the uncompressed page down to the swap
    device.
    
    >
    > Thank you,
    > Taeil
    >
    > On 9/18/18, 7:15 AM, "Dan Streetman" <ddstreet@ieee.org> wrote:
    >
    >     On Mon, Sep 17, 2018 at 7:10 PM Um, Taeil <taeilum@amazon.com> wrote:
    >     >
    >     > Currently, we allocate PAGE_SIZE * 2 for zswap_dstmem which is used as compression destination buffer.
    >     >
    >     > However, we pass only half of the size (PAGE_SIZE) to crypto_comp_compress.
    >     >
    >     > This might not be a problem for CPU based existing lzo, lz4 crypto compression driver implantation.
    >     >
    >     > However, this could be a problem for some H/W acceleration compression drivers, which honor destination buffer size when it prepares H/W resources.
    >
    >     How exactly could it be a problem?
    >
    >     >
    >     > Actually, this patch is aligned with what zram is passing when it calls crypto_comp_compress.
    >     >
    >     > The following simple patch will solve this problem. I tested it with existing crypto/lzo.c and crypto/lz4.c compression driver and it works fine.
    >     >
    >     >
    >     >
    >     >
    >     >
    >     > --- mm/zswap.c.orig       2018-09-14 14:36:37.984199232 -0700
    >     >
    >     > +++ mm/zswap.c             2018-09-14 14:36:53.340189681 -0700
    >     >
    >     > @@ -1001,7 +1001,7 @@ static int zswap_frontswap_store(unsigne
    >     >
    >     >                 struct zswap_entry *entry, *dupentry;
    >     >
    >     >                 struct crypto_comp *tfm;
    >     >
    >     >                 int ret;
    >     >
    >     > -              unsigned int hlen, dlen = PAGE_SIZE;
    >     >
    >     > +             unsigned int hlen, dlen = PAGE_SIZE * 2;
    >     >
    >     >                 unsigned long handle, value;
    >     >
    >     >                 char *buf;
    >     >
    >     >                 u8 *src, *dst;
    >     >
    >     >
    >     >
    >     >
    >     >
    >     >
    >     >
    >     > Thank you,
    >     >
    >     > Taeil
    >     >
    >     >
    >
    >
    >