[REGRESSION] [PATCH] block: uninitialized ioc->nr_tasks triggers WARN_ON

[REGRESSION] [PATCH] block: uninitialized ioc->nr_tasks triggers WARN_ON

[Olof Johansson]

Hi,

I'm using the old-fashioned 'dump' backup tool, and I noticed that it spews the
below warning as of 3.5-rc1 and later (3.4 is fine):

[   10.886893] ------------[ cut here ]------------
[   10.886904] WARNING: at include/linux/iocontext.h:140 copy_process+0x1488/0x1560()
[   10.886905] Hardware name: Bochs
[   10.886906] Modules linked in:
[   10.886908] Pid: 2430, comm: dump Not tainted 3.5.0-rc7+ #27
[   10.886908] Call Trace:
[   10.886911]  [<ffffffff8107ce8a>] warn_slowpath_common+0x7a/0xb0
[   10.886912]  [<ffffffff8107ced5>] warn_slowpath_null+0x15/0x20
[   10.886913]  [<ffffffff8107c088>] copy_process+0x1488/0x1560
[   10.886914]  [<ffffffff8107c244>] do_fork+0xb4/0x340
[   10.886918]  [<ffffffff8108effa>] ? recalc_sigpending+0x1a/0x50
[   10.886919]  [<ffffffff8108f6b2>] ? __set_task_blocked+0x32/0x80
[   10.886920]  [<ffffffff81091afa>] ? __set_current_blocked+0x3a/0x60
[   10.886923]  [<ffffffff81051db3>] sys_clone+0x23/0x30
[   10.886925]  [<ffffffff8179bd73>] stub_clone+0x13/0x20
[   10.886927]  [<ffffffff8179baa2>] ? system_call_fastpath+0x16/0x1b
[   10.886928] ---[ end trace 32a14af7ee6a590b ]---

Reproducing is easy, I can hit it on a KVM system with a very basic
config (x86_64 make defconfig + enable the drivers needed). To hit it,
just install dump (on debian/ubuntu, not sure what the package might be
called on Fedora), and:

dump -o -f /tmp/foo /

You'll see the warning in dmesg once it forks off the I/O process and
starts dumping filesystem contents.

I bisected it down to the following commit:

commit f6e8d01bee036460e03bd4f6a79d014f98ba712e
Author: Tejun Heo <tj@kernel.org>
Date:   Mon Mar 5 13:15:26 2012 -0800

    block: add io_context->active_ref
    
    Currently ioc->nr_tasks is used to decide two things - whether an ioc
    is done issuing IOs and whether it's shared by multiple tasks.  This
    patch separate out the first into ioc->active_ref, which is acquired
    and released using {get|put}_io_context_active() respectively.
    
    This will be used to associate bio's with a given task.  This patch
    doesn't introduce any visible behavior change.
    
    Signed-off-by: Tejun Heo <tj@kernel.org>
    Cc: Vivek Goyal <vgoyal@redhat.com>
    Signed-off-by: Jens Axboe <axboe@kernel.dk>


It seems like the init of ioc->nr_tasks was removed in that patch,
so it starts out at 0 instead of 1.

Tejun, is the right thing here to add back the init, or should something else
be done?

The below patch removes the warning, but I haven't done any more extensive
testing on it.


Signed-off-by: Olof Johansson <olof@lixom.net>

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 893b800..fab4cdd 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -244,6 +244,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
 
 	/* initialize */
 	atomic_long_set(&ioc->refcount, 1);
+	atomic_set(&ioc->nr_tasks, 1);
 	atomic_set(&ioc->active_ref, 1);
 	spin_lock_init(&ioc->lock);
 	INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC | __GFP_HIGH);

Re: [REGRESSION] [PATCH] block: uninitialized ioc->nr_tasks triggers WARN_ON

[Tejun Heo]

Hello, Olof.

