On 01/03/2018 03:03 PM, Coly Li wrote:
> 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 <colyli@xxxxxxx>
> ---
> 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@xxxxxxx +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)
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
