aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorGlauber Costa <glommer@parallels.com>2013-02-21 18:16:41 -0500
committerJens Axboe <axboe@kernel.dk>2013-02-22 04:42:46 -0500
commita3cc86c2f00839453d2dbeb46bfc44e885b073db (patch)
tree8942e1acba4b7450144b7b6f8221fa0f1e69c326
parent7414d4f64b73cc30c600b4fe0a9cbc24cedc4285 (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>
-rw-r--r--block/cfq-iosched.c2
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,