[PATCH] bcache: set task state correctly in allocator_wait()

[PATCH] bcache: set task state correctly in allocator_wait()

[Coly Li]

Kthread function bch_allocator_thread() references allocator_wait(ca, cond)
and when kthread_should_stop() is true, this kthread exits.

The problem is, if kthread_should_stop() is true, macro allocator_wait()
calls "return 0" with current task state TASK_INTERRUPTIBLE. After function
bch_allocator_thread() returns to do_exit(), there are some blocking
operations are called, then a kenrel warning is popped up by __might_sleep
from kernel/sched/core.c,
  "WARNING: do not call blocking ops when !TASK_RUNNING; state=1 set at [xxxx]"

If the task is interrupted and preempted out, since its status is
TASK_INTERRUPTIBLE, it means scheduler won't pick it back to run forever,
and the allocator thread may hang in do_exit().

This patch sets allocator kthread state back to TASK_RUNNING before it
returns to do_exit(), which avoids a potential deadlock.

Signed-off-by: Coly Li <colyli@suse.de>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/alloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index a27d85232ce1..996ebbabd819 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -286,9 +286,12 @@ do {									\
 		if (cond)						\
 			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);			\
-- 
2.13.6

Re: [PATCH] bcache: set task state correctly in allocator_wait()

[Hannes Reinecke]

On 11/22/2017 01:33 PM, Coly Li wrote:
> Kthread function bch_allocator_thread() references allocator_wait(ca, cond)
> and when kthread_should_stop() is true, this kthread exits.
> 
> The problem is, if kthread_should_stop() is true, macro allocator_wait()
> calls "return 0" with current task state TASK_INTERRUPTIBLE. After function
> bch_allocator_thread() returns to do_exit(), there are some blocking
> operations are called, then a kenrel warning is popped up by __might_sleep
> from kernel/sched/core.c,
>   "WARNING: do not call blocking ops when !TASK_RUNNING; state=1 set at [xxxx]"
> 
> If the task is interrupted and preempted out, since its status is
> TASK_INTERRUPTIBLE, it means scheduler won't pick it back to run forever,
> and the allocator thread may hang in do_exit().
> 
> This patch sets allocator kthread state back to TASK_RUNNING before it
> returns to do_exit(), which avoids a potential deadlock.
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/bcache/alloc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index a27d85232ce1..996ebbabd819 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -286,9 +286,12 @@ do {									\
>  		if (cond)						\
>  			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);			\
> 
_Actually_ there is a push to remove all kthreads in the kernel, as they
don't play nice together with RT.
So while you're at it, do you think it'd be possible to convert it to a
workqueue? Sebastian will be happy to help you here, right, Sebastian?

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)

Re: [PATCH] bcache: set task state correctly in allocator_wait()

[Sebastian Andrzej Siewior]

On 2017-11-22 15:10:51 [+0100], Hannes Reinecke wrote:
> On 11/22/2017 01:33 PM, Coly Li wrote:
> > Kthread function bch_allocator_thread() references allocator_wait(ca, cond)
> > and when kthread_should_stop() is true, this kthread exits.
> > 
> > The problem is, if kthread_should_stop() is true, macro allocator_wait()
> > calls "return 0" with current task state TASK_INTERRUPTIBLE. After function
> > bch_allocator_thread() returns to do_exit(), there are some blocking
> > operations are called, then a kenrel warning is popped up by __might_sleep
> > from kernel/sched/core.c,
> >   "WARNING: do not call blocking ops when !TASK_RUNNING; state=1 set at [xxxx]"
> > 
> > If the task is interrupted and preempted out, since its status is
> > TASK_INTERRUPTIBLE, it means scheduler won't pick it back to run forever,
> > and the allocator thread may hang in do_exit().
> > 
> > This patch sets allocator kthread state back to TASK_RUNNING before it
> > returns to do_exit(), which avoids a potential deadlock.
> > 
> > Signed-off-by: Coly Li <colyli@suse.de>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/md/bcache/alloc.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> > index a27d85232ce1..996ebbabd819 100644
> > --- a/drivers/md/bcache/alloc.c
> > +++ b/drivers/md/bcache/alloc.c
> > @@ -286,9 +286,12 @@ do {									\
> >  		if (cond)						\
> >  			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);			\
> > 
> _Actually_ there is a push to remove all kthreads in the kernel, as they
> don't play nice together with RT.

