Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Mike Galbraith 
On Mon, 2016-10-17 at 19:18 +0200, Mike Galbraith wrote:

> BTW, 4.8 either needs the btrfs deadlock fix (0ccd05285e7f) or the LTP
> testcase has to be hacked to not test btrfs.  It also fails the first
> time it's run in 4.8/4.8-rt, doesn't do that in master/tip-rt.

Belay that, the first run failure is there in master/tip too, it's just
intermittent.  From there on, it's solid.  Hohum.

-Mike

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Mike Galbraith 
On Mon, 2016-10-17 at 19:18 +0200, Mike Galbraith wrote:

> BTW, 4.8 either needs the btrfs deadlock fix (0ccd05285e7f) or the LTP
> testcase has to be hacked to not test btrfs.  It also fails the first
> time it's run in 4.8/4.8-rt, doesn't do that in master/tip-rt.

Belay that, the first run failure is there in master/tip too, it's just
intermittent.  From there on, it's solid.  Hohum.

-Mike

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Mike Galbraith 
On Mon, 2016-10-17 at 18:29 +0200, Sebastian Andrzej Siewior wrote:
> On 2016-10-17 18:19:00 [+0200], Mike Galbraith wrote:
> > I used a local lock first, but lockdep was unhappy with it.  Ok,
> > back
> > to the drawing board.  Seems to work, but...
> 
> locallock can be taken recursively so unless preemption was already
> disabled, lockdep shouldn't complain. But then from the small context
> it should not be taken recursively.

FWIW here's the lockdep gripe.

BTW, 4.8 either needs the btrfs deadlock fix (0ccd05285e7f) or the LTP
testcase has to be hacked to not test btrfs.  It also fails the first
time it's run in 4.8/4.8-rt, doesn't do that in master/tip-rt.

[  130.090247] zram: Added device: zram0
[  130.163407] zram0: detected capacity change from 0 to 536870912
[  131.760327] zram: 4188 (zram01) Attribute compr_data_size (and others) will 
be removed. See zram documentation.