On Tue, Jul 17, 2012 at 02:36:43PM -0700, Olof Johansson wrote:
> It seems like the init of ioc->nr_tasks was removed in that patch,
> so it starts out at 0 instead of 1.
> 
> Tejun, is the right thing here to add back the init, or should something else
> be done?
>
> The below patch removes the warning, but I haven't done any more extensive
> testing on it.
> 
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>

Right, the patch shouldn't have replaced the init.

 Acked-by: Tejun Heo <tj@kernel.org>

Fortunately, the effect of the bug is limited.  ioc->nr_tasks only
used to veto block cgroup migration if a task has ioc which is shared
by multiple tasks.  Currently, the only known program using CLONE_IO
is dump and even if somebody migrates some threads of a single dump
instance to a different block cgroup, the result won't be catastrophic
although block cgroup policies would become ambiguous.  IMHO, it
should be okay to route this through -stable after 3.5.  Jens?

Thanks.

-- 
tejun

Re: [REGRESSION] [PATCH] block: uninitialized ioc->nr_tasks triggers WARN_ON

[Maxim V. Patlasov]

Hi Tejun,

07/18/2012 02:24 AM, Tejun Heo пишет:
> Hello, Olof.
>
> On Tue, Jul 17, 2012 at 02:36:43PM -0700, Olof Johansson wrote:
>> It seems like the init of ioc->nr_tasks was removed in that patch,
>> so it starts out at 0 instead of 1.
>>
>> Tejun, is the right thing here to add back the init, or should something else
>> be done?
>>
>> The below patch removes the warning, but I haven't done any more extensive
>> testing on it.
>>
>>
>> Signed-off-by: Olof Johansson <olof@lixom.net>
> Right, the patch shouldn't have replaced the init.
>
>   Acked-by: Tejun Heo <tj@kernel.org>
>
> Fortunately, the effect of the bug is limited.  ioc->nr_tasks only
> used to veto block cgroup migration if a task has ioc which is shared
> by multiple tasks.  Currently, the only known program using CLONE_IO
> is dump and even if somebody migrates some threads of a single dump
> instance to a different block cgroup, the result won't be catastrophic
> although block cgroup policies would become ambiguous.  IMHO, it
> should be okay to route this through -stable after 3.5.  Jens?

Please notice that annoying WARN_ON comes from world-visible 
ioc_task_link(). So any third-party module using ioc_task_link() ends up 
in that clutter in logs. E.g. OpenVZ ploop block-device uses 
ioc_task_link().

Thanks,
Maxim

Re: [REGRESSION] [PATCH] block: uninitialized ioc->nr_tasks triggers WARN_ON

[Tejun Heo]

Hello,

On Tue, Jul 24, 2012 at 05:35:30PM +0400, Maxim V. Patlasov wrote:
> 07/18/2012 02:24 AM, Tejun Heo пишет:
> >Hello, Olof.
> >
> >On Tue, Jul 17, 2012 at 02:36:43PM -0700, Olof Johansson wrote:
> >>It seems like the init of ioc->nr_tasks was removed in that patch,
> >>so it starts out at 0 instead of 1.
> >>
> >>Tejun, is the right thing here to add back the init, or should something else
> >>be done?
> >>
> >>The below patch removes the warning, but I haven't done any more extensive
> >>testing on it.
> >>
> >>
> >>Signed-off-by: Olof Johansson <olof@lixom.net>
> >Right, the patch shouldn't have replaced the init.
> >
> >  Acked-by: Tejun Heo <tj@kernel.org>
> >
> >Fortunately, the effect of the bug is limited.  ioc->nr_tasks only
> >used to veto block cgroup migration if a task has ioc which is shared
> >by multiple tasks.  Currently, the only known program using CLONE_IO
> >is dump and even if somebody migrates some threads of a single dump
> >instance to a different block cgroup, the result won't be catastrophic
> >although block cgroup policies would become ambiguous.  IMHO, it
> >should be okay to route this through -stable after 3.5.  Jens?
> 
> Please notice that annoying WARN_ON comes from world-visible
> ioc_task_link(). So any third-party module using ioc_task_link()
> ends up in that clutter in logs. E.g. OpenVZ ploop block-device uses
> ioc_task_link().

