Thread (34 messages) 34 messages, 3 authors, 2018-01-08

Re: [PATCH v1 01/10] bcache: exit bch_writeback_thread() with proper task state

From: Hannes Reinecke <hare@suse.de>
Date: 2018-01-08 07:09:20
Also in: linux-bcache

On 01/03/2018 03:03 PM, Coly Li wrote:
quoted hunk ↗ jump to hunk
Kernel thread routine bch_writeback_thread() has the following code block,

452                         set_current_state(TASK_INTERRUPTIBLE);
453
454                         if (kthread_should_stop())
455                                 return 0;
456
457                         schedule();
458                         continue;

At line 452, its status is set to TASK_INTERRUPTIBLE, and at line 454 if
kthread_should_stop() is true, a "return 0" at line 455 will to function
kernel/kthread.c:kthread() 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]".

Indeed 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.

In this patch, TASK_INTERRUPTIBLE is set after kthread_should_stop(),
so writeback kernel thread can exist and enter do_exit() with
TASK_RUNNING state. Warning message from might_sleep() is removed.

Signed-off-by: Coly Li <redacted>
---
 drivers/md/bcache/writeback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 56a37884ca8b..a57149803df6 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -449,11 +449,11 @@ static int bch_writeback_thread(void *arg)
 		    (!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())
 				return 0;
 
+			set_current_state(TASK_INTERRUPTIBLE);
 			schedule();
 			continue;
 		}
Actually, TASK_INTERRUPTIBLE is okay for kthread_should_stop(); you just
need to set it to 'TASK_RUNNING' before calling 'return 0'.

So I think a fix like

set_current_state(TASK_INTERRUPTIBLE);

if (kthread_should_stop()) {
    set_current_state(TASK_RUNNING);
    return 0;
}

schedule();

would be better.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help