[  131.760923] ==
[  131.760923] [ INFO: possible circular locking dependency detected ]
[  131.760924] 4.8.2-rt1-virgin_debug #20 Tainted: GE  
[  131.760924] ---
[  131.760924] zram01/4188 is trying to acquire lock:
[  131.760928]  ((null)){+.+...}, at: [] 
zcomp_stream_get+0x44/0xd0 [zram]
[  131.760929] but task is already holding lock:
[  131.760932]  (>lock){+.+...}, at: [] 
zs_map_object+0x8b/0x2e0
[  131.760932] which lock already depends on the new lock.
[  131.760932] the existing dependency chain (in reverse order) is:
[  131.760933] -> #2 (>lock){+.+...}:
[  131.760936][] lock_acquire+0xbd/0x260
[  131.760939][] rt_read_lock+0x47/0x60
[  131.760940][] zs_map_object+0x8b/0x2e0
[  131.760941][] zram_bvec_rw+0x383/0x850 [zram]
[  131.760942][] zram_make_request+0x19d/0x3b6 [zram]
[  131.760944][] generic_make_request+0x10e/0x2e0
[  131.760944][] submit_bio+0x6d/0x150
[  131.760947][] submit_bh_wbc+0x15c/0x1a0
[  131.760948][] __block_write_full_page+0x12c/0x3b0
[  131.760949][] block_write_full_page+0xff/0x130
[  131.760951][] blkdev_writepage+0x18/0x20
[  131.760953][] __writepage+0x16/0x50
[  131.760954][] write_cache_pages+0x2af/0x690
[  131.760955][] generic_writepages+0x46/0x60
[  131.760957][] blkdev_writepages+0x2f/0x40
[  131.760958][] do_writepages+0x21/0x40
[  131.760959][] __filemap_fdatawrite_range+0xaa/0xf0
[  131.760960][] filemap_write_and_wait+0x40/0x80
[  131.760961][] __sync_blockdev+0x1f/0x40
[  131.760961][] __blkdev_put+0x78/0x3a0
[  131.760962][] blkdev_put+0x4e/0x150
[  131.760963][] blkdev_close+0x28/0x30
[  131.760964][] __fput+0xfb/0x230
[  131.760965][] fput+0xe/0x10
[  131.760967][] task_work_run+0x83/0xc0
[  131.760968][] exit_to_usermode_loop+0xb4/0xee
[  131.760970][] syscall_return_slowpath+0xbb/0x130
[  131.760971][] entry_SYSCALL_64_fastpath+0xbb/0xbd
[  131.760971] -> #1 (>lock){+.+...}:
[  131.760973][] lock_acquire+0xbd/0x260
[  131.760974][] _mutex_lock+0x31/0x40
[  131.760975][] zs_map_object+0x48/0x2e0
[  131.760976][] zram_bvec_rw+0x383/0x850 [zram]
[  131.760977][] zram_make_request+0x19d/0x3b6 [zram]
[  131.760978][] generic_make_request+0x10e/0x2e0
[  131.760978][] submit_bio+0x6d/0x150
[  131.760979][] submit_bh_wbc+0x15c/0x1a0
[  131.760980][] __block_write_full_page+0x12c/0x3b0
[  131.760982][] block_write_full_page+0xff/0x130
[  131.760983][] blkdev_writepage+0x18/0x20
[  131.760984][] __writepage+0x16/0x50
[  131.760985][] write_cache_pages+0x2af/0x690
[  131.760986][] generic_writepages+0x46/0x60
[  131.760987][] blkdev_writepages+0x2f/0x40
[  131.760988][] do_writepages+0x21/0x40
[  131.760989][] __filemap_fdatawrite_range+0xaa/0xf0
[  131.760990][] filemap_write_and_wait+0x40/0x80
[  131.760990][] __sync_blockdev+0x1f/0x40
[  131.760991][] __blkdev_put+0x78/0x3a0
[  131.760992][] blkdev_put+0x4e/0x150
[  131.760992][] blkdev_close+0x28/0x30
[  131.760993][] __fput+0xfb/0x230
[  131.760994][] fput+0xe/0x10
[  131.760995][] task_work_run+0x83/0xc0
[  131.760996][] exit_to_usermode_loop+0xb4/0xee
[  131.760996][] syscall_return_slowpath+0xbb/0x130
[  131.760997][] entry_SYSCALL_64_fastpath+0xbb/0xbd
[  131.760998] -> #0 ((null)){+.+...}:
[  131.760999][] __lock_acquire+0x162c/0x1660
[  131.761000][] lock_acquire+0xbd/0x260
[  131.761001][] rt_spin_lock__no_mg+0x5a/0x70
[  131.761002][] zcomp_stream_get+0x44/0xd0 [zram]
[  131.761003][] 
zram_decompress_page.isra.17+0xc4/0x150 [zram]
[  131.761004][] zram_bvec_rw+0x4f4/0x850 [zram]
[  131.761005][]

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Mike Galbraith 
On Mon, 2016-10-17 at 18:29 +0200, Sebastian Andrzej Siewior wrote:
> On 2016-10-17 18:19:00 [+0200], Mike Galbraith wrote:
> > I used a local lock first, but lockdep was unhappy with it.  Ok,
> > back
> > to the drawing board.  Seems to work, but...
> 
> locallock can be taken recursively so unless preemption was already
> disabled, lockdep shouldn't complain. But then from the small context
> it should not be taken recursively.

FWIW here's the lockdep gripe.

BTW, 4.8 either needs the btrfs deadlock fix (0ccd05285e7f) or the LTP
testcase has to be hacked to not test btrfs.  It also fails the first
time it's run in 4.8/4.8-rt, doesn't do that in master/tip-rt.

[  130.090247] zram: Added device: zram0
[  130.163407] zram0: detected capacity change from 0 to 536870912
[  131.760327] zram: 4188 (zram01) Attribute compr_data_size (and others) will 
be removed. See zram documentation.

