[PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[Paul Gortmaker]

We ran into a splat during reboot that was caused by the percpu_ref_kill
garbage collection in the fs/aio code.

After researching a bit, it was determined that this was the same
problem reported by Mike a few months ago[1].  In that thread, several
possibilities were discussed, but no patches were added to -rt in
the end out of that discussion.

This implements one of the possible solutions discussed there, which
is -rt specific and not mainline appropriate.

After using that patch, we found another new follow on splat that
was also in aio garbage collection and so we deal with that by using
the callback ("confirm") version of percpu_ref_kill to delay the
schedule_work to a non-atomic context to avoid the second splat.  I
think this second patch could be used in mainline, if one desired to
do so.

[1] https://lkml.org/lkml/2014/6/8/10

Paul.
---

Paul Gortmaker (2):
  aio: don't take the ctx_lock during rcu garbage collection at reboot
  aio: fix splat in free_ioctx_reqs by using percpu callback

 fs/aio.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH-rt 1/2] aio: don't take the ctx_lock during rcu garbage collection at reboot

[Paul Gortmaker]

On reboot in 3.14-rt we see this splat:

BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:905
in_atomic(): 1, irqs_disabled(): 0, pid: 84, name: rcuc/12
Preemption disabled at:[<ffffffff810b29bc>] rcu_cpu_kthread+0x28c/0x700

CPU: 18 PID: 121 Comm: rcuc/18 Tainted: G      D      3.14.33-rt28-WR7.0.0.0_ovp+ #2
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS SE5C600.86B.02.01.0002.082220131453 08/22/2013
 ffff880fad3e7a70 ffff880fb11cfd58 ffffffff81a0bcee 0000000000000000
 ffff880fb11cfd70 ffffffff8107b5eb ffff880fad3e7a40 ffff880fb11cfd88
 ffffffff81a136b0 ffff880fad3e7900 ffff880fb11cfdb0 ffffffff811c925c
Call Trace:
 [<ffffffff81a0bcee>] dump_stack+0x4e/0x7a
 [<ffffffff8107b5eb>] __might_sleep+0xdb/0x150
 [<ffffffff81a136b0>] rt_spin_lock+0x20/0x50
 [<ffffffff811c925c>] free_ioctx_users+0x2c/0xe0
 [<ffffffff8140136b>] percpu_ref_kill_rcu+0x11b/0x120
 [<ffffffff810aded5>] rcu_cpu_kthread+0x295/0x6f0
 [<ffffffff81076d8d>] smpboot_thread_fn+0x17d/0x2b0
 [<ffffffff81a110f0>] ? schedule+0x30/0xa0
 [<ffffffff81076c10>] ? SyS_setgroups+0x170/0x170
 [<ffffffff8106f4a4>] kthread+0xc4/0xe0
 [<ffffffff8106f3e0>] ? flush_kthread_worker+0x90/0x90
 [<ffffffff81a147ec>] ret_from_fork+0x7c/0xb0
 [<ffffffff8106f3e0>] ? flush_kthread_worker+0x90/0x90

The problem is as stated in lib/percpu-refcount.c:

 * Note that @release must not sleep - it may potentially be called from RCU
 * callback context by percpu_ref_kill().

So in -rt the non-raw spinlock trips due to this.  The same problem was
reported by Mike Galbraith earlier[1] but no conclusive fix was added
to the -rt series from that thread.  In one post[2], Mike mentions:

   I was sorely tempted to post a tiny variant that dropped taking
   ctx_lock in free_ioctx_users() entirely, as someone diddling with
   no reference didn't make sense.

I was thinking the same thing but with a safety net in where we
check and warn if anyone else tries to take the lock while we are
doing the final garbage collection, which is what we now do here.

[1] https://lkml.org/lkml/2014/6/8/10
[2] https://lkml.org/lkml/2014/6/9/979

Cc: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 fs/aio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/aio.c b/fs/aio.c
index 2f7e8c2e3e76..d806f7223822 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -539,9 +539,14 @@ static void free_ioctx_users(struct percpu_ref *ref)
 	struct kioctx *ctx = container_of(ref, struct kioctx, users);
 	struct kiocb *req;
 
+#ifndef CONFIG_PREEMPT_RT_BASE
 	spin_lock_irq(&ctx->ctx_lock);
+#endif
 
 	while (!list_empty(&ctx->active_reqs)) {
+#ifdef CONFIG_PREEMPT_RT_BASE
+		WARN_ON(spin_is_locked(&ctx->ctx_lock));
+#endif
 		req = list_first_entry(&ctx->active_reqs,
 				       struct kiocb, ki_list);
 
@@ -549,7 +554,9 @@ static void free_ioctx_users(struct percpu_ref *ref)
 		kiocb_cancel(ctx, req);
 	}
 
