[PATCH] bcache: bch_writeback_thread() is not freezable

[PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

bch_writeback_thread() is calling try_to_freeze(), but that's just an 
expensive no-op given the fact that the thread is not marked freezable.

I/O helper kthreads, exactly such as the bcache writeback thread, actually 
shouldn't be freezable, because they are potentially necessary for 
finalizing the image write-out.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/md/bcache/writeback.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index b9346cd..6012367 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -12,7 +12,6 @@
 #include "writeback.h"
 
 #include <linux/delay.h>
-#include <linux/freezer.h>
 #include <linux/kthread.h>
 #include <trace/events/bcache.h>
 
@@ -228,7 +227,6 @@ static void read_dirty(struct cached_dev *dc)
 	 */
 
 	while (!kthread_should_stop()) {
-		try_to_freeze();
 
 		w = bch_keybuf_next(&dc->writeback_keys);
 		if (!w)
@@ -433,7 +431,6 @@ static int bch_writeback_thread(void *arg)
 			if (kthread_should_stop())
 				return 0;
 
-			try_to_freeze();
 			schedule();
 			continue;
 		}
-- 
Jiri Kosina
SUSE Labs

[PATCH] bcache: bch_allocator_thread() is not freezable

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

bch_allocator_thread() is calling try_to_freeze(), but that's just an 
expensive no-op given the fact that the thread is not marked freezable.

Bucket allocator has to be up and running to the very last stages of the 
suspend, as the bcache I/O that's in flight (think of writing an 
hibernation image to a swap device served by bcache).

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/md/bcache/alloc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index 8eeab72..ca4abe1 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -64,7 +64,6 @@
 #include "btree.h"
 
 #include <linux/blkdev.h>
-#include <linux/freezer.h>
 #include <linux/kthread.h>
 #include <linux/random.h>
 #include <trace/events/bcache.h>
@@ -288,7 +287,6 @@ do {									\
 		if (kthread_should_stop())				\
 			return 0;					\
 									\
-		try_to_freeze();					\
 		schedule();						\
 		mutex_lock(&(ca)->set->bucket_lock);			\
 	}								\

-- 
Jiri Kosina
SUSE Labs

[PATCH] bcache: bch_gc_thread() is not freezable

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

bch_gc_thread() doesn't mark itself freezable, so calling try_to_freeze() 
in its context is just an expensive no-op.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/md/bcache/btree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 22b9e34..eab505e 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -27,7 +27,6 @@
 
 #include <linux/slab.h>
 #include <linux/bitops.h>
-#include <linux/freezer.h>
 #include <linux/hash.h>
 #include <linux/kthread.h>
 #include <linux/prefetch.h>
@@ -1787,7 +1786,6 @@ again:
 
 		mutex_unlock(&c->bucket_lock);
 
-		try_to_freeze();
 		schedule();
 	}
 
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Eric Wheeler]

On Tue, 19 Apr 2016, Jiri Kosina wrote:

> From: Jiri Kosina <jkosina@suse.cz>
> 
> bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> expensive no-op given the fact that the thread is not marked freezable.
> 
> I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> shouldn't be freezable, because they are potentially necessary for 
> finalizing the image write-out.

This is good timing, as Maciej Piechotka just reported a hang when 
suspending his system.

What is the proper way to safely support suspend?  Assuming the 
try_to_freeze() calls are in the right place, should we simply 
set_freezable() on these kthreads?


--
Eric Wheeler


> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/md/bcache/writeback.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index b9346cd..6012367 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -12,7 +12,6 @@
>  #include "writeback.h"
>  
>  #include <linux/delay.h>
> -#include <linux/freezer.h>
>  #include <linux/kthread.h>
>  #include <trace/events/bcache.h>
>  
> @@ -228,7 +227,6 @@ static void read_dirty(struct cached_dev *dc)
>  	 */
>  
>  	while (!kthread_should_stop()) {
> -		try_to_freeze();
>  
>  		w = bch_keybuf_next(&dc->writeback_keys);
>  		if (!w)
> @@ -433,7 +431,6 @@ static int bch_writeback_thread(void *arg)
>  			if (kthread_should_stop())
>  				return 0;
>  
> -			try_to_freeze();
>  			schedule();
>  			continue;
>  		}
> -- 
> Jiri Kosina
> SUSE Labs
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Tue, 19 Apr 2016, Eric Wheeler wrote:

> > bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> > expensive no-op given the fact that the thread is not marked freezable.
> > 
> > I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> > shouldn't be freezable, because they are potentially necessary for 
> > finalizing the image write-out.
> 
> This is good timing, as Maciej Piechotka just reported a hang when 
> suspending his system.

Could you please point me to the actual report? Thanks.

> What is the proper way to safely support suspend?  Assuming the 
> try_to_freeze() calls are in the right place, should we simply 
> set_freezable() on these kthreads?

