[PATCH] elevator switching race

There's a race between shutting down one io scheduler and firing up the
next, in which a new io could enter and cause the io scheduler to be
invoked with bad or NULL data.

To fix this, we need to maintain the queue lock for a bit longer.
Unfortunately we cannot do that, since the elevator init requires to be
run without the lock held.  This isn't easily fixable, without also
changing the mempool API.  So split the initialization into two parts,
and alloc-init operation and an attach operation.  Then we can
preallocate the io scheduler and related structures, and run the attach
inside the lock after we detach the old one.

This patch has survived 30 minutes of 1 second io scheduler switching
with a very busy io load.

Signed-off-by: Jens Axboe <axboe@suse.de>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/block/as-iosched.c b/block/as-iosched.c
index e25a5d79ab2715b2f3a07ce3331e46cd45ab86f1..a7caf35ca0c2258e03575b03d503c102f951b938 100644 (file)
--- a/block/as-iosched.c
+++ b/block/as-iosched.c
@@ -1648,17 +1648,17 @@ static void as_exit_queue(elevator_t *e)
  * initialize elevator private data (as_data), and alloc a arq for
  * each request on the free lists
  */
-static int as_init_queue(request_queue_t *q, elevator_t *e)
+static void *as_init_queue(request_queue_t *q, elevator_t *e)
 {
        struct as_data *ad;
        int i;
 
        if (!arq_pool)
-               return -ENOMEM;
+               return NULL;
 
        ad = kmalloc_node(sizeof(*ad), GFP_KERNEL, q->node);
        if (!ad)
-               return -ENOMEM;
+               return NULL;
        memset(ad, 0, sizeof(*ad));
 
        ad->q = q; /* Identify what queue the data belongs to */
@@ -1667,7 +1667,7 @@ static int as_init_queue(request_queue_t *q, elevator_t *e)
                                GFP_KERNEL, q->node);
        if (!ad->hash) {
                kfree(ad);
-               return -ENOMEM;
+               return NULL;
        }
 
        ad->arq_pool = mempool_create_node(BLKDEV_MIN_RQ, mempool_alloc_slab,
@@ -1675,7 +1675,7 @@ static int as_init_queue(request_queue_t *q, elevator_t *e)
        if (!ad->arq_pool) {
                kfree(ad->hash);
                kfree(ad);
-               return -ENOMEM;
+               return NULL;
        }
 
        /* anticipatory scheduling helpers */
@@ -1696,14 +1696,13 @@ static int as_init_queue(request_queue_t *q, elevator_t *e)
        ad->antic_expire = default_antic_expire;
        ad->batch_expire[REQ_SYNC] = default_read_batch_expire;
        ad->batch_expire[REQ_ASYNC] = default_write_batch_expire;
-       e->elevator_data = ad;
 
        ad->current_batch_expires = jiffies + ad->batch_expire[REQ_SYNC];
        ad->write_batch_count = ad->batch_expire[REQ_ASYNC] / 10;
        if (ad->write_batch_count < 2)
                ad->write_batch_count = 2;
 
-       return 0;
+       return ad;
 }
 
 /*

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 8e9d84825e1c519500288da6602b208fcec33995..a46d030e092aa60cba5f1b513cde94378f6b9ce7 100644 (file)
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2251,14 +2251,14 @@ static void cfq_exit_queue(elevator_t *e)
        kfree(cfqd);
 }
 
-static int cfq_init_queue(request_queue_t *q, elevator_t *e)
+static void *cfq_init_queue(request_queue_t *q, elevator_t *e)
 {
        struct cfq_data *cfqd;
        int i;
 
        cfqd = kmalloc(sizeof(*cfqd), GFP_KERNEL);
        if (!cfqd)
-               return -ENOMEM;
+               return NULL;
 
        memset(cfqd, 0, sizeof(*cfqd));
 
@@ -2288,8 +2288,6 @@ static int cfq_init_queue(request_queue_t *q, elevator_t *e)
        for (i = 0; i < CFQ_QHASH_ENTRIES; i++)
                INIT_HLIST_HEAD(&cfqd->cfq_hash[i]);
 
-       e->elevator_data = cfqd;
-
        cfqd->queue = q;
 
        cfqd->max_queued = q->nr_requests / 4;
@@ -2316,14 +2314,14 @@ static int cfq_init_queue(request_queue_t *q, elevator_t *e)
        cfqd->cfq_slice_async_rq = cfq_slice_async_rq;
        cfqd->cfq_slice_idle = cfq_slice_idle;
 
-       return 0;
+       return cfqd;
 out_crqpool:
        kfree(cfqd->cfq_hash);
 out_cfqhash:
        kfree(cfqd->crq_hash);
 out_crqhash:
        kfree(cfqd);
-       return -ENOMEM;
+       return NULL;
 }
 
 static void cfq_slab_kill(void)

diff --git a/block/deadline-iosched.c b/block/deadline-iosched.c
index 399fa1e60e1fd6b7ca8200b7d5ef14a67b0b7672..3bd0415a9828673c6bb28a6cd52d1fe6fac730d2 100644 (file)
--- a/block/deadline-iosched.c
+++ b/block/deadline-iosched.c
@@ -613,24 +613,24 @@ static void deadline_exit_queue(elevator_t *e)
  * initialize elevator private data (deadline_data), and alloc a drq for
  * each request on the free lists
  */
