WARNING: at include/linux/iocontext.h:140 copy_io+0xb9/0x130()

PATCH Applied Sasha Levin 5 participants 16 messages May 1, 2012 Block layer

Share & tools

Sasha Levin

author

2012-05-01 12:04 UTC #0 lore raw

Hi all,

I've stumbled on the warning at the bottom with today's linux-next within a KVM guest doing syscall fuzzing using trinity.

I haven't encountered this warning before, and it looks like it's not easy to reproduce it within the guest. Looking at the code in that area I guess it's some sorts of a rare race condition.

The only recent commits in that area are the work done on blk cgroup. It is enabled in the test kernel.

[  418.101833] WARNING: at include/linux/iocontext.h:140 copy_io+0xb9/0x130()
[  418.108177] Pid: 29267, comm: trinity Tainted: G        W    3.4.0-rc5-next-20120501-sasha #104
[  418.108180] Call Trace:
[  418.108186]  [<ffffffff810b6c67>] warn_slowpath_common+0x87/0xb0
[  418.108189]  [<ffffffff810b6ca5>] warn_slowpath_null+0x15/0x20
[  418.108192]  [<ffffffff810b44d9>] copy_io+0xb9/0x130
[  418.108196]  [<ffffffff810b5ae4>] copy_process+0x884/0xf90
[  418.108200]  [<ffffffff810f17c5>] ? sched_clock_local+0x25/0x90
[  418.108203]  [<ffffffff810b65b7>] do_fork+0x137/0x240
[  418.108208]  [<ffffffff810e808e>] ? sub_preempt_count+0xae/0xf0
[  418.108212]  [<ffffffff82d92cf9>] ? _raw_spin_unlock_irq+0x59/0x80
[  418.108216]  [<ffffffff82d93be5>] ? sysret_check+0x22/0x5d
[  418.108219]  [<ffffffff81057b13>] sys_clone+0x23/0x30
[  418.108222]  [<ffffffff82d93f43>] stub_clone+0x13/0x20
[  418.108225]  [<ffffffff82d93bb9>] ? system_call_fastpath+0x16/0x1b
[  418.108228] ---[ end trace 8f6ca168297608bb ]---

Tejun Heo

2012-05-01 16:17 UTC #1 lore raw

PATCH 1/1 block: iocontext->nr_tasks should be initialized to one

create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.

This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).

Fix it by initializing it to one in create_task_io_context().

Signed-off-by Tejun Heo <[email protected]>

Reported-by Sasha Levin <[email protected]>

LKML-Reference: <1335873936.16988.148.camel@lappy>
Cc: [email protected]

---
 block/blk-ioc.c |    1 +
 1 file changed, 1 insertion(+)

block/blk-ioc.c +1 -0

index 1e2d53b..c942409 100644
@@ -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->active_ref, 1);
+	atomic_set(&ioc->nr_tasks, 1);
 	spin_lock_init(&ioc->lock);
 	INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC | __GFP_HIGH);
 	INIT_HLIST_HEAD(&ioc->icq_list);

Jens Axboe

Applied

2012-05-01 18:02 UTC #2 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 9 lines · Tejun Heo

create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).

CLONE_IO is an exported interface, it can be set from clone(2). Otherwise Sasha would not have hit this :-)

Thanks, applied.

Signature · 1 line

Jens Axboe

Tejun Heo

2012-05-01 18:09 UTC #3 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Tejun Heo
create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.

Quoted 8 lines

Quoted 5 lines
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).
CLONE_IO is an exported interface, it can be set from clone(2). Otherwise Sasha would not have hit this :-)

Yeah, but with pthread not exposing it, I'm very skeptical how much, if any, use it's getting. With its incompatibility with blk-cgroup and cfq being able to merge coop request streams, I'm not sure how much we need it. Maybe we can just make it noop?

Thanks.

Signature · 1 line

tejun

Jens Axboe

2012-05-01 18:18 UTC #4 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 12 lines · Tejun Heo

Quoted 11 lines · Jens Axboe
Quoted 9 lines · Tejun Heo
create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).

Quoted 7 lines

Quoted 2 lines
CLONE_IO is an exported interface, it can be set from clone(2). Otherwise Sasha would not have hit this :-)
Yeah, but with pthread not exposing it, I'm very skeptical how much, if any, use it's getting. With its incompatibility with blk-cgroup and cfq being able to merge coop request streams, I'm not sure how much we need it. Maybe we can just make it noop?

It's a lot more robust and specific than hoping to get coop merging. For cfq, it also implies that multiple threads sharing an io context should be accounted as one.

But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)

Signature · 1 line

Jens Axboe

Jeff Moyer

2012-05-01 18:31 UTC #5 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 21 lines · Jens Axboe

Quoted 12 lines · Tejun Heo
Quoted 11 lines · Jens Axboe
Quoted 9 lines · Tejun Heo
create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).
Quoted 7 lines
Quoted 2 lines
CLONE_IO is an exported interface, it can be set from clone(2). Otherwise Sasha would not have hit this :-)
Yeah, but with pthread not exposing it, I'm very skeptical how much, if any, use it's getting. With its incompatibility with blk-cgroup and cfq being able to merge coop request streams, I'm not sure how much we need it. Maybe we can just make it noop?

