FAILED: patch "[PATCH] zram: do not use copy_page with non-page aligned address" failed to apply to 3.18-stable tree

From d72e9a7a93e4f8e9e52491921d99e0c8aa89eb4e Mon Sep 17 00:00:00 2001

The patch below does not apply to the 3.18-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

From 042b3d57120146ab24b284713d44a9c1b37935b3 Mon Sep 17 00:00:00 2001

Hello,

drivers/block/zram/zram_drv.c static int zram_decompress_page(struct zram *zram, char *mem, u32 index) · line 434

Quoted 9 lines · Minchan Kim

index 3920ee45aa59..7e94459a489a 100644

drivers/block/zram/zram_drv.c +1 -1

@@ -431,13 +431,13 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 
 	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		clear_page(mem);
+		memset(mem, 0, PAGE_SIZE);

after a quick look I haven't found a clear_page() that would impose alignment requirements. but ok, `mem' _can_ be kmalloc-ed there, yes.

a side question:
how bad would it hurt to switch to page_size aligned allocator partial IO instead of slab allocator? and just use potentially faster *_page() functions instead of mem* functions? I think people normally don't see

partial IOs (well, unless they use some 'weird' filesystems). while copy_page()->memcpy() happens to everyone.

drivers/block/zram/zram_drv.c static int zram_decompress_page(struct zram *zram, char *mem, u32 index) · line 440

Quoted 4 lines

drivers/block/zram/zram_drv.c +1 -1

 	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO);
 	if (size == PAGE_SIZE)
-		copy_page(mem, cmem);
+		memcpy(mem, cmem, PAGE_SIZE);

I think here we have both page_size aligned mem' and page_size aligned cmem'. `mem' is guaranteed to be kmapp-ed ->bv_page, which is allocated by alloc_page(), and zs_map_object() returns a kmapp-ed alloc_page()-ed page for huge)class object (which is the case here, isn't it?). so we can keep copy_page()? am I wrong? I'm afraid we can slow down a regularly

executed (semi-hot) path here.

Quoted 2 lines

drivers/block/zram/zram_drv.c +0 -0

@@ -612,7 +612,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 

drivers/block/zram/zram_drv.c static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, · line 615

Quoted 4 lines

drivers/block/zram/zram_drv.c +1 -1

 	if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
 		src = kmap_atomic(page);
-		copy_page(cmem, src);
+		memcpy(cmem, src, PAGE_SIZE);

here src' is kmapp-ed ->bv_page, which is always page_size aligned. it's allocated by alloc_page(). cmem' is also page_size aligned, isn't it? seems that we can use copy_page(). am I missing something? I'm afraid we can slow down a regularly executed (semi-hot) path here.

Quoted 7 lines

after a quick look I haven't found a clear_page() that would impose alignment requirements. but ok, `mem' _can_ be kmalloc-ed there, yes.

a side question:
how bad would it hurt to switch to page_size aligned allocator partial IO instead of slab allocator? and just use potentially faster *_page() functions instead of mem* functions? I think people normally don't see

You meant alloc_page or wanted to create new own slab cache with PAGE_SIZE alighment requirement?

Either way, it makes more changes so it would be not proper for -stable material, IMHO. As well, this path(zero-page) would be rare so it wouldn't hurt performance, either.

Quoted 4 lines

partial IOs (well, unless they use some 'weird' filesystems). while copy_page()->memcpy() happens to everyone.

Quoted 10 lines

drivers/block/zram/zram_drv.c static int zram_decompress_page(struct zram *zram, char *mem, u32 index) · line 440

Quoted 4 lines

drivers/block/zram/zram_drv.c +1 -1

 	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO);
 	if (size == PAGE_SIZE)
-		copy_page(mem, cmem);
+		memcpy(mem, cmem, PAGE_SIZE);

I think here we have both page_size aligned mem' and page_size aligned cmem'. `mem' is guaranteed to be kmapp-ed ->bv_page, which is allocated by alloc_page(), and zs_map_object() returns a kmapp-ed alloc_page()-ed page for huge)class object (which is the case here, isn't it?). so we can keep copy_page()? am I wrong? I'm afraid we can slow down a regularly

No, you're right but as I wrote in description, I don't want to rely on zsmalloc's internal implementation.

And this bad compress ratio path would be rare, too.

Quoted 4 lines

here src' is kmapp-ed ->bv_page, which is always page_size aligned. it's allocated by alloc_page(). cmem' is also page_size aligned, isn't it? seems that we can use copy_page(). am I missing something? I'm afraid we can slow down a regularly executed (semi-hot) path here.

I understand your concern about slow down but I belive thre are not performance sensitive paths because they are rare so I wanted to bias to safety for -stable material and we can make it fast in mainline. :)

On (04/19/17 15:20), Minchan Kim wrote:
[..]

Quoted 10 lines

Quoted 7 lines

after a quick look I haven't found a clear_page() that would impose alignment requirements. but ok, `mem' _can_ be kmalloc-ed there, yes.

a side question:
how bad would it hurt to switch to page_size aligned allocator partial IO instead of slab allocator? and just use potentially faster *_page() functions instead of mem* functions? I think people normally don't see

You meant alloc_page or wanted to create new own slab cache with PAGE_SIZE alighment requirement?