-static int deadline_init_queue(request_queue_t *q, elevator_t *e)
+static void *deadline_init_queue(request_queue_t *q, elevator_t *e)
 {
        struct deadline_data *dd;
        int i;
 
        if (!drq_pool)
-               return -ENOMEM;
+               return NULL;
 
        dd = kmalloc_node(sizeof(*dd), GFP_KERNEL, q->node);
        if (!dd)
-               return -ENOMEM;
+               return NULL;
        memset(dd, 0, sizeof(*dd));
 
        dd->hash = kmalloc_node(sizeof(struct list_head)*DL_HASH_ENTRIES,
                                GFP_KERNEL, q->node);
        if (!dd->hash) {
                kfree(dd);
-               return -ENOMEM;
+               return NULL;
        }
 
        dd->drq_pool = mempool_create_node(BLKDEV_MIN_RQ, mempool_alloc_slab,
@@ -638,7 +638,7 @@ static int deadline_init_queue(request_queue_t *q, elevator_t *e)
        if (!dd->drq_pool) {
                kfree(dd->hash);
                kfree(dd);
-               return -ENOMEM;
+               return NULL;
        }
 
        for (i = 0; i < DL_HASH_ENTRIES; i++)
@@ -653,8 +653,7 @@ static int deadline_init_queue(request_queue_t *q, elevator_t *e)
        dd->writes_starved = writes_starved;
        dd->front_merges = 1;
        dd->fifo_batch = fifo_batch;
-       e->elevator_data = dd;
-       return 0;
+       return dd;
 }
 
 static void deadline_put_request(request_queue_t *q, struct request *rq)

diff --git a/block/elevator.c b/block/elevator.c
index 8768a367fdded922a17b1c7c47ac24ec993a1628..a0afdd317ceffb3d59f230c775da6c03740a426f 100644 (file)
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -121,16 +121,16 @@ static struct elevator_type *elevator_get(const char *name)
        return e;
 }
 
-static int elevator_attach(request_queue_t *q, struct elevator_queue *eq)
+static void *elevator_init_queue(request_queue_t *q, struct elevator_queue *eq)
 {
-       int ret = 0;
+       return eq->ops->elevator_init_fn(q, eq);
+}
 
+static void elevator_attach(request_queue_t *q, struct elevator_queue *eq,
+                          void *data)
+{
        q->elevator = eq;
-
-       if (eq->ops->elevator_init_fn)
-               ret = eq->ops->elevator_init_fn(q, eq);
-
-       return ret;
+       eq->elevator_data = data;
 }
 
 static char chosen_elevator[16];
@@ -181,6 +181,7 @@ int elevator_init(request_queue_t *q, char *name)
        struct elevator_type *e = NULL;
        struct elevator_queue *eq;
        int ret = 0;
+       void *data;
 
        INIT_LIST_HEAD(&q->queue_head);
        q->last_merge = NULL;
@@ -202,10 +203,13 @@ int elevator_init(request_queue_t *q, char *name)
        if (!eq)
                return -ENOMEM;
 