+#ifndef CONFIG_PREEMPT_RT_BASE
 	spin_unlock_irq(&ctx->ctx_lock);
+#endif
 
 	percpu_ref_kill(&ctx->reqs);
 	percpu_ref_put(&ctx->reqs);
-- 
2.1.0

Re: [PATCH-rt 1/2] aio: don't take the ctx_lock during rcu garbage collection at reboot

[Benjamin LaHaise]

On Sun, Feb 15, 2015 at 03:52:38PM -0500, Paul Gortmaker wrote:
> On reboot in 3.14-rt we see this splat:
> 
> BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:905
> in_atomic(): 1, irqs_disabled(): 0, pid: 84, name: rcuc/12
> Preemption disabled at:[<ffffffff810b29bc>] rcu_cpu_kthread+0x28c/0x700
> 
> CPU: 18 PID: 121 Comm: rcuc/18 Tainted: G      D      3.14.33-rt28-WR7.0.0.0_ovp+ #2
> Hardware name: Intel Corporation S2600CP/S2600CP, BIOS SE5C600.86B.02.01.0002.082220131453 08/22/2013
>  ffff880fad3e7a70 ffff880fb11cfd58 ffffffff81a0bcee 0000000000000000
>  ffff880fb11cfd70 ffffffff8107b5eb ffff880fad3e7a40 ffff880fb11cfd88
>  ffffffff81a136b0 ffff880fad3e7900 ffff880fb11cfdb0 ffffffff811c925c
> Call Trace:
>  [<ffffffff81a0bcee>] dump_stack+0x4e/0x7a
>  [<ffffffff8107b5eb>] __might_sleep+0xdb/0x150
>  [<ffffffff81a136b0>] rt_spin_lock+0x20/0x50
>  [<ffffffff811c925c>] free_ioctx_users+0x2c/0xe0
>  [<ffffffff8140136b>] percpu_ref_kill_rcu+0x11b/0x120
>  [<ffffffff810aded5>] rcu_cpu_kthread+0x295/0x6f0
>  [<ffffffff81076d8d>] smpboot_thread_fn+0x17d/0x2b0
>  [<ffffffff81a110f0>] ? schedule+0x30/0xa0
>  [<ffffffff81076c10>] ? SyS_setgroups+0x170/0x170
>  [<ffffffff8106f4a4>] kthread+0xc4/0xe0
>  [<ffffffff8106f3e0>] ? flush_kthread_worker+0x90/0x90
>  [<ffffffff81a147ec>] ret_from_fork+0x7c/0xb0
>  [<ffffffff8106f3e0>] ? flush_kthread_worker+0x90/0x90
> 
> The problem is as stated in lib/percpu-refcount.c:
> 
>  * Note that @release must not sleep - it may potentially be called from RCU
>  * callback context by percpu_ref_kill().
> 
> So in -rt the non-raw spinlock trips due to this.  The same problem was
> reported by Mike Galbraith earlier[1] but no conclusive fix was added
> to the -rt series from that thread.  In one post[2], Mike mentions:
> 
>    I was sorely tempted to post a tiny variant that dropped taking
>    ctx_lock in free_ioctx_users() entirely, as someone diddling with
>    no reference didn't make sense.
> 
> I was thinking the same thing but with a safety net in where we
> check and warn if anyone else tries to take the lock while we are
> doing the final garbage collection, which is what we now do here.

Your code introduces a bug: the lock is necessary to protect teardown 
initiated cancellation of an iocb against the simultaneous completion of 
that iocb.  Checking the is locked state is not sufficient protection and 
can lead to awful to debug bugs.  So this is a NAK, unless you want to 
introduce more bugs.  You need to actually fix the problem while somehow 
maintaining the consistency the lock provides.

		-ben

