rescue cfq from RCU death sprial :)
Subsystems (10)
Tejun Heo
maintainerPATCH 1/1 rescue cfq from RCU death sprial :)
Hello,
Over the years, cfq has developed an interesting synchronization scheme around cic's (cfq_io_context). Each cic is association between an ioc (io_context) and a q (request_queue). As it lives across two different locking domains, synchronization has some level of inherent complexity - nesting one way and makes the other way difficult.
cfq used RCU in rather creative ways to resolve the locking order problem and also to cut on some of locking overhead - cic traversals from both sides depend on RCU and removal synchronization is done by setting cic->key to a different value.
Even when used according to usual conventions, RCU can be a bit challenging to get right; unfortunately, this unconventional use of RCU in CFQ seems to have devolved into ambiguous pile of partial fixes
- lazy dropping to avoid unlinking race, elevator_ops->trim() and percpu cic counting to deal with lingering ioc's on module unload, and so on. There doesn't seem to be any guiding design regarding synchronization at this point.
This is a lost cause and the code is extremely fragile and holes aren't too difficult to spot. The following one is from cfqd going away too soon after ioc and q exits raced.
sd 0:0:1:0: [sdb] Synchronizing SCSI cache
sd 0:0:1:0: [sdb] Stopping disk
ata1.01: disabled
scsi: killing requests for dead queue
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 RSP: 0018:ffff88001bd79de8 EFLAGS: 00010296 RAX: 000000000000002a RBX: ffff88001daa6188 RCX: 0000000000000001 RDX: 0000000000000000 RSI: ffffffff81afdaf1 RDI: ffffffff810950b7 RBP: ffff88001bd79e18 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000001 R12: 6b6b6b6b6b6b6b6b R13: ffff88001b7490a0 R14: 0000000000000064 R15: ffff88001bd79f00 FS: 00007f4452475720(0000) GS:ffff88001fd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b CR2: 00007f6c11760000 CR3: 000000001b713000 CR4: 00000000000006e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process hexdump (pid: 599, threadinfo ffff88001bd78000, task ffff88001a60e600) Stack: ffff88001bd79e68 ffff88001b4f3bd8 ffff88001b4f3bd8 ffffffff813975d0 ffff88001b7490f0 ffff88001e9ca040 ffff88001bd79e58 ffffffff81395a4a ffffffff813959f0 ffffffff820883c0 ffff88001e9ca040 ffff88001b4f3bd8 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
Also, the complex and unconventional synchronization made the code very inaccessible and elevator/io_context APIs non-modular.
The thing is that locking scheme here, although non-trivial, can be much simpler and conventional. The only actual performance benefit which can be gained from use of RCU is not acquiring ioc->lock during cic lookup on request issue path. Nothing else actually needs or benefits from RCU. cic adding and removal from queue side can be done with straight forward nested locking.
The only complex part is cic removal from ioc side; however, reverse-order double lock dancing is a well-known trick. It isn't the prettiest thing in the world but definitely is way better and more understandable than the current scheme. And with fast path optimization, it doesn't have to be noticeably more expensive either.
So, that's what this patchset does. It replaces the current RCU based magic sync scheme with plain double locking w/ only lookup part using RCU. The changes don't add any locking overhead to fast path and even slow path overhead shouldn't be noticeable at all.
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-0003 are misc preps.
0004-0005 fix and udpate io_context get/put.
0006-0007 are cfq preps.
0008-0010 updates cic locking such that cic's are added and removed under both locks and looked up under queue_lock.
0011-0013 drop no longer necessary stuff.
I tried to test most paths w/ injected conditions and instrumentations but this stuff definitely needs a lot more baking, so definitely not material for this merge window.
And here are future plans.
* With locking updated, it becomes much easier to reorganize ioc/cic interface such that blk-ioc can handle most of cic management so that cfq can simply request and use cic's, so that's the next step.
* blkcg support basically suffers from the same problem both in locking and API separation, so that'll be the next one once ioc side is done.
This patchset is on top of
block:for-3.2/core 334c2b0b8b "blk-throttle: use queue_is_locked() instead..."
+ [1] patchset "further updates to blk_cleanup_queue(), take#2"
and available in the following git branch.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-cfq-sync
diffstat follows.
block/blk-cgroup.c | 11 - block/blk-core.c | 32 +-- block/blk-ioc.c | 356 +++++++++++++++++++++++++-------- block/blk-sysfs.c | 2 block/blk.h | 12 + block/bsg.c | 4 block/cfq-iosched.c | 485 ++++++++++++++-------------------------------- block/elevator.c | 16 - block/genhd.c | 2 drivers/scsi/scsi_scan.c | 2 fs/ioprio.c | 24 -- include/linux/blkdev.h | 11 - include/linux/elevator.h | 18 - include/linux/iocontext.h | 40 +-- kernel/fork.c | 8 lib/idr.c | 11 - 16 files changed, 515 insertions(+), 519 deletions(-)
Thank you.
--
tejun
Tejun Heo
maintainerPATCH 1/13 ida: make ida_simple_get/put() IRQ safe
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.
--- lib/idr.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)
lib/idr.c +7 -4
@@ -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);
Signature · 1 line
1.7.3.1
Re: [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe
Quoted 6 lines · Tejun Heo
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 <[email protected]>Cc Rusty Russell <[email protected]>
Thanks,
Rusty.
Tejun Heo
maintainerRe: [PATCH 01/13] ida: make ida_simple_get/put() IRQ safe
Hello,
Quoted 9 lines · Rusty Russell
Quoted 6 lines · Tejun Heo
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 <[email protected]>Cc Rusty Russell <[email protected]>Acked-by Rusty Russell <[email protected]>
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.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 2/13 block, cfq: move cfqd->cic_index to q->id
In reply to Tejun Heocfq 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.
--- 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(-)
block/blk-core.c +16 -8
@@ -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);
block/blk-sysfs.c +2 -0
@@ -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); }
block/blk.h +3 -0
@@ -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,
block/cfq-iosched.c +5 -47
114 lines not on this page · raw message
include/linux/blkdev.h +6 -0
@@ -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;
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 3/13 block: misc ioc cleanups
In reply to 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.
--- block/blk-ioc.c | 72 ++++++++++++++++++++++---------------------- include/linux/iocontext.h | 12 ++----- 2 files changed, 39 insertions(+), 45 deletions(-)
block/blk-ioc.c +36 -36
@@ -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) {
include/linux/iocontext.h +3 -9
@@ -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
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 4/13 block: make ioc get/put interface more conventional and fix race on alloction
In reply to Tejun HeoIgnoring 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.
--- 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(-)
block/blk-cgroup.c +5 -4
@@ -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)
block/blk-ioc.c +66 -33
137 lines not on this page · raw message
block/blk.h +4 -0
@@ -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 */
block/cfq-iosched.c +9 -9
@@ -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 ; }
fs/ioprio.c +3 -18
@@ -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);
include/linux/iocontext.h +2 -2
@@ -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) { }
kernel/fork.c +5 -3
@@ -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 ;
Signature · 1 line
1.7.3.1
Re: [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction
On Tue, Oct 25, 2011 at 06:48:30PM -0700, Tejun Heo wrote:
[..]
Quoted 4 lines
block/blk-cgroup.c +0
-0@@ -1645,11 +1645,12 @@ static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk) {struct io_context *ioc;
block/blk-cgroup.c static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk) · line 1653
Quoted 10 lines
block/blk-cgroup.c +5
-4-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.
[..]
block/blk.h static inline void blk_throtl_exit(struct request_queue *q) { } · line 216
Quoted 4 lines
block/blk.h +3
-0+extern char __blk_test_mode[2 ]; +#define blk_test_mode (__blk_test_mode[0]) +
Looks like leftover of your testing code.
Thanks
Vivek
Tejun Heo
maintainerRe: [PATCH 04/13] block: make ioc get/put interface more conventional and fix race on alloction
Hello,
Quoted 6 lines · Vivek Goyal
We really don't need to create a new io context here if it is not present.
cgroup_changedis 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.
Quoted 6 lines
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 ofget_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.
Quoted 5 lines
block/blk.h static inline void blk_throtl_exit(struct request_queue *q) { } · line 216
Quoted 3 lines
block/blk.h +3
-0+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.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 4/13 block: make ioc get/put interface more conventional and fix race on alloction
In reply to Tejun HeoIgnoring 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.
--- 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(-)
block/blk-cgroup.c +5 -4
@@ -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)
block/blk-ioc.c +66 -33
137 lines not on this page · raw message
block/blk.h +1 -0
@@ -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,
block/cfq-iosched.c +9 -9
@@ -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 ; }
fs/ioprio.c +3 -18
@@ -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);
include/linux/iocontext.h +2 -2
@@ -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) { }
kernel/fork.c +5 -3
@@ -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 ;
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 5/13 block: misc updates to blk_get_queue()
In reply to 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.
--- 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(-)
block/blk-core.c +4 -4
@@ -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);
block/blk.h +5 -0
@@ -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);
block/bsg.c +1 -3
@@ -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 ();
block/genhd.c +1 -1
@@ -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" );
drivers/scsi/scsi_scan.c +1 -1
@@ -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);
include/linux/blkdev.h +1 -1
@@ -865,7 +865,7 @@ extern int blk_rq_map_sg(struct request_queue *, struct request *, struct scatteextern 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 *);
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 6/13 block, cfq: misc updates to cfq_io_context
In reply to Tejun HeoMake 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.
--- block/cfq-iosched.c | 58 ++++++++++++++++++++++++-------------------- include/linux/iocontext.h | 1 + 2 files changed, 33 insertions(+), 26 deletions(-)
block/cfq-iosched.c +32 -26
@@ -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);
include/linux/iocontext.h +1 -0
@@ -15,6 +15,7 @@ struct cfq_ttime {struct cfq_io_context {void *key; +struct request_queue *q;struct cfq_queue *cfqq[2 ];
Signature · 1 line
1.7.3.1
Re: [PATCH 06/13] block, cfq: misc updates to cfq_io_context
Quoted 4 lines · Tejun Heo
Make the following changes to prepare for ioc/cic management cleanup.
* Add
cic->qso 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
Tejun Heo
maintainerRe: [PATCH 06/13] block, cfq: misc updates to cfq_io_context
Hello, Vivek.
Quoted 9 lines · Vivek Goyal
Quoted 4 lines · Tejun Heo
Make the following changes to prepare for ioc/cic management cleanup.
* Add
cic->qso that ioc can determine the associated queue without querying cfq. This will eventually replace ->key.Not sure what's the advantage of storing
cic->qinstead ofcic->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.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 7/13 block, cfq: move ioc ioprio/cgroup changed handling to cic
In reply to Tejun Heoioprio/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.
--- 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(-)
block/blk-cgroup.c +1 -1
@@ -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); } }
block/blk-ioc.c +45 -0
51 lines not on this page · raw message
block/cfq-iosched.c +9 -19
@@ -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)
fs/ioprio.c +1 -2
@@ -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); }
include/linux/iocontext.h +9 -5
@@ -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) { }
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 8/13 block, cfq: fix race condition in cic creation path and tighten locking
In reply to Tejun Heocfq_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.
--- block/cfq-iosched.c | 135 ++++++++++++++++++++++++++++---------------------- 1 files changed, 76 insertions(+), 59 deletions(-)
block/cfq-iosched.c +76 -59
@@ -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 ; }
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 9/13 block, cfq: fix cic lookup locking
In reply to 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.
--- block/cfq-iosched.c | 69 ++++++++++++++++++++++++++------------------------ 1 files changed, 36 insertions(+), 33 deletions(-)
block/cfq-iosched.c +35 -32
@@ -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; }
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 10/13 block, cfq: unlink cfq_io_context's immediately
In reply to Tejun Heocic 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.
--- 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(-)
block/blk-cgroup.c +1 -1
@@ -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 ); } }
block/blk-ioc.c +135 -29
206 lines not on this page · raw message
block/cfq-iosched.c +8 -36
76 lines not on this page · raw message
fs/ioprio.c +1 -1
@@ -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;
include/linux/blkdev.h +3 -0
@@ -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 */
include/linux/iocontext.h +8 -4
@@ -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
kernel/fork.c +1 -1
@@ -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 ;
Signature · 1 line
1.7.3.1
Re: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
On Tue, Oct 25, 2011 at 06:48:36PM -0700, Tejun Heo wrote:
[..]
Quoted 40 lines
block/blk-ioc.c +37
-2+/* +* 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); + }
block/blk-ioc.c void get_io_context(struct io_context *ioc) · line 94
Quoted 10 lines
block/blk-ioc.c +10
-0+ + 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
Tejun Heo
maintainerRe: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
Hello,
Quoted 7 lines · Vivek Goyal
block/blk-ioc.c void get_io_context(struct io_context *ioc) · line 94
Quoted 4 lines
block/blk-ioc.c +4
-0+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 inioc_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.
Signature · 1 line
tejun
Re: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
Quoted 11 lines · Tejun Heo
Hello,
Quoted 7 lines · Vivek Goyal
block/blk-ioc.c void get_io_context(struct io_context *ioc) · line 94
Quoted 4 lines
block/blk-ioc.c +4
-0+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 inioc_release_depth_dec(cic->q);
Quoted 2 lines
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
Tejun Heo
maintainerRe: [PATCH 10/13] block, cfq: unlink cfq_io_context's immediately
Hello, Vivek.
Quoted 4 lines · Vivek Goyal
So the only reason that cic needs to be rcu protected because in
cfq_cic_lookup()we can accessioc->ioc_datawhich can point to a cic which is not ours and we are not holding queue lock for that and we can't accesscic->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.
Quoted 2 lines
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.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 10/13 block, cfq: unlink cfq_io_context's immediately
In reply to Tejun Heocic 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.--- 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(-)
block/blk-cgroup.c +1 -1
@@ -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 ); } }
block/blk-ioc.c +137 -29
208 lines not on this page · raw message
block/cfq-iosched.c +8 -36
76 lines not on this page · raw message
fs/ioprio.c +1 -1
@@ -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;
include/linux/blkdev.h +3 -0
@@ -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 */
include/linux/iocontext.h +8 -4
@@ -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
kernel/fork.c +1 -1
@@ -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 ;
Signature · 1 line
1.7.3.1
Re: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
On Wed, Oct 26, 2011 at 02:31:07PM -0700, Tejun Heo wrote:
[..]
block/blk-ioc.c void get_io_context(struct io_context *ioc) · line 128
Quoted 13 lines
block/blk-ioc.c +6
-1-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 <[email protected]> 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.
Thanks
Vivek
Tejun Heo
maintainerRe: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
Hello,
Quoted 15 lines · Vivek Goyal
block/blk-ioc.c void get_io_context(struct io_context *ioc) · line 128
Quoted 4 lines · Tejun Heo
block/blk-ioc.c +2
-0BUG_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 oflockdep_assert_held()in case of !CONFIG_LOCKDEP.commit
334c2b0b8b2ab186fa198413386cba41fffcb4f2Author: Jens Axboe <[email protected]> Date: Tue Oct 25 15:51:48 2011 +0200
Quoted 6 lines
blk-throttle: use
queue_is_locked()instead oflockdep_is_held()We can't use the latter if !CONFIG_LOCKDEP.
Reported-by Sedat Dilek <[email protected]>Signed-off-by Jens Axboe <[email protected]>
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.
Signature · 1 line
tejun
Re: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
Quoted 18 lines · Tejun Heo
Hello,
Quoted 15 lines · Vivek Goyal
block/blk-ioc.c void get_io_context(struct io_context *ioc) · line 128
Quoted 4 lines · Tejun Heo
block/blk-ioc.c +2
-0BUG_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 oflockdep_assert_held()in case of !CONFIG_LOCKDEP.commit
334c2b0b8b2ab186fa198413386cba41fffcb4f2Author: Jens Axboe <[email protected]> Date: Tue Oct 25 15:51:48 2011 +0200
Quoted 8 lines
Quoted 6 lines
blk-throttle: use
queue_is_locked()instead oflockdep_is_held()We can't use the latter if !CONFIG_LOCKDEP.
Reported-by Sedat Dilek <[email protected]>Signed-off-by Jens Axboe <[email protected]>Eh?
lockdep.hhas 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
Tejun Heo
maintainerRe: [PATCH UPDATED 10/13] block, cfq: unlink cfq_io_context's immediately
Quoted 8 lines · Vivek Goyal
I can't seem to find the dummy definition in
include/linux/lockdep.hfor !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.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 11/13 block, cfq: remove delayed unlink
In reply to Tejun HeoNow 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.
--- block/cfq-iosched.c | 92 +++++----------------------------------------- block/elevator.c | 16 -------- include/linux/elevator.h | 1 - 3 files changed, 10 insertions(+), 99 deletions(-)
block/cfq-iosched.c +10 -82
@@ -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",
block/elevator.c +0 -16
@@ -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);
include/linux/elevator.h +0 -1
@@ -63,7 +63,6 @@ struct elevator_opselevator_init_fn *elevator_init_fn;elevator_exit_fn *elevator_exit_fn; -void (*trim)(struct io_context *); };#define ELV_NAME_MAX (16)
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 12/13 block, cfq: kill ioc_gone
In reply to Tejun HeoNow 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.
--- block/cfq-iosched.c | 43 +++++-------------------------------------- include/linux/elevator.h | 17 ----------------- 2 files changed, 5 insertions(+), 55 deletions(-)
block/cfq-iosched.c +5 -38
@@ -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 (); }
include/linux/elevator.h +0 -17
@@ -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
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerPATCH 13/13 block, cfq: kill cic->key
In reply to Tejun HeoNow that lazy paths are removed, cfqd_dead_key() is meaningless and cic->q can be used whereever cic->key is used. Kill cic->key.
--- block/cfq-iosched.c | 26 +++++--------------------- include/linux/iocontext.h | 1 - 2 files changed, 5 insertions(+), 22 deletions(-)
block/cfq-iosched.c +5 -21
@@ -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 */
include/linux/iocontext.h +0 -1
@@ -20,7 +20,6 @@ enum { };struct cfq_io_context { -void *key;struct request_queue *q;struct cfq_queue *cfqq[2 ];
Signature · 1 line
1.7.3.1
Tejun Heo
maintainerHello, again.
Quoted 15 lines · Tejun Heo
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.
Signature · 1 line
tejun
Vivek Goyal
Acked-byQuoted 23 lines · Tejun Heo
Hello, again.
Quoted 15 lines · Tejun Heo
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.
Quoted 8 lines
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.gitblock-cfq-syncWhen 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.
Thanks
Vivek
Tejun Heo
maintainerQuoted 4 lines · Vivek Goyal
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 <[email protected]>
Awesome, thanks.
Signature · 1 line
tejun