| Message ID | 20180103140325.63175-2-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/03/2018 06:03 AM, 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@suse.de> Reviewed-by: Michael Lyle <mlyle@lyle.org>
On 04/01/2018 1:08 AM, Michael Lyle wrote: > On 01/03/2018 06:03 AM, 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@suse.de> Hi Mike, I feel uncomfortable with this patch, can you help me to double check ? In my patch, I set task state to TASK_INTERRUPTIBLE after checking the following conditions, 469 if (!atomic_read(&dc->has_dirty) || 470 (!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) && 471 !dc->writeback_running)) { And set_current_state(TASK_INTERRUPTIBLE) is after up_write(&dc->writeback_lock), there should be a potential race window between up_write() and set_current_state() in my patch. If wake_up_process(dc->writeback_thread) is called in this window, and then current state is set to TASK_INTERRUPTIBLE again, writeback thread will lose a chance to be waken up. It seems I need to call set_current_state(TASK_INTERRUPTIBLE) before up_write(&dc->writeback_lock), then dc->writeback_lock will protect and avoid the race. Or move set_current_state(TASK_INTERRUPTIBLE) to the location before the condition checks in line 469-471. This race window is very small but I believe it exists, I will fix it in v2 patch set. Thanks. Coly Li
Coly--- On Fri, Jan 5, 2018 at 9:05 AM, Coly Li <colyli@suse.de> wrote: > It seems I need to call set_current_state(TASK_INTERRUPTIBLE) before > up_write(&dc->writeback_lock), then dc->writeback_lock will protect and > avoid the race. Or move set_current_state(TASK_INTERRUPTIBLE) to the > location before the condition checks in line 469-471. I think you're right. Sorry I missed this in my review. I will pull this from my testing/for-416 set. Mike
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@suse.de> > --- > 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
On 08/01/2018 3:09 PM, Hannes Reinecke wrote: > 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@suse.de> >> --- >> 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. Hi Hannes, Yes, this is same as v2 patch does. Thanks. Coly Li
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; }
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@suse.de> --- drivers/md/bcache/writeback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)