[PATCH AUTOSEL for 3.18 096/101] bcache: properly set task state in bch_writeback_thread()

[Sasha Levin]

From: Coly Li 

[ Upstream commit 99361bbf26337186f02561109c17a4c4b1a7536a ]

Kernel thread routine bch_writeback_thread() has the following code block,

447 down_write(>writeback_lock);
448~450 if (check conditions) {
451 up_write(>writeback_lock);
452 set_current_state(TASK_INTERRUPTIBLE);
453
454 if (kthread_should_stop())
455 return 0;
456
457 schedule();
458 continue;
459 }

If condition check is true, its task state is set to TASK_INTERRUPTIBLE
and call schedule() to wait for others to wake up it.

There are 2 issues in current code,
1, Task state is set to TASK_INTERRUPTIBLE after the condition checks, if
   another process changes the condition and call wake_up_process(dc->
   writeback_thread), then at line 452 task state is set back to
   TASK_INTERRUPTIBLE, the writeback kernel thread will lose a chance to be
   waken up.
2, At line 454 if kthread_should_stop() is true, writeback kernel thread
   will return to kernel/kthread.c:kthread() with TASK_INTERRUPTIBLE and
   call do_exit(). It is not good to enter do_exit() with task state
   TASK_INTERRUPTIBLE, in following code path might_sleep() is called and a
   warning message is reported by __might_sleep(): "WARNING: do not call
   blocking ops when !TASK_RUNNING; state=1 set at []".

For the first issue, task state should be set before condition checks.
Ineed because dc->writeback_lock is required when modifying all the
conditions, calling set_current_state() inside code block where dc->
writeback_lock is hold is safe. But this is quite implicit, so I still move
set_current_state() before all the condition checks.

For the second issue, frankley speaking it does not hurt when kernel thread
exits with TASK_INTERRUPTIBLE state, but this warning message scares users,
makes them feel there might be something risky with bcache and hurt their
data.  Setting task state to TASK_RUNNING before returning fixes this
problem.

In alloc.c:allocator_wait(), there is also a similar issue, and is also
fixed in this patch.

Changelog:
v3: merge two similar fixes into one patch
v2: fix the race issue in v1 patch.
v1: initial buggy fix.

