diff options
author | Glauber Costa <glommer@parallels.com> | 2013-02-21 18:16:41 -0500 |
---|---|---|
committer | Jens Axboe <axboe@kernel.dk> | 2013-02-22 04:42:46 -0500 |
commit | a3cc86c2f00839453d2dbeb46bfc44e885b073db (patch) | |
tree | 8942e1acba4b7450144b7b6f8221fa0f1e69c326 /block | |
parent | 7414d4f64b73cc30c600b4fe0a9cbc24cedc4285 (diff) |
cfq: fix lock imbalance with failed allocations
While stress-running very-small container scenarios with the Kernel Memory
Controller, I've run into a lockdep-detected lock imbalance in
cfq-iosched.c.
I'll apologize beforehand for not posting a backlog: I didn't anticipate
it would be so hard to reproduce, so I didn't save my serial output and
went directly on debugging. Turns out that it did not happen again in
more than 20 runs, making it a quite rare pattern.
But here is my analysis:
When we are in very low-memory situations, we will arrive at
cfq_find_alloc_queue and may not find a queue, having to resort to the oom
queue, in an rcu-locked condition:
if (!cfqq || cfqq == &cfqd->oom_cfqq)
[ ... ]
Next, we will release the rcu lock, and try to allocate a queue, retrying
if we succeed:
rcu_read_unlock();
spin_unlock_irq(cfqd->queue->queue_lock);
new_cfqq = kmem_cache_alloc_node(cfq_pool,
gfp_mask | __GFP_ZERO,
cfqd->queue->node);
spin_lock_irq(cfqd->queue->queue_lock);
if (new_cfqq)
goto retry;
We are unlocked at this point, but it should be fine, since we will
reacquire the rcu_read_lock when we retry.
Except of course, that we may not retry: the allocation may very well fail
and we'll keep on going through the flow:
The next branch is:
if (cfqq) {
[ ... ]
} else
cfqq = &cfqd->oom_cfqq;
And right before exiting, we'll issue rcu_read_unlock().
Being already unlocked, this is the likely source of our imbalance. Since
cfqq is either already NULL or made NULL in the first statement of the
outter branch, the only viable alternative here seems to be to return the
oom queue right away in case of allocation failure.
Please review the following patch and apply if you agree with my analysis.
Signed-off-by: Glauber Costa <glommer@parallels.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Diffstat (limited to 'block')
-rw-r--r-- | block/cfq-iosched.c | 2 |
1 files changed, 2 insertions, 0 deletions
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c index b66365b6ba77..1bf9307e8f56 100644 --- a/block/cfq-iosched.c +++ b/block/cfq-iosched.c | |||
@@ -3594,6 +3594,8 @@ retry: | |||
3594 | spin_lock_irq(cfqd->queue->queue_lock); | 3594 | spin_lock_irq(cfqd->queue->queue_lock); |
3595 | if (new_cfqq) | 3595 | if (new_cfqq) |
3596 | goto retry; | 3596 | goto retry; |
3597 | else | ||
3598 | return &cfqd->oom_cfqq; | ||
3597 | } else { | 3599 | } else { |
3598 | cfqq = kmem_cache_alloc_node(cfq_pool, | 3600 | cfqq = kmem_cache_alloc_node(cfq_pool, |
3599 | gfp_mask | __GFP_ZERO, | 3601 | gfp_mask | __GFP_ZERO, |