Patchworkβ [1/2] Disk hot removal causing oopses and fixes

login
register
about
Submitter Jarkko Lavinen
Date 2009-11-03 11:54:44
Message ID <20091103115444.GA8507@angel.research.nokia.com>
Download mbox | patch
Permalink /patch/57280/
State New
Headers show

Comments

Jarkko Lavinen - 2009-11-03 11:54:44
Hi Steven

Sorry for late reply.

> It has to reference-count its objects so that they are not freed as long
> as they are used by upper layers,

The block layer and device removal seems to be designed from
top-down approach.  Althouh disc is referenced from
__blkdev_get(), disc's request queue is not.  Also
blk_cleanup_queue() calls elevator_exit() without caring if
anyone still uses the elevator.

A remedy would be to take reference of the request queue in
__blkdev_get() and move elevator_exit() from blk_cleanup_queue()
to blk_release_queue().

I've tried this and it works fine. I am unable to cause oops within
the elevator or anywhere else with hot card removal.  This is how
it should be, since first the queue is marked dead and no new
requests are added into queue and old requests are flushed with
elevator_exit before releasing it and the dead status takes care
they don't even reach to requst issue function.

The caveat is ihat when request queue is initialzed with
blk_init_queue(), caller provides a pointer to spinlock.  When
blk_cleanup_queue() is called, drivers release the lock before
elevator_exit() is finished.  In mmc driver the struct containing
queue_lock is released immediately after returning blk_cleanup_queue().
Then later when request queue is released, elevator_exit() tries to
use the released spinlock.

Kind of hack workaround is to reset the queue_lock pointer to
request queue's internal lock in blk_cleanup_queue().

Jarkko Lavvinen
---
From 8ccee6132f376d78bb6e355016ecc06c9ca47b9f Mon Sep 17 00:00:00 2001
From: Jarkko Lavinen <jarkko.lavinen@nokia.com>
Date: Tue, 3 Nov 2009 09:48:00 +0200
Subject: [PATCH] block: Move elevator exit from blk_cleanup_queue() to blk_release_queue()

If block_cleanup_queue() should not remove elevator if it is still
in use. Better call the elevator exit in blk_release_queue() when
all the reference to queue are gone.  Since drivers could use queue
locks which are freed after returning from blk_cleanup_queue(),
switch the queue lock to internal one.

Signed-off-by: Jarkko Lavinen <jarkko.lavinen@nokia.com>
---
 block/blk-core.c  |    8 ++++++--
 block/blk-sysfs.c |    3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)
Stefan Richter - 2009-11-03 19:51:29
Jarkko Lavinen wrote:
> Hi Steven
> 
> Sorry for late reply.
> 
>> It has to reference-count its objects so that they are not freed as long
>> as they are used by upper layers,
> 
> The block layer and device removal seems to be designed from
> top-down approach.  Althouh disc is referenced from
> __blkdev_get(), disc's request queue is not.  Also
> blk_cleanup_queue() calls elevator_exit() without caring if
> anyone still uses the elevator.
[...]

I still don't understand how there can be a problem here.  Shouldn't the
sequence be:

 1. low-level determines that a device went away
 2. low-level takes note that from now on no new requests must be
    enqueued anymore
 3. low-level calls blk_cleanup_queue
 4. blk_cleanup_queue waits until remaining requests are done
    (it calls blk_sync_queue)
 5. blk_cleanup_queue cleans up block layer data
 6. low-level can now clean up/ free its own data

Does the MMC layer miss step 2?  Because without this, step 4 would be
in vain.

Patch

From 8ccee6132f376d78bb6e355016ecc06c9ca47b9f Mon Sep 17 00:00:00 2001
From: Jarkko Lavinen <jarkko.lavinen@nokia.com>
Date: Tue, 3 Nov 2009 09:48:00 +0200
Subject: [PATCH] block: Move elevator exit from blk_cleanup_queue() to blk_release_queue()

If block_cleanup_queue() should not remove elevator if it is still
in use. Better call the elevator exit in blk_release_queue() when
all the reference to queue are gone.  Since drivers could use queue
locks which are freed after returning from blk_cleanup_queue(),
switch the queue lock to internal one.

Signed-off-by: Jarkko Lavinen <jarkko.lavinen@nokia.com>
---
 block/blk-core.c  |    8 ++++++--
 block/blk-sysfs.c |    3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9daf621..8d7d5aa 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -449,6 +449,8 @@  void blk_put_queue(struct request_queue *q)
 
 void blk_cleanup_queue(struct request_queue *q)
 {
+	spinlock_t *oldlock;
+
 	/*
 	 * We know we have process context here, so we can be a little
 	 * cautious and ensure that pending block actions on this device
@@ -461,8 +463,10 @@  void blk_cleanup_queue(struct request_queue *q)
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
 	mutex_unlock(&q->sysfs_lock);
 
-	if (q->elevator)
-		elevator_exit(q->elevator);
+	oldlock = q->queue_lock;
+	spin_lock_irq(oldlock);
+	q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(oldlock);
 
 	blk_put_queue(q);
 }
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 21e275d..8ac3e5b 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -304,6 +304,9 @@  static void blk_release_queue(struct kobject *kobj)
 		container_of(kobj, struct request_queue, kobj);
 	struct request_list *rl = &q->rq;
 
+	if (q->elevator)
+		elevator_exit(q->elevator);
+
 	blk_sync_queue(q);
 
 	if (rl->rq_pool)
-- 
1.6.5