Signed-off-by: Coly Li 
Reviewed-by: Hannes Reinecke 
Reviewed-by: Michael Lyle 
Cc: Michael Lyle 
Cc: Junhui Tang 
Signed-off-by: Jens Axboe 
Signed-off-by: Sasha Levin 
---
 drivers/md/bcache/alloc.c | 4 +++-
 drivers/md/bcache/writeback.c | 7 +--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index ea47980949ef..c1da2321bf26 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -285,8 +285,10 @@ do {   
\
break;  \
\
mutex_unlock(&(ca)->set->bucket_lock);  \
-   if (kthread_should_stop())  \
+   if (kthread_should_stop()) {\
+   set_current_state(TASK_RUNNING);\
return 0;   \
+   }   \
\
try_to_freeze();\
schedule(); \
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index b0667b321a3f..50726f12a7c3 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -425,19 +425,22 @@ static int bch_writeback_thread(void *arg)
 
while (!kthread_should_stop()) {
down_write(>writeback_lock);
+   set_current_state(TASK_INTERRUPTIBLE);
if (!atomic_read(>has_dirty) ||
(!test_bit(BCACHE_DEV_DETACHING, >disk.flags) &&
 !dc->writeback_running)) {
up_write(>writeback_lock);
-   set_current_state(TASK_INTERRUPTIBLE);
 
-   if (kthread_should_stop())
+   if (kthread_should_stop()) {
+   set_current_state(TASK_RUNNING);
return 0;
+   }
 
try_to_freeze();
schedule();
continue;
}
+   set_current_state(TASK_RUNNING);
 
searched_full_index = refill_dirty(dc);
 
-- 
2.15.1

[PATCH AUTOSEL for 3.18 096/101] bcache: properly set task state in bch_writeback_thread()

[Sasha Levin]

From: Coly Li 

[ Upstream commit 99361bbf26337186f02561109c17a4c4b1a7536a ]

Kernel thread routine bch_writeback_thread() has the following code block,

447 down_write(>writeback_lock);
448~450 if (check conditions) {
451 up_write(>writeback_lock);
452 set_current_state(TASK_INTERRUPTIBLE);
453
454 if (kthread_should_stop())
455 return 0;
456
457 schedule();
458 continue;
459 }

If condition check is true, its task state is set to TASK_INTERRUPTIBLE
and call schedule() to wait for others to wake up it.

There are 2 issues in current code,
1, Task state is set to TASK_INTERRUPTIBLE after the condition checks, if
   another process changes the condition and call wake_up_process(dc->
   writeback_thread), then at line 452 task state is set back to
   TASK_INTERRUPTIBLE, the writeback kernel thread will lose a chance to be
   waken up.
2, At line 454 if kthread_should_stop() is true, writeback kernel thread
   will return to kernel/kthread.c:kthread() with TASK_INTERRUPTIBLE and
   call do_exit(). It is not good to enter do_exit() with task state
   TASK_INTERRUPTIBLE, in following code path might_sleep() is called and a
   warning message is reported by __might_sleep(): "WARNING: do not call
   blocking ops when !TASK_RUNNING; state=1 set at []".

For the first issue, task state should be set before condition checks.
Ineed because dc->writeback_lock is required when modifying all the
conditions, calling set_current_state() inside code block where dc->
writeback_lock is hold is safe. But this is quite implicit, so I still move
set_current_state() before all the condition checks.

For the second issue, frankley speaking it does not hurt when kernel thread
exits with TASK_INTERRUPTIBLE state, but this warning message scares users,
makes them feel there might be something risky with bcache and hurt their
data.  Setting task state to TASK_RUNNING before returning fixes this
problem.

In alloc.c:allocator_wait(), there is also a similar issue, and is also
fixed in this patch.

Changelog:
v3: merge two similar fixes into one patch
v2: fix the race issue in v1 patch.
v1: initial buggy fix.

Signed-off-by: Coly Li 
Reviewed-by: Hannes Reinecke 
Reviewed-by: Michael Lyle 
Cc: Michael Lyle 
Cc: Junhui Tang 
Signed-off-by: Jens Axboe 
Signed-off-by: Sasha Levin 
---
 drivers/md/bcache/alloc.c | 4 +++-
 drivers/md/bcache/writeback.c | 7 +--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index ea47980949ef..c1da2321bf26 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -285,8 +285,10 @@ do {   
\
break;  \
\
mutex_unlock(&(ca)->set->bucket_lock);  \
-   if (kthread_should_stop())  \
+   if (kthread_should_stop()) {\
+   set_current_state(TASK_RUNNING);\
return 0;   \
+   }   \
\
try_to_freeze();\
schedule(); \
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index b0667b321a3f..50726f12a7c3 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -425,19 +425,22 @@ static int bch_writeback_thread(void *arg)
 
while (!kthread_should_stop()) {
down_write(>writeback_lock);
+   set_current_state(TASK_INTERRUPTIBLE);
if (!atomic_read(>has_dirty) ||
(!test_bit(BCACHE_DEV_DETACHING, >disk.flags) &&
 !dc->writeback_running)) {
up_write(>writeback_lock);
-   set_current_state(TASK_INTERRUPTIBLE);
 
-   if (kthread_should_stop())
+   if (kthread_should_stop()) {
+   set_current_state(TASK_RUNNING);
return 0;
+   }
 
try_to_freeze();
schedule();
continue;
}
+   set_current_state(TASK_RUNNING);
 
searched_full_index = refill_dirty(dc);
 
-- 
2.15.1