* [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 4:42 ` Rusty Russell
2011-10-26 1:48 ` [PATCH 02/13] block, cfq: move cfqd->cic_index to q->id Tejun Heo
` (12 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo, Rusty Russell
It's often convenient to be able to release resource from IRQ context.
Make ida_simple_*() use irqsave/restore spin ops so that they are IRQ
safe.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
lib/idr.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/lib/idr.c b/lib/idr.c
index db040ce..dabdf79 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -944,6 +944,7 @@ int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
{
int ret, id;
unsigned int max;
+ unsigned long flags;
BUG_ON((int)start < 0);
BUG_ON((int)end < 0);
@@ -959,7 +960,7 @@ again:
if (!ida_pre_get(ida, gfp_mask))
return -ENOMEM;
- spin_lock(&simple_ida_lock);
+ spin_lock_irqsave(&simple_ida_lock, flags);
ret = ida_get_new_above(ida, start, &id);
if (!ret) {
if (id > max) {
@@ -969,7 +970,7 @@ again:
ret = id;
}
}
- spin_unlock(&simple_ida_lock);
+ spin_unlock_irqrestore(&simple_ida_lock, flags);
if (unlikely(ret == -EAGAIN))
goto again;
@@ -985,10 +986,12 @@ EXPORT_SYMBOL(ida_simple_get);
*/
void ida_simple_remove(struct ida *ida, unsigned int id)
{
+ unsigned long flags;
+
BUG_ON((int)id < 0);
- spin_lock(&simple_ida_lock);
+ spin_lock_irqsave(&simple_ida_lock, flags);
ida_remove(ida, id);
- spin_unlock(&simple_ida_lock);
+ spin_unlock_irqrestore(&simple_ida_lock, flags);
}
EXPORT_SYMBOL(ida_simple_remove);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe
2011-10-26 1:48 ` [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe Tejun Heo
@ 2011-10-26 4:42 ` Rusty Russell
2011-10-26 20:28 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Rusty Russell @ 2011-10-26 4:42 UTC (permalink / raw)
To: Tejun Heo, axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
On Tue, 25 Oct 2011 18:48:27 -0700, Tejun Heo <tj@kernel.org> wrote:
> It's often convenient to be able to release resource from IRQ context.
> Make ida_simple_*() use irqsave/restore spin ops so that they are IRQ
> safe.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe
2011-10-26 4:42 ` Rusty Russell
@ 2011-10-26 20:28 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 20:28 UTC (permalink / raw)
Cc: axboe, vgoyal, ctalbott, rni, linux-kernel
Hello,
On Wed, Oct 26, 2011 at 03:12:17PM +1030, Rusty Russell wrote:
> On Tue, 25 Oct 2011 18:48:27 -0700, Tejun Heo <tj@kernel.org> wrote:
> > It's often convenient to be able to release resource from IRQ context.
> > Make ida_simple_*() use irqsave/restore spin ops so that they are IRQ
> > safe.
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Heh, I just realized this patch doesn't need to be part of this
series. I originally had ida free from rcu and thus needed this but
later on removed it. I'll route it separately.
Thank you.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 02/13] block, cfq: move cfqd->cic_index to q->id
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
2011-10-26 1:48 ` [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 03/13] block: misc ioc cleanups Tejun Heo
` (11 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
cfq allocates per-queue id using ida and uses it to index cic radix
tree from io_context. Move it to q->id and allocate on queue init and
free on queue release. This simplifies cfq a bit and will allow for
further improvements of io context life-cycle management.
This patch doesn't introduce any functional difference.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-core.c | 24 ++++++++++++++-------
block/blk-sysfs.c | 2 +
block/blk.h | 3 ++
block/cfq-iosched.c | 52 ++++-------------------------------------------
include/linux/blkdev.h | 6 +++++
5 files changed, 32 insertions(+), 55 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 8267409..d7304d8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -39,6 +39,8 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_remap);
EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_complete);
+DEFINE_IDA(blk_queue_ida);
+
/*
* For the allocated request tables
*/
@@ -469,6 +471,10 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
if (!q)
return NULL;
+ q->id = ida_simple_get(&blk_queue_ida, 0, 0, GFP_KERNEL);
+ if (q->id < 0)
+ goto fail_q;
+
q->backing_dev_info.ra_pages =
(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
q->backing_dev_info.state = 0;
@@ -476,15 +482,11 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
q->backing_dev_info.name = "block";
err = bdi_init(&q->backing_dev_info);
- if (err) {
- kmem_cache_free(blk_requestq_cachep, q);
- return NULL;
- }
+ if (err)
+ goto fail_id;
- if (blk_throtl_init(q)) {
- kmem_cache_free(blk_requestq_cachep, q);
- return NULL;
- }
+ if (blk_throtl_init(q))
+ goto fail_id;
setup_timer(&q->backing_dev_info.laptop_mode_wb_timer,
laptop_mode_timer_fn, (unsigned long) q);
@@ -507,6 +509,12 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
q->queue_lock = &q->__queue_lock;
return q;
+
+fail_id:
+ ida_simple_remove(&blk_queue_ida, q->id);
+fail_q:
+ kmem_cache_free(blk_requestq_cachep, q);
+ return NULL;
}
EXPORT_SYMBOL(blk_alloc_queue_node);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index f0b2ca8..5b4b4ab 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -494,6 +494,8 @@ static void blk_release_queue(struct kobject *kobj)
blk_trace_shutdown(q);
bdi_destroy(&q->backing_dev_info);
+
+ ida_simple_remove(&blk_queue_ida, q->id);
kmem_cache_free(blk_requestq_cachep, q);
}
diff --git a/block/blk.h b/block/blk.h
index f8c797c..1a97609 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -1,6 +1,8 @@
#ifndef BLK_INTERNAL_H
#define BLK_INTERNAL_H
+#include <linux/idr.h>
+
/* Amount of time in which a process may batch requests */
#define BLK_BATCH_TIME (HZ/50UL)
@@ -9,6 +11,7 @@
extern struct kmem_cache *blk_requestq_cachep;
extern struct kobj_type blk_queue_ktype;
+extern struct ida blk_queue_ida;
void init_request_from_bio(struct request *req, struct bio *bio);
void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 16ace89..ec3f5e8b 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -65,9 +65,6 @@ static DEFINE_PER_CPU(unsigned long, cfq_ioc_count);
static struct completion *ioc_gone;
static DEFINE_SPINLOCK(ioc_gone_lock);
-static DEFINE_SPINLOCK(cic_index_lock);
-static DEFINE_IDA(cic_index_ida);
-
#define CFQ_PRIO_LISTS IOPRIO_BE_NR
#define cfq_class_idle(cfqq) ((cfqq)->ioprio_class == IOPRIO_CLASS_IDLE)
#define cfq_class_rt(cfqq) ((cfqq)->ioprio_class == IOPRIO_CLASS_RT)
@@ -290,7 +287,6 @@ struct cfq_data {
unsigned int cfq_group_idle;
unsigned int cfq_latency;
- unsigned int cic_index;
struct list_head cic_list;
/*
@@ -484,7 +480,7 @@ static inline void cic_set_cfqq(struct cfq_io_context *cic,
static inline void *cfqd_dead_key(struct cfq_data *cfqd)
{
- return (void *)(cfqd->cic_index << CIC_DEAD_INDEX_SHIFT | CIC_DEAD_KEY);
+ return (void *)(cfqd->queue->id << CIC_DEAD_INDEX_SHIFT | CIC_DEAD_KEY);
}
static inline struct cfq_data *cic_to_cfqd(struct cfq_io_context *cic)
@@ -3105,7 +3101,7 @@ cfq_drop_dead_cic(struct cfq_data *cfqd, struct io_context *ioc,
BUG_ON(rcu_dereference_check(ioc->ioc_data,
lockdep_is_held(&ioc->lock)) == cic);
- radix_tree_delete(&ioc->radix_root, cfqd->cic_index);
+ radix_tree_delete(&ioc->radix_root, cfqd->queue->id);
hlist_del_rcu(&cic->cic_list);
spin_unlock_irqrestore(&ioc->lock, flags);
@@ -3133,7 +3129,7 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
}
do {
- cic = radix_tree_lookup(&ioc->radix_root, cfqd->cic_index);
+ cic = radix_tree_lookup(&ioc->radix_root, cfqd->queue->id);
rcu_read_unlock();
if (!cic)
break;
@@ -3169,8 +3165,7 @@ static int cfq_cic_link(struct cfq_data *cfqd, struct io_context *ioc,
cic->key = cfqd;
spin_lock_irqsave(&ioc->lock, flags);
- ret = radix_tree_insert(&ioc->radix_root,
- cfqd->cic_index, cic);
+ ret = radix_tree_insert(&ioc->radix_root, cfqd->queue->id, cic);
if (!ret)
hlist_add_head_rcu(&cic->cic_list, &ioc->cic_list);
spin_unlock_irqrestore(&ioc->lock, flags);
@@ -3944,10 +3939,6 @@ static void cfq_exit_queue(struct elevator_queue *e)
cfq_shutdown_timer_wq(cfqd);
- spin_lock(&cic_index_lock);
- ida_remove(&cic_index_ida, cfqd->cic_index);
- spin_unlock(&cic_index_lock);
-
/*
* Wait for cfqg->blkg->key accessors to exit their grace periods.
* Do this wait only if there are other unlinked groups out
@@ -3969,24 +3960,6 @@ static void cfq_exit_queue(struct elevator_queue *e)
kfree(cfqd);
}
-static int cfq_alloc_cic_index(void)
-{
- int index, error;
-
- do {
- if (!ida_pre_get(&cic_index_ida, GFP_KERNEL))
- return -ENOMEM;
-
- spin_lock(&cic_index_lock);
- error = ida_get_new(&cic_index_ida, &index);
- spin_unlock(&cic_index_lock);
- if (error && error != -EAGAIN)
- return error;
- } while (error);
-
- return index;
-}
-
static void *cfq_init_queue(struct request_queue *q)
{
struct cfq_data *cfqd;
@@ -3994,23 +3967,9 @@ static void *cfq_init_queue(struct request_queue *q)
struct cfq_group *cfqg;
struct cfq_rb_root *st;
- i = cfq_alloc_cic_index();
- if (i < 0)
- return NULL;
-
cfqd = kmalloc_node(sizeof(*cfqd), GFP_KERNEL | __GFP_ZERO, q->node);
- if (!cfqd) {
- spin_lock(&cic_index_lock);
- ida_remove(&cic_index_ida, i);
- spin_unlock(&cic_index_lock);
+ if (!cfqd)
return NULL;
- }
-
- /*
- * Don't need take queue_lock in the routine, since we are
- * initializing the ioscheduler, and nobody is using cfqd
- */
- cfqd->cic_index = i;
/* Init root service tree */
cfqd->grp_service_tree = CFQ_RB_ROOT;
@@ -4294,7 +4253,6 @@ static void __exit cfq_exit(void)
*/
if (elv_ioc_count_read(cfq_ioc_count))
wait_for_completion(&all_gone);
- ida_destroy(&cic_index_ida);
cfq_slab_kill();
}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6a4ab84..565a4ed 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -311,6 +311,12 @@ struct request_queue {
unsigned long queue_flags;
/*
+ * ida allocated id for this queue. Used to index queues from
+ * ioctx.
+ */
+ int id;
+
+ /*
* queue needs bounce pages for pages above this limit
*/
gfp_t bounce_gfp;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 03/13] block: misc ioc cleanups
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
2011-10-26 1:48 ` [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe Tejun Heo
2011-10-26 1:48 ` [PATCH 02/13] block, cfq: move cfqd->cic_index to q->id Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction Tejun Heo
` (10 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
* int return from put_io_context() wasn't used by anybody. Make it
return void like other put functions and docbook-fy the function
comment.
* Reorder dummy declarations for !CONFIG_BLOCK case a bit.
* Make alloc_ioc_context() use __GFP_ZERO allocation, take init out of
if block and drop 0'ing.
* Docbook-fy current_io_context() comment.
This patch doesn't introduce any functional change.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-ioc.c | 72 ++++++++++++++++++++++----------------------
include/linux/iocontext.h | 12 ++-----
2 files changed, 39 insertions(+), 45 deletions(-)
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 6f9bbd9..8bebf06 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -27,26 +27,28 @@ static void cfq_dtor(struct io_context *ioc)
}
}
-/*
- * IO Context helper functions. put_io_context() returns 1 if there are no
- * more users of this io context, 0 otherwise.
+/**
+ * put_io_context - put a reference of io_context
+ * @ioc: io_context to put
+ *
+ * Decrement reference count of @ioc and release it if the count reaches
+ * zero.
*/
-int put_io_context(struct io_context *ioc)
+void put_io_context(struct io_context *ioc)
{
if (ioc == NULL)
- return 1;
+ return;
- BUG_ON(atomic_long_read(&ioc->refcount) == 0);
+ BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
- if (atomic_long_dec_and_test(&ioc->refcount)) {
- rcu_read_lock();
- cfq_dtor(ioc);
- rcu_read_unlock();
+ if (!atomic_long_dec_and_test(&ioc->refcount))
+ return;
- kmem_cache_free(iocontext_cachep, ioc);
- return 1;
- }
- return 0;
+ rcu_read_lock();
+ cfq_dtor(ioc);
+ rcu_read_unlock();
+
+ kmem_cache_free(iocontext_cachep, ioc);
}
EXPORT_SYMBOL(put_io_context);
@@ -84,33 +86,31 @@ struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
{
struct io_context *ioc;
- ioc = kmem_cache_alloc_node(iocontext_cachep, gfp_flags, node);
- if (ioc) {
- atomic_long_set(&ioc->refcount, 1);
- atomic_set(&ioc->nr_tasks, 1);
- spin_lock_init(&ioc->lock);
- ioc->ioprio_changed = 0;
- ioc->ioprio = 0;
- ioc->last_waited = 0; /* doesn't matter... */
- ioc->nr_batch_requests = 0; /* because this is 0 */
- INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
- INIT_HLIST_HEAD(&ioc->cic_list);
- ioc->ioc_data = NULL;
-#if defined(CONFIG_BLK_CGROUP) || defined(CONFIG_BLK_CGROUP_MODULE)
- ioc->cgroup_changed = 0;
-#endif
- }
+ ioc = kmem_cache_alloc_node(iocontext_cachep, gfp_flags | __GFP_ZERO,
+ node);
+ if (unlikely(!ioc))
+ return NULL;
+
+ /* initialize */
+ atomic_long_set(&ioc->refcount, 1);
+ atomic_set(&ioc->nr_tasks, 1);
+ spin_lock_init(&ioc->lock);
+ INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
+ INIT_HLIST_HEAD(&ioc->cic_list);
return ioc;
}
-/*
- * If the current task has no IO context then create one and initialise it.
- * Otherwise, return its existing IO context.
+/**
+ * current_io_context - get io_context of %current
+ * @gfp_flags: allocation flags, used if allocation is necessary
+ * @node: allocation node, used if allocation is necessary
*
- * This returned IO context doesn't have a specifically elevated refcount,
- * but since the current task itself holds a reference, the context can be
- * used in general code, so long as it stays within `current` context.
+ * Return io_context of %current. If it doesn't exist, it is created with
+ * @gfp_flags and @node. The returned io_context does NOT have its
+ * reference count incremented. Because io_context is exited only on task
+ * exit, %current can be sure that the returned io_context is valid and
+ * alive as long as it is executing.
*/
struct io_context *current_io_context(gfp_t gfp_flags, int node)
{
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 5037a0a..8a6ecb66 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -76,20 +76,14 @@ static inline struct io_context *ioc_task_link(struct io_context *ioc)
struct task_struct;
#ifdef CONFIG_BLOCK
-int put_io_context(struct io_context *ioc);
+void put_io_context(struct io_context *ioc);
void exit_io_context(struct task_struct *task);
struct io_context *get_io_context(gfp_t gfp_flags, int node);
struct io_context *alloc_io_context(gfp_t gfp_flags, int node);
#else
-static inline void exit_io_context(struct task_struct *task)
-{
-}
-
struct io_context;
-static inline int put_io_context(struct io_context *ioc)
-{
- return 1;
-}
+static inline void put_io_context(struct io_context *ioc) { }
+static inline void exit_io_context(struct task_struct *task) { }
#endif
#endif
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (2 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 03/13] block: misc ioc cleanups Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 16:01 ` Vivek Goyal
2011-10-26 21:30 ` [PATCH UPDATED " Tejun Heo
2011-10-26 1:48 ` [PATCH 05/13] block: misc updates to blk_get_queue() Tejun Heo
` (9 subsequent siblings)
13 siblings, 2 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
Ignoring copy_io() during fork, io_context can be allocated from two
places - current_io_context() and set_task_ioprio(). The former is
always called from local task while the latter can be called from
different task. The synchornization between them are peculiar and
dubious.
* current_io_context() doesn't grab task_lock() and assumes that if it
saw %NULL ->io_context, it would stay that way until allocation and
assignment is complete. It has smp_wmb() between alloc/init and
assignment.
* set_task_ioprio() grabs task_lock() for assignment and does
smp_read_barrier_depends() between "ioc = task->io_context" and "if
(ioc)". Unfortunately, this doesn't achieve anything - the latter
is not a dependent load of the former. ie, if ioc itself were being
dereferenced "ioc->xxx", it would mean something (not sure what tho)
but as the code currently stands, the dependent read barrier is
noop.
As only one of the the two test-assignment sequences is task_lock()
protected, the task_lock() can't do much about race between the two.
Nothing prevents current_io_context() and set_task_ioprio() allocating
its own ioc for the same task and overwriting the other's.
Also, set_task_ioprio() can race with exiting task and create a new
ioc after exit_io_context() is finished.
ioc get/put doesn't have any reason to be complex. The only hot path
is accessing the existing ioc of %current, which is simple to achieve
given that ->io_context is never destroyed as long as the task is
alive. All other paths can happily go through task_lock() like all
other task sub structures without impacting anything.
This patch updates ioc get/put so that it becomes more conventional.
* alloc_io_context() is replaced with get_task_io_context(). This is
the only interface which can acquire access to ioc of another task.
On return, the caller has an explicit reference to the object which
should be put using put_io_context() afterwards.
* The functionality of current_io_context() remains the same but when
creating a new ioc, it shares the code path with
get_task_io_context() and always goes through task_lock().
* get_io_context() now means incrementing ref on an ioc which the
caller already has access to (be that an explicit refcnt or implicit
%current one).
* PF_EXITING inhibits creation of new io_context and once
exit_io_context() is finished, it's guaranteed that both ioc
acquisition functions return %NULL.
* All users are updated. Most are trivial but
smp_read_barrier_depends() removal from cfq_get_io_context() needs a
bit of explanation. I suppose the original intention was to ensure
ioc->ioprio is visible when set_task_ioprio() allocates new
io_context and installs it; however, this wouldn't have worked
because set_task_ioprio() doesn't have wmb between init and install.
There are other problems with this which will be fixed in another
patch.
* While at it, use NUMA_NO_NODE instead of -1 for wildcard node
specification.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-cgroup.c | 9 ++--
block/blk-ioc.c | 99 ++++++++++++++++++++++++++++++---------------
block/blk.h | 4 ++
block/cfq-iosched.c | 18 ++++----
fs/ioprio.c | 21 +--------
include/linux/iocontext.h | 4 +-
kernel/fork.c | 8 ++-
7 files changed, 94 insertions(+), 69 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 8f630ce..4b001dc 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1645,11 +1645,12 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
{
struct io_context *ioc;
- task_lock(tsk);
- ioc = tsk->io_context;
- if (ioc)
+ /* we don't lose anything even if ioc allocation fails */
+ ioc = get_task_io_context(tsk, GFP_ATOMIC, NUMA_NO_NODE);
+ if (ioc) {
ioc->cgroup_changed = 1;
- task_unlock(tsk);
+ put_io_context(ioc);
+ }
}
void blkio_policy_register(struct blkio_policy_type *blkiop)
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 8bebf06..b13ed96 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -16,6 +16,19 @@
*/
static struct kmem_cache *iocontext_cachep;
+/**
+ * get_io_context - increment reference count to io_context
+ * @ioc: io_context to get
+ *
+ * Increment reference count to @ioc.
+ */
+void get_io_context(struct io_context *ioc)
+{
+ BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
+ atomic_long_inc(&ioc->refcount);
+}
+EXPORT_SYMBOL(get_io_context);
+
static void cfq_dtor(struct io_context *ioc)
{
if (!hlist_empty(&ioc->cic_list)) {
@@ -71,6 +84,9 @@ void exit_io_context(struct task_struct *task)
{
struct io_context *ioc;
+ /* PF_EXITING prevents new io_context from being attached to @task */
+ WARN_ON_ONCE(!(current->flags & PF_EXITING));
+
task_lock(task);
ioc = task->io_context;
task->io_context = NULL;
@@ -82,7 +98,9 @@ void exit_io_context(struct task_struct *task)
put_io_context(ioc);
}
-struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
+static struct io_context *create_task_io_context(struct task_struct *task,
+ gfp_t gfp_flags, int node,
+ bool take_ref)
{
struct io_context *ioc;
@@ -98,6 +116,20 @@ struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
INIT_HLIST_HEAD(&ioc->cic_list);
+ /* try to install, somebody might already have beaten us to it */
+ task_lock(task);
+
+ if (!task->io_context && !(task->flags & PF_EXITING)) {
+ task->io_context = ioc;
+ } else {
+ kmem_cache_free(iocontext_cachep, ioc);
+ ioc = task->io_context;
+ }
+
+ if (ioc && take_ref)
+ get_io_context(ioc);
+
+ task_unlock(task);
return ioc;
}
@@ -114,46 +146,47 @@ struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
*/
struct io_context *current_io_context(gfp_t gfp_flags, int node)
{
- struct task_struct *tsk = current;
- struct io_context *ret;
-
- ret = tsk->io_context;
- if (likely(ret))
- return ret;
-
- ret = alloc_io_context(gfp_flags, node);
- if (ret) {
- /* make sure set_task_ioprio() sees the settings above */
- smp_wmb();
- tsk->io_context = ret;
- }
+ might_sleep_if(gfp_flags & __GFP_WAIT);
- return ret;
+ if (current->io_context)
+ return current->io_context;
+
+ return create_task_io_context(current, gfp_flags, node, false);
}
+EXPORT_SYMBOL(current_io_context);
-/*
- * If the current task has no IO context then create one and initialise it.
- * If it does have a context, take a ref on it.
+/**
+ * get_task_io_context - get io_context of a task
+ * @task: task of interest
+ * @gfp_flags: allocation flags, used if allocation is necessary
+ * @node: allocation node, used if allocation is necessary
+ *
+ * Return io_context of @task. If it doesn't exist, it is created with
+ * @gfp_flags and @node. The returned io_context has its reference count
+ * incremented.
*
- * This is always called in the context of the task which submitted the I/O.
+ * This function always goes through task_lock() and it's better to use
+ * current_io_context() + get_io_context() for %current.
*/
-struct io_context *get_io_context(gfp_t gfp_flags, int node)
+struct io_context *get_task_io_context(struct task_struct *task,
+ gfp_t gfp_flags, int node)
{
- struct io_context *ioc = NULL;
-
- /*
- * Check for unlikely race with exiting task. ioc ref count is
- * zero when ioc is being detached.
- */
- do {
- ioc = current_io_context(gfp_flags, node);
- if (unlikely(!ioc))
- break;
- } while (!atomic_long_inc_not_zero(&ioc->refcount));
+ struct io_context *ioc;
- return ioc;
+ might_sleep_if(gfp_flags & __GFP_WAIT);
+
+ task_lock(task);
+ ioc = task->io_context;
+ if (likely(ioc)) {
+ get_io_context(ioc);
+ task_unlock(task);
+ return ioc;
+ }
+ task_unlock(task);
+
+ return create_task_io_context(task, gfp_flags, node, true);
}
-EXPORT_SYMBOL(get_io_context);
+EXPORT_SYMBOL(get_task_io_context);
static int __init blk_ioc_init(void)
{
diff --git a/block/blk.h b/block/blk.h
index 1a97609..f2b36d8 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -123,6 +123,7 @@ static inline int blk_should_fake_timeout(struct request_queue *q)
}
#endif
+void get_io_context(struct io_context *ioc);
struct io_context *current_io_context(gfp_t gfp_flags, int node);
int ll_back_merge_fn(struct request_queue *q, struct request *req,
@@ -210,4 +211,7 @@ static inline void blk_throtl_exit(struct request_queue *q) { }
static inline void blk_throtl_release(struct request_queue *q) { }
#endif /* CONFIG_BLK_DEV_THROTTLING */
+extern char __blk_test_mode[2];
+#define blk_test_mode (__blk_test_mode[0])
+
#endif /* BLK_INTERNAL_H */
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ec3f5e8b..d42d89c 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -14,6 +14,7 @@
#include <linux/rbtree.h>
#include <linux/ioprio.h>
#include <linux/blktrace_api.h>
+#include "blk.h"
#include "cfq.h"
/*
@@ -3194,13 +3195,13 @@ static struct cfq_io_context *
cfq_get_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
{
struct io_context *ioc = NULL;
- struct cfq_io_context *cic;
+ struct cfq_io_context *cic = NULL;
might_sleep_if(gfp_mask & __GFP_WAIT);
- ioc = get_io_context(gfp_mask, cfqd->queue->node);
+ ioc = current_io_context(gfp_mask, cfqd->queue->node);
if (!ioc)
- return NULL;
+ goto err;
cic = cfq_cic_lookup(cfqd, ioc);
if (cic)
@@ -3211,10 +3212,10 @@ cfq_get_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
goto err;
if (cfq_cic_link(cfqd, ioc, cic, gfp_mask))
- goto err_free;
-
+ goto err;
out:
- smp_read_barrier_depends();
+ get_io_context(ioc);
+
if (unlikely(ioc->ioprio_changed))
cfq_ioc_set_ioprio(ioc);
@@ -3223,10 +3224,9 @@ out:
cfq_ioc_set_cgroup(ioc);
#endif
return cic;
-err_free:
- cfq_cic_free(cic);
err:
- put_io_context(ioc);
+ if (cic)
+ cfq_cic_free(cic);
return NULL;
}
diff --git a/fs/ioprio.c b/fs/ioprio.c
index 7da2a06..a4cb730 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -47,28 +47,13 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
if (err)
return err;
- task_lock(task);
- do {
- ioc = task->io_context;
- /* see wmb() in current_io_context() */
- smp_read_barrier_depends();
- if (ioc)
- break;
-
- ioc = alloc_io_context(GFP_ATOMIC, -1);
- if (!ioc) {
- err = -ENOMEM;
- break;
- }
- task->io_context = ioc;
- } while (1);
-
- if (!err) {
+ ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
+ if (ioc) {
ioc->ioprio = ioprio;
ioc->ioprio_changed = 1;
+ put_io_context(ioc);
}
- task_unlock(task);
return err;
}
EXPORT_SYMBOL_GPL(set_task_ioprio);
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 8a6ecb66..28bb621 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -78,8 +78,8 @@ struct task_struct;
#ifdef CONFIG_BLOCK
void put_io_context(struct io_context *ioc);
void exit_io_context(struct task_struct *task);
-struct io_context *get_io_context(gfp_t gfp_flags, int node);
-struct io_context *alloc_io_context(gfp_t gfp_flags, int node);
+struct io_context *get_task_io_context(struct task_struct *task,
+ gfp_t gfp_flags, int node);
#else
struct io_context;
static inline void put_io_context(struct io_context *ioc) { }
diff --git a/kernel/fork.c b/kernel/fork.c
index 8e6b6f4..11da931 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -878,6 +878,7 @@ static int copy_io(unsigned long clone_flags, struct task_struct *tsk)
{
#ifdef CONFIG_BLOCK
struct io_context *ioc = current->io_context;
+ struct io_context *new_ioc;
if (!ioc)
return 0;
@@ -889,11 +890,12 @@ static int copy_io(unsigned long clone_flags, struct task_struct *tsk)
if (unlikely(!tsk->io_context))
return -ENOMEM;
} else if (ioprio_valid(ioc->ioprio)) {
- tsk->io_context = alloc_io_context(GFP_KERNEL, -1);
- if (unlikely(!tsk->io_context))
+ new_ioc = get_task_io_context(tsk, GFP_KERNEL, NUMA_NO_NODE);
+ if (unlikely(!new_ioc))
return -ENOMEM;
- tsk->io_context->ioprio = ioc->ioprio;
+ new_ioc->ioprio = ioc->ioprio;
+ put_io_context(new_ioc);
}
#endif
return 0;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction
2011-10-26 1:48 ` [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction Tejun Heo
@ 2011-10-26 16:01 ` Vivek Goyal
2011-10-26 19:29 ` Tejun Heo
2011-10-26 21:30 ` [PATCH UPDATED " Tejun Heo
1 sibling, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-26 16:01 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Tue, Oct 25, 2011 at 06:48:30PM -0700, Tejun Heo wrote:
[..]
> @@ -1645,11 +1645,12 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> {
> struct io_context *ioc;
>
> - task_lock(tsk);
> - ioc = tsk->io_context;
> - if (ioc)
> + /* we don't lose anything even if ioc allocation fails */
> + ioc = get_task_io_context(tsk, GFP_ATOMIC, NUMA_NO_NODE);
> + if (ioc) {
> ioc->cgroup_changed = 1;
> - task_unlock(tsk);
> + put_io_context(ioc);
> + }
We really don't need to create a new io context here if it is not present.
cgroup_changed is set so that reference to old sync cfqq is dropped and
a new one is assigned when new IO comes in. Given the fact that io context
is not even present for the task being attached, there is no way there
is associated cic and associated cfqq. So no need to set changed_cgroup=1.
Will it make sense to pass in additional parameter to get_task_io_context()
so that it takes a reference if ioc exists otherwise returns NULL. Or may
be another variant of get_task_io_context() function.
This is a minor issue though. Will just avoid creating iocontext memory
for task if one is not using CFQ in the system.
[..]
>
> +extern char __blk_test_mode[2];
> +#define blk_test_mode (__blk_test_mode[0])
> +
Looks like leftover of your testing code.
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction
2011-10-26 16:01 ` Vivek Goyal
@ 2011-10-26 19:29 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 19:29 UTC (permalink / raw)
To: Vivek Goyal; +Cc: axboe, ctalbott, rni, linux-kernel
Hello,
On Wed, Oct 26, 2011 at 12:01:01PM -0400, Vivek Goyal wrote:
> We really don't need to create a new io context here if it is not present.
> cgroup_changed is set so that reference to old sync cfqq is dropped and
> a new one is assigned when new IO comes in. Given the fact that io context
> is not even present for the task being attached, there is no way there
> is associated cic and associated cfqq. So no need to set changed_cgroup=1.
>
> Will it make sense to pass in additional parameter to get_task_io_context()
> so that it takes a reference if ioc exists otherwise returns NULL. Or may
> be another variant of get_task_io_context() function.
>
> This is a minor issue though. Will just avoid creating iocontext memory
> for task if one is not using CFQ in the system.
Yeah, something like GFP_NOALLOC would have been nice for cases like
this. That said, adding another param or API for this doesn't sound
too hot to me. It's an extremely marginal optimization. Let's just
let it be.
> > +extern char __blk_test_mode[2];
> > +#define blk_test_mode (__blk_test_mode[0])
> > +
>
> Looks like leftover of your testing code.
Heh... what does that doing there? :) Will update the patch & git
tree.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH UPDATED 04/13] block: make ioc get/put interface more conventional and fix race on alloction
2011-10-26 1:48 ` [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction Tejun Heo
2011-10-26 16:01 ` Vivek Goyal
@ 2011-10-26 21:30 ` Tejun Heo
1 sibling, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 21:30 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel
Ignoring copy_io() during fork, io_context can be allocated from two
places - current_io_context() and set_task_ioprio(). The former is
always called from local task while the latter can be called from
different task. The synchornization between them are peculiar and
dubious.
* current_io_context() doesn't grab task_lock() and assumes that if it
saw %NULL ->io_context, it would stay that way until allocation and
assignment is complete. It has smp_wmb() between alloc/init and
assignment.
* set_task_ioprio() grabs task_lock() for assignment and does
smp_read_barrier_depends() between "ioc = task->io_context" and "if
(ioc)". Unfortunately, this doesn't achieve anything - the latter
is not a dependent load of the former. ie, if ioc itself were being
dereferenced "ioc->xxx", it would mean something (not sure what tho)
but as the code currently stands, the dependent read barrier is
noop.
As only one of the the two test-assignment sequences is task_lock()
protected, the task_lock() can't do much about race between the two.
Nothing prevents current_io_context() and set_task_ioprio() allocating
its own ioc for the same task and overwriting the other's.
Also, set_task_ioprio() can race with exiting task and create a new
ioc after exit_io_context() is finished.
ioc get/put doesn't have any reason to be complex. The only hot path
is accessing the existing ioc of %current, which is simple to achieve
given that ->io_context is never destroyed as long as the task is
alive. All other paths can happily go through task_lock() like all
other task sub structures without impacting anything.
This patch updates ioc get/put so that it becomes more conventional.
* alloc_io_context() is replaced with get_task_io_context(). This is
the only interface which can acquire access to ioc of another task.
On return, the caller has an explicit reference to the object which
should be put using put_io_context() afterwards.
* The functionality of current_io_context() remains the same but when
creating a new ioc, it shares the code path with
get_task_io_context() and always goes through task_lock().
* get_io_context() now means incrementing ref on an ioc which the
caller already has access to (be that an explicit refcnt or implicit
%current one).
* PF_EXITING inhibits creation of new io_context and once
exit_io_context() is finished, it's guaranteed that both ioc
acquisition functions return %NULL.
* All users are updated. Most are trivial but
smp_read_barrier_depends() removal from cfq_get_io_context() needs a
bit of explanation. I suppose the original intention was to ensure
ioc->ioprio is visible when set_task_ioprio() allocates new
io_context and installs it; however, this wouldn't have worked
because set_task_ioprio() doesn't have wmb between init and install.
There are other problems with this which will be fixed in another
patch.
* While at it, use NUMA_NO_NODE instead of -1 for wildcard node
specification.
-v2: Vivek spotted contamination from debug patch. Removed.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Vivek Goyal <vgoyal@redhat.com>
---
block/blk-cgroup.c | 9 ++--
block/blk-ioc.c | 99 ++++++++++++++++++++++++++++++---------------
block/blk.h | 1 +
block/cfq-iosched.c | 18 ++++----
fs/ioprio.c | 21 +--------
include/linux/iocontext.h | 4 +-
kernel/fork.c | 8 ++-
7 files changed, 91 insertions(+), 69 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 8f630ce..4b001dc 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1645,11 +1645,12 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
{
struct io_context *ioc;
- task_lock(tsk);
- ioc = tsk->io_context;
- if (ioc)
+ /* we don't lose anything even if ioc allocation fails */
+ ioc = get_task_io_context(tsk, GFP_ATOMIC, NUMA_NO_NODE);
+ if (ioc) {
ioc->cgroup_changed = 1;
- task_unlock(tsk);
+ put_io_context(ioc);
+ }
}
void blkio_policy_register(struct blkio_policy_type *blkiop)
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 8bebf06..b13ed96 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -16,6 +16,19 @@
*/
static struct kmem_cache *iocontext_cachep;
+/**
+ * get_io_context - increment reference count to io_context
+ * @ioc: io_context to get
+ *
+ * Increment reference count to @ioc.
+ */
+void get_io_context(struct io_context *ioc)
+{
+ BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
+ atomic_long_inc(&ioc->refcount);
+}
+EXPORT_SYMBOL(get_io_context);
+
static void cfq_dtor(struct io_context *ioc)
{
if (!hlist_empty(&ioc->cic_list)) {
@@ -71,6 +84,9 @@ void exit_io_context(struct task_struct *task)
{
struct io_context *ioc;
+ /* PF_EXITING prevents new io_context from being attached to @task */
+ WARN_ON_ONCE(!(current->flags & PF_EXITING));
+
task_lock(task);
ioc = task->io_context;
task->io_context = NULL;
@@ -82,7 +98,9 @@ void exit_io_context(struct task_struct *task)
put_io_context(ioc);
}
-struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
+static struct io_context *create_task_io_context(struct task_struct *task,
+ gfp_t gfp_flags, int node,
+ bool take_ref)
{
struct io_context *ioc;
@@ -98,6 +116,20 @@ struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
INIT_HLIST_HEAD(&ioc->cic_list);
+ /* try to install, somebody might already have beaten us to it */
+ task_lock(task);
+
+ if (!task->io_context && !(task->flags & PF_EXITING)) {
+ task->io_context = ioc;
+ } else {
+ kmem_cache_free(iocontext_cachep, ioc);
+ ioc = task->io_context;
+ }
+
+ if (ioc && take_ref)
+ get_io_context(ioc);
+
+ task_unlock(task);
return ioc;
}
@@ -114,46 +146,47 @@ struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
*/
struct io_context *current_io_context(gfp_t gfp_flags, int node)
{
- struct task_struct *tsk = current;
- struct io_context *ret;
-
- ret = tsk->io_context;
- if (likely(ret))
- return ret;
-
- ret = alloc_io_context(gfp_flags, node);
- if (ret) {
- /* make sure set_task_ioprio() sees the settings above */
- smp_wmb();
- tsk->io_context = ret;
- }
+ might_sleep_if(gfp_flags & __GFP_WAIT);
- return ret;
+ if (current->io_context)
+ return current->io_context;
+
+ return create_task_io_context(current, gfp_flags, node, false);
}
+EXPORT_SYMBOL(current_io_context);
-/*
- * If the current task has no IO context then create one and initialise it.
- * If it does have a context, take a ref on it.
+/**
+ * get_task_io_context - get io_context of a task
+ * @task: task of interest
+ * @gfp_flags: allocation flags, used if allocation is necessary
+ * @node: allocation node, used if allocation is necessary
+ *
+ * Return io_context of @task. If it doesn't exist, it is created with
+ * @gfp_flags and @node. The returned io_context has its reference count
+ * incremented.
*
- * This is always called in the context of the task which submitted the I/O.
+ * This function always goes through task_lock() and it's better to use
+ * current_io_context() + get_io_context() for %current.
*/
-struct io_context *get_io_context(gfp_t gfp_flags, int node)
+struct io_context *get_task_io_context(struct task_struct *task,
+ gfp_t gfp_flags, int node)
{
- struct io_context *ioc = NULL;
-
- /*
- * Check for unlikely race with exiting task. ioc ref count is
- * zero when ioc is being detached.
- */
- do {
- ioc = current_io_context(gfp_flags, node);
- if (unlikely(!ioc))
- break;
- } while (!atomic_long_inc_not_zero(&ioc->refcount));
+ struct io_context *ioc;
- return ioc;
+ might_sleep_if(gfp_flags & __GFP_WAIT);
+
+ task_lock(task);
+ ioc = task->io_context;
+ if (likely(ioc)) {
+ get_io_context(ioc);
+ task_unlock(task);
+ return ioc;
+ }
+ task_unlock(task);
+
+ return create_task_io_context(task, gfp_flags, node, true);
}
-EXPORT_SYMBOL(get_io_context);
+EXPORT_SYMBOL(get_task_io_context);
static int __init blk_ioc_init(void)
{
diff --git a/block/blk.h b/block/blk.h
index aae4d88..fc3c41b 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -122,6 +122,7 @@ static inline int blk_should_fake_timeout(struct request_queue *q)
}
#endif
+void get_io_context(struct io_context *ioc);
struct io_context *current_io_context(gfp_t gfp_flags, int node);
int ll_back_merge_fn(struct request_queue *q, struct request *req,
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ec3f5e8b..d42d89c 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -14,6 +14,7 @@
#include <linux/rbtree.h>
#include <linux/ioprio.h>
#include <linux/blktrace_api.h>
+#include "blk.h"
#include "cfq.h"
/*
@@ -3194,13 +3195,13 @@ static struct cfq_io_context *
cfq_get_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
{
struct io_context *ioc = NULL;
- struct cfq_io_context *cic;
+ struct cfq_io_context *cic = NULL;
might_sleep_if(gfp_mask & __GFP_WAIT);
- ioc = get_io_context(gfp_mask, cfqd->queue->node);
+ ioc = current_io_context(gfp_mask, cfqd->queue->node);
if (!ioc)
- return NULL;
+ goto err;
cic = cfq_cic_lookup(cfqd, ioc);
if (cic)
@@ -3211,10 +3212,10 @@ cfq_get_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
goto err;
if (cfq_cic_link(cfqd, ioc, cic, gfp_mask))
- goto err_free;
-
+ goto err;
out:
- smp_read_barrier_depends();
+ get_io_context(ioc);
+
if (unlikely(ioc->ioprio_changed))
cfq_ioc_set_ioprio(ioc);
@@ -3223,10 +3224,9 @@ out:
cfq_ioc_set_cgroup(ioc);
#endif
return cic;
-err_free:
- cfq_cic_free(cic);
err:
- put_io_context(ioc);
+ if (cic)
+ cfq_cic_free(cic);
return NULL;
}
diff --git a/fs/ioprio.c b/fs/ioprio.c
index 7da2a06..a4cb730 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -47,28 +47,13 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
if (err)
return err;
- task_lock(task);
- do {
- ioc = task->io_context;
- /* see wmb() in current_io_context() */
- smp_read_barrier_depends();
- if (ioc)
- break;
-
- ioc = alloc_io_context(GFP_ATOMIC, -1);
- if (!ioc) {
- err = -ENOMEM;
- break;
- }
- task->io_context = ioc;
- } while (1);
-
- if (!err) {
+ ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
+ if (ioc) {
ioc->ioprio = ioprio;
ioc->ioprio_changed = 1;
+ put_io_context(ioc);
}
- task_unlock(task);
return err;
}
EXPORT_SYMBOL_GPL(set_task_ioprio);
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 8a6ecb66..28bb621 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -78,8 +78,8 @@ struct task_struct;
#ifdef CONFIG_BLOCK
void put_io_context(struct io_context *ioc);
void exit_io_context(struct task_struct *task);
-struct io_context *get_io_context(gfp_t gfp_flags, int node);
-struct io_context *alloc_io_context(gfp_t gfp_flags, int node);
+struct io_context *get_task_io_context(struct task_struct *task,
+ gfp_t gfp_flags, int node);
#else
struct io_context;
static inline void put_io_context(struct io_context *ioc) { }
diff --git a/kernel/fork.c b/kernel/fork.c
index 8e6b6f4..11da931 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -878,6 +878,7 @@ static int copy_io(unsigned long clone_flags, struct task_struct *tsk)
{
#ifdef CONFIG_BLOCK
struct io_context *ioc = current->io_context;
+ struct io_context *new_ioc;
if (!ioc)
return 0;
@@ -889,11 +890,12 @@ static int copy_io(unsigned long clone_flags, struct task_struct *tsk)
if (unlikely(!tsk->io_context))
return -ENOMEM;
} else if (ioprio_valid(ioc->ioprio)) {
- tsk->io_context = alloc_io_context(GFP_KERNEL, -1);
- if (unlikely(!tsk->io_context))
+ new_ioc = get_task_io_context(tsk, GFP_KERNEL, NUMA_NO_NODE);
+ if (unlikely(!new_ioc))
return -ENOMEM;
- tsk->io_context->ioprio = ioc->ioprio;
+ new_ioc->ioprio = ioc->ioprio;
+ put_io_context(new_ioc);
}
#endif
return 0;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 05/13] block: misc updates to blk_get_queue()
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (3 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 06/13] block, cfq: misc updates to cfq_io_context Tejun Heo
` (8 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
* blk_get_queue() is peculiar in that it returns 0 on success and 1 on
failure instead of 0 / -errno or boolean. Update it such that it
returns %true on success and %false on failure.
* Make sure the caller checks for the return value.
* Separate out __blk_get_queue() which doesn't check whether @q is
dead and put it in blk.h. This will be used later.
This patch doesn't introduce any functional changes.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-core.c | 8 ++++----
block/blk.h | 5 +++++
block/bsg.c | 4 +---
block/genhd.c | 2 +-
drivers/scsi/scsi_scan.c | 2 +-
include/linux/blkdev.h | 2 +-
6 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index d7304d8..db936e3 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -621,14 +621,14 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
}
EXPORT_SYMBOL(blk_init_allocated_queue_node);
-int blk_get_queue(struct request_queue *q)
+bool blk_get_queue(struct request_queue *q)
{
if (likely(!blk_queue_dead(q))) {
- kobject_get(&q->kobj);
- return 0;
+ __blk_get_queue(q);
+ return true;
}
- return 1;
+ return false;
}
EXPORT_SYMBOL(blk_get_queue);
diff --git a/block/blk.h b/block/blk.h
index f2b36d8..676a476 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -13,6 +13,11 @@ extern struct kmem_cache *blk_requestq_cachep;
extern struct kobj_type blk_queue_ktype;
extern struct ida blk_queue_ida;
+static inline void __blk_get_queue(struct request_queue *q)
+{
+ kobject_get(&q->kobj);
+}
+
void init_request_from_bio(struct request *req, struct bio *bio);
void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
struct bio *bio);
diff --git a/block/bsg.c b/block/bsg.c
index 702f131..167d586 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -769,12 +769,10 @@ static struct bsg_device *bsg_add_device(struct inode *inode,
struct file *file)
{
struct bsg_device *bd;
- int ret;
#ifdef BSG_DEBUG
unsigned char buf[32];
#endif
- ret = blk_get_queue(rq);
- if (ret)
+ if (!blk_get_queue(rq))
return ERR_PTR(-ENXIO);
bd = bsg_alloc_device();
diff --git a/block/genhd.c b/block/genhd.c
index d261b73..156a4c3 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -615,7 +615,7 @@ void add_disk(struct gendisk *disk)
* Take an extra ref on queue which will be put on disk_release()
* so that it sticks around as long as @disk is there.
*/
- WARN_ON_ONCE(blk_get_queue(disk->queue));
+ WARN_ON_ONCE(!blk_get_queue(disk->queue));
retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
"bdi");
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 44e8ca3..f8103f4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -297,7 +297,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
kfree(sdev);
goto out;
}
- blk_get_queue(sdev->request_queue);
+ WARN_ON_ONCE(!blk_get_queue(sdev->request_queue));
sdev->request_queue->queuedata = sdev;
scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 565a4ed..19037fb 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -865,7 +865,7 @@ extern int blk_rq_map_sg(struct request_queue *, struct request *, struct scatte
extern void blk_dump_rq_flags(struct request *, char *);
extern long nr_blockdev_pages(void);
-int blk_get_queue(struct request_queue *);
+bool __must_check blk_get_queue(struct request_queue *);
struct request_queue *blk_alloc_queue(gfp_t);
struct request_queue *blk_alloc_queue_node(gfp_t, int);
extern void blk_put_queue(struct request_queue *);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 06/13] block, cfq: misc updates to cfq_io_context
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (4 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 05/13] block: misc updates to blk_get_queue() Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-27 15:39 ` Vivek Goyal
2011-10-26 1:48 ` [PATCH 07/13] block, cfq: move ioc ioprio/cgroup changed handling to cic Tejun Heo
` (7 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
Make the following changes to prepare for ioc/cic management cleanup.
* Add cic->q so that ioc can determine the associated queue without
querying cfq. This will eventually replace ->key.
* Factor out cfq_release_cic() from cic_free_func(). This function
assumes that the caller handled locking.
* Rename __cfq_exit_single_io_context() to cfq_exit_cic() and make it
take only @cic.
* Restructure cfq_cic_link() for future updates.
This patch doesn't introduce any functional changes.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/cfq-iosched.c | 58 ++++++++++++++++++++++++--------------------
include/linux/iocontext.h | 1 +
2 files changed, 33 insertions(+), 26 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index d42d89c..a612ca6 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2709,21 +2709,26 @@ static void cfq_cic_free(struct cfq_io_context *cic)
call_rcu(&cic->rcu_head, cfq_cic_free_rcu);
}
-static void cic_free_func(struct io_context *ioc, struct cfq_io_context *cic)
+static void cfq_release_cic(struct cfq_io_context *cic)
{
- unsigned long flags;
+ struct io_context *ioc = cic->ioc;
unsigned long dead_key = (unsigned long) cic->key;
BUG_ON(!(dead_key & CIC_DEAD_KEY));
-
- spin_lock_irqsave(&ioc->lock, flags);
radix_tree_delete(&ioc->radix_root, dead_key >> CIC_DEAD_INDEX_SHIFT);
hlist_del_rcu(&cic->cic_list);
- spin_unlock_irqrestore(&ioc->lock, flags);
-
cfq_cic_free(cic);
}
+static void cic_free_func(struct io_context *ioc, struct cfq_io_context *cic)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&ioc->lock, flags);
+ cfq_release_cic(cic);
+ spin_unlock_irqrestore(&ioc->lock, flags);
+}
+
/*
* Must be called with rcu_read_lock() held or preemption otherwise disabled.
* Only two callers of this - ->dtor() which is called with the rcu_read_lock(),
@@ -2773,9 +2778,9 @@ static void cfq_exit_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq)
cfq_put_queue(cfqq);
}
-static void __cfq_exit_single_io_context(struct cfq_data *cfqd,
- struct cfq_io_context *cic)
+static void cfq_exit_cic(struct cfq_io_context *cic)
{
+ struct cfq_data *cfqd = cic_to_cfqd(cic);
struct io_context *ioc = cic->ioc;
list_del_init(&cic->queue_list);
@@ -2823,7 +2828,7 @@ static void cfq_exit_single_io_context(struct io_context *ioc,
*/
smp_read_barrier_depends();
if (cic->key == cfqd)
- __cfq_exit_single_io_context(cfqd, cic);
+ cfq_exit_cic(cic);
spin_unlock_irqrestore(q->queue_lock, flags);
}
@@ -3161,28 +3166,29 @@ static int cfq_cic_link(struct cfq_data *cfqd, struct io_context *ioc,
int ret;
ret = radix_tree_preload(gfp_mask);
- if (!ret) {
- cic->ioc = ioc;
- cic->key = cfqd;
+ if (ret)
+ goto out;
- spin_lock_irqsave(&ioc->lock, flags);
- ret = radix_tree_insert(&ioc->radix_root, cfqd->queue->id, cic);
- if (!ret)
- hlist_add_head_rcu(&cic->cic_list, &ioc->cic_list);
- spin_unlock_irqrestore(&ioc->lock, flags);
+ cic->ioc = ioc;
+ cic->key = cfqd;
+ cic->q = cfqd->queue;
+
+ spin_lock_irqsave(&ioc->lock, flags);
+ ret = radix_tree_insert(&ioc->radix_root, cfqd->queue->id, cic);
+ if (!ret)
+ hlist_add_head_rcu(&cic->cic_list, &ioc->cic_list);
+ spin_unlock_irqrestore(&ioc->lock, flags);
- radix_tree_preload_end();
+ radix_tree_preload_end();
- if (!ret) {
- spin_lock_irqsave(cfqd->queue->queue_lock, flags);
- list_add(&cic->queue_list, &cfqd->cic_list);
- spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
- }
+ if (!ret) {
+ spin_lock_irqsave(cfqd->queue->queue_lock, flags);
+ list_add(&cic->queue_list, &cfqd->cic_list);
+ spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
}
-
+out:
if (ret)
printk(KERN_ERR "cfq: cic link failed!\n");
-
return ret;
}
@@ -3922,7 +3928,7 @@ static void cfq_exit_queue(struct elevator_queue *e)
struct cfq_io_context,
queue_list);
- __cfq_exit_single_io_context(cfqd, cic);
+ cfq_exit_cic(cic);
}
cfq_put_async_queues(cfqd);
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 28bb621..079aea8 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -15,6 +15,7 @@ struct cfq_ttime {
struct cfq_io_context {
void *key;
+ struct request_queue *q;
struct cfq_queue *cfqq[2];
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 06/13] block, cfq: misc updates to cfq_io_context
2011-10-26 1:48 ` [PATCH 06/13] block, cfq: misc updates to cfq_io_context Tejun Heo
@ 2011-10-27 15:39 ` Vivek Goyal
2011-10-27 16:24 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-27 15:39 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Tue, Oct 25, 2011 at 06:48:32PM -0700, Tejun Heo wrote:
> Make the following changes to prepare for ioc/cic management cleanup.
>
> * Add cic->q so that ioc can determine the associated queue without
> querying cfq. This will eventually replace ->key.
Not sure what's the advantage of storing cic->q instead of cic->cfqd.
In blk-ioc code getting to cic's queue becomes easier but then reaching
to cfqd in cfq code becomes much longer.
static inline struct cfq_data *cic_to_cfqd(struct cfq_io_context *cic)
{
return cic->q->elevator->elevator_data;
}
May be storing cic->cfqd is not a bad idea.
For blk-ioc.
cic_queue = cic->cfqd->queue;
And for CFQ
cic_cfqd = cic->cfqd;
None of the paths is too long for either code. It is a very minor point
though.
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 06/13] block, cfq: misc updates to cfq_io_context
2011-10-27 15:39 ` Vivek Goyal
@ 2011-10-27 16:24 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-27 16:24 UTC (permalink / raw)
To: Vivek Goyal; +Cc: axboe, ctalbott, rni, linux-kernel
Hello, Vivek.
On Thu, Oct 27, 2011 at 11:39:01AM -0400, Vivek Goyal wrote:
> On Tue, Oct 25, 2011 at 06:48:32PM -0700, Tejun Heo wrote:
> > Make the following changes to prepare for ioc/cic management cleanup.
> >
> > * Add cic->q so that ioc can determine the associated queue without
> > querying cfq. This will eventually replace ->key.
>
> Not sure what's the advantage of storing cic->q instead of cic->cfqd.
> In blk-ioc code getting to cic's queue becomes easier but then reaching
> to cfqd in cfq code becomes much longer.
It's primarily for API separation. Currently, io_context and cfq are
intermixed without clear layer of separation. The optimization of
avoiding one indirect deref at this level isn't worth blurring API
boundaries especially when the cachelines being derefed are known to
be hot (we're deep in elevator path after all). I'm working on a
patchset to make proper separation between them. If reducing that one
indrect load proves to be worthwhile (I'm extremely skeptical tho), we
can use another another caching entry in cic.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 07/13] block, cfq: move ioc ioprio/cgroup changed handling to cic
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (5 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 06/13] block, cfq: misc updates to cfq_io_context Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 08/13] block, cfq: fix race condition in cic creation path and tighten locking Tejun Heo
` (6 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
ioprio/cgroup change was handled by marking the changed state in ioc
and, on the following access to the ioc, performing RCU-protected
iteration through all cic's grabbing the matching queue_lock.
This patch moves the changed state to each cic. When ioprio or cgroup
changes, the respective bit is set on all cic's of the ioc and when
each of those cic (not ioc) is accessed, change is applied for that
specific ioc-queue pair.
This also fixes the following two race conditions between setting and
clearing of changed states.
* Missing barrier between assign/load of ioprio and ioprio_changed
allowed applying old ioprio.
* Change requests could happen between application of change and
clearing of changed variables.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-cgroup.c | 2 +-
block/blk-ioc.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
block/cfq-iosched.c | 28 +++++++++-------------------
fs/ioprio.c | 3 +--
include/linux/iocontext.h | 14 +++++++++-----
5 files changed, 65 insertions(+), 27 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4b001dc..dc00835 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1648,7 +1648,7 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
/* we don't lose anything even if ioc allocation fails */
ioc = get_task_io_context(tsk, GFP_ATOMIC, NUMA_NO_NODE);
if (ioc) {
- ioc->cgroup_changed = 1;
+ ioc_cgroup_changed(ioc);
put_io_context(ioc);
}
}
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index b13ed96..6f59fba 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -188,6 +188,51 @@ struct io_context *get_task_io_context(struct task_struct *task,
}
EXPORT_SYMBOL(get_task_io_context);
+void ioc_set_changed(struct io_context *ioc, int which)
+{
+ struct cfq_io_context *cic;
+ struct hlist_node *n;
+
+ hlist_for_each_entry(cic, n, &ioc->cic_list, cic_list)
+ set_bit(which, &cic->changed);
+}
+
+/**
+ * ioc_ioprio_changed - notify ioprio change
+ * @ioc: io_context of interest
+ * @ioprio: new ioprio
+ *
+ * @ioc's ioprio has changed to @ioprio. Set %CIC_IOPRIO_CHANGED for all
+ * cic's. iosched is responsible for checking the bit and applying it on
+ * request issue path.
+ */
+void ioc_ioprio_changed(struct io_context *ioc, int ioprio)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&ioc->lock, flags);
+ ioc->ioprio = ioprio;
+ ioc_set_changed(ioc, CIC_IOPRIO_CHANGED);
+ spin_unlock_irqrestore(&ioc->lock, flags);
+}
+
+/**
+ * ioc_cgroup_changed - notify cgroup change
+ * @ioc: io_context of interest
+ *
+ * @ioc's cgroup has changed. Set %CIC_CGROUP_CHANGED for all cic's.
+ * iosched is responsible for checking the bit and applying it on request
+ * issue path.
+ */
+void ioc_cgroup_changed(struct io_context *ioc)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&ioc->lock, flags);
+ ioc_set_changed(ioc, CIC_CGROUP_CHANGED);
+ spin_unlock_irqrestore(&ioc->lock, flags);
+}
+
static int __init blk_ioc_init(void)
{
iocontext_cachep = kmem_cache_create("blkdev_ioc",
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index a612ca6..51aece2 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2904,7 +2904,7 @@ static void cfq_init_prio_data(struct cfq_queue *cfqq, struct io_context *ioc)
cfq_clear_cfqq_prio_changed(cfqq);
}
-static void changed_ioprio(struct io_context *ioc, struct cfq_io_context *cic)
+static void changed_ioprio(struct cfq_io_context *cic)
{
struct cfq_data *cfqd = cic_to_cfqd(cic);
struct cfq_queue *cfqq;
@@ -2933,12 +2933,6 @@ static void changed_ioprio(struct io_context *ioc, struct cfq_io_context *cic)
spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
}
-static void cfq_ioc_set_ioprio(struct io_context *ioc)
-{
- call_for_each_cic(ioc, changed_ioprio);
- ioc->ioprio_changed = 0;
-}
-
static void cfq_init_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq,
pid_t pid, bool is_sync)
{
@@ -2960,7 +2954,7 @@ static void cfq_init_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq,
}
#ifdef CONFIG_CFQ_GROUP_IOSCHED
-static void changed_cgroup(struct io_context *ioc, struct cfq_io_context *cic)
+static void changed_cgroup(struct cfq_io_context *cic)
{
struct cfq_queue *sync_cfqq = cic_to_cfqq(cic, 1);
struct cfq_data *cfqd = cic_to_cfqd(cic);
@@ -2986,12 +2980,6 @@ static void changed_cgroup(struct io_context *ioc, struct cfq_io_context *cic)
spin_unlock_irqrestore(q->queue_lock, flags);
}
-
-static void cfq_ioc_set_cgroup(struct io_context *ioc)
-{
- call_for_each_cic(ioc, changed_cgroup);
- ioc->cgroup_changed = 0;
-}
#endif /* CONFIG_CFQ_GROUP_IOSCHED */
static struct cfq_queue *
@@ -3222,13 +3210,15 @@ cfq_get_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
out:
get_io_context(ioc);
- if (unlikely(ioc->ioprio_changed))
- cfq_ioc_set_ioprio(ioc);
-
+ if (unlikely(cic->changed)) {
+ if (test_and_clear_bit(CIC_IOPRIO_CHANGED, &cic->changed))
+ changed_ioprio(cic);
#ifdef CONFIG_CFQ_GROUP_IOSCHED
- if (unlikely(ioc->cgroup_changed))
- cfq_ioc_set_cgroup(ioc);
+ if (test_and_clear_bit(CIC_CGROUP_CHANGED, &cic->changed))
+ changed_cgroup(cic);
#endif
+ }
+
return cic;
err:
if (cic)
diff --git a/fs/ioprio.c b/fs/ioprio.c
index a4cb730..f8d8429 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -49,8 +49,7 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
if (ioc) {
- ioc->ioprio = ioprio;
- ioc->ioprio_changed = 1;
+ ioc_ioprio_changed(ioc, ioprio);
put_io_context(ioc);
}
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 079aea8..2c2b6da 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -13,6 +13,11 @@ struct cfq_ttime {
unsigned long ttime_mean;
};
+enum {
+ CIC_IOPRIO_CHANGED,
+ CIC_CGROUP_CHANGED,
+};
+
struct cfq_io_context {
void *key;
struct request_queue *q;
@@ -26,6 +31,8 @@ struct cfq_io_context {
struct list_head queue_list;
struct hlist_node cic_list;
+ unsigned long changed;
+
void (*dtor)(struct io_context *); /* destructor */
void (*exit)(struct io_context *); /* called on task exit */
@@ -44,11 +51,6 @@ struct io_context {
spinlock_t lock;
unsigned short ioprio;
- unsigned short ioprio_changed;
-
-#if defined(CONFIG_BLK_CGROUP) || defined(CONFIG_BLK_CGROUP_MODULE)
- unsigned short cgroup_changed;
-#endif
/*
* For request batching
@@ -81,6 +83,8 @@ void put_io_context(struct io_context *ioc);
void exit_io_context(struct task_struct *task);
struct io_context *get_task_io_context(struct task_struct *task,
gfp_t gfp_flags, int node);
+void ioc_ioprio_changed(struct io_context *ioc, int ioprio);
+void ioc_cgroup_changed(struct io_context *ioc);
#else
struct io_context;
static inline void put_io_context(struct io_context *ioc) { }
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 08/13] block, cfq: fix race condition in cic creation path and tighten locking
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (6 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 07/13] block, cfq: move ioc ioprio/cgroup changed handling to cic Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 09/13] block, cfq: fix cic lookup locking Tejun Heo
` (5 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
cfq_get_io_context() would fail if multiple tasks race to insert cic's
for the same association. This patch restructures
cfq_get_io_context() such that slow path insertion race is handled
properly.
Note that the restructuring also makes cfq_get_io_context() called
under queue_lock and performs both ioc and cfqd insertions while
holding both ioc and queue locks. This is part of on-going locking
tightening and will be used to simplify synchronization rules.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/cfq-iosched.c | 135 ++++++++++++++++++++++++++++----------------------
1 files changed, 76 insertions(+), 59 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 51aece2..181a63d 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2908,13 +2908,10 @@ static void changed_ioprio(struct cfq_io_context *cic)
{
struct cfq_data *cfqd = cic_to_cfqd(cic);
struct cfq_queue *cfqq;
- unsigned long flags;
if (unlikely(!cfqd))
return;
- spin_lock_irqsave(cfqd->queue->queue_lock, flags);
-
cfqq = cic->cfqq[BLK_RW_ASYNC];
if (cfqq) {
struct cfq_queue *new_cfqq;
@@ -2929,8 +2926,6 @@ static void changed_ioprio(struct cfq_io_context *cic)
cfqq = cic->cfqq[BLK_RW_SYNC];
if (cfqq)
cfq_mark_cfqq_prio_changed(cfqq);
-
- spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
}
static void cfq_init_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq,
@@ -2958,7 +2953,6 @@ static void changed_cgroup(struct cfq_io_context *cic)
{
struct cfq_queue *sync_cfqq = cic_to_cfqq(cic, 1);
struct cfq_data *cfqd = cic_to_cfqd(cic);
- unsigned long flags;
struct request_queue *q;
if (unlikely(!cfqd))
@@ -2966,8 +2960,6 @@ static void changed_cgroup(struct cfq_io_context *cic)
q = cfqd->queue;
- spin_lock_irqsave(q->queue_lock, flags);
-
if (sync_cfqq) {
/*
* Drop reference to sync queue. A new sync queue will be
@@ -2977,8 +2969,6 @@ static void changed_cgroup(struct cfq_io_context *cic)
cic_set_cfqq(cic, NULL, 1);
cfq_put_queue(sync_cfqq);
}
-
- spin_unlock_irqrestore(q->queue_lock, flags);
}
#endif /* CONFIG_CFQ_GROUP_IOSCHED */
@@ -3142,16 +3132,31 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
return cic;
}
-/*
- * Add cic into ioc, using cfqd as the search key. This enables us to lookup
- * the process specific cfq io context when entered from the block layer.
- * Also adds the cic to a per-cfqd list, used when this queue is removed.
+/**
+ * cfq_create_cic - create and link a cfq_io_context
+ * @cfqd: cfqd of interest
+ * @gfp_mask: allocation mask
+ *
+ * Make sure cfq_io_context linking %current->io_context and @cfqd exists.
+ * If ioc and/or cic doesn't exist, they will be created using @gfp_mask.
*/
-static int cfq_cic_link(struct cfq_data *cfqd, struct io_context *ioc,
- struct cfq_io_context *cic, gfp_t gfp_mask)
+static int cfq_create_cic(struct cfq_data *cfqd, gfp_t gfp_mask)
{
- unsigned long flags;
- int ret;
+ struct request_queue *q = cfqd->queue;
+ struct cfq_io_context *cic = NULL;
+ struct io_context *ioc;
+ int ret = -ENOMEM;
+
+ might_sleep_if(gfp_mask & __GFP_WAIT);
+
+ /* allocate stuff */
+ ioc = current_io_context(gfp_mask, q->node);
+ if (!ioc)
+ goto out;
+
+ cic = cfq_alloc_io_context(cfqd, gfp_mask);
+ if (!cic)
+ goto out;
ret = radix_tree_preload(gfp_mask);
if (ret)
@@ -3161,53 +3166,72 @@ static int cfq_cic_link(struct cfq_data *cfqd, struct io_context *ioc,
cic->key = cfqd;
cic->q = cfqd->queue;
- spin_lock_irqsave(&ioc->lock, flags);
- ret = radix_tree_insert(&ioc->radix_root, cfqd->queue->id, cic);
- if (!ret)
- hlist_add_head_rcu(&cic->cic_list, &ioc->cic_list);
- spin_unlock_irqrestore(&ioc->lock, flags);
-
- radix_tree_preload_end();
+ /* lock both q and ioc and try to link @cic */
+ spin_lock_irq(q->queue_lock);
+ spin_lock(&ioc->lock);
- if (!ret) {
- spin_lock_irqsave(cfqd->queue->queue_lock, flags);
+ ret = radix_tree_insert(&ioc->radix_root, q->id, cic);
+ if (likely(!ret)) {
+ hlist_add_head_rcu(&cic->cic_list, &ioc->cic_list);
list_add(&cic->queue_list, &cfqd->cic_list);
- spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
+ cic = NULL;
+ } else if (ret == -EEXIST) {
+ /* someone else already did it */
+ ret = 0;
}
+
+ spin_unlock(&ioc->lock);
+ spin_unlock_irq(q->queue_lock);
+
+ radix_tree_preload_end();
out:
if (ret)
printk(KERN_ERR "cfq: cic link failed!\n");
+ if (cic)
+ cfq_cic_free(cic);
return ret;
}
-/*
- * Setup general io context and cfq io context. There can be several cfq
- * io contexts per general io context, if this process is doing io to more
- * than one device managed by cfq.
+/**
+ * cfq_get_io_context - acquire cfq_io_context and bump refcnt on io_context
+ * @cfqd: cfqd to setup cic for
+ * @gfp_mask: allocation mask
+ *
+ * Return cfq_io_context associating @cfqd and %current->io_context and
+ * bump refcnt on io_context. If ioc or cic doesn't exist, they're created
+ * using @gfp_mask.
+ *
+ * Must be called under queue_lock which may be released and re-acquired.
+ * This function also may sleep depending on @gfp_mask.
*/
static struct cfq_io_context *
cfq_get_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
{
- struct io_context *ioc = NULL;
+ struct request_queue *q = cfqd->queue;
struct cfq_io_context *cic = NULL;
+ struct io_context *ioc;
+ int err;
+
+ lockdep_assert_held(q->queue_lock);
+
+ while (true) {
+ /* fast path */
+ ioc = current->io_context;
+ if (likely(ioc)) {
+ cic = cfq_cic_lookup(cfqd, ioc);
+ if (likely(cic))
+ break;
+ }
- might_sleep_if(gfp_mask & __GFP_WAIT);
-
- ioc = current_io_context(gfp_mask, cfqd->queue->node);
- if (!ioc)
- goto err;
-
- cic = cfq_cic_lookup(cfqd, ioc);
- if (cic)
- goto out;
-
- cic = cfq_alloc_io_context(cfqd, gfp_mask);
- if (cic == NULL)
- goto err;
+ /* slow path - unlock, create missing ones and retry */
+ spin_unlock_irq(q->queue_lock);
+ err = cfq_create_cic(cfqd, gfp_mask);
+ spin_lock_irq(q->queue_lock);
+ if (err)
+ return NULL;
+ }
- if (cfq_cic_link(cfqd, ioc, cic, gfp_mask))
- goto err;
-out:
+ /* bump @ioc's refcnt and handle changed notifications */
get_io_context(ioc);
if (unlikely(cic->changed)) {
@@ -3220,10 +3244,6 @@ out:
}
return cic;
-err:
- if (cic)
- cfq_cic_free(cic);
- return NULL;
}
static void
@@ -3759,14 +3779,11 @@ cfq_set_request(struct request_queue *q, struct request *rq, gfp_t gfp_mask)
const int rw = rq_data_dir(rq);
const bool is_sync = rq_is_sync(rq);
struct cfq_queue *cfqq;
- unsigned long flags;
might_sleep_if(gfp_mask & __GFP_WAIT);
+ spin_lock_irq(q->queue_lock);
cic = cfq_get_io_context(cfqd, gfp_mask);
-
- spin_lock_irqsave(q->queue_lock, flags);
-
if (!cic)
goto queue_fail;
@@ -3802,12 +3819,12 @@ new_queue:
rq->elevator_private[0] = cic;
rq->elevator_private[1] = cfqq;
rq->elevator_private[2] = cfq_ref_get_cfqg(cfqq->cfqg);
- spin_unlock_irqrestore(q->queue_lock, flags);
+ spin_unlock_irq(q->queue_lock);
return 0;
queue_fail:
cfq_schedule_dispatch(cfqd);
- spin_unlock_irqrestore(q->queue_lock, flags);
+ spin_unlock_irq(q->queue_lock);
cfq_log(cfqd, "set_request fail");
return 1;
}
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 09/13] block, cfq: fix cic lookup locking
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (7 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 08/13] block, cfq: fix race condition in cic creation path and tighten locking Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately Tejun Heo
` (4 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
* cfq_cic_lookup() may be called without queue_lock and multiple tasks
can execute it simultaneously for the same shared ioc. Nothing
prevents them racing each other and trying to drop the same dead cic
entry multiple times.
* smp_wmb() in cfq_exit_cic() doesn't really do anything and nothing
prevents cfq_cic_lookup() seeing stale cic->key. This usually
doesn't blow up because by the time cic is exited, all requests have
been drained and new requests are terminated before going through
elevator. However, it can still be triggered by plug merge path
which doesn't grab queue_lock and thus can't check DEAD state
reliably.
This patch updates lookup locking such that,
* Lookup is always performed under queue_lock. This doesn't add any
more locking. The only issue is cfq_allow_merge() which can be
called from plug merge path without holding any lock. For now, this
is worked around by using cic of the request to merge into, which is
guaranteed to have the same ioc. For longer term, I think it would
be best to separate out plug merge method from regular one.
* Spurious ioc->lock locking around cic lookup hint assignment
dropped.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/cfq-iosched.c | 69 ++++++++++++++++++++++++++------------------------
1 files changed, 36 insertions(+), 33 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 181a63d..e617b08 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1682,12 +1682,19 @@ static int cfq_allow_merge(struct request_queue *q, struct request *rq,
return false;
/*
- * Lookup the cfqq that this bio will be queued with. Allow
- * merge only if rq is queued there.
+ * Lookup the cfqq that this bio will be queued with and allow
+ * merge only if rq is queued there. This function can be called
+ * from plug merge without queue_lock. In such cases, ioc of @rq
+ * and %current are guaranteed to be equal. Avoid lookup which
+ * requires queue_lock by using @rq's cic.
*/
- cic = cfq_cic_lookup(cfqd, current->io_context);
- if (!cic)
- return false;
+ if (current->io_context == RQ_CIC(rq)->ioc) {
+ cic = RQ_CIC(rq);
+ } else {
+ cic = cfq_cic_lookup(cfqd, current->io_context);
+ if (!cic)
+ return false;
+ }
cfqq = cic_to_cfqq(cic, cfq_bio_sync(bio));
return cfqq == RQ_CFQQ(rq);
@@ -2784,21 +2791,15 @@ static void cfq_exit_cic(struct cfq_io_context *cic)
struct io_context *ioc = cic->ioc;
list_del_init(&cic->queue_list);
+ cic->key = cfqd_dead_key(cfqd);
/*
- * Make sure dead mark is seen for dead queues
+ * Both setting lookup hint to and clearing it from @cic are done
+ * under queue_lock. If it's not pointing to @cic now, it never
+ * will. Hint assignment itself can race safely.
*/
- smp_wmb();
- cic->key = cfqd_dead_key(cfqd);
-
- rcu_read_lock();
- if (rcu_dereference(ioc->ioc_data) == cic) {
- rcu_read_unlock();
- spin_lock(&ioc->lock);
+ if (rcu_dereference_raw(ioc->ioc_data) == cic)
rcu_assign_pointer(ioc->ioc_data, NULL);
- spin_unlock(&ioc->lock);
- } else
- rcu_read_unlock();
if (cic->cfqq[BLK_RW_ASYNC]) {
cfq_exit_cfqq(cfqd, cic->cfqq[BLK_RW_ASYNC]);
@@ -3092,12 +3093,20 @@ cfq_drop_dead_cic(struct cfq_data *cfqd, struct io_context *ioc,
cfq_cic_free(cic);
}
+/**
+ * cfq_cic_lookup - lookup cfq_io_context
+ * @cfqd: the associated cfq_data
+ * @ioc: the associated io_context
+ *
+ * Look up cfq_io_context associated with @cfqd - @ioc pair. Must be
+ * called with queue_lock held.
+ */
static struct cfq_io_context *
cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
{
struct cfq_io_context *cic;
- unsigned long flags;
+ lockdep_assert_held(cfqd->queue->queue_lock);
if (unlikely(!ioc))
return NULL;
@@ -3107,28 +3116,22 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
* we maintain a last-hit cache, to avoid browsing over the tree
*/
cic = rcu_dereference(ioc->ioc_data);
- if (cic && cic->key == cfqd) {
- rcu_read_unlock();
- return cic;
- }
+ if (cic && cic->key == cfqd)
+ goto out;
do {
cic = radix_tree_lookup(&ioc->radix_root, cfqd->queue->id);
- rcu_read_unlock();
if (!cic)
break;
- if (unlikely(cic->key != cfqd)) {
- cfq_drop_dead_cic(cfqd, ioc, cic);
- rcu_read_lock();
- continue;
+ if (likely(cic->key == cfqd)) {
+ /* hint assignment itself can race safely */
+ rcu_assign_pointer(ioc->ioc_data, cic);
+ break;
}
-
- spin_lock_irqsave(&ioc->lock, flags);
- rcu_assign_pointer(ioc->ioc_data, cic);
- spin_unlock_irqrestore(&ioc->lock, flags);
- break;
+ cfq_drop_dead_cic(cfqd, ioc, cic);
} while (1);
-
+out:
+ rcu_read_unlock();
return cic;
}
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (8 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 09/13] block, cfq: fix cic lookup locking Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 19:48 ` Vivek Goyal
2011-10-26 21:31 ` [PATCH UPDATED " Tejun Heo
2011-10-26 1:48 ` [PATCH 11/13] block, cfq: remove delayed unlink Tejun Heo
` (3 subsequent siblings)
13 siblings, 2 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
cic is association between io_context and request_queue. A cic is
linked from both ioc and q and should be destroyed when either one
goes away. As ioc and q both have their own locks, locking becomes a
bit complex - both orders work for removal from one but not from the
other.
Currently, cfq tries to circumvent this locking order issue with RCU.
ioc->lock nests inside queue_lock but the radix tree and cic's are
also protected by RCU allowing either side to walk their lists without
grabbing lock.
This rather unconventional use of RCU quickly devolves into extremely
fragile convolution. e.g. The following is from cfqd going away too
soon after ioc and q exits raced.
general protection fault: 0000 [#1] PREEMPT SMP
CPU 2
Modules linked in:
[ 88.503444]
Pid: 599, comm: hexdump Not tainted 3.1.0-rc10-work+ #158 Bochs Bochs
RIP: 0010:[<ffffffff81397628>] [<ffffffff81397628>] cfq_exit_single_io_context+0x58/0xf0
...
Call Trace:
[<ffffffff81395a4a>] call_for_each_cic+0x5a/0x90
[<ffffffff81395ab5>] cfq_exit_io_context+0x15/0x20
[<ffffffff81389130>] exit_io_context+0x100/0x140
[<ffffffff81098a29>] do_exit+0x579/0x850
[<ffffffff81098d5b>] do_group_exit+0x5b/0xd0
[<ffffffff81098de7>] sys_exit_group+0x17/0x20
[<ffffffff81b02f2b>] system_call_fastpath+0x16/0x1b
The only real hot path here is cic lookup during request
initialization and avoiding extra locking requires very confined use
of RCU. This patch makes cic removal from both ioc and request_queue
perform double-locking and unlink immediately.
* From q side, the change is almost trivial as ioc->lock nests inside
queue_lock. It just needs to grab each ioc->lock as it walks
cic_list and unlink it.
* From ioc side, it's a bit more difficult because of inversed lock
order. ioc needs its lock to walk its cic_list but can't grab the
matching queue_lock and needs to perform unlock-relock dancing.
Unlinking is now wholly done from put_io_context() and fast path is
optimized by using the queue_lock the caller already holds, which is
by far the most common case. If the ioc accessed multiple devices,
it tries with trylock. In unlikely cases of fast path failure, it
falls back to full double-locking dance from workqueue.
Double-locking isn't the prettiest thing in the world but it's *far*
simpler and more understandable than RCU trick without adding any
meaningful overhead.
This still leaves a lot of now unnecessary RCU logics. Future patches
will trim them.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-cgroup.c | 2 +-
block/blk-ioc.c | 164 +++++++++++++++++++++++++++++++++++++--------
block/cfq-iosched.c | 44 ++----------
fs/ioprio.c | 2 +-
include/linux/blkdev.h | 3 +
include/linux/iocontext.h | 12 ++-
kernel/fork.c | 2 +-
7 files changed, 157 insertions(+), 72 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index dc00835..2788693 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1649,7 +1649,7 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
ioc = get_task_io_context(tsk, GFP_ATOMIC, NUMA_NO_NODE);
if (ioc) {
ioc_cgroup_changed(ioc);
- put_io_context(ioc);
+ put_io_context(ioc, NULL);
}
}
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 6f59fba..a5aeb39 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -29,55 +29,162 @@ void get_io_context(struct io_context *ioc)
}
EXPORT_SYMBOL(get_io_context);
-static void cfq_dtor(struct io_context *ioc)
+/*
+ * Releasing ioc may nest into another put_io_context() leading to nested
+ * fast path release. As the ioc's can't be the same, this is okay but
+ * makes lockdep whine. Keep track of nesting and use it as subclass.
+ */
+#ifdef CONFIG_LOCKDEP
+#define ioc_release_depth(q) ((q) ? (q)->ioc_release_depth : 0)
+#define ioc_release_depth_inc(q) (q)->ioc_release_depth++
+#define ioc_release_depth_dec(q) (q)->ioc_release_depth--
+#else
+#define ioc_release_depth(q) 0
+#define ioc_release_depth_inc(q) do { } while (0)
+#define ioc_release_depth_dec(q) do { } while (0)
+#endif
+
+/*
+ * Slow path for ioc release in put_io_context(). Performs double-lock
+ * dancing to unlink all cic's and then frees ioc.
+ */
+static void ioc_release_fn(struct work_struct *work)
{
- if (!hlist_empty(&ioc->cic_list)) {
- struct cfq_io_context *cic;
+ struct io_context *ioc = container_of(work, struct io_context,
+ release_work);
+ struct request_queue *last_q = NULL;
+
+ spin_lock_irq(&ioc->lock);
+
+ while (!hlist_empty(&ioc->cic_list)) {
+ struct cfq_io_context *cic = hlist_entry(ioc->cic_list.first,
+ struct cfq_io_context,
+ cic_list);
+ if (cic->q != last_q) {
+ struct request_queue *this_q = cic->q;
+
+ /*
+ * Need to switch to @this_q. Once we release
+ * @ioc->lock, it can go away along with @cic.
+ * Hold on to it.
+ */
+ __blk_get_queue(this_q);
+
+ /*
+ * blk_put_queue() might sleep thanks to kobject
+ * idiocy. Always release both locks, put and
+ * restart.
+ */
+ if (last_q) {
+ spin_unlock(last_q->queue_lock);
+ spin_unlock_irq(&ioc->lock);
+ blk_put_queue(last_q);
+ } else {
+ spin_unlock_irq(&ioc->lock);
+ }
+
+ last_q = this_q;
+ spin_lock_irq(this_q->queue_lock);
+ spin_lock(&ioc->lock);
+ continue;
+ }
+ ioc_release_depth_inc(cic->q);
+ cic->exit(cic);
+ cic->release(cic);
+ ioc_release_depth_dec(cic->q);
+ }
- cic = hlist_entry(ioc->cic_list.first, struct cfq_io_context,
- cic_list);
- cic->dtor(ioc);
+ if (last_q) {
+ spin_unlock(last_q->queue_lock);
+ spin_unlock_irq(&ioc->lock);
+ blk_put_queue(last_q);
+ } else {
+ spin_unlock_irq(&ioc->lock);
}
+
+ kmem_cache_free(iocontext_cachep, ioc);
}
/**
* put_io_context - put a reference of io_context
* @ioc: io_context to put
+ * @locked_q: request_queue the caller is holding queue_lock of (hint)
*
* Decrement reference count of @ioc and release it if the count reaches
- * zero.
+ * zero. If the caller is holding queue_lock of a queue, it can indicate
+ * that with @locked_q. This is an optimization hint and the caller is
+ * allowed to pass in %NULL even when it's holding a queue_lock.
*/
-void put_io_context(struct io_context *ioc)
+void put_io_context(struct io_context *ioc, struct request_queue *locked_q)
{
+ struct request_queue *last_q = locked_q;
+ unsigned long flags;
+
if (ioc == NULL)
return;
BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
+ if (locked_q)
+ lockdep_assert_held(locked_q->queue_lock);
if (!atomic_long_dec_and_test(&ioc->refcount))
return;
- rcu_read_lock();
- cfq_dtor(ioc);
- rcu_read_unlock();
-
- kmem_cache_free(iocontext_cachep, ioc);
-}
-EXPORT_SYMBOL(put_io_context);
+ /*
+ * Destroy @ioc. This is a bit messy because cic's are chained
+ * from both ioc and queue, and ioc->lock nests inside queue_lock.
+ * The inner ioc->lock should be held to walk our cic_list and then
+ * for each cic the outer matching queue_lock should be grabbed.
+ * ie. We need to do reverse-order double lock dancing.
+ *
+ * Another twist is that we are often called with one of the
+ * matching queue_locks held as indicated by @locked_q, which
+ * prevents performing double-lock dance for other queues.
+ *
+ * So, we do it in two stages. The fast path uses the queue_lock
+ * the caller is holding and, if other queues need to be accessed,
+ * uses trylock to avoid introducing locking dependency. This can
+ * handle most cases, especially if @ioc was performing IO on only
+ * single device.
+ *
+ * If trylock doesn't cut it, we defer to @ioc->release_work which
+ * can do all the double-locking dancing.
+ */
+ spin_lock_irqsave_nested(&ioc->lock, flags,
+ ioc_release_depth(locked_q));
+
+ while (!hlist_empty(&ioc->cic_list)) {
+ struct cfq_io_context *cic = hlist_entry(ioc->cic_list.first,
+ struct cfq_io_context,
+ cic_list);
+ if (cic->q != last_q) {
+ if (last_q && last_q != locked_q)
+ spin_unlock(last_q->queue_lock);
+ last_q = NULL;
+
+ if (!spin_trylock(cic->q->queue_lock))
+ break;
+ last_q = cic->q;
+ continue;
+ }
+ ioc_release_depth_inc(cic->q);
+ cic->exit(cic);
+ cic->release(cic);
+ ioc_release_depth_dec(cic->q);
+ }
-static void cfq_exit(struct io_context *ioc)
-{
- rcu_read_lock();
+ if (last_q && last_q != locked_q)
+ spin_unlock(last_q->queue_lock);
- if (!hlist_empty(&ioc->cic_list)) {
- struct cfq_io_context *cic;
+ spin_unlock_irqrestore(&ioc->lock, flags);
- cic = hlist_entry(ioc->cic_list.first, struct cfq_io_context,
- cic_list);
- cic->exit(ioc);
- }
- rcu_read_unlock();
+ /* if no cic's left, we're done; otherwise, kick release_work */
+ if (hlist_empty(&ioc->cic_list))
+ kmem_cache_free(iocontext_cachep, ioc);
+ else
+ schedule_work(&ioc->release_work);
}
+EXPORT_SYMBOL(put_io_context);
/* Called by the exiting task */
void exit_io_context(struct task_struct *task)
@@ -92,10 +199,8 @@ void exit_io_context(struct task_struct *task)
task->io_context = NULL;
task_unlock(task);
- if (atomic_dec_and_test(&ioc->nr_tasks))
- cfq_exit(ioc);
-
- put_io_context(ioc);
+ atomic_dec(&ioc->nr_tasks);
+ put_io_context(ioc, NULL);
}
static struct io_context *create_task_io_context(struct task_struct *task,
@@ -115,6 +220,7 @@ static struct io_context *create_task_io_context(struct task_struct *task,
spin_lock_init(&ioc->lock);
INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
INIT_HLIST_HEAD(&ioc->cic_list);
+ INIT_WORK(&ioc->release_work, ioc_release_fn);
/* try to install, somebody might already have beaten us to it */
task_lock(task);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index e617b08..6cc6065 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1778,7 +1778,7 @@ __cfq_slice_expired(struct cfq_data *cfqd, struct cfq_queue *cfqq,
cfqd->active_queue = NULL;
if (cfqd->active_cic) {
- put_io_context(cfqd->active_cic->ioc);
+ put_io_context(cfqd->active_cic->ioc, cfqd->queue);
cfqd->active_cic = NULL;
}
}
@@ -2812,38 +2812,6 @@ static void cfq_exit_cic(struct cfq_io_context *cic)
}
}
-static void cfq_exit_single_io_context(struct io_context *ioc,
- struct cfq_io_context *cic)
-{
- struct cfq_data *cfqd = cic_to_cfqd(cic);
-
- if (cfqd) {
- struct request_queue *q = cfqd->queue;
- unsigned long flags;
-
- spin_lock_irqsave(q->queue_lock, flags);
-
- /*
- * Ensure we get a fresh copy of the ->key to prevent
- * race between exiting task and queue
- */
- smp_read_barrier_depends();
- if (cic->key == cfqd)
- cfq_exit_cic(cic);
-
- spin_unlock_irqrestore(q->queue_lock, flags);
- }
-}
-
-/*
- * The process that ioc belongs to has exited, we need to clean up
- * and put the internal structures we have that belongs to that process.
- */
-static void cfq_exit_io_context(struct io_context *ioc)
-{
- call_for_each_cic(ioc, cfq_exit_single_io_context);
-}
-
static struct cfq_io_context *
cfq_alloc_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
{
@@ -2855,8 +2823,8 @@ cfq_alloc_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
cic->ttime.last_end_request = jiffies;
INIT_LIST_HEAD(&cic->queue_list);
INIT_HLIST_NODE(&cic->cic_list);
- cic->dtor = cfq_free_io_context;
- cic->exit = cfq_exit_io_context;
+ cic->exit = cfq_exit_cic;
+ cic->release = cfq_release_cic;
elv_ioc_count_inc(cfq_ioc_count);
}
@@ -3726,7 +3694,7 @@ static void cfq_put_request(struct request *rq)
BUG_ON(!cfqq->allocated[rw]);
cfqq->allocated[rw]--;
- put_io_context(RQ_CIC(rq)->ioc);
+ put_io_context(RQ_CIC(rq)->ioc, cfqq->cfqd->queue);
rq->elevator_private[0] = NULL;
rq->elevator_private[1] = NULL;
@@ -3937,8 +3905,12 @@ static void cfq_exit_queue(struct elevator_queue *e)
struct cfq_io_context *cic = list_entry(cfqd->cic_list.next,
struct cfq_io_context,
queue_list);
+ struct io_context *ioc = cic->ioc;
+ spin_lock(&ioc->lock);
cfq_exit_cic(cic);
+ cfq_release_cic(cic);
+ spin_unlock(&ioc->lock);
}
cfq_put_async_queues(cfqd);
diff --git a/fs/ioprio.c b/fs/ioprio.c
index f8d8429..10b88d3 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -50,7 +50,7 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
if (ioc) {
ioc_ioprio_changed(ioc, ioprio);
- put_io_context(ioc);
+ put_io_context(ioc, NULL);
}
return err;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 19037fb..e4c10a0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -393,6 +393,9 @@ struct request_queue {
/* Throttle data */
struct throtl_data *td;
#endif
+#ifdef CONFIG_LOCKDEP
+ int ioc_release_depth;
+#endif
};
#define QUEUE_FLAG_QUEUED 1 /* uses generic tag queueing */
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 2c2b6da..01e8631 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -3,6 +3,7 @@
#include <linux/radix-tree.h>
#include <linux/rcupdate.h>
+#include <linux/workqueue.h>
struct cfq_queue;
struct cfq_ttime {
@@ -33,8 +34,8 @@ struct cfq_io_context {
unsigned long changed;
- void (*dtor)(struct io_context *); /* destructor */
- void (*exit)(struct io_context *); /* called on task exit */
+ void (*exit)(struct cfq_io_context *);
+ void (*release)(struct cfq_io_context *);
struct rcu_head rcu_head;
};
@@ -61,6 +62,8 @@ struct io_context {
struct radix_tree_root radix_root;
struct hlist_head cic_list;
void __rcu *ioc_data;
+
+ struct work_struct release_work;
};
static inline struct io_context *ioc_task_link(struct io_context *ioc)
@@ -79,7 +82,7 @@ static inline struct io_context *ioc_task_link(struct io_context *ioc)
struct task_struct;
#ifdef CONFIG_BLOCK
-void put_io_context(struct io_context *ioc);
+void put_io_context(struct io_context *ioc, struct request_queue *locked_q);
void exit_io_context(struct task_struct *task);
struct io_context *get_task_io_context(struct task_struct *task,
gfp_t gfp_flags, int node);
@@ -87,7 +90,8 @@ void ioc_ioprio_changed(struct io_context *ioc, int ioprio);
void ioc_cgroup_changed(struct io_context *ioc);
#else
struct io_context;
-static inline void put_io_context(struct io_context *ioc) { }
+static inline void put_io_context(struct io_context *ioc,
+ struct request_queue *locked_q) { }
static inline void exit_io_context(struct task_struct *task) { }
#endif
diff --git a/kernel/fork.c b/kernel/fork.c
index 11da931..79e2909 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -895,7 +895,7 @@ static int copy_io(unsigned long clone_flags, struct task_struct *tsk)
return -ENOMEM;
new_ioc->ioprio = ioc->ioprio;
- put_io_context(new_ioc);
+ put_io_context(new_ioc, NULL);
}
#endif
return 0;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 1:48 ` [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately Tejun Heo
@ 2011-10-26 19:48 ` Vivek Goyal
2011-10-26 19:55 ` Tejun Heo
2011-10-26 21:31 ` [PATCH UPDATED " Tejun Heo
1 sibling, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-26 19:48 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Tue, Oct 25, 2011 at 06:48:36PM -0700, Tejun Heo wrote:
[..]
> +/*
> + * Slow path for ioc release in put_io_context(). Performs double-lock
> + * dancing to unlink all cic's and then frees ioc.
> + */
> +static void ioc_release_fn(struct work_struct *work)
> {
> - if (!hlist_empty(&ioc->cic_list)) {
> - struct cfq_io_context *cic;
> + struct io_context *ioc = container_of(work, struct io_context,
> + release_work);
> + struct request_queue *last_q = NULL;
> +
> + spin_lock_irq(&ioc->lock);
> +
> + while (!hlist_empty(&ioc->cic_list)) {
> + struct cfq_io_context *cic = hlist_entry(ioc->cic_list.first,
> + struct cfq_io_context,
> + cic_list);
> + if (cic->q != last_q) {
> + struct request_queue *this_q = cic->q;
> +
> + /*
> + * Need to switch to @this_q. Once we release
> + * @ioc->lock, it can go away along with @cic.
> + * Hold on to it.
> + */
> + __blk_get_queue(this_q);
> +
> + /*
> + * blk_put_queue() might sleep thanks to kobject
> + * idiocy. Always release both locks, put and
> + * restart.
> + */
> + if (last_q) {
> + spin_unlock(last_q->queue_lock);
> + spin_unlock_irq(&ioc->lock);
> + blk_put_queue(last_q);
> + } else {
> + spin_unlock_irq(&ioc->lock);
> + }
> +
> + last_q = this_q;
> + spin_lock_irq(this_q->queue_lock);
> + spin_lock(&ioc->lock);
> + continue;
> + }
> + ioc_release_depth_inc(cic->q);
> + cic->exit(cic);
> + cic->release(cic);
> + ioc_release_depth_dec(cic->q);
cic->release(cic) can free the cic? Are we accessing cic after freeing it
up in ioc_release_depth_dec(cic->q);
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 19:48 ` Vivek Goyal
@ 2011-10-26 19:55 ` Tejun Heo
2011-10-26 20:38 ` Vivek Goyal
0 siblings, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 19:55 UTC (permalink / raw)
To: Vivek Goyal; +Cc: axboe, ctalbott, rni, linux-kernel
Hello,
On Wed, Oct 26, 2011 at 03:48:57PM -0400, Vivek Goyal wrote:
> > + ioc_release_depth_inc(cic->q);
> > + cic->exit(cic);
> > + cic->release(cic);
> > + ioc_release_depth_dec(cic->q);
>
> cic->release(cic) can free the cic? Are we accessing cic after freeing it
> up in ioc_release_depth_dec(cic->q);
As cic is RCU freed and we're running w/ irq disabled, this shouldn't
blow up, but yeah, I'll update it to use local variable instead.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 19:55 ` Tejun Heo
@ 2011-10-26 20:38 ` Vivek Goyal
2011-10-26 20:54 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-26 20:38 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Wed, Oct 26, 2011 at 12:55:06PM -0700, Tejun Heo wrote:
> Hello,
>
> On Wed, Oct 26, 2011 at 03:48:57PM -0400, Vivek Goyal wrote:
> > > + ioc_release_depth_inc(cic->q);
> > > + cic->exit(cic);
> > > + cic->release(cic);
> > > + ioc_release_depth_dec(cic->q);
> >
> > cic->release(cic) can free the cic? Are we accessing cic after freeing it
> > up in ioc_release_depth_dec(cic->q);
>
> As cic is RCU freed and we're running w/ irq disabled, this shouldn't
> blow up, but yeah, I'll update it to use local variable instead.
Tejun,
So the only reason that cic needs to be rcu protected because in
cfq_cic_lookup() we can access ioc->ioc_data which can point to a
cic which is not ours and we are not holding queue lock for that and
we can't access cic->q ?
Rest of the places we are either holding ioc_lock or accessing our
own cic under associated queue lock so it should be safe?
I am wondering if there should be a small text file explaining all
the locking in this area.
Good to see that you are cleaning it all up.
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 20:38 ` Vivek Goyal
@ 2011-10-26 20:54 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 20:54 UTC (permalink / raw)
To: Vivek Goyal; +Cc: axboe, ctalbott, rni, linux-kernel
Hello, Vivek.
On Wed, Oct 26, 2011 at 04:38:46PM -0400, Vivek Goyal wrote:
> So the only reason that cic needs to be rcu protected because in
> cfq_cic_lookup() we can access ioc->ioc_data which can point to a
> cic which is not ours and we are not holding queue lock for that and
> we can't access cic->q ?
About right but to be more exact.
* Indexes from ioc are protected by ioc->lock - both the radix tree
and hint. Both are RCU managed.
* cic structures themselves are protected by cic->q->queue_lock and
RCU managed.
* Additions and removals are done holding both ioc and q locks.
* Lookup is performed with RCU read locked and q->queue_lock held.
Indexes and cic's themselves are RCU protected so can be traversed
and cic can be dereferenced even if it's pointing to a different
queue (and thus protected under different queue_lock).
However, cic's pointing to the q which is being looked up can be
added only under the same queue_lock, so on "cic->q == q" we're
guaranteed to be looking at the right thing.
> I am wondering if there should be a small text file explaining all
> the locking in this area.
Oh, I'm gonna be restructuring ioc interface and adding ample
explanations on locking rules there.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 1:48 ` [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately Tejun Heo
2011-10-26 19:48 ` Vivek Goyal
@ 2011-10-26 21:31 ` Tejun Heo
2011-10-27 14:31 ` Vivek Goyal
1 sibling, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 21:31 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel
cic is association between io_context and request_queue. A cic is
linked from both ioc and q and should be destroyed when either one
goes away. As ioc and q both have their own locks, locking becomes a
bit complex - both orders work for removal from one but not from the
other.
Currently, cfq tries to circumvent this locking order issue with RCU.
ioc->lock nests inside queue_lock but the radix tree and cic's are
also protected by RCU allowing either side to walk their lists without
grabbing lock.
This rather unconventional use of RCU quickly devolves into extremely
fragile convolution. e.g. The following is from cfqd going away too
soon after ioc and q exits raced.
general protection fault: 0000 [#1] PREEMPT SMP
CPU 2
Modules linked in:
[ 88.503444]
Pid: 599, comm: hexdump Not tainted 3.1.0-rc10-work+ #158 Bochs Bochs
RIP: 0010:[<ffffffff81397628>] [<ffffffff81397628>] cfq_exit_single_io_context+0x58/0xf0
...
Call Trace:
[<ffffffff81395a4a>] call_for_each_cic+0x5a/0x90
[<ffffffff81395ab5>] cfq_exit_io_context+0x15/0x20
[<ffffffff81389130>] exit_io_context+0x100/0x140
[<ffffffff81098a29>] do_exit+0x579/0x850
[<ffffffff81098d5b>] do_group_exit+0x5b/0xd0
[<ffffffff81098de7>] sys_exit_group+0x17/0x20
[<ffffffff81b02f2b>] system_call_fastpath+0x16/0x1b
The only real hot path here is cic lookup during request
initialization and avoiding extra locking requires very confined use
of RCU. This patch makes cic removal from both ioc and request_queue
perform double-locking and unlink immediately.
* From q side, the change is almost trivial as ioc->lock nests inside
queue_lock. It just needs to grab each ioc->lock as it walks
cic_list and unlink it.
* From ioc side, it's a bit more difficult because of inversed lock
order. ioc needs its lock to walk its cic_list but can't grab the
matching queue_lock and needs to perform unlock-relock dancing.
Unlinking is now wholly done from put_io_context() and fast path is
optimized by using the queue_lock the caller already holds, which is
by far the most common case. If the ioc accessed multiple devices,
it tries with trylock. In unlikely cases of fast path failure, it
falls back to full double-locking dance from workqueue.
Double-locking isn't the prettiest thing in the world but it's *far*
simpler and more understandable than RCU trick without adding any
meaningful overhead.
This still leaves a lot of now unnecessary RCU logics. Future patches
will trim them.
-v2: Vivek pointed out that cic->q was being dereferenced after
cic->release() was called. Updated to use local variable @this_q
instead.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Vivek Goyal <vgoyal@redhat.com>
---
block/blk-cgroup.c | 2 +-
block/blk-ioc.c | 166 +++++++++++++++++++++++++++++++++++++--------
block/cfq-iosched.c | 44 ++----------
fs/ioprio.c | 2 +-
include/linux/blkdev.h | 3 +
include/linux/iocontext.h | 12 ++-
kernel/fork.c | 2 +-
7 files changed, 159 insertions(+), 72 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index dc00835..2788693 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1649,7 +1649,7 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
ioc = get_task_io_context(tsk, GFP_ATOMIC, NUMA_NO_NODE);
if (ioc) {
ioc_cgroup_changed(ioc);
- put_io_context(ioc);
+ put_io_context(ioc, NULL);
}
}
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 6f59fba..fb23965 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -29,55 +29,164 @@ void get_io_context(struct io_context *ioc)
}
EXPORT_SYMBOL(get_io_context);
-static void cfq_dtor(struct io_context *ioc)
+/*
+ * Releasing ioc may nest into another put_io_context() leading to nested
+ * fast path release. As the ioc's can't be the same, this is okay but
+ * makes lockdep whine. Keep track of nesting and use it as subclass.
+ */
+#ifdef CONFIG_LOCKDEP
+#define ioc_release_depth(q) ((q) ? (q)->ioc_release_depth : 0)
+#define ioc_release_depth_inc(q) (q)->ioc_release_depth++
+#define ioc_release_depth_dec(q) (q)->ioc_release_depth--
+#else
+#define ioc_release_depth(q) 0
+#define ioc_release_depth_inc(q) do { } while (0)
+#define ioc_release_depth_dec(q) do { } while (0)
+#endif
+
+/*
+ * Slow path for ioc release in put_io_context(). Performs double-lock
+ * dancing to unlink all cic's and then frees ioc.
+ */
+static void ioc_release_fn(struct work_struct *work)
{
- if (!hlist_empty(&ioc->cic_list)) {
- struct cfq_io_context *cic;
+ struct io_context *ioc = container_of(work, struct io_context,
+ release_work);
+ struct request_queue *last_q = NULL;
+
+ spin_lock_irq(&ioc->lock);
+
+ while (!hlist_empty(&ioc->cic_list)) {
+ struct cfq_io_context *cic = hlist_entry(ioc->cic_list.first,
+ struct cfq_io_context,
+ cic_list);
+ struct request_queue *this_q = cic->q;
+
+ if (this_q != last_q) {
+ /*
+ * Need to switch to @this_q. Once we release
+ * @ioc->lock, it can go away along with @cic.
+ * Hold on to it.
+ */
+ __blk_get_queue(this_q);
+
+ /*
+ * blk_put_queue() might sleep thanks to kobject
+ * idiocy. Always release both locks, put and
+ * restart.
+ */
+ if (last_q) {
+ spin_unlock(last_q->queue_lock);
+ spin_unlock_irq(&ioc->lock);
+ blk_put_queue(last_q);
+ } else {
+ spin_unlock_irq(&ioc->lock);
+ }
+
+ last_q = this_q;
+ spin_lock_irq(this_q->queue_lock);
+ spin_lock(&ioc->lock);
+ continue;
+ }
+ ioc_release_depth_inc(this_q);
+ cic->exit(cic);
+ cic->release(cic);
+ ioc_release_depth_dec(this_q);
+ }
- cic = hlist_entry(ioc->cic_list.first, struct cfq_io_context,
- cic_list);
- cic->dtor(ioc);
+ if (last_q) {
+ spin_unlock(last_q->queue_lock);
+ spin_unlock_irq(&ioc->lock);
+ blk_put_queue(last_q);
+ } else {
+ spin_unlock_irq(&ioc->lock);
}
+
+ kmem_cache_free(iocontext_cachep, ioc);
}
/**
* put_io_context - put a reference of io_context
* @ioc: io_context to put
+ * @locked_q: request_queue the caller is holding queue_lock of (hint)
*
* Decrement reference count of @ioc and release it if the count reaches
- * zero.
+ * zero. If the caller is holding queue_lock of a queue, it can indicate
+ * that with @locked_q. This is an optimization hint and the caller is
+ * allowed to pass in %NULL even when it's holding a queue_lock.
*/
-void put_io_context(struct io_context *ioc)
+void put_io_context(struct io_context *ioc, struct request_queue *locked_q)
{
+ struct request_queue *last_q = locked_q;
+ unsigned long flags;
+
if (ioc == NULL)
return;
BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
+ if (locked_q)
+ lockdep_assert_held(locked_q->queue_lock);
if (!atomic_long_dec_and_test(&ioc->refcount))
return;
- rcu_read_lock();
- cfq_dtor(ioc);
- rcu_read_unlock();
-
- kmem_cache_free(iocontext_cachep, ioc);
-}
-EXPORT_SYMBOL(put_io_context);
+ /*
+ * Destroy @ioc. This is a bit messy because cic's are chained
+ * from both ioc and queue, and ioc->lock nests inside queue_lock.
+ * The inner ioc->lock should be held to walk our cic_list and then
+ * for each cic the outer matching queue_lock should be grabbed.
+ * ie. We need to do reverse-order double lock dancing.
+ *
+ * Another twist is that we are often called with one of the
+ * matching queue_locks held as indicated by @locked_q, which
+ * prevents performing double-lock dance for other queues.
+ *
+ * So, we do it in two stages. The fast path uses the queue_lock
+ * the caller is holding and, if other queues need to be accessed,
+ * uses trylock to avoid introducing locking dependency. This can
+ * handle most cases, especially if @ioc was performing IO on only
+ * single device.
+ *
+ * If trylock doesn't cut it, we defer to @ioc->release_work which
+ * can do all the double-locking dancing.
+ */
+ spin_lock_irqsave_nested(&ioc->lock, flags,
+ ioc_release_depth(locked_q));
+
+ while (!hlist_empty(&ioc->cic_list)) {
+ struct cfq_io_context *cic = hlist_entry(ioc->cic_list.first,
+ struct cfq_io_context,
+ cic_list);
+ struct request_queue *this_q = cic->q;
+
+ if (this_q != last_q) {
+ if (last_q && last_q != locked_q)
+ spin_unlock(last_q->queue_lock);
+ last_q = NULL;
+
+ if (!spin_trylock(this_q->queue_lock))
+ break;
+ last_q = this_q;
+ continue;
+ }
+ ioc_release_depth_inc(this_q);
+ cic->exit(cic);
+ cic->release(cic);
+ ioc_release_depth_dec(this_q);
+ }
-static void cfq_exit(struct io_context *ioc)
-{
- rcu_read_lock();
+ if (last_q && last_q != locked_q)
+ spin_unlock(last_q->queue_lock);
- if (!hlist_empty(&ioc->cic_list)) {
- struct cfq_io_context *cic;
+ spin_unlock_irqrestore(&ioc->lock, flags);
- cic = hlist_entry(ioc->cic_list.first, struct cfq_io_context,
- cic_list);
- cic->exit(ioc);
- }
- rcu_read_unlock();
+ /* if no cic's left, we're done; otherwise, kick release_work */
+ if (hlist_empty(&ioc->cic_list))
+ kmem_cache_free(iocontext_cachep, ioc);
+ else
+ schedule_work(&ioc->release_work);
}
+EXPORT_SYMBOL(put_io_context);
/* Called by the exiting task */
void exit_io_context(struct task_struct *task)
@@ -92,10 +201,8 @@ void exit_io_context(struct task_struct *task)
task->io_context = NULL;
task_unlock(task);
- if (atomic_dec_and_test(&ioc->nr_tasks))
- cfq_exit(ioc);
-
- put_io_context(ioc);
+ atomic_dec(&ioc->nr_tasks);
+ put_io_context(ioc, NULL);
}
static struct io_context *create_task_io_context(struct task_struct *task,
@@ -115,6 +222,7 @@ static struct io_context *create_task_io_context(struct task_struct *task,
spin_lock_init(&ioc->lock);
INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
INIT_HLIST_HEAD(&ioc->cic_list);
+ INIT_WORK(&ioc->release_work, ioc_release_fn);
/* try to install, somebody might already have beaten us to it */
task_lock(task);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index e617b08..6cc6065 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1778,7 +1778,7 @@ __cfq_slice_expired(struct cfq_data *cfqd, struct cfq_queue *cfqq,
cfqd->active_queue = NULL;
if (cfqd->active_cic) {
- put_io_context(cfqd->active_cic->ioc);
+ put_io_context(cfqd->active_cic->ioc, cfqd->queue);
cfqd->active_cic = NULL;
}
}
@@ -2812,38 +2812,6 @@ static void cfq_exit_cic(struct cfq_io_context *cic)
}
}
-static void cfq_exit_single_io_context(struct io_context *ioc,
- struct cfq_io_context *cic)
-{
- struct cfq_data *cfqd = cic_to_cfqd(cic);
-
- if (cfqd) {
- struct request_queue *q = cfqd->queue;
- unsigned long flags;
-
- spin_lock_irqsave(q->queue_lock, flags);
-
- /*
- * Ensure we get a fresh copy of the ->key to prevent
- * race between exiting task and queue
- */
- smp_read_barrier_depends();
- if (cic->key == cfqd)
- cfq_exit_cic(cic);
-
- spin_unlock_irqrestore(q->queue_lock, flags);
- }
-}
-
-/*
- * The process that ioc belongs to has exited, we need to clean up
- * and put the internal structures we have that belongs to that process.
- */
-static void cfq_exit_io_context(struct io_context *ioc)
-{
- call_for_each_cic(ioc, cfq_exit_single_io_context);
-}
-
static struct cfq_io_context *
cfq_alloc_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
{
@@ -2855,8 +2823,8 @@ cfq_alloc_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
cic->ttime.last_end_request = jiffies;
INIT_LIST_HEAD(&cic->queue_list);
INIT_HLIST_NODE(&cic->cic_list);
- cic->dtor = cfq_free_io_context;
- cic->exit = cfq_exit_io_context;
+ cic->exit = cfq_exit_cic;
+ cic->release = cfq_release_cic;
elv_ioc_count_inc(cfq_ioc_count);
}
@@ -3726,7 +3694,7 @@ static void cfq_put_request(struct request *rq)
BUG_ON(!cfqq->allocated[rw]);
cfqq->allocated[rw]--;
- put_io_context(RQ_CIC(rq)->ioc);
+ put_io_context(RQ_CIC(rq)->ioc, cfqq->cfqd->queue);
rq->elevator_private[0] = NULL;
rq->elevator_private[1] = NULL;
@@ -3937,8 +3905,12 @@ static void cfq_exit_queue(struct elevator_queue *e)
struct cfq_io_context *cic = list_entry(cfqd->cic_list.next,
struct cfq_io_context,
queue_list);
+ struct io_context *ioc = cic->ioc;
+ spin_lock(&ioc->lock);
cfq_exit_cic(cic);
+ cfq_release_cic(cic);
+ spin_unlock(&ioc->lock);
}
cfq_put_async_queues(cfqd);
diff --git a/fs/ioprio.c b/fs/ioprio.c
index f8d8429..10b88d3 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -50,7 +50,7 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
if (ioc) {
ioc_ioprio_changed(ioc, ioprio);
- put_io_context(ioc);
+ put_io_context(ioc, NULL);
}
return err;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 19037fb..e4c10a0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -393,6 +393,9 @@ struct request_queue {
/* Throttle data */
struct throtl_data *td;
#endif
+#ifdef CONFIG_LOCKDEP
+ int ioc_release_depth;
+#endif
};
#define QUEUE_FLAG_QUEUED 1 /* uses generic tag queueing */
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 2c2b6da..01e8631 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -3,6 +3,7 @@
#include <linux/radix-tree.h>
#include <linux/rcupdate.h>
+#include <linux/workqueue.h>
struct cfq_queue;
struct cfq_ttime {
@@ -33,8 +34,8 @@ struct cfq_io_context {
unsigned long changed;
- void (*dtor)(struct io_context *); /* destructor */
- void (*exit)(struct io_context *); /* called on task exit */
+ void (*exit)(struct cfq_io_context *);
+ void (*release)(struct cfq_io_context *);
struct rcu_head rcu_head;
};
@@ -61,6 +62,8 @@ struct io_context {
struct radix_tree_root radix_root;
struct hlist_head cic_list;
void __rcu *ioc_data;
+
+ struct work_struct release_work;
};
static inline struct io_context *ioc_task_link(struct io_context *ioc)
@@ -79,7 +82,7 @@ static inline struct io_context *ioc_task_link(struct io_context *ioc)
struct task_struct;
#ifdef CONFIG_BLOCK
-void put_io_context(struct io_context *ioc);
+void put_io_context(struct io_context *ioc, struct request_queue *locked_q);
void exit_io_context(struct task_struct *task);
struct io_context *get_task_io_context(struct task_struct *task,
gfp_t gfp_flags, int node);
@@ -87,7 +90,8 @@ void ioc_ioprio_changed(struct io_context *ioc, int ioprio);
void ioc_cgroup_changed(struct io_context *ioc);
#else
struct io_context;
-static inline void put_io_context(struct io_context *ioc) { }
+static inline void put_io_context(struct io_context *ioc,
+ struct request_queue *locked_q) { }
static inline void exit_io_context(struct task_struct *task) { }
#endif
diff --git a/kernel/fork.c b/kernel/fork.c
index 11da931..79e2909 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -895,7 +895,7 @@ static int copy_io(unsigned long clone_flags, struct task_struct *tsk)
return -ENOMEM;
new_ioc->ioprio = ioc->ioprio;
- put_io_context(new_ioc);
+ put_io_context(new_ioc, NULL);
}
#endif
return 0;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-26 21:31 ` [PATCH UPDATED " Tejun Heo
@ 2011-10-27 14:31 ` Vivek Goyal
2011-10-27 16:17 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-27 14:31 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Wed, Oct 26, 2011 at 02:31:07PM -0700, Tejun Heo wrote:
[..]
> -void put_io_context(struct io_context *ioc)
> +void put_io_context(struct io_context *ioc, struct request_queue *locked_q)
> {
> + struct request_queue *last_q = locked_q;
> + unsigned long flags;
> +
> if (ioc == NULL)
> return;
>
> BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
> + if (locked_q)
> + lockdep_assert_held(locked_q->queue_lock);
>
Looks like lockdep_assert_held() can't be used if !CONFIG_LOCKDEP. So
Jens had put following patch to fix one compilation issue. You might
want to provide a null definition of lockdep_assert_held() in case
of !CONFIG_LOCKDEP.
commit 334c2b0b8b2ab186fa198413386cba41fffcb4f2
Author: Jens Axboe <axboe@kernel.dk>
Date: Tue Oct 25 15:51:48 2011 +0200
blk-throttle: use queue_is_locked() instead of lockdep_is_held()
We can't use the latter if !CONFIG_LOCKDEP.
Reported-by: Sedat Dilek <sedat.dilek@googlemail.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-27 14:31 ` Vivek Goyal
@ 2011-10-27 16:17 ` Tejun Heo
2011-10-27 17:05 ` Vivek Goyal
0 siblings, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-10-27 16:17 UTC (permalink / raw)
To: Vivek Goyal; +Cc: axboe, ctalbott, rni, linux-kernel
Hello,
On Thu, Oct 27, 2011 at 10:31:19AM -0400, Vivek Goyal wrote:
> On Wed, Oct 26, 2011 at 02:31:07PM -0700, Tejun Heo wrote:
> > BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
> > + if (locked_q)
> > + lockdep_assert_held(locked_q->queue_lock);
> >
>
> Looks like lockdep_assert_held() can't be used if !CONFIG_LOCKDEP. So
> Jens had put following patch to fix one compilation issue. You might
> want to provide a null definition of lockdep_assert_held() in case
> of !CONFIG_LOCKDEP.
>
> commit 334c2b0b8b2ab186fa198413386cba41fffcb4f2
> Author: Jens Axboe <axboe@kernel.dk>
> Date: Tue Oct 25 15:51:48 2011 +0200
>
> blk-throttle: use queue_is_locked() instead of lockdep_is_held()
>
> We can't use the latter if !CONFIG_LOCKDEP.
>
> Reported-by: Sedat Dilek <sedat.dilek@googlemail.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
Eh? lockdep.h has dummy definition if !CONFIG_LOCKDEP. If
lockdep_is_held() is causing compilation failure on !CONFIG_LOCKDEP,
it's more likely to be have been caused by missing include than
anything else. Jens, what was the failure? lockdep_is_held()
provides way better protection against locking mistakes than
spin_is_locked().
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-27 16:17 ` Tejun Heo
@ 2011-10-27 17:05 ` Vivek Goyal
2011-10-27 17:11 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-27 17:05 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Thu, Oct 27, 2011 at 09:17:01AM -0700, Tejun Heo wrote:
> Hello,
>
> On Thu, Oct 27, 2011 at 10:31:19AM -0400, Vivek Goyal wrote:
> > On Wed, Oct 26, 2011 at 02:31:07PM -0700, Tejun Heo wrote:
> > > BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
> > > + if (locked_q)
> > > + lockdep_assert_held(locked_q->queue_lock);
> > >
> >
> > Looks like lockdep_assert_held() can't be used if !CONFIG_LOCKDEP. So
> > Jens had put following patch to fix one compilation issue. You might
> > want to provide a null definition of lockdep_assert_held() in case
> > of !CONFIG_LOCKDEP.
> >
> > commit 334c2b0b8b2ab186fa198413386cba41fffcb4f2
> > Author: Jens Axboe <axboe@kernel.dk>
> > Date: Tue Oct 25 15:51:48 2011 +0200
> >
> > blk-throttle: use queue_is_locked() instead of lockdep_is_held()
> >
> > We can't use the latter if !CONFIG_LOCKDEP.
> >
> > Reported-by: Sedat Dilek <sedat.dilek@googlemail.com>
> > Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> Eh? lockdep.h has dummy definition if !CONFIG_LOCKDEP.
I can't seem to find the dummy definition in include/linux/lockdep.h for
!CONFIG_LOCKDEP.
I see only following under CONFIG_LOCKDEP.
#define lockdep_is_held(lock) lock_is_held(&(lock)->dep_map)
and nothing under #else clause.
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
2011-10-27 17:05 ` Vivek Goyal
@ 2011-10-27 17:11 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-27 17:11 UTC (permalink / raw)
To: Vivek Goyal; +Cc: axboe, ctalbott, rni, linux-kernel
On Thu, Oct 27, 2011 at 01:05:24PM -0400, Vivek Goyal wrote:
> I can't seem to find the dummy definition in include/linux/lockdep.h for
> !CONFIG_LOCKDEP.
>
> I see only following under CONFIG_LOCKDEP.
>
> #define lockdep_is_held(lock) lock_is_held(&(lock)->dep_map)
>
> and nothing under #else clause.
Hmmm... commit f607c6685774811b8112e124f10a053d77015485 added
lockdep_assert_held() and the dummy one as added together. Of course
lockdep_is_held() isn't defined if !CONFIG_LOCKDEP - the truth value
cannot be determined without lockdep and thus dummy one cannot be
defined, but dummy lockdep_assert_held() can be defined and is, and
that's what what should be used.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 11/13] block, cfq: remove delayed unlink
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (9 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 12/13] block, cfq: kill ioc_gone Tejun Heo
` (2 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
Now that all cic's are immediately unlinked from both ioc and queue,
lazy dropping from lookup path and trimming on elevator unregister are
unnecessary. Kill them and remove now unused elevator_ops->trim().
This also leaves call_for_each_cic() without any user. Removed.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/cfq-iosched.c | 92 +++++-----------------------------------------
block/elevator.c | 16 --------
include/linux/elevator.h | 1 -
3 files changed, 10 insertions(+), 99 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 6cc6065..ff44435 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2669,24 +2669,6 @@ static void cfq_put_queue(struct cfq_queue *cfqq)
cfq_put_cfqg(cfqg);
}
-/*
- * Call func for each cic attached to this ioc.
- */
-static void
-call_for_each_cic(struct io_context *ioc,
- void (*func)(struct io_context *, struct cfq_io_context *))
-{
- struct cfq_io_context *cic;
- struct hlist_node *n;
-
- rcu_read_lock();
-
- hlist_for_each_entry_rcu(cic, n, &ioc->cic_list, cic_list)
- func(ioc, cic);
-
- rcu_read_unlock();
-}
-
static void cfq_cic_free_rcu(struct rcu_head *head)
{
struct cfq_io_context *cic;
@@ -2727,31 +2709,6 @@ static void cfq_release_cic(struct cfq_io_context *cic)
cfq_cic_free(cic);
}
-static void cic_free_func(struct io_context *ioc, struct cfq_io_context *cic)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&ioc->lock, flags);
- cfq_release_cic(cic);
- spin_unlock_irqrestore(&ioc->lock, flags);
-}
-
-/*
- * Must be called with rcu_read_lock() held or preemption otherwise disabled.
- * Only two callers of this - ->dtor() which is called with the rcu_read_lock(),
- * and ->trim() which is called with the task lock held
- */
-static void cfq_free_io_context(struct io_context *ioc)
-{
- /*
- * ioc->refcount is zero here, or we are called from elv_unregister(),
- * so no more cic's are allowed to be linked into this ioc. So it
- * should be ok to iterate over the known list, we will see all cic's
- * since no new ones are added.
- */
- call_for_each_cic(ioc, cic_free_func);
-}
-
static void cfq_put_cooperator(struct cfq_queue *cfqq)
{
struct cfq_queue *__cfqq, *next;
@@ -3037,30 +2994,6 @@ cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct io_context *ioc,
return cfqq;
}
-/*
- * We drop cfq io contexts lazily, so we may find a dead one.
- */
-static void
-cfq_drop_dead_cic(struct cfq_data *cfqd, struct io_context *ioc,
- struct cfq_io_context *cic)
-{
- unsigned long flags;
-
- WARN_ON(!list_empty(&cic->queue_list));
- BUG_ON(cic->key != cfqd_dead_key(cfqd));
-
- spin_lock_irqsave(&ioc->lock, flags);
-
- BUG_ON(rcu_dereference_check(ioc->ioc_data,
- lockdep_is_held(&ioc->lock)) == cic);
-
- radix_tree_delete(&ioc->radix_root, cfqd->queue->id);
- hlist_del_rcu(&cic->cic_list);
- spin_unlock_irqrestore(&ioc->lock, flags);
-
- cfq_cic_free(cic);
-}
-
/**
* cfq_cic_lookup - lookup cfq_io_context
* @cfqd: the associated cfq_data
@@ -3078,26 +3011,22 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
if (unlikely(!ioc))
return NULL;
- rcu_read_lock();
-
/*
- * we maintain a last-hit cache, to avoid browsing over the tree
+ * cic's are indexed from @ioc using radix tree and hint pointer,
+ * both of which are protected with RCU. All removals are done
+ * holding both q and ioc locks, and we're holding q lock - if we
+ * find a cic which points to us, it's guaranteed to be valid.
*/
+ rcu_read_lock();
cic = rcu_dereference(ioc->ioc_data);
if (cic && cic->key == cfqd)
goto out;
- do {
- cic = radix_tree_lookup(&ioc->radix_root, cfqd->queue->id);
- if (!cic)
- break;
- if (likely(cic->key == cfqd)) {
- /* hint assignment itself can race safely */
- rcu_assign_pointer(ioc->ioc_data, cic);
- break;
- }
- cfq_drop_dead_cic(cfqd, ioc, cic);
- } while (1);
+ cic = radix_tree_lookup(&ioc->radix_root, cfqd->queue->id);
+ if (cic && cic->key == cfqd)
+ rcu_assign_pointer(ioc->ioc_data, cic); /* allowed to race */
+ else
+ cic = NULL;
out:
rcu_read_unlock();
return cic;
@@ -4182,7 +4111,6 @@ static struct elevator_type iosched_cfq = {
.elevator_may_queue_fn = cfq_may_queue,
.elevator_init_fn = cfq_init_queue,
.elevator_exit_fn = cfq_exit_queue,
- .trim = cfq_free_io_context,
},
.elevator_attrs = cfq_attrs,
.elevator_name = "cfq",
diff --git a/block/elevator.c b/block/elevator.c
index 51447dd..da18de1 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -903,22 +903,6 @@ EXPORT_SYMBOL_GPL(elv_register);
void elv_unregister(struct elevator_type *e)
{
- struct task_struct *g, *p;
-
- /*
- * Iterate every thread in the process to remove the io contexts.
- */
- if (e->ops.trim) {
- read_lock(&tasklist_lock);
- do_each_thread(g, p) {
- task_lock(p);
- if (p->io_context)
- e->ops.trim(p->io_context);
- task_unlock(p);
- } while_each_thread(g, p);
- read_unlock(&tasklist_lock);
- }
-
spin_lock(&elv_list_lock);
list_del_init(&e->list);
spin_unlock(&elv_list_lock);
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 34d71f5..2f6193e 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -63,7 +63,6 @@ struct elevator_ops
elevator_init_fn *elevator_init_fn;
elevator_exit_fn *elevator_exit_fn;
- void (*trim)(struct io_context *);
};
#define ELV_NAME_MAX (16)
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 12/13] block, cfq: kill ioc_gone
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (10 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 11/13] block, cfq: remove delayed unlink Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 1:48 ` [PATCH 13/13] block, cfq: kill cic->key Tejun Heo
2011-10-26 21:36 ` [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
Now that cic's are immediately unlinked under both locks, there's no
need to count and drain cic's before module unload. RCU callback
completion is waited with rcu_barrier().
While at it, remove residual RCU operations on cic_list.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/cfq-iosched.c | 43 +++++--------------------------------------
include/linux/elevator.h | 17 -----------------
2 files changed, 5 insertions(+), 55 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ff44435..ae7791a 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -62,10 +62,6 @@ static const int cfq_hist_divisor = 4;
static struct kmem_cache *cfq_pool;
static struct kmem_cache *cfq_ioc_pool;
-static DEFINE_PER_CPU(unsigned long, cfq_ioc_count);
-static struct completion *ioc_gone;
-static DEFINE_SPINLOCK(ioc_gone_lock);
-
#define CFQ_PRIO_LISTS IOPRIO_BE_NR
#define cfq_class_idle(cfqq) ((cfqq)->ioprio_class == IOPRIO_CLASS_IDLE)
#define cfq_class_rt(cfqq) ((cfqq)->ioprio_class == IOPRIO_CLASS_RT)
@@ -2671,26 +2667,8 @@ static void cfq_put_queue(struct cfq_queue *cfqq)
static void cfq_cic_free_rcu(struct rcu_head *head)
{
- struct cfq_io_context *cic;
-
- cic = container_of(head, struct cfq_io_context, rcu_head);
-
- kmem_cache_free(cfq_ioc_pool, cic);
- elv_ioc_count_dec(cfq_ioc_count);
-
- if (ioc_gone) {
- /*
- * CFQ scheduler is exiting, grab exit lock and check
- * the pending io context count. If it hits zero,
- * complete ioc_gone and set it back to NULL
- */
- spin_lock(&ioc_gone_lock);
- if (ioc_gone && !elv_ioc_count_read(cfq_ioc_count)) {
- complete(ioc_gone);
- ioc_gone = NULL;
- }
- spin_unlock(&ioc_gone_lock);
- }
+ kmem_cache_free(cfq_ioc_pool,
+ container_of(head, struct cfq_io_context, rcu_head));
}
static void cfq_cic_free(struct cfq_io_context *cic)
@@ -2705,7 +2683,7 @@ static void cfq_release_cic(struct cfq_io_context *cic)
BUG_ON(!(dead_key & CIC_DEAD_KEY));
radix_tree_delete(&ioc->radix_root, dead_key >> CIC_DEAD_INDEX_SHIFT);
- hlist_del_rcu(&cic->cic_list);
+ hlist_del(&cic->cic_list);
cfq_cic_free(cic);
}
@@ -2782,7 +2760,6 @@ cfq_alloc_io_context(struct cfq_data *cfqd, gfp_t gfp_mask)
INIT_HLIST_NODE(&cic->cic_list);
cic->exit = cfq_exit_cic;
cic->release = cfq_release_cic;
- elv_ioc_count_inc(cfq_ioc_count);
}
return cic;
@@ -3072,7 +3049,7 @@ static int cfq_create_cic(struct cfq_data *cfqd, gfp_t gfp_mask)
ret = radix_tree_insert(&ioc->radix_root, q->id, cic);
if (likely(!ret)) {
- hlist_add_head_rcu(&cic->cic_list, &ioc->cic_list);
+ hlist_add_head(&cic->cic_list, &ioc->cic_list);
list_add(&cic->queue_list, &cfqd->cic_list);
cic = NULL;
} else if (ret == -EEXIST) {
@@ -4156,19 +4133,9 @@ static int __init cfq_init(void)
static void __exit cfq_exit(void)
{
- DECLARE_COMPLETION_ONSTACK(all_gone);
blkio_policy_unregister(&blkio_policy_cfq);
elv_unregister(&iosched_cfq);
- ioc_gone = &all_gone;
- /* ioc_gone's update must be visible before reading ioc_count */
- smp_wmb();
-
- /*
- * this also protects us from entering cfq_slab_kill() with
- * pending RCU callbacks
- */
- if (elv_ioc_count_read(cfq_ioc_count))
- wait_for_completion(&all_gone);
+ rcu_barrier(); /* make sure all cic RCU frees are complete */
cfq_slab_kill();
}
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 2f6193e..a5a88f1 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -194,22 +194,5 @@ enum {
INIT_LIST_HEAD(&(rq)->csd.list); \
} while (0)
-/*
- * io context count accounting
- */
-#define elv_ioc_count_mod(name, __val) this_cpu_add(name, __val)
-#define elv_ioc_count_inc(name) this_cpu_inc(name)
-#define elv_ioc_count_dec(name) this_cpu_dec(name)
-
-#define elv_ioc_count_read(name) \
-({ \
- unsigned long __val = 0; \
- int __cpu; \
- smp_wmb(); \
- for_each_possible_cpu(__cpu) \
- __val += per_cpu(name, __cpu); \
- __val; \
-})
-
#endif /* CONFIG_BLOCK */
#endif
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 13/13] block, cfq: kill cic->key
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (11 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 12/13] block, cfq: kill ioc_gone Tejun Heo
@ 2011-10-26 1:48 ` Tejun Heo
2011-10-26 21:36 ` [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
13 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 1:48 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel, Tejun Heo
Now that lazy paths are removed, cfqd_dead_key() is meaningless and
cic->q can be used whereever cic->key is used. Kill cic->key.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/cfq-iosched.c | 26 +++++---------------------
include/linux/iocontext.h | 1 -
2 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ae7791a..3b07ce1 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -472,22 +472,9 @@ static inline void cic_set_cfqq(struct cfq_io_context *cic,
cic->cfqq[is_sync] = cfqq;
}
-#define CIC_DEAD_KEY 1ul
-#define CIC_DEAD_INDEX_SHIFT 1
-
-static inline void *cfqd_dead_key(struct cfq_data *cfqd)
-{
- return (void *)(cfqd->queue->id << CIC_DEAD_INDEX_SHIFT | CIC_DEAD_KEY);
-}
-
static inline struct cfq_data *cic_to_cfqd(struct cfq_io_context *cic)
{
- struct cfq_data *cfqd = cic->key;
-
- if (unlikely((unsigned long) cfqd & CIC_DEAD_KEY))
- return NULL;
-
- return cfqd;
+ return cic->q->elevator->elevator_data;
}
/*
@@ -2679,10 +2666,8 @@ static void cfq_cic_free(struct cfq_io_context *cic)
static void cfq_release_cic(struct cfq_io_context *cic)
{
struct io_context *ioc = cic->ioc;
- unsigned long dead_key = (unsigned long) cic->key;
- BUG_ON(!(dead_key & CIC_DEAD_KEY));
- radix_tree_delete(&ioc->radix_root, dead_key >> CIC_DEAD_INDEX_SHIFT);
+ radix_tree_delete(&ioc->radix_root, cic->q->id);
hlist_del(&cic->cic_list);
cfq_cic_free(cic);
}
@@ -2726,7 +2711,6 @@ static void cfq_exit_cic(struct cfq_io_context *cic)
struct io_context *ioc = cic->ioc;
list_del_init(&cic->queue_list);
- cic->key = cfqd_dead_key(cfqd);
/*
* Both setting lookup hint to and clearing it from @cic are done
@@ -2982,6 +2966,7 @@ cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct io_context *ioc,
static struct cfq_io_context *
cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
{
+ struct request_queue *q = cfqd->queue;
struct cfq_io_context *cic;
lockdep_assert_held(cfqd->queue->queue_lock);
@@ -2996,11 +2981,11 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
*/
rcu_read_lock();
cic = rcu_dereference(ioc->ioc_data);
- if (cic && cic->key == cfqd)
+ if (cic && cic->q == q)
goto out;
cic = radix_tree_lookup(&ioc->radix_root, cfqd->queue->id);
- if (cic && cic->key == cfqd)
+ if (cic && cic->q == q)
rcu_assign_pointer(ioc->ioc_data, cic); /* allowed to race */
else
cic = NULL;
@@ -3040,7 +3025,6 @@ static int cfq_create_cic(struct cfq_data *cfqd, gfp_t gfp_mask)
goto out;
cic->ioc = ioc;
- cic->key = cfqd;
cic->q = cfqd->queue;
/* lock both q and ioc and try to link @cic */
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 01e8631..b2b75a5 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -20,7 +20,6 @@ enum {
};
struct cfq_io_context {
- void *key;
struct request_queue *q;
struct cfq_queue *cfqq[2];
--
1.7.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :)
2011-10-26 1:48 [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
` (12 preceding siblings ...)
2011-10-26 1:48 ` [PATCH 13/13] block, cfq: kill cic->key Tejun Heo
@ 2011-10-26 21:36 ` Tejun Heo
2011-10-27 15:32 ` Vivek Goyal
13 siblings, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-10-26 21:36 UTC (permalink / raw)
To: axboe, vgoyal; +Cc: ctalbott, rni, linux-kernel
Hello, again.
On Tue, Oct 25, 2011 at 06:48:26PM -0700, Tejun Heo wrote:
> This patchset contains the following 13 patches.
>
> 0001-ida-make-ida_simple_get-put-IRQ-safe.patch
> 0002-block-cfq-move-cfqd-cic_index-to-q-id.patch
> 0003-block-misc-ioc-cleanups.patch
> 0004-block-make-ioc-get-put-interface-more-conventional-a.patch
> 0005-block-misc-updates-to-blk_get_queue.patch
> 0006-block-cfq-misc-updates-to-cfq_io_context.patch
> 0007-block-cfq-move-ioc-ioprio-cgroup-changed-handling-to.patch
> 0008-block-cfq-fix-race-condition-in-cic-creation-path-an.patch
> 0009-block-cfq-fix-cic-lookup-locking.patch
> 0010-block-cfq-unlink-cfq_io_context-s-immediately.patch
> 0011-block-cfq-remove-delayed-unlink.patch
> 0012-block-cfq-kill-ioc_gone.patch
> 0013-block-cfq-kill-cic-key.patch
0001 removed (realized it's not necessary anymore) and sent out
separately. 0004 and 0010 updated to address issues Vivek pointed
out. Both don't affect other patches.
git branch is updated to reflect the changes including dropping of ide
related changes from the previous patchset. You won't be able to pull
this directly as I couldn't use commits for the previous patchset from
the block tree (they're not there yet).
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-cfq-sync
When you want the whole series reposted, let me know.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :)
2011-10-26 21:36 ` [PATCHSET block:for-3.2/core] rescue cfq from RCU death sprial :) Tejun Heo
@ 2011-10-27 15:32 ` Vivek Goyal
2011-10-27 16:10 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Vivek Goyal @ 2011-10-27 15:32 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, ctalbott, rni, linux-kernel
On Wed, Oct 26, 2011 at 02:36:29PM -0700, Tejun Heo wrote:
> Hello, again.
>
> On Tue, Oct 25, 2011 at 06:48:26PM -0700, Tejun Heo wrote:
> > This patchset contains the following 13 patches.
> >
> > 0001-ida-make-ida_simple_get-put-IRQ-safe.patch
> > 0002-block-cfq-move-cfqd-cic_index-to-q-id.patch
> > 0003-block-misc-ioc-cleanups.patch
> > 0004-block-make-ioc-get-put-interface-more-conventional-a.patch
> > 0005-block-misc-updates-to-blk_get_queue.patch
> > 0006-block-cfq-misc-updates-to-cfq_io_context.patch
> > 0007-block-cfq-move-ioc-ioprio-cgroup-changed-handling-to.patch
> > 0008-block-cfq-fix-race-condition-in-cic-creation-path-an.patch
> > 0009-block-cfq-fix-cic-lookup-locking.patch
> > 0010-block-cfq-unlink-cfq_io_context-s-immediately.patch
> > 0011-block-cfq-remove-delayed-unlink.patch
> > 0012-block-cfq-kill-ioc_gone.patch
> > 0013-block-cfq-kill-cic-key.patch
>
> 0001 removed (realized it's not necessary anymore) and sent out
> separately. 0004 and 0010 updated to address issues Vivek pointed
> out. Both don't affect other patches.
>
> git branch is updated to reflect the changes including dropping of ide
> related changes from the previous patchset. You won't be able to pull
> this directly as I couldn't use commits for the previous patchset from
> the block tree (they're not there yet).
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-cfq-sync
>
> When you want the whole series reposted, let me know.
These changes look good to me. It simplifies the locking (yes despite
double lock dance :-)) and gets rid of some race conditions.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Thanks
Vivek
^ permalink raw reply [flat|nested] 33+ messages in thread