From: Jiri Kosina <[email protected]>
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 <[email protected]>
---
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
From: Jiri Kosina <[email protected]>
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 <[email protected]>
---
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
From: Jiri Kosina <[email protected]>
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 <[email protected]>
---
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
On Tue, 19 Apr 2016, Jiri Kosina wrote:
> From: Jiri Kosina <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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
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
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