blk: queue cleanup at device shutdown

blk: queue cleanup at device shutdown

[Geoff Levand]

Hi Jens,

I'm wondering about what is needed in ps3disk_remove(), to clean
up the request_queue (priv->queue) the driver uses.
ps3disk_remove() is called from the main device_shutdown()
routine via the ps3_system_bus remove routine.

The current code follows, and as can be seen, there is no check
or handling of the queue before blk_cleanup_queue() is called.
Can it be assumed that the queue is empty at this point,
or is something else needed here?

-Geoff

static int ps3disk_remove(struct ps3_system_bus_device *_dev)
{
	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
	struct ps3disk_private *priv = dev->sbd.core.driver_data;

	mutex_lock(&ps3disk_mask_mutex);
	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
		    &ps3disk_mask);
	mutex_unlock(&ps3disk_mask_mutex);

	del_gendisk(priv->gendisk);
	blk_cleanup_queue(priv->queue);
	put_disk(priv->gendisk);
	dev_notice(&dev->sbd.core, "Synchronizing disk cache\n");
	ps3disk_sync_cache(dev);
	ps3stor_teardown(dev);
	kfree(dev->bounce_buf);
	dev->bounce_buf = NULL;
	kfree(priv);
	dev->sbd.core.driver_data = NULL;
	return 0;
}

Re: blk: queue cleanup at device shutdown

[Jens Axboe]

On Thu, Sep 11 2008, Geoff Levand wrote:
> Hi Jens,
> 
> I'm wondering about what is needed in ps3disk_remove(), to clean
> up the request_queue (priv->queue) the driver uses.
> ps3disk_remove() is called from the main device_shutdown()
> routine via the ps3_system_bus remove routine.
> 
> The current code follows, and as can be seen, there is no check
> or handling of the queue before blk_cleanup_queue() is called.
> Can it be assumed that the queue is empty at this point,
> or is something else needed here?

The queue should be empty, at least there should be no one else
submitting IO for the device given the contet in which it is called. But
I suppose we could have pending IO (given the kexec context you
explained to me in person), so the below patch could be needed. It may
actually be a good idea to put this into blk_cleanup_queue(), since we
should have process context there anyway.

diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
index d797e20..dce64d0 100644
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -537,6 +537,8 @@ static int ps3disk_remove(struct ps3_system_bus_device *_dev)
 	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
 	struct ps3disk_private *priv = dev->sbd.core.driver_data;
 
+	blk_sync_queue(priv->queue);
+
 	mutex_lock(&ps3disk_mask_mutex);
 	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
 		    &ps3disk_mask);

-- 
Jens Axboe