Unfortunately, this is really a tricky question; the issue is that frezing 
semantics is rather undefined for kthreads. For starters, please see

	https://lwn.net/Articles/662703/
	http://lkml.org/lkml/2007/4/27/608

I don't belive in freezable kthreads which serve as I/O helpers. Such 
threads simply have to keep going until the image is written out and 
machine powered down.

So I'd like to start with understanding how bcache is preventning suspend. 
Maciej?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Eric Wheeler]

On Wed, 20 Apr 2016, Jiri Kosina wrote:
> On Tue, 19 Apr 2016, Eric Wheeler wrote:
> 
> > > bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> > > expensive no-op given the fact that the thread is not marked freezable.
> > > 
> > > I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> > > shouldn't be freezable, because they are potentially necessary for 
> > > finalizing the image write-out.
> > 
> > This is good timing, as Maciej Piechotka just reported a hang when 
> > suspending his system.
> 
> Could you please point me to the actual report? Thanks.
>
> On Tue, 19 Apr 2016, Maciej Piechotka wrote:
> Eric Wheeler <bcache <at> lists.ewheeler.net> writes:
> > Interesting.  Can you collect the dmesg output as it suspends/resumes via
> > serial or something other means?
>
> I'll try to capture the output today.

No technical data yet, but this is the thread:  

http://comments.gmane.org/gmane.linux.kernel.bcache.devel/3820

> > What is the proper way to safely support suspend?  Assuming the 
> > try_to_freeze() calls are in the right place, should we simply 
> > set_freezable() on these kthreads?
> 
> Unfortunately, this is really a tricky question; the issue is that frezing 
> semantics is rather undefined for kthreads. For starters, please see
> 
> 	https://lwn.net/Articles/662703/
> 	http://lkml.org/lkml/2007/4/27/608

Interesting indeed.  So suspend should succeed independent of kernel 
threads since we want to get rid of freezable kthreads? 

Does this also mean that IO kthreads will always break suspend?

> I don't belive in freezable kthreads which serve as I/O helpers. Such 
> threads simply have to keep going until the image is written out and 
> machine powered down.
> 
> So I'd like to start with understanding how bcache is preventning suspend. 
> Maciej?

We await backtraces from Maciej, but I can say that bcache uses only two 
kthreads, one for garbage collection and another for writeback.  

Speculation: The writeback thread can (probably) be made unrunnable at any 
time without issue since it is (should be) fully asynchronous.  However, 
garbage collection might deadlock if the GC thread is unrunnable while 
hibernate (suspend?) IO is writing through bcache while bcache waits for 
GC to complete under allocation contention.  I'm not familiar with the 
bcache allocator details, so anyone else please chime here.

Presumably, GC is only unsafe during writes to the cache for blocks that 
are not yet cached but would cause a cache allocation.  If so, then 
perhaps we can hook the pending suspend, set cache_mode to "writearound"  
to prevent btree changes, and restore the cache_mode on resume. It will be 
interesting to see the backtrace if Maciej can get one out of the system.


--
Eric Wheeler

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Fri, 22 Apr 2016, Eric Wheeler wrote:

> > I'll try to capture the output today.
> 
> No technical data yet, but this is the thread:  
> 
> http://comments.gmane.org/gmane.linux.kernel.bcache.devel/3820

Thanks. It's highly unlikely that this will be fixed by these three 
patches though (the code should be functionally equivalent before and 
after).

> > > What is the proper way to safely support suspend?  Assuming the 
> > > try_to_freeze() calls are in the right place, should we simply 
> > > set_freezable() on these kthreads?
> > 
> > Unfortunately, this is really a tricky question; the issue is that frezing 
> > semantics is rather undefined for kthreads. For starters, please see
> > 
> > 	https://lwn.net/Articles/662703/
> > 	http://lkml.org/lkml/2007/4/27/608
> 
> Interesting indeed.  So suspend should succeed independent of kernel 
> threads since we want to get rid of freezable kthreads? 

Basically kthreads periodically run and are scheduled out, that by itself 
doesn't interfere with suspend. Once they are all scheduled out, suspend 
happens.

> Does this also mean that IO kthreads will always break suspend?

I am afraid that "IO kthreads" isn't really exact enough expression. 
Kthreads which are essential for making sure that I/O completes, they 
actually have to obviously keep running before the whole image is written 
out.

The other threads basically keep on with their own business before they 
all end up in schedule().

> Speculation: The writeback thread can (probably) be made unrunnable at any 
> time without issue since it is (should be) fully asynchronous.  However, 
> garbage collection might deadlock if the GC thread is unrunnable while 
> hibernate (suspend?) IO is writing through bcache while bcache waits for 
> GC to complete under allocation contention.  I'm not familiar with the 
> bcache allocator details, so anyone else please chime here.
> 
> Presumably, GC is only unsafe during writes to the cache for blocks that 
> are not yet cached but would cause a cache allocation.  If so, then 
> perhaps we can hook the pending suspend, set cache_mode to "writearound"  
> to prevent btree changes, and restore the cache_mode on resume. It will be 
> interesting to see the backtrace if Maciej can get one out of the system.

