Re: [PATCH v2 2/2 "v9"] dm: only initialize full request_queue for request-based device

[Kiyoshi Ueda <k-ueda@ct.jp.nec.com>]

Hi Mike,

On 05/25/2010 08:46 AM +0900, Mike Snitzer wrote:
> Index: linux-2.6/drivers/md/dm-ioctl.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-ioctl.c
> +++ linux-2.6/drivers/md/dm-ioctl.c
> @@ -1201,6 +1203,15 @@ static int table_load(struct dm_ioctl *p
>  		goto out;
>  	}
>  
> +	/* setup md->queue to reflect md's and table's type (may block) */
> +	r = dm_setup_md_queue(md);
> +	if (r) {
> +		DMWARN("unable to setup device queue for this table.");
> +		dm_table_destroy(t);
> +		dm_unlock_md_type(md);
> +		goto out;
> +	}
> +

dm_setup_md_queue() should be put just after dm_set_md_type() of patch#1.
Then, you can make sure that dm_setup_md_queue() is called only once
and dm_setup_md_queue() should be much simpler.


> +/*
> + * Fully initialize a request-based queue (->elevator, ->request_fn, etc).
> + */
> +static int dm_init_request_based_queue(struct mapped_device *md)
> +{
> +	struct request_queue *q = NULL;
> +
> +	/* Avoid re-initializing the queue if already fully initialized */
> +	if (!md->queue->elevator) {
> +		/* Fully initialize the queue */
> +		q = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
> +		if (!q)
> +			return 0;

When blk_init_allocated_queue() fails, the block-layer seems not to
guarantee that the queue is still available.
You should create appropriate block-layer interfaces and/or take proper
recovery action here.
However, if the failure is not realistic, rather than complicating this
patch, you can just put a big comment and WARN_ON() for now, and fix it
in a separate patch-set.


> +static void dm_clear_request_based_queue(struct mapped_device *md)
> +{
> +	if (dm_bio_based_md_queue(md))
> +		return; /* queue already bio-based */
> +
> +	/* Unregister elevator from sysfs and clear ->request_fn */
> +	elv_unregister_queue(md->queue);
> +	md->queue->request_fn = NULL;
> +}
> +
> +/*
> + * Setup the DM device's queue based on md's type
> + */
> +int dm_setup_md_queue(struct mapped_device *md)
> +{
> +	BUG_ON(!mutex_is_locked(&md->type_lock));
> +	BUG_ON(dm_unknown_md_type(md));
> +
> +	if (dm_request_based_md_type(md)) {
> +		if (!dm_init_request_based_queue(md)) {
> +			DMWARN("Cannot initialize queue for Request-based dm");
> +			return -EINVAL;
> +		}
> +	} else if (dm_bio_based_md_type(md))
> +		dm_clear_request_based_queue(md);
> +
> +	return 0;
> +}

These unregister/clear works shouldn't be needed if dm_setup_md_queue()
is called only once as I mentioned above.

Thanks,
Kiyoshi Ueda