Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function (v2)

[Nitin Gupta]

On Mon, Oct 29, 2012 at 11:05 AM, Sergey Senozhatsky
<sergey.senozhatsky@xxxxxxxxx> wrote:
>   zram: factor-out zram_decompress_page() function
>
>   zram_bvec_read() shared decompress functionality with zram_read_before_write() function.
>   Factor-out and make commonly used zram_decompress_page() function, which also simplified
>   error handling in zram_bvec_read().
>
>   V2: changed debug message for handle_zero_page() case to a more general one
>
>   Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
>
> ---
>
>  drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 6edefde..bd5efdf 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -183,62 +183,25 @@ static inline int is_partial_io(struct bio_vec *bvec)
>         return bvec->bv_len != PAGE_SIZE;
>  }
>
> -static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> -                         u32 index, int offset, struct bio *bio)
> +static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  {
> -       int ret;
> -       size_t clen;
> -       struct page *page;
> -       unsigned char *user_mem, *cmem, *uncmem = NULL;
> -
> -       page = bvec->bv_page;
> -
> -       if (zram_test_flag(zram, index, ZRAM_ZERO)) {
> -               handle_zero_page(bvec);
> -               return 0;
> -       }
> +       int ret = LZO_E_OK;
> +       size_t clen = PAGE_SIZE;
> +       unsigned char *cmem;
> +       unsigned long handle = zram->table[index].handle;
>
> -       /* Requested page is not present in compressed area */
> -       if (unlikely(!zram->table[index].handle)) {
> -               pr_debug("Read before write: sector=%lu, size=%u",
> -                        (ulong)(bio->bi_sector), bio->bi_size);
> -               handle_zero_page(bvec);
> +       if (!handle || zram_test_flag(zram, index, ZRAM_ZERO)) {
> +               memset(mem, 0, PAGE_SIZE);
>                 return 0;
>         }
>
> -       if (is_partial_io(bvec)) {
> -               /* Use  a temporary buffer to decompress the page */
> -               uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
> -               if (!uncmem) {
> -                       pr_info("Error allocating temp memory!\n");
> -                       return -ENOMEM;
> -               }
> -       }
> -
> -       user_mem = kmap_atomic(page);
> -       if (!is_partial_io(bvec))
> -               uncmem = user_mem;
> -       clen = PAGE_SIZE;
> -
> -       cmem = zs_map_object(zram->mem_pool, zram->table[index].handle,
> -                               ZS_MM_RO);
> -
> -       if (zram->table[index].size == PAGE_SIZE) {
> -               memcpy(uncmem, cmem, PAGE_SIZE);
> -               ret = LZO_E_OK;
> -       } else {
> +       cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
> +       if (zram->table[index].size == PAGE_SIZE)
> +               memcpy(mem, cmem, PAGE_SIZE);
> +       else
>                 ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
> -                                   uncmem, &clen);
> -       }
> -
> -       if (is_partial_io(bvec)) {
> -               memcpy(user_mem + bvec->bv_offset, uncmem + offset,
> -                      bvec->bv_len);
> -               kfree(uncmem);
> -       }
> -
> -       zs_unmap_object(zram->mem_pool, zram->table[index].handle);
> -       kunmap_atomic(user_mem);
> +                                               mem, &clen);
> +       zs_unmap_object(zram->mem_pool, handle);
>
>         /* Should NEVER happen. Return bio error if it does. */
>         if (unlikely(ret != LZO_E_OK)) {
> @@ -247,36 +210,58 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>                 return ret;
>         }
>
> -       flush_dcache_page(page);
> -
>         return 0;
>  }
>
> -static int zram_read_before_write(struct zram *zram, char *mem, u32 index)
> +static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> +                         u32 index, int offset, struct bio *bio)
>  {
>         int ret;
> -       size_t clen = PAGE_SIZE;
> -       unsigned char *cmem;
> -       unsigned long handle = zram->table[index].handle;
> +       struct page *page;
> +       unsigned char *user_mem, *uncmem = NULL;
>
> -       if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
> -               memset(mem, 0, PAGE_SIZE);
> +       page = bvec->bv_page;
> +
> +       if (unlikely(!zram->table[index].handle) ||
> +                       zram_test_flag(zram, index, ZRAM_ZERO)) {
> +               pr_debug("Handle zero page: sector=%lu, size=%u",
> +                        (ulong)(bio->bi_sector), bio->bi_size);
> +               handle_zero_page(bvec);

Nothing should be printed (even a debug only message) for
the ZRAM_ZERO case. This case can be quite common for certain
kinds of data and would cause a huge log spew.  Also (!handle) case
is not the same as zero-filled page case, so this message would
be misleading.

So, we should either get rid of this warning entirely or only do
pr_debug("Read before write ....") for (!handle) case and log nothing
for ZRAM_ZERO case.

Nitin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/