[  131.760923] ==
[  131.760923] [ INFO: possible circular locking dependency detected ]
[  131.760924] 4.8.2-rt1-virgin_debug #20 Tainted: GE  
[  131.760924] ---
[  131.760924] zram01/4188 is trying to acquire lock:
[  131.760928]  ((null)){+.+...}, at: [] 
zcomp_stream_get+0x44/0xd0 [zram]
[  131.760929] but task is already holding lock:
[  131.760932]  (>lock){+.+...}, at: [] 
zs_map_object+0x8b/0x2e0
[  131.760932] which lock already depends on the new lock.
[  131.760932] the existing dependency chain (in reverse order) is:
[  131.760933] -> #2 (>lock){+.+...}:
[  131.760936][] lock_acquire+0xbd/0x260
[  131.760939][] rt_read_lock+0x47/0x60
[  131.760940][] zs_map_object+0x8b/0x2e0
[  131.760941][] zram_bvec_rw+0x383/0x850 [zram]
[  131.760942][] zram_make_request+0x19d/0x3b6 [zram]
[  131.760944][] generic_make_request+0x10e/0x2e0
[  131.760944][] submit_bio+0x6d/0x150
[  131.760947][] submit_bh_wbc+0x15c/0x1a0
[  131.760948][] __block_write_full_page+0x12c/0x3b0
[  131.760949][] block_write_full_page+0xff/0x130
[  131.760951][] blkdev_writepage+0x18/0x20
[  131.760953][] __writepage+0x16/0x50
[  131.760954][] write_cache_pages+0x2af/0x690
[  131.760955][] generic_writepages+0x46/0x60
[  131.760957][] blkdev_writepages+0x2f/0x40
[  131.760958][] do_writepages+0x21/0x40
[  131.760959][] __filemap_fdatawrite_range+0xaa/0xf0
[  131.760960][] filemap_write_and_wait+0x40/0x80
[  131.760961][] __sync_blockdev+0x1f/0x40
[  131.760961][] __blkdev_put+0x78/0x3a0
[  131.760962][] blkdev_put+0x4e/0x150
[  131.760963][] blkdev_close+0x28/0x30
[  131.760964][] __fput+0xfb/0x230
[  131.760965][] fput+0xe/0x10
[  131.760967][] task_work_run+0x83/0xc0
[  131.760968][] exit_to_usermode_loop+0xb4/0xee
[  131.760970][] syscall_return_slowpath+0xbb/0x130
[  131.760971][] entry_SYSCALL_64_fastpath+0xbb/0xbd
[  131.760971] -> #1 (>lock){+.+...}:
[  131.760973][] lock_acquire+0xbd/0x260
[  131.760974][] _mutex_lock+0x31/0x40
[  131.760975][] zs_map_object+0x48/0x2e0
[  131.760976][] zram_bvec_rw+0x383/0x850 [zram]
[  131.760977][] zram_make_request+0x19d/0x3b6 [zram]
[  131.760978][] generic_make_request+0x10e/0x2e0
[  131.760978][] submit_bio+0x6d/0x150
[  131.760979][] submit_bh_wbc+0x15c/0x1a0
[  131.760980][] __block_write_full_page+0x12c/0x3b0
[  131.760982][] block_write_full_page+0xff/0x130
[  131.760983][] blkdev_writepage+0x18/0x20
[  131.760984][] __writepage+0x16/0x50
[  131.760985][] write_cache_pages+0x2af/0x690
[  131.760986][] generic_writepages+0x46/0x60
[  131.760987][] blkdev_writepages+0x2f/0x40
[  131.760988][] do_writepages+0x21/0x40
[  131.760989][] __filemap_fdatawrite_range+0xaa/0xf0
[  131.760990][] filemap_write_and_wait+0x40/0x80
[  131.760990][] __sync_blockdev+0x1f/0x40
[  131.760991][] __blkdev_put+0x78/0x3a0
[  131.760992][] blkdev_put+0x4e/0x150
[  131.760992][] blkdev_close+0x28/0x30
[  131.760993][] __fput+0xfb/0x230
[  131.760994][] fput+0xe/0x10
[  131.760995][] task_work_run+0x83/0xc0
[  131.760996][] exit_to_usermode_loop+0xb4/0xee
[  131.760996][] syscall_return_slowpath+0xbb/0x130
[  131.760997][] entry_SYSCALL_64_fastpath+0xbb/0xbd
[  131.760998] -> #0 ((null)){+.+...}:
[  131.760999][] __lock_acquire+0x162c/0x1660
[  131.761000][] lock_acquire+0xbd/0x260
[  131.761001][] rt_spin_lock__no_mg+0x5a/0x70
[  131.761002][] zcomp_stream_get+0x44/0xd0 [zram]
[  131.761003][] 
zram_decompress_page.isra.17+0xc4/0x150 [zram]
[  131.761004][] zram_bvec_rw+0x4f4/0x850 [zram]
[  131.761005][]

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Sebastian Andrzej Siewior 
On 2016-10-17 18:19:00 [+0200], Mike Galbraith wrote:
> I used a local lock first, but lockdep was unhappy with it.  Ok, back
> to the drawing board.  Seems to work, but...

locallock can be taken recursively so unless preemption was already
disabled, lockdep shouldn't complain. But then from the small context
it should not be taken recursively.

>   -Mike

Sebastian

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Sebastian Andrzej Siewior 
On 2016-10-17 18:19:00 [+0200], Mike Galbraith wrote:
> I used a local lock first, but lockdep was unhappy with it.  Ok, back
> to the drawing board.  Seems to work, but...

locallock can be taken recursively so unless preemption was already
disabled, lockdep shouldn't complain. But then from the small context
it should not be taken recursively.

>   -Mike

