block: uninitialized ioc->nr_tasks triggers WARN_ON
PATCH 1/1 block: uninitialized ioc->nr_tasks triggers WARN_ON
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 <[email protected]> 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.
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.
block/blk-ioc.c +1 -0
@@ -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);
Hello, Olof.
Quoted 11 lines · Olof Johansson
It seems like the init of
ioc->nr_taskswas 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 <[email protected]>
Right, the patch shouldn't have replaced the init.
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.
Signature · 1 line
tejun
Hi Tejun,
07/18/2012 02:24 AM, Tejun Heo пишет:
Quoted 18 lines
Hello, Olof.
Quoted 11 lines · Olof Johansson
It seems like the init of
ioc->nr_taskswas 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 <[email protected]>Right, the patch shouldn't have replaced the init.
Acked-by Tejun Heo <[email protected]>
Quoted 7 lines
Fortunately, the effect of the bug is limited.
ioc->nr_tasksonly used to veto block cgroup migration if a task has ioc which is shared by multiple tasks. Currently, the only known program usingCLONE_IOis 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
Hello,
Quoted 27 lines · Maxim V. Patlasov
07/18/2012 02:24 AM, Tejun Heo пишет:
Quoted 18 lines
Hello, Olof.
Quoted 11 lines · Olof Johansson
It seems like the init of
ioc->nr_taskswas 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 <[email protected]>Right, the patch shouldn't have replaced the init.
Acked-by Tejun Heo <[email protected]>Quoted 7 lines
Fortunately, the effect of the bug is limited.
ioc->nr_tasksonly used to veto block cgroup migration if a task has ioc which is shared by multiple tasks. Currently, the only known program usingCLONE_IOis 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?
Quoted 4 lines
Please notice that annoying
WARN_ONcomes from world-visibleioc_task_link(). So any third-party module usingioc_task_link()ends up in that clutter in logs. E.g. OpenVZ ploop block-device usesioc_task_link().
This should go through block tree. Jens, ping.
Thanks.
Signature · 1 line
tejun
Jens Axboe
maintainerQuoted 71 lines · 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_64make 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
f6e8d01bee036460e03bd4f6a79d014f98ba712eAuthor: Tejun Heo <[email protected]> Date: Mon Mar 5 13:15:26 2012 -0800block: add
io_context->active_refCurrently
ioc->nr_tasksis 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 intoioc->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 <[email protected]>Cc Vivek Goyal <[email protected]>Signed-off-by Jens Axboe <[email protected]>It seems like the init of
ioc->nr_taskswas 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 <[email protected]>block/blk-ioc.c +0
-0
block/blk-ioc.c int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node) · line 250
Quoted 7 lines
block/blk-ioc.c +1
-0@@ -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.
Signature · 1 line
Jens Axboe