Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1382457 > unrolled thread

[PATCH] bcache: bch_writeback_thread() is not freezable

Back to article view | Back to linux.kernel

Contents

  [PATCH] bcache: bch_writeback_thread() is not freezable Jiri Kosina <jikos@kernel.org> - 2016-04-19 14:40 +0200
    [PATCH] bcache: bch_allocator_thread() is not freezable Jiri Kosina <jikos@kernel.org> - 2016-04-19 14:40 +0200
    Re: [PATCH] bcache: bch_writeback_thread() is not freezable Eric Wheeler <bcache@lists.ewheeler.net> - 2016-04-19 23:10 +0200
      Re: [PATCH] bcache: bch_writeback_thread() is not freezable Jiri Kosina <jikos@kernel.org> - 2016-04-20 00:30 +0200
        Re: [PATCH] bcache: bch_writeback_thread() is not freezable Eric Wheeler <bcache@lists.ewheeler.net> - 2016-04-22 03:30 +0200
          Re: [PATCH] bcache: bch_writeback_thread() is not freezable Jiri Kosina <jikos@kernel.org> - 2016-04-25 10:30 +0200

#1382457 — [PATCH] bcache: bch_writeback_thread() is not freezable

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

[toc] | [next] | [standalone]

#1382458 — [PATCH] bcache: bch_allocator_thread() is not freezable

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

[toc] | [prev] | [next] | [standalone]

#1382843

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
> 

[toc] | [prev] | [next] | [standalone]

#1382871

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

[toc] | [prev] | [next] | [standalone]

#1384659

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

[toc] | [prev] | [next] | [standalone]

#1386100

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web