> [1] https://lkml.org/lkml/2014/6/8/10
> [2] https://lkml.org/lkml/2014/6/9/979
> 
> Cc: Mike Galbraith <umgwanakikbuti@gmail.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  fs/aio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 2f7e8c2e3e76..d806f7223822 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -539,9 +539,14 @@ static void free_ioctx_users(struct percpu_ref *ref)
>  	struct kioctx *ctx = container_of(ref, struct kioctx, users);
>  	struct kiocb *req;
>  
> +#ifndef CONFIG_PREEMPT_RT_BASE
>  	spin_lock_irq(&ctx->ctx_lock);
> +#endif
>  
>  	while (!list_empty(&ctx->active_reqs)) {
> +#ifdef CONFIG_PREEMPT_RT_BASE
> +		WARN_ON(spin_is_locked(&ctx->ctx_lock));
> +#endif
>  		req = list_first_entry(&ctx->active_reqs,
>  				       struct kiocb, ki_list);
>  
> @@ -549,7 +554,9 @@ static void free_ioctx_users(struct percpu_ref *ref)
>  		kiocb_cancel(ctx, req);
>  	}
>  
> +#ifndef CONFIG_PREEMPT_RT_BASE
>  	spin_unlock_irq(&ctx->ctx_lock);
> +#endif
>  
>  	percpu_ref_kill(&ctx->reqs);
>  	percpu_ref_put(&ctx->reqs);
> -- 
> 2.1.0

-- 
"Thought is the essence of where you are now."

[PATCH-rt 2/2] aio: fix splat in free_ioctx_reqs by using percpu callback

[Paul Gortmaker]

Even after we fix the use of a non raw lock in free_ioctx_users()
we still get another follow on splat.  This one looks as follows:

BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:905
in_atomic(): 1, irqs_disabled(): 0, pid: 14, name: rcuc/0
Preemption disabled at:[<ffffffff810ada55>] rcu_cpu_kthread+0x295/0x6f0

CPU: 0 PID: 14 Comm: rcuc/0 Not tainted 3.14.31-rt28-WR7.0.0.0_ovp+ #5
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS SE5C600.86B.02.01.0002.082220131453 08/22/2013
 000000000000c3c0 ffff880fb393fd28 ffffffff819ffe6b 0000000000000000
 ffff880fb393fd40 ffffffff8107b6cb ffff880ffe60c3c0 ffff880fb393fd58
 ffffffff81a077c0 ffff880ffe60c3c0 ffff880fb393fd98 ffffffff81066f05
Call Trace:
 [<ffffffff819ffe6b>] dump_stack+0x4e/0x7a
 [<ffffffff8107b6cb>] __might_sleep+0xdb/0x150
 [<ffffffff81a077c0>] rt_spin_lock+0x20/0x50
 [<ffffffff81066f05>] queue_work_on+0x65/0x110
 [<ffffffff811c714b>] free_ioctx_reqs+0x5b/0x60
 [<ffffffff813ff06b>] percpu_ref_kill_rcu+0x11b/0x120
 [<ffffffff810ada55>] rcu_cpu_kthread+0x295/0x6f0
 [<ffffffff81076c7d>] smpboot_thread_fn+0x17d/0x2b0
 [<ffffffff81a05220>] ? schedule+0x30/0xa0
 [<ffffffff81076b00>] ? SyS_setgroups+0x170/0x170
 [<ffffffff8106f3f4>] kthread+0xc4/0xe0
 [<ffffffff8106f330>] ? flush_kthread_worker+0x90/0x90
 [<ffffffff81a088ec>] ret_from_fork+0x7c/0xb0
 [<ffffffff8106f330>] ? flush_kthread_worker+0x90/0x90

This is caused by schedule_work() being called from free_ioctx_reqs(),
and the underlying reason is the same, that this release fcn...

  * Note that @release must not sleep - it may potentially be called from RCU
  * callback context by percpu_ref_kill().

and the sleep is spin_lock_irqsave(&lv->lock, lv->flags) inside
queue_work_on().

We can fix this up by using the callback version percpu_ref_kill
and register the callback to do the schedule_work().  With this
in place a reboot doesn't splat during aio garbage collection.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 fs/aio.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index d806f7223822..db1ce6d73cc7 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -524,6 +524,11 @@ static void free_ioctx_reqs(struct percpu_ref *ref)
 	/* At this point we know that there are no any in-flight requests */
 	if (ctx->requests_done)
 		complete(ctx->requests_done);
+}
+
+static void free_ioctx_reqs_callback(struct percpu_ref *ref)
+{
+	struct kioctx *ctx = container_of(ref, struct kioctx, reqs);
 
 	INIT_WORK(&ctx->free_work, free_ioctx);
 	schedule_work(&ctx->free_work);
@@ -558,7 +563,7 @@ static void free_ioctx_users(struct percpu_ref *ref)
 	spin_unlock_irq(&ctx->ctx_lock);
 #endif
 
-	percpu_ref_kill(&ctx->reqs);
+	percpu_ref_kill_and_confirm(&ctx->reqs, free_ioctx_reqs_callback);
 	percpu_ref_put(&ctx->reqs);
 }
 
