lkml.org 
[lkml]   [2006]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: How to cleanly shut down a block device
On Tue, Nov 14, 2006 at 03:34:25PM +0100, Pierre Ossman wrote:
> How about this patch? It is basically the same as the previous, but it
> also sets queuedata to NULL and tests for that. It does not address if
> someone still has dependencies on the queue but hasn't gotten itself a
> reference (as I haven't gotten any word on if that is a problem):
>
> diff --git a/drivers/mmc/mmc_block.c b/drivers/mmc/mmc_block.c
> index f9027c8..5025abe 100644
> --- a/drivers/mmc/mmc_block.c
> +++ b/drivers/mmc/mmc_block.c
> @@ -83,7 +83,6 @@ static void mmc_blk_put(struct mmc_blk_d
> md->usage--;
> if (md->usage == 0) {
> put_disk(md->disk);
> - mmc_cleanup_queue(&md->queue);
> kfree(md);
> }
> mutex_unlock(&open_lock);
> @@ -553,12 +552,11 @@ static void mmc_blk_remove(struct mmc_ca
> if (md) {
> int devidx;
>
> + /* Stop new requests from getting into the queue */
> del_gendisk(md->disk);

I'm not entirely convinced that del_gendisk() will prevent any new
requests getting into the queue, which is why the comment below
says:

>
> - /*
> - * I think this is needed.
> - */
> - md->disk->queue = NULL;

Yes, del_gendisk() removes the disk from view for new opens, but what
about existing users?

As the code currently stands, when I remove a mounted MMC card, I get:

generic_make_request: Trying to access nonexistent block-device mmcblk0p1 (26)
FAT: Directory bread(block 26) failed
generic_make_request: Trying to access nonexistent block-device mmcblk0p1 (27)
FAT: Directory bread(block 27) failed
... etc ...

and this comes from:

q = bdev_get_queue(bio->bi_bdev);
if (!q) {
printk(KERN_ERR ...

in block/ll_rw_blk.c. bdev_get_queue merely does:

return bdev->bd_disk->queue;

> + /* Then flush out any already in there */
> + mmc_cleanup_queue(&md->queue);

Now, the thing is calling mmc_cleanup_queue() will shutdown the thread
and wait for it to complete its business, free the scatterlist, and
call blk_cleanup_queue() on that queue.

Ergo, once we've shut down the thread, we should not get any further
MMC requests, so:

> diff --git a/drivers/mmc/mmc_queue.c b/drivers/mmc/mmc_queue.c
> index 4ccdd82..b6769e2 100644
> --- a/drivers/mmc/mmc_queue.c
> +++ b/drivers/mmc/mmc_queue.c
> @@ -111,6 +111,19 @@ static int mmc_queue_thread(void *d)
> static void mmc_request(request_queue_t *q)
> {
> struct mmc_queue *mq = q->queuedata;
> + struct request *req;
> + int ret;
> +
> + if (!mq) {
> + printk(KERN_ERR "MMC: killing requests for dead queue\n");
> + while ((req = elv_next_request(q)) != NULL) {
> + do {
> + ret = end_that_request_chunk(req, 0,
> + req->current_nr_sectors << 9);
> + } while (ret);
> + }
> + return;
> + }
>
> if (!mq->req)
> wake_up(&mq->thread_wq);
> @@ -179,6 +192,15 @@ EXPORT_SYMBOL(mmc_init_queue);
>
> void mmc_cleanup_queue(struct mmc_queue *mq)
> {
> + request_queue_t *q = mq->queue;
> + unsigned long flags;
> +
> + /* Mark that we should start throwing out stragglers */
> + spin_lock_irqsave(q->queue_lock, flags);
> + q->queuedata = NULL;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + /* Then terminate our worker thread */
> mq->flags |= MMC_QUEUE_EXIT;
> wake_up(&mq->thread_wq);
> wait_for_completion(&mq->thread_complete);

These hunks do not achieve us anything.

However, what we _do_ need to do is to arrange for the MMC queue thread
to error out all pending requests before dying if MMC_QUEUE_EXIT is set.
That's already handled since the queue thread only ever exits if there
are no requests pending _and_ MMC_QUEUE_EXIT has been set.

So I think all you really need is this change:

diff --git a/drivers/mmc/mmc_block.c b/drivers/mmc/mmc_block.c
index f9027c8..9173067 100644
--- a/drivers/mmc/mmc_block.c
+++ b/drivers/mmc/mmc_block.c
@@ -83,7 +83,6 @@ static void mmc_blk_put(struct mmc_blk_d
md->usage--;
if (md->usage == 0) {
put_disk(md->disk);
- mmc_cleanup_queue(&md->queue);
kfree(md);
}
mutex_unlock(&open_lock);
@@ -559,6 +558,7 @@ static void mmc_blk_remove(struct mmc_ca
* I think this is needed.
*/
md->disk->queue = NULL;
+ mmc_cleanup_queue(&md->queue);

devidx = md->disk->first_minor >> MMC_SHIFT;
__clear_bit(devidx, dev_use);

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-11-23 22:07    [W:0.095 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site