with RT? If RT as in PREEMPT-RT then this is news to me. The reason why
I removed the per-CPU kthreads in the scsi driver(s) was because it was
nonsense in regards to CPU-hotplug and workqueue infrastructure is way
nicer for that. Not to mention that it made the code simpler.

> So while you're at it, do you think it'd be possible to convert it to a
> workqueue? Sebastian will be happy to help you here, right, Sebastian?
If commit 4b9bc86d5a99 ("fcoe: convert to kworker") does not explain I
can try to assist.

> Cheers,
> 
> Hannes

Sebastian

Re: [PATCH] bcache: set task state correctly in allocator_wait()

[Coly Li]

On 22/11/2017 10:55 PM, Sebastian Andrzej Siewior wrote:
> On 2017-11-22 15:10:51 [+0100], Hannes Reinecke wrote:
>> On 11/22/2017 01:33 PM, Coly Li wrote:
>>> Kthread function bch_allocator_thread() references allocator_wait(ca, cond)
>>> and when kthread_should_stop() is true, this kthread exits.
>>>
>>> The problem is, if kthread_should_stop() is true, macro allocator_wait()
>>> calls "return 0" with current task state TASK_INTERRUPTIBLE. After function
>>> bch_allocator_thread() returns to do_exit(), there are some blocking
>>> operations are called, then a kenrel warning is popped up by __might_sleep
>>> from kernel/sched/core.c,
>>>   "WARNING: do not call blocking ops when !TASK_RUNNING; state=1 set at [xxxx]"
>>>
>>> If the task is interrupted and preempted out, since its status is
>>> TASK_INTERRUPTIBLE, it means scheduler won't pick it back to run forever,
>>> and the allocator thread may hang in do_exit().
>>>
>>> This patch sets allocator kthread state back to TASK_RUNNING before it
>>> returns to do_exit(), which avoids a potential deadlock.
>>>
>>> Signed-off-by: Coly Li <colyli@suse.de>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/md/bcache/alloc.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
>>> index a27d85232ce1..996ebbabd819 100644
>>> --- a/drivers/md/bcache/alloc.c
>>> +++ b/drivers/md/bcache/alloc.c
>>> @@ -286,9 +286,12 @@ do {									\
>>>  		if (cond)						\
>>>  			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);			\
>>>
>> _Actually_ there is a push to remove all kthreads in the kernel, as they
>> don't play nice together with RT.
> 
> with RT? If RT as in PREEMPT-RT then this is news to me. The reason why
> I removed the per-CPU kthreads in the scsi driver(s) was because it was
> nonsense in regards to CPU-hotplug and workqueue infrastructure is way
> nicer for that. Not to mention that it made the code simpler.
> 
>> So while you're at it, do you think it'd be possible to convert it to a
>> workqueue? Sebastian will be happy to help you here, right, Sebastian?
> If commit 4b9bc86d5a99 ("fcoe: convert to kworker") does not explain I
> can try to assist.

Hi Hannes and Sebastian,

Thanks for the informative input. I see the point why convert from
kthread to per-cpu kworker. Bucket allocation is not a very hot code
path to deserve per-cpu work queue, a per-cached device work queue is
enough, as other places where kworker is used in bcache code. Bcache
used to have circular dependency issue on kworker queue, unless I attach
the kworker to a new and separate workqueue, there might be potential
possibility to introduce a new circular locking on global workqueues.

It is better to make less modification for now, and later after Michael
finishes all locking clean up, we can come to see the kthread to kworker
conversion.

Thanks.

Coly Li

Re: [PATCH] bcache: set task state correctly in allocator_wait()

[Michael Lyle]

Hey everyone--

On 11/22/2017 06:10 AM, Hannes Reinecke wrote:
> _Actually_ there is a push to remove all kthreads in the kernel, as they
> don't play nice together with RT.
> So while you're at it, do you think it'd be possible to convert it to a
> workqueue? Sebastian will be happy to help you here, right, Sebastian?

I don't see a reason why moving this away from a thread would be
advantageous.  There's neither devastating complexity in the current
form nor a need for additional concurrency (at least right now).

As it turns out, we're broken for RT for other reasons, but that has to
do with our use of closures and releasing locks in different threads
from where they're allocated.  The natural answer is completions, but
that's not easily done with the current structure of the code.

This current code is neither a problem for RT nor for
performance/legibility on mainline.

Mike