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

[Coly Li <colyli@suse.de>]

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