That would be really helpful. Or crashdump image.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Tue, 19 Apr 2016, Jiri Kosina wrote:

> From: Jiri Kosina <jkosina@suse.cz>
> 
> bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> expensive no-op given the fact that the thread is not marked freezable.
> 
> I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> shouldn't be freezable, because they are potentially necessary for 
> finalizing the image write-out.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Let's send a friendly ping on these three patches after two weeks...

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Eric Wheeler]

On Mon, 2 May 2016, Jiri Kosina wrote:
> On Tue, 19 Apr 2016, Jiri Kosina wrote:
> 
> > From: Jiri Kosina <jkosina@suse.cz>
> > 
> > bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> > expensive no-op given the fact that the thread is not marked freezable.
> > 
> > I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> > shouldn't be freezable, because they are potentially necessary for 
> > finalizing the image write-out.
> > 
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> 
> Let's send a friendly ping on these three patches after two weeks...

Maciej,

Were you able to get a backtrace?


--
Eric Wheeler


> 
> -- 
> Jiri Kosina
> SUSE Labs
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Tue, 10 May 2016, Eric Wheeler wrote:

> > > bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> > > expensive no-op given the fact that the thread is not marked freezable.
> > > 
> > > I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> > > shouldn't be freezable, because they are potentially necessary for 
> > > finalizing the image write-out.
> > > 
> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > 
> > Let's send a friendly ping on these three patches after two weeks...
> 
> Maciej,
> 
> Were you able to get a backtrace?

While I'd definitely like to understand the issues in interaction between 
hibernation and bcache Maciej is supposedly seeing, this shouldn't hold 
the application of the three patches in this series, as they are just a 
cleanup that doesn't affect the behavior of the code in question.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Mon, 2 May 2016, Jiri Kosina wrote:

> > From: Jiri Kosina <jkosina@suse.cz>
> > 
> > bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> > expensive no-op given the fact that the thread is not marked freezable.
> > 
> > I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> > shouldn't be freezable, because they are potentially necessary for 
> > finalizing the image write-out.
> > 
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> 
> Let's send a friendly ping on these three patches after two weeks...

After a month, I think, it's time for another ping.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Wed, 18 May 2016, Jiri Kosina wrote:

> > > bch_writeback_thread() is calling try_to_freeze(), but that's just an 
> > > expensive no-op given the fact that the thread is not marked freezable.
> > > 
> > > I/O helper kthreads, exactly such as the bcache writeback thread, actually 
> > > shouldn't be freezable, because they are potentially necessary for 
> > > finalizing the image write-out.
> > > 
> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > 
> > Let's send a friendly ping on these three patches after two weeks...
> 
> After a month, I think, it's time for another ping.

[ adding Jens to CC ]

Ok, I call it officially ridiculous now. I guess I'll be resending through 
akpm.

Is it a time to claim (by MAINTAINERS entry) the in-tree bcache orphaned?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jens Axboe]

On 05/23/2016 05:05 PM, Jiri Kosina wrote:
> On Wed, 18 May 2016, Jiri Kosina wrote:
>
>>>> bch_writeback_thread() is calling try_to_freeze(), but that's just an
>>>> expensive no-op given the fact that the thread is not marked freezable.
>>>>
>>>> I/O helper kthreads, exactly such as the bcache writeback thread, actually
>>>> shouldn't be freezable, because they are potentially necessary for
>>>> finalizing the image write-out.
>>>>
>>>> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>>>
>>> Let's send a friendly ping on these three patches after two weeks...
>>
>> After a month, I think, it's time for another ping.
>
> [ adding Jens to CC ]
>
> Ok, I call it officially ridiculous now. I guess I'll be resending through
> akpm.

You can just send it to me, I've funneled bcache stable fixes before.

> Is it a time to claim (by MAINTAINERS entry) the in-tree bcache orphaned?

I think so.

-- 
Jens Axboe

Re: [PATCH] bcache: bch_writeback_thread() is not freezable

[Jiri Kosina]

On Tue, 24 May 2016, Jens Axboe wrote:

> > Ok, I call it officially ridiculous now. I guess I'll be resending through
> > akpm.
> 
> You can just send it to me, I've funneled bcache stable fixes before.
> 
> > Is it a time to claim (by MAINTAINERS entry) the in-tree bcache orphaned?
> 
> I think so.

Ok, thanks. I'll add another patch to the series that does this change and 
send it your way later today.

-- 
Jiri Kosina
SUSE Labs