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

[Mike Snitzer]

On Tue, May 25 2010 at  7:18am -0400,
Kiyoshi Ueda <k-ueda at ct.jp.nec.com> wrote:

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

Good idea.

> > +/*
> > + * 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.

Ouch, yes this portion of blk_init_allocated_queue_node() is certainly
problematic:

        if (blk_init_free_list(q)) {
                kmem_cache_free(blk_requestq_cachep, q);
                return NULL;
        }

Cc'ing Jens as I think it would be advantageous for us to push the above
kmem_cache_free() into the callers where it really makes sense, e.g.:
blk_init_queue_node().

So on blk_init_allocated_queue_node() failure blk_init_queue_node() will
take care to cleanup the queue that it assumes it is managing
completely.

My patch (linux-2.6-block.git's commit: 01effb0) that split out
blk_init_allocated_queue_node() from blk_init_queue_node() opened up
this issue.  I'm fairly confident we'll get it fixed by the time 2.6.35
ships.

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

I'll hold off on the WARN_ON().  I think we need the request_queue to
remain allocated even after blk_init_allocated_queue_node() failure.

> > +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.

Indeed.  I'll get v3 of the patchset out with appropriate edits.

Thanks,
Mike