bcache: properly set task state in bch_writeback_thread()

Web:        https://git.kernel.org/torvalds/c/99361bbf26337186f02561109c17a4c4b1a7536a
Commit:     99361bbf26337186f02561109c17a4c4b1a7536a
Parent:     c4dc2497d50d9c6fb16aa0d07b6a14f3b2adb1e0
Refname:    refs/heads/master
Author:     Coly Li <[email protected]>
AuthorDate: Wed Feb 7 11:41:41 2018 -0800
Committer:  Jens Axboe <[email protected]>
CommitDate: Wed Feb 7 12:50:01 2018 -0700

    bcache: properly set task state in bch_writeback_thread()
    
    Kernel thread routine bch_writeback_thread() has the following code block,
    
    447         down_write(&dc->writeback_lock);
    448~450     if (check conditions) {
    451                 up_write(&dc->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 [xxxx]".
    
    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 <[email protected]>
    Reviewed-by: Hannes Reinecke <[email protected]>
    Reviewed-by: Michael Lyle <[email protected]>
    Cc: Michael Lyle <[email protected]>
    Cc: Junhui Tang <[email protected]>
    Signed-off-by: Jens Axboe <[email protected]>
---
 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 6cc6c0f9c3a9..458e1d38577d 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -287,8 +287,10 @@ do {									\
 			break;						\
 									\
 		mutex_unlock(&(ca)->set->bucket_lock);			\
-		if (kthread_should_stop())				\
+		if (kthread_should_stop()) {				\
+			set_current_state(TASK_RUNNING);		\
 			return 0;					\
+		}							\
 									\
 		schedule();						\
 		mutex_lock(&(ca)->set->bucket_lock);			\
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 51306a19ab03..58218f7e77c3 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -564,18 +564,21 @@ static int bch_writeback_thread(void *arg)
 
 	while (!kthread_should_stop()) {
 		down_write(&dc->writeback_lock);
+		set_current_state(TASK_INTERRUPTIBLE);
 		if (!atomic_read(&dc->has_dirty) ||
 		    (!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) &&
 		     !dc->writeback_running)) {
 			up_write(&dc->writeback_lock);
-			set_current_state(TASK_INTERRUPTIBLE);
 
-			if (kthread_should_stop())
+			if (kthread_should_stop()) {
+				set_current_state(TASK_RUNNING);
 				return 0;
+			}
 
 			schedule();
 			continue;
 		}
+		set_current_state(TASK_RUNNING);
 
 		searched_full_index = refill_dirty(dc);
 
--
To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html