-       ret = elevator_attach(q, eq);
-       if (ret)
+       data = elevator_init_queue(q, eq);
+       if (!data) {
                kobject_put(&eq->kobj);
+               return -ENOMEM;
+       }
 
+       elevator_attach(q, eq, data);
        return ret;
 }
 
@@ -722,13 +726,16 @@ int elv_register_queue(struct request_queue *q)
        return error;
 }
 
+static void __elv_unregister_queue(elevator_t *e)
+{
+       kobject_uevent(&e->kobj, KOBJ_REMOVE);
+       kobject_del(&e->kobj);
+}
+
 void elv_unregister_queue(struct request_queue *q)
 {
-       if (q) {
-               elevator_t *e = q->elevator;
-               kobject_uevent(&e->kobj, KOBJ_REMOVE);
-               kobject_del(&e->kobj);
-       }
+       if (q)
+               __elv_unregister_queue(q->elevator);
 }
 
 int elv_register(struct elevator_type *e)
@@ -780,6 +787,7 @@ EXPORT_SYMBOL_GPL(elv_unregister);
 static int elevator_switch(request_queue_t *q, struct elevator_type *new_e)
 {
        elevator_t *old_elevator, *e;
+       void *data;
 
        /*
         * Allocate new elevator
@@ -788,6 +796,12 @@ static int elevator_switch(request_queue_t *q, struct elevator_type *new_e)
        if (!e)
                return 0;
 
+       data = elevator_init_queue(q, e);
+       if (!data) {
+               kobject_put(&e->kobj);
+               return 0;
+       }
+
        /*
         * Turn on BYPASS and drain all requests w/ elevator private data
         */
@@ -806,19 +820,19 @@ static int elevator_switch(request_queue_t *q, struct elevator_type *new_e)
                elv_drain_elevator(q);
        }
 
-       spin_unlock_irq(q->queue_lock);
-
        /*
-        * unregister old elevator data
+        * Remember old elevator.
         */
-       elv_unregister_queue(q);
        old_elevator = q->elevator;
 
        /*
         * attach and start new elevator
         */
-       if (elevator_attach(q, e))
-               goto fail;
+       elevator_attach(q, e, data);
+
+       spin_unlock_irq(q->queue_lock);
+
+       __elv_unregister_queue(old_elevator);
 
        if (elv_register_queue(q))
                goto fail_register;
@@ -837,7 +851,6 @@ fail_register:
         */
        elevator_exit(e);
        e = NULL;
-fail:
        q->elevator = old_elevator;
        elv_register_queue(q);
        clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);

diff --git a/block/noop-iosched.c b/block/noop-iosched.c
index f370e4a7fe6d7a26e66f988774c002af6b772725..56a7c620574f86c0338a431354344d149314ce0f 100644 (file)
--- a/block/noop-iosched.c
+++ b/block/noop-iosched.c
@@ -65,16 +65,15 @@ noop_latter_request(request_queue_t *q, struct request *rq)
        return list_entry(rq->queuelist.next, struct request, queuelist);
 }
 
-static int noop_init_queue(request_queue_t *q, elevator_t *e)
+static void *noop_init_queue(request_queue_t *q, elevator_t *e)
 {
        struct noop_data *nd;
 
        nd = kmalloc(sizeof(*nd), GFP_KERNEL);
        if (!nd)
-               return -ENOMEM;
+               return NULL;
        INIT_LIST_HEAD(&nd->queue);
-       e->elevator_data = nd;
-       return 0;
+       return nd;
 }
 
 static void noop_exit_queue(elevator_t *e)

diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index ad133fcfb239ddbf16e6961e88f47161e8d1447e..1713ace808bfb4d581ccfff53aa4b0c42ebfa9ef 100644 (file)
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -21,7 +21,7 @@ typedef void (elevator_put_req_fn) (request_queue_t *, struct request *);
 typedef void (elevator_activate_req_fn) (request_queue_t *, struct request *);
 typedef void (elevator_deactivate_req_fn) (request_queue_t *, struct request *);
 
-typedef int (elevator_init_fn) (request_queue_t *, elevator_t *);
+typedef void *(elevator_init_fn) (request_queue_t *, elevator_t *);
 typedef void (elevator_exit_fn) (elevator_t *);
 
 struct elevator_ops