[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>

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;
 }
 
 /*

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)

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)

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;
 }
 
-void elv_unregister_queue(struct request_queue *q)
+static void __elv_unregister_queue(elevator_t *e)
 {
-	if (q) {
-		elevator_t *e = q->elevator;
 	kobject_uevent(&e->kobj, KOBJ_REMOVE);
 	kobject_del(&e->kobj);
-	}
+}
+
+void elv_unregister_queue(struct request_queue *q)
+{
+	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 @@ static int elevator_switch(request_queue_t *q, struct elevator_type *new_e)
 	 */
 	elevator_exit(e);
 	e = NULL;
-fail:
 	q->elevator = old_elevator;
 	elv_register_queue(q);
 	clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);

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)

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