This should go through block tree.  Jens, ping.

Thanks.

-- 
tejun

Re: [REGRESSION] [PATCH] block: uninitialized ioc->nr_tasks triggers WARN_ON

[Jens Axboe]

On 07/17/2012 11:36 PM, Olof Johansson wrote:
> Hi,
> 
> I'm using the old-fashioned 'dump' backup tool, and I noticed that it spews the
> below warning as of 3.5-rc1 and later (3.4 is fine):
> 
> [   10.886893] ------------[ cut here ]------------
> [   10.886904] WARNING: at include/linux/iocontext.h:140 copy_process+0x1488/0x1560()
> [   10.886905] Hardware name: Bochs
> [   10.886906] Modules linked in:
> [   10.886908] Pid: 2430, comm: dump Not tainted 3.5.0-rc7+ #27
> [   10.886908] Call Trace:
> [   10.886911]  [<ffffffff8107ce8a>] warn_slowpath_common+0x7a/0xb0
> [   10.886912]  [<ffffffff8107ced5>] warn_slowpath_null+0x15/0x20
> [   10.886913]  [<ffffffff8107c088>] copy_process+0x1488/0x1560
> [   10.886914]  [<ffffffff8107c244>] do_fork+0xb4/0x340
> [   10.886918]  [<ffffffff8108effa>] ? recalc_sigpending+0x1a/0x50
> [   10.886919]  [<ffffffff8108f6b2>] ? __set_task_blocked+0x32/0x80
> [   10.886920]  [<ffffffff81091afa>] ? __set_current_blocked+0x3a/0x60
> [   10.886923]  [<ffffffff81051db3>] sys_clone+0x23/0x30
> [   10.886925]  [<ffffffff8179bd73>] stub_clone+0x13/0x20
> [   10.886927]  [<ffffffff8179baa2>] ? system_call_fastpath+0x16/0x1b
> [   10.886928] ---[ end trace 32a14af7ee6a590b ]---
> 
> Reproducing is easy, I can hit it on a KVM system with a very basic
> config (x86_64 make defconfig + enable the drivers needed). To hit it,
> just install dump (on debian/ubuntu, not sure what the package might be
> called on Fedora), and:
> 
> dump -o -f /tmp/foo /
> 
> You'll see the warning in dmesg once it forks off the I/O process and
> starts dumping filesystem contents.
> 
> I bisected it down to the following commit:
> 
> commit f6e8d01bee036460e03bd4f6a79d014f98ba712e
> Author: Tejun Heo <tj@kernel.org>
> Date:   Mon Mar 5 13:15:26 2012 -0800
> 
>     block: add io_context->active_ref
>     
>     Currently ioc->nr_tasks is used to decide two things - whether an ioc
>     is done issuing IOs and whether it's shared by multiple tasks.  This
>     patch separate out the first into ioc->active_ref, which is acquired
>     and released using {get|put}_io_context_active() respectively.
>     
>     This will be used to associate bio's with a given task.  This patch
>     doesn't introduce any visible behavior change.
>     
>     Signed-off-by: Tejun Heo <tj@kernel.org>
>     Cc: Vivek Goyal <vgoyal@redhat.com>
>     Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> 
> It seems like the init of ioc->nr_tasks was removed in that patch,
> so it starts out at 0 instead of 1.
> 
> Tejun, is the right thing here to add back the init, or should something else
> be done?
> 
> The below patch removes the warning, but I haven't done any more extensive
> testing on it.
> 
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> 
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 893b800..fab4cdd 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -244,6 +244,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
>  
>  	/* initialize */
>  	atomic_long_set(&ioc->refcount, 1);
> +	atomic_set(&ioc->nr_tasks, 1);
>  	atomic_set(&ioc->active_ref, 1);
>  	spin_lock_init(&ioc->lock);
>  	INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC | __GFP_HIGH);

Thanks Olof, analysis (and patch) look correct. Applied for 3.6.

-- 
Jens Axboe