Sebastian

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Mike Galbraith 
On Mon, 2016-10-17 at 16:24 +0200, Sebastian Andrzej Siewior wrote:
> On 2016-10-16 05:14:22 [+0200], Mike Galbraith wrote:
> > 
> > In v4.7, the driver switched to percpu compression streams, disabling
> > preemption (get/put_cpu_ptr()).  Use get/put_cpu_light() instead.
> 
> I am not convinced that this will work. Nothing prevents
> zram_bvec_write() to be reentrant on the same CPU what I can tell from
> browsing over the code and since it uses zstrm->buffer for compression
> it can go wrong. Also I don't know if crypto's tfm handler can be used
> in parallel for any ops (it usually does not work for crypto).
> 
> I suggest a local lock or a good reason why the this patch works.

I used a local lock first, but lockdep was unhappy with it.  Ok, back
to the drawing board.  Seems to work, but...

-Mike

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Mike Galbraith 
On Mon, 2016-10-17 at 16:24 +0200, Sebastian Andrzej Siewior wrote:
> On 2016-10-16 05:14:22 [+0200], Mike Galbraith wrote:
> > 
> > In v4.7, the driver switched to percpu compression streams, disabling
> > preemption (get/put_cpu_ptr()).  Use get/put_cpu_light() instead.
> 
> I am not convinced that this will work. Nothing prevents
> zram_bvec_write() to be reentrant on the same CPU what I can tell from
> browsing over the code and since it uses zstrm->buffer for compression
> it can go wrong. Also I don't know if crypto's tfm handler can be used
> in parallel for any ops (it usually does not work for crypto).
> 
> I suggest a local lock or a good reason why the this patch works.

I used a local lock first, but lockdep was unhappy with it.  Ok, back
to the drawing board.  Seems to work, but...

-Mike

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Sebastian Andrzej Siewior 
On 2016-10-16 05:14:22 [+0200], Mike Galbraith wrote:
> 
> In v4.7, the driver switched to percpu compression streams, disabling
> preemption (get/put_cpu_ptr()).  Use get/put_cpu_light() instead.

I am not convinced that this will work. Nothing prevents
zram_bvec_write() to be reentrant on the same CPU what I can tell from
browsing over the code and since it uses zstrm->buffer for compression
it can go wrong. Also I don't know if crypto's tfm handler can be used
in parallel for any ops (it usually does not work for crypto).

I suggest a local lock or a good reason why the this patch works.

Sebastian

Re: [patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-17 Thread Sebastian Andrzej Siewior 
On 2016-10-16 05:14:22 [+0200], Mike Galbraith wrote:
> 
> In v4.7, the driver switched to percpu compression streams, disabling
> preemption (get/put_cpu_ptr()).  Use get/put_cpu_light() instead.

I am not convinced that this will work. Nothing prevents
zram_bvec_write() to be reentrant on the same CPU what I can tell from
browsing over the code and since it uses zstrm->buffer for compression
it can go wrong. Also I don't know if crypto's tfm handler can be used
in parallel for any ops (it usually does not work for crypto).

I suggest a local lock or a good reason why the this patch works.

Sebastian

[patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-15 Thread Mike Galbraith 

In v4.7, the driver switched to percpu compression streams, disabling
preemption (get/put_cpu_ptr()).  Use get/put_cpu_light() instead.

Signed-off-by: Mike Galbraith 
---
 drivers/block/zram/zcomp.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -118,12 +118,12 @@ ssize_t zcomp_available_show(const char
 
 struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
 {
-   return *get_cpu_ptr(comp->stream);
+   return *per_cpu_ptr(comp->stream, get_cpu_light());
 }
 
 void zcomp_stream_put(struct zcomp *comp)
 {
-   put_cpu_ptr(comp->stream);
+   put_cpu_light();
 }
 
 int zcomp_compress(struct zcomp_strm *zstrm,

[patch] drivers/zram: Don't disable preemption in zcomp_stream_get/put()

2016-10-15 Thread Mike Galbraith 

In v4.7, the driver switched to percpu compression streams, disabling
preemption (get/put_cpu_ptr()).  Use get/put_cpu_light() instead.

Signed-off-by: Mike Galbraith 
---
 drivers/block/zram/zcomp.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -118,12 +118,12 @@ ssize_t zcomp_available_show(const char
 
 struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
 {
-   return *get_cpu_ptr(comp->stream);
+   return *per_cpu_ptr(comp->stream, get_cpu_light());
 }
 
 void zcomp_stream_put(struct zcomp *comp)
 {
-   put_cpu_ptr(comp->stream);
+   put_cpu_light();
 }
 
 int zcomp_compress(struct zcomp_strm *zstrm,