-- 
2.1.0

Re: [PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[Sebastian Andrzej Siewior]

* Paul Gortmaker | 2015-02-15 15:52:37 [-0500]:

>We ran into a splat during reboot that was caused by the percpu_ref_kill
>garbage collection in the fs/aio code.

Could you please check if
    https://lkml.org/lkml/2015/2/16/391
works for you?

Sebastian

Re: [PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[Mike Galbraith]

On Wed, 2015-02-18 at 12:28 +0100, Sebastian Andrzej Siewior wrote:
> * Paul Gortmaker | 2015-02-15 15:52:37 [-0500]:
> 
> >We ran into a splat during reboot that was caused by the percpu_ref_kill
> >garbage collection in the fs/aio code.
> 
> Could you please check if
>     https://lkml.org/lkml/2015/2/16/391
> works for you?

I did not see a gripe with that and ltp.

	-Mike

Re: [PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[Paul Gortmaker]

On 2015-02-18 08:33 AM, Mike Galbraith wrote:
> On Wed, 2015-02-18 at 12:28 +0100, Sebastian Andrzej Siewior wrote:
>> * Paul Gortmaker | 2015-02-15 15:52:37 [-0500]:
>>
>>> We ran into a splat during reboot that was caused by the percpu_ref_kill
>>> garbage collection in the fs/aio code.
>>
>> Could you please check if
>>      https://lkml.org/lkml/2015/2/16/391
>> works for you?
>
> I did not see a gripe with that and ltp.

Same here ; set aside my two commits and applied the
sworkqueue implementation and the proposed aio patch
to the 3.14-rt tree and it doesn't splat on reboot.

Thanks,
Paul.
--

>
> 	-Mike
>

Re: [PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[Benjamin LaHaise]

On Fri, Feb 20, 2015 at 12:31:17PM -0500, Paul Gortmaker wrote:
> On 2015-02-18 08:33 AM, Mike Galbraith wrote:
> >On Wed, 2015-02-18 at 12:28 +0100, Sebastian Andrzej Siewior wrote:
> >>* Paul Gortmaker | 2015-02-15 15:52:37 [-0500]:
> >>
> >>>We ran into a splat during reboot that was caused by the percpu_ref_kill
> >>>garbage collection in the fs/aio code.
> >>
> >>Could you please check if
> >>     https://lkml.org/lkml/2015/2/16/391
> >>works for you?
> >
> >I did not see a gripe with that and ltp.
> 
> Same here ; set aside my two commits and applied the
> sworkqueue implementation and the proposed aio patch
> to the 3.14-rt tree and it doesn't splat on reboot.

Sebastian, is this safe for non-RT kernels?  If so, could you please resend 
against mainline with a Signed-off-by so we can get this properly fixed?

		-ben

> Thanks,
> Paul.
> --
> 
> >
> >	-Mike
> >

-- 
"Thought is the essence of where you are now."

Re: [PATCH-rt 0/2] Fix up aio splats at reboot garbage collection

[Paul Gortmaker]

On 2015-02-20 12:39 PM, Benjamin LaHaise wrote:
> On Fri, Feb 20, 2015 at 12:31:17PM -0500, Paul Gortmaker wrote:
>> On 2015-02-18 08:33 AM, Mike Galbraith wrote:
>>> On Wed, 2015-02-18 at 12:28 +0100, Sebastian Andrzej Siewior wrote:
>>>> * Paul Gortmaker | 2015-02-15 15:52:37 [-0500]:
>>>>
>>>>> We ran into a splat during reboot that was caused by the percpu_ref_kill
>>>>> garbage collection in the fs/aio code.
>>>>
>>>> Could you please check if
>>>>      https://lkml.org/lkml/2015/2/16/391
>>>> works for you?
>>>
>>> I did not see a gripe with that and ltp.
>>
>> Same here ; set aside my two commits and applied the
>> sworkqueue implementation and the proposed aio patch
>> to the 3.14-rt tree and it doesn't splat on reboot.
>
> Sebastian, is this safe for non-RT kernels?  If so, could you please resend
> against mainline with a Signed-off-by so we can get this properly fixed?

The simple waitqueue and simple workqueue are -rt specific
additions; while we probably want to get those mainline
since they bring size optimizations, they aren't in there
yet, so we can't use this in mainline yet.

P.
--

>
> 		-ben
>
>> Thanks,
>> Paul.
>> --
>>
>>>
>>> 	-Mike
>>>
>