Quoted 7 lines

It's a lot more robust and specific than hoping to get coop merging. For cfq, it also implies that multiple threads sharing an io context should be accounted as one.
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)

I know of one project: the venerable dump/restore utility uses CLONE_IO.

Cheers,
Jeff

Vivek Goyal

2012-05-01 18:36 UTC #6 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

On Tue, May 01, 2012 at 02:31:07PM -0400, Jeff Moyer wrote:

[..]

Quoted 5 lines

Quoted 3 lines
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)
I know of one project: the venerable dump/restore utility uses CLONE_IO.

I thought you wrote cooperating queue logic to fix dump as it was not using CLONE_IO and IO from multiple threads was going in separate queues.

Thanks
Vivek

Jeff Moyer

2012-05-01 19:20 UTC #7 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 7 lines · Vivek Goyal

On Tue, May 01, 2012 at 02:31:07PM -0400, Jeff Moyer wrote:
[..]
Quoted 4 lines
Quoted 3 lines
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)

Quoted 5 lines

Quoted 1 line
I know of one project: the venerable dump/restore utility uses CLONE_IO.
I thought you wrote cooperating queue logic to fix dump as it was not using CLONE_IO and IO from multiple threads was going in separate queues.

That's correct. I believe I sent the patch for dump before the kernel patch was accepted. Plus, it can't hurt, right?

Cheers,
Jeff

Vivek Goyal

2012-05-01 18:59 UTC #8 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 9 lines · Jeff Moyer

Quoted 7 lines · Vivek Goyal
On Tue, May 01, 2012 at 02:31:07PM -0400, Jeff Moyer wrote:
[..]
Quoted 4 lines
Quoted 3 lines
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
I know of one project: the venerable dump/restore utility uses CLONE_IO.
I thought you wrote cooperating queue logic to fix dump as it was not using CLONE_IO and IO from multiple threads was going in separate queues.
That's correct. I believe I sent the patch for dump before the kernel patch was accepted. Plus, it can't hurt, right?

Ok, so now you have fixed dump to use CLONE_IO.

So only other user of coop thing remaining potentially is qemu. I was doing some qemu testing where threads were doing IO to nearby area but no coop merging was taking place. So not sure in practice how well does it work. Thought, that's irrlevant for this discussion. Thought of mentioning this observation.

Thanks
Vivek

Jeff Moyer

2012-05-01 19:04 UTC #9 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 16 lines · Vivek Goyal

Quoted 15 lines · Jeff Moyer
Quoted 12 lines · Vivek Goyal
On Tue, May 01, 2012 at 02:31:07PM -0400, Jeff Moyer wrote:
[..]
Quoted 5 lines
Quoted 3 lines
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)
I know of one project: the venerable dump/restore utility uses CLONE_IO.
I thought you wrote cooperating queue logic to fix dump as it was not using CLONE_IO and IO from multiple threads was going in separate queues.

Quoted 6 lines

Quoted 2 lines
That's correct. I believe I sent the patch for dump before the kernel patch was accepted. Plus, it can't hurt, right?
Ok, so now you have fixed dump to use CLONE_IO.
So only other user of coop thing remaining potentially is qemu. I was

No, that's not the *only* other potential user. ;-) I wouldn't be surprised if nfsd benefitted from the merging. I also wouldn't be surprised if other 3rd party apps did. Are you trying to make a case to get rid of the queue merging logic?

Quoted 3 lines

doing some qemu testing where threads were doing IO to nearby area but no coop merging was taking place. So not sure in practice how well does it work.

Well, that sounds like it warrants further investigation.

Quoted 2 lines

Thought, that's irrlevant for this discussion. Thought of mentioning this observation.

If you can provide a reproducer, I'll be happy to take a look.

Cheers,
Jeff

Vivek Goyal

2012-05-01 19:08 UTC #10 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 21 lines · Jeff Moyer

Quoted 19 lines · Vivek Goyal
Quoted 17 lines · Jeff Moyer
Quoted 7 lines · Vivek Goyal
On Tue, May 01, 2012 at 02:31:07PM -0400, Jeff Moyer wrote:
[..]
Quoted 4 lines
Quoted 3 lines
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)
Quoted 5 lines
Quoted 1 line
I know of one project: the venerable dump/restore utility uses CLONE_IO.
I thought you wrote cooperating queue logic to fix dump as it was not using CLONE_IO and IO from multiple threads was going in separate queues.
That's correct. I believe I sent the patch for dump before the kernel patch was accepted. Plus, it can't hurt, right?

Quoted 8 lines

Quoted 3 lines
Ok, so now you have fixed dump to use CLONE_IO.
So only other user of coop thing remaining potentially is qemu. I was
No, that's not the *only* other potential user. ;-) I wouldn't be surprised if nfsd benefitted from the merging. I also wouldn't be surprised if other 3rd party apps did. Are you trying to make a case to get rid of the queue merging logic?