alloc_page. yes.

Quoted 3 lines

Either way, it makes more changes so it would be not proper for -stable material, IMHO. As well, this path(zero-page) would be rare so it wouldn't hurt performance, either.

oh, sure. I meant upstream, not -stable. sorry for confusion.

Quoted 15 lines

Quoted 14 lines

partial IOs (well, unless they use some 'weird' filesystems). while copy_page()->memcpy() happens to everyone.

drivers/block/zram/zram_drv.c static int zram_decompress_page(struct zram *zram, char *mem, u32 index) · line 440

Quoted 4 lines

drivers/block/zram/zram_drv.c +1 -1

 	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO);
 	if (size == PAGE_SIZE)
-		copy_page(mem, cmem);
+		memcpy(mem, cmem, PAGE_SIZE);

I think here we have both page_size aligned mem' and page_size aligned cmem'. `mem' is guaranteed to be kmapp-ed ->bv_page, which is allocated by alloc_page(), and zs_map_object() returns a kmapp-ed alloc_page()-ed page for huge)class object (which is the case here, isn't it?). so we can keep copy_page()? am I wrong? I'm afraid we can slow down a regularly

Quoted 2 lines

No, you're right but as I wrote in description, I don't want to rely on zsmalloc's internal implementation.

well, zsmalloc() won't change that much in -stable ;) but up to you.

Reviewed-by Sergey Senozhatsky <[email protected]>

Quoted 4 lines

And this bad compress ratio path would be rare, too.

"bad compression" is not uncommon. I wish it was, but it's not. we can't really guarantee/expect anything, it's up to compression algorithm and data.

I added two zram.stat counters -- bad_compression and good_compression. and did

(quoted diff) +5 -0

+       if (unlikely(comp_len > max_zpage_size)) {
+               atomic64_inc(&zram->stats.bad_compression);
                comp_len = PAGE_SIZE;
+       } else {
+               atomic64_inc(&zram->stats.good_compression);
+       }

in zram_compress().

and in mm_stat_show()

ret = scnprintf(buf, PAGE_SIZE,

(quoted diff) +3 -1

-                       "%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
+                       "%8llu %8llu %8llu %8lu %8ld %8llu %8lu %8llu %8llu\n",
 
...
 
+                       (u64)atomic64_read(&zram->stats.bad_compression),
+                       (u64)atomic64_read(&zram->stats.good_compression));

so bad_compression first, good_compression last.

and here are some of the results (LZ0):

cat /sys/block/zram0/mm_stat
224010240 186613588 188694528 0 188694528 15958 0 37818 16878

'bad' compression twice as common as 'good' compression on some of my workloads.

cat /sys/block/zram0/mm_stat
482918400 404083224 409276416 0 409276416 15138 0 76789 45188

# cat /sys/block/zram0/mm_stat
919818240 581986600 596762624        0 596762624    14296        0   119052   187966

=== a different test:

# cat /sys/block/zram0/mm_stat 
2976886784 2061379701 2087194624        0 2087194624     5752        0   387983   341593

=== another test:

# cat /sys/block/zram0/mm_stat 
975462400 341683056 356196352        0 356196352     9172        0    15833   224226

ok, not too bad.

# cat /sys/block/zram0/mm_stat
1841291264 933397356 958345216        0 958345216     5940        0   116175   336128

# cat /sys/block/zram0/mm_stat
3100012544 2183029827 2208866304        0 2208866304     5447        0   417539   342450

# cat /sys/block/zram0/mm_stat 
3338133504 2265031857 2294886400        0 2294886400     1199        0   420382   402257

meh.

# cat /sys/block/zram0/mm_stat 
3057053696 2128156096 2153943040        0 2295697408      104     3903   420530   420055

close to 50/50.

# cat /sys/block/zram0/mm_stat 
3094814720 2142308405 2168918016        0 2295697408      296     3903   420727   431247

# cat /sys/block/zram0/mm_stat
3177267200 2172831422 2201022464        0 2295697408      346     3903   421139   455748

# cat /sys/block/zram0/mm_stat 
3338510336 2269447044 2299387904        0 2299387904     1179     3983   434806   483659

end of test.

[..]

Quoted 9 lines

Quoted 4 lines

here src' is kmapp-ed ->bv_page, which is always page_size aligned. it's allocated by alloc_page(). cmem' is also page_size aligned, isn't it? seems that we can use copy_page(). am I missing something? I'm afraid we can slow down a regularly executed (semi-hot) path here.

I understand your concern about slow down but I belive thre are not performance sensitive paths because they are rare so I wanted to bias to safety for -stable material and we can make it fast in mainline. :)

ok.

	-ss

Quoted 4 lines

Quoted 1 line

And this bad compress ratio path would be rare, too.

"bad compression" is not uncommon. I wish it was, but it's not. we can't really guarantee/expect anything, it's up to compression algorithm and data.

True. Thanks for the testing!
What are your workloads?

I think user shouldn't use zram for such lots of bad compression workload if it isn't temporal :).

Quoted 6 lines · On Tue, Apr 18, 2017 at 02:18:28PM +0200, [email protected] wrote:

The patch below does not apply to the 3.18-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 3 lines

thanks,

greg k-h

Hi Greg,

I backported this patch for v3.18.49. Could you merge this?