No, just counting who benefits from coop logic.

Quoted 11 lines

Quoted 4 lines
doing some qemu testing where threads were doing IO to nearby area but no coop merging was taking place. So not sure in practice how well does it work.
Well, that sounds like it warrants further investigation.
Quoted 2 lines
Thought, that's irrlevant for this discussion. Thought of mentioning this observation.
If you can provide a reproducer, I'll be happy to take a look.

I will dig. This was in the context of trying to solve qemu related IO slowdown with CFQ. Will let you know if I find a concrete reproducer.

Thanks
Vivek

Jens Axboe

2012-05-01 18:37 UTC #11 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

In reply to Jeff Moyer

Quoted 27 lines · Jeff Moyer

Quoted 25 lines · Jens Axboe
Quoted 12 lines · Tejun Heo
Quoted 11 lines · Jens Axboe
Quoted 9 lines · Tejun Heo
create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).
Quoted 7 lines
Quoted 2 lines
CLONE_IO is an exported interface, it can be set from clone(2). Otherwise Sasha would not have hit this :-)
Yeah, but with pthread not exposing it, I'm very skeptical how much, if any, use it's getting. With its incompatibility with blk-cgroup and cfq being able to merge coop request streams, I'm not sure how much we need it. Maybe we can just make it noop?
It's a lot more robust and specific than hoping to get coop merging. For cfq, it also implies that multiple threads sharing an io context should be accounted as one.

Quoted 5 lines

Quoted 3 lines
But as to actual users, I really don't know. I agree it's probably not that widely used. If google still had that code search, we could get a better idea :-)
I know of one project: the venerable dump/restore utility uses CLONE_IO.

Thanks Jeff, now I remember the specifics of what we tested. IIRC, we also did numbers back then comparing the coop merging vs specifically using CLONE_IO.

Signature · 1 line

Jens Axboe

Jeff Moyer

2012-05-01 18:50 UTC #12 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 7 lines · Jens Axboe

Quoted 2 lines · Jeff Moyer
Jens Axboe <[email protected]> writes:
I know of one project: the venerable dump/restore utility uses CLONE_IO.
Thanks Jeff, now I remember the specifics of what we tested. IIRC, we also did numbers back then comparing the coop merging vs specifically using CLONE_IO.

Maybe? I picked through some old emails but couldn't find that exact comparison. It would have been a smart thing to do, though!

-Jeff

Jens Axboe

2012-05-01 19:03 UTC #13 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 6 lines · Jeff Moyer

Quoted 4 lines · Jens Axboe
Quoted 2 lines · Jeff Moyer
Jens Axboe <[email protected]> writes:
I know of one project: the venerable dump/restore utility uses CLONE_IO.

Quoted 6 lines

Quoted 3 lines
Thanks Jeff, now I remember the specifics of what we tested. IIRC, we also did numbers back then comparing the coop merging vs specifically using CLONE_IO.
Maybe? I picked through some old emails but couldn't find that exact comparison. It would have been a smart thing to do, though!

It might have been in a bugzilla thing, unfortunately. But yes, it'd be the smart thing to do, hence I was pretty sure that you did it!

Signature · 1 line

Jens Axboe

Jens Axboe

2012-05-01 18:05 UTC #14 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).

Quoted 6 lines

Fix it by initializing it to one in create_task_io_context().
Signed-off-by Tejun Heo <[email protected]>
Reported-by Sasha Levin <[email protected]>
LKML-Reference: <1335873936.16988.148.camel@lappy>
Cc: [email protected]

BTW, this only affects for-3.5/core, it's not a mainline bug. So I've dropped the stable CC.

Signature · 1 line

Jens Axboe

Tejun Heo

2012-05-01 18:06 UTC #15 lore raw

Re: [PATCH v3.4-rc5] block: iocontext->nr_tasks should be initialized to one

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Tejun Heo
create_task_io_context() left ioc->nr_tasks at zero; however, a newly created ioc should have its nr_tasks initialized to one as it begins attached to the task creating it.
This affects only CLONE_IO which currently doesn't seem to have any actual user. Sasha triggered WARN_ON_ONCE() in ioc_task_link() using syscall fuzzer. Even when it happens, the failure mode isn't critical (blk-cgroup may allow attaching a CLONE_IO'd task to a cgroup when it shouldn't and blkcg limits may behave weirdly).
Fix it by initializing it to one in create_task_io_context().

Quoted 7 lines

Quoted 4 lines
Signed-off-by Tejun Heo <[email protected]>
Reported-by Sasha Levin <[email protected]>
LKML-Reference: <1335873936.16988.148.camel@lappy>
Cc: [email protected]
BTW, this only affects for-3.5/core, it's not a mainline bug. So I've dropped the stable CC.

Ah, sorry about that. Got confused which one got in when.

Thanks.

Signature · 1 line

tejun