[PATCH 2/2] Initialize mempool and elevator only for request-based dm devices

[PATCH 2/2] Initialize mempool and elevator only for request-based dm devices

[Nikanth Karthikesan]

Intialize the request_queue and elevator only when the device is marked as
a request-based device. This avoids unnecessary creation of mempool for
requests. Also we wrongly initialize the elevator even for bio-based devices.
As the /sys/block/dm-*/queue/scheduler is exported for device-mapper devices,
it is possible to confuse with scheduler options for bio-based devices where
scheduler is not at all used.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 8a311ea..b01dfbe 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1749,22 +1749,21 @@ static struct mapped_device *alloc_dev(int minor)
 	INIT_LIST_HEAD(&md->uevent_list);
 	spin_lock_init(&md->uevent_lock);
 
-	md->queue = blk_init_queue(dm_request_fn, NULL);
+	md->queue = blk_alloc_queue(GFP_KERNEL);
 	if (!md->queue)
 		goto bad_queue;
 
 	/*
 	 * Request-based dm devices cannot be stacked on top of bio-based dm
-	 * devices.  The type of this dm device has not been decided yet,
-	 * although we initialized the queue using blk_init_queue().
+	 * devices. The type of this dm device has not been decided yet.
 	 * The type is decided at the first table loading time.
 	 * To prevent problematic device stacking, clear the queue flag
 	 * for request stacking support until then.
 	 *
 	 * This queue is new, so no concurrency on the queue_flags.
 	 */
+	md->queue->queue_flags = QUEUE_FLAG_DEFAULT;
 	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
-	md->saved_make_request_fn = md->queue->make_request_fn;
 	md->queue->queuedata = md;
 	md->queue->backing_dev_info.congested_fn = dm_any_congested;
 	md->queue->backing_dev_info.congested_data = md;
@@ -1772,9 +1771,6 @@ static struct mapped_device *alloc_dev(int minor)
 	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
 	md->queue->unplug_fn = dm_unplug_all;
 	blk_queue_merge_bvec(md->queue, dm_merge_bvec);
-	blk_queue_softirq_done(md->queue, dm_softirq_done);
-	blk_queue_prep_rq(md->queue, dm_prep_fn);
-	blk_queue_lld_busy(md->queue, dm_lld_busy);
 
 	md->disk = alloc_disk(1);
 	if (!md->disk)
@@ -2203,7 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
 		goto out;
 	}
 
-	__unbind(md);
+	if (md->map)
+		__unbind(md);
+	else {
+	/* new device is being marked as either request-based or bio-based */
+		if (dm_table_request_based(table)) {
+			/* Initialize queue for request-based dm */
+			r = blk_init_allocated_queue(md->queue, dm_request_fn,
+								 NULL);
+			if (r)
+				goto out;
+			md->saved_make_request_fn = md->queue->make_request_fn;
+			blk_queue_make_request(md->queue, dm_request);
+			blk_queue_softirq_done(md->queue, dm_softirq_done);
+			blk_queue_prep_rq(md->queue, dm_prep_fn);
+			blk_queue_lld_busy(md->queue, dm_lld_busy);
+
+		}
+	}
+
 	r = __bind(md, table, &limits);
 
 out:

Re: [PATCH 2/2] Initialize mempool and elevator only for request-based dm devices

[Mike Snitzer]

On Sat, Aug 08 2009 at 12:56am -0400,
Nikanth Karthikesan <knikanth@suse.de> wrote:

> Intialize the request_queue and elevator only when the device is marked as
> a request-based device. This avoids unnecessary creation of mempool for
> requests. Also we wrongly initialize the elevator even for bio-based devices.
> As the /sys/block/dm-*/queue/scheduler is exported for device-mapper devices,
> it is possible to confuse with scheduler options for bio-based devices where
> scheduler is not at all used.
> 
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

I like how this clearly splits out request-based DM queue initialization.

More comments below.

> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 8a311ea..b01dfbe 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2203,7 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
>  		goto out;
>  	}
>  
> -	__unbind(md);
> +	if (md->map)
> +		__unbind(md);
> +	else {
> +	/* new device is being marked as either request-based or bio-based */
> +		if (dm_table_request_based(table)) {
> +			/* Initialize queue for request-based dm */
> +			r = blk_init_allocated_queue(md->queue, dm_request_fn,
> +								 NULL);
> +			if (r)
> +				goto out;
> +			md->saved_make_request_fn = md->queue->make_request_fn;
> +			blk_queue_make_request(md->queue, dm_request);

This blk_queue_make_request() is redundant as it was already established
in alloc_dev().  The fact that its behavior is now altered as a result
of the md->saved_make_request_fn assignment doesn't change the fact that
the queue will be using dm_request().  Am I missing something?

Also, I think the code should be cleaned up as follows:

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index b01dfbe..1b8aa43 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2199,25 +2199,18 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
 		goto out;
 	}
 
-	if (md->map)
-		__unbind(md);
-	else {
-	/* new device is being marked as either request-based or bio-based */
-		if (dm_table_request_based(table)) {
-			/* Initialize queue for request-based dm */
-			r = blk_init_allocated_queue(md->queue, dm_request_fn,
-								 NULL);
-			if (r)
-				goto out;
-			md->saved_make_request_fn = md->queue->make_request_fn;
-			blk_queue_make_request(md->queue, dm_request);
-			blk_queue_softirq_done(md->queue, dm_softirq_done);
-			blk_queue_prep_rq(md->queue, dm_prep_fn);
-			blk_queue_lld_busy(md->queue, dm_lld_busy);
-
-		}
+	if (!md->map && dm_table_request_based(table)) {
+		/* Initialize queue for request-based dm */
+		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (r)
+			goto out;
+		md->saved_make_request_fn = md->queue->make_request_fn;
+		blk_queue_softirq_done(md->queue, dm_softirq_done);
+		blk_queue_prep_rq(md->queue, dm_prep_fn);
+		blk_queue_lld_busy(md->queue, dm_lld_busy);
 	}
 
+	__unbind(md);
 	r = __bind(md, table, &limits);
 
 out:

Re: [PATCH 2/2] Initialize mempool and elevator only for request-based dm devices

[Nikanth Karthikesan]

On Saturday 08 August 2009 21:51:59 Mike Snitzer wrote:
> On Sat, Aug 08 2009 at 12:56am -0400,
>
> Nikanth Karthikesan <knikanth@suse.de> wrote:
> > Intialize the request_queue and elevator only when the device is marked
> > as a request-based device. This avoids unnecessary creation of mempool
> > for requests. Also we wrongly initialize the elevator even for bio-based
> > devices. As the /sys/block/dm-*/queue/scheduler is exported for
> > device-mapper devices, it is possible to confuse with scheduler options
> > for bio-based devices where scheduler is not at all used.
> >
> > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> I like how this clearly splits out request-based DM queue initialization.
>

Thanks.

> More comments below.
>
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 8a311ea..b01dfbe 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -2203,7 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct
> > dm_table *table) goto out;
> >  	}
> >
> > -	__unbind(md);
> > +	if (md->map)
> > +		__unbind(md);
> > +	else {
> > +	/* new device is being marked as either request-based or bio-based */
> > +		if (dm_table_request_based(table)) {
> > +			/* Initialize queue for request-based dm */
> > +			r = blk_init_allocated_queue(md->queue, dm_request_fn,
> > +								 NULL);
> > +			if (r)
> > +				goto out;
> > +			md->saved_make_request_fn = md->queue->make_request_fn;
> > +			blk_queue_make_request(md->queue, dm_request);
>
> This blk_queue_make_request() is redundant as it was already established
> in alloc_dev().  The fact that its behavior is now altered as a result
> of the md->saved_make_request_fn assignment doesn't change the fact that
> the queue will be using dm_request().  Am I missing something?
>

blk_init_allocated_queue() would reset it to __make_request(). So we need to 
initialize it again. I would add a comment in v2 of the patch.

> Also, I think the code should be cleaned up as follows:
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index b01dfbe..1b8aa43 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2199,25 +2199,18 @@ int dm_swap_table(struct mapped_device *md, struct
> dm_table *table) goto out;
>  	}
>
> -	if (md->map)
> -		__unbind(md);
> -	else {
> -	/* new device is being marked as either request-based or bio-based */
> -		if (dm_table_request_based(table)) {
> -			/* Initialize queue for request-based dm */
> -			r = blk_init_allocated_queue(md->queue, dm_request_fn,
> -								 NULL);
> -			if (r)
> -				goto out;
> -			md->saved_make_request_fn = md->queue->make_request_fn;
> -			blk_queue_make_request(md->queue, dm_request);
> -			blk_queue_softirq_done(md->queue, dm_softirq_done);
> -			blk_queue_prep_rq(md->queue, dm_prep_fn);
> -			blk_queue_lld_busy(md->queue, dm_lld_busy);
> -
> -		}
> +	if (!md->map && dm_table_request_based(table)) {
> +		/* Initialize queue for request-based dm */
> +		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
> +		if (r)
> +			goto out;
> +		md->saved_make_request_fn = md->queue->make_request_fn;
> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
>  	}
>
> +	__unbind(md);
>  	r = __bind(md, table, &limits);
>
>  out:

I thought of this. But personally, I did not prefer this, as this makes it 
clear that we do __unbind() only on not new devices. But, yes it needs more 
level of nesting and looks ugly. Changed in v2 of the patch.

Thanks
Nikanth

[PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Nikanth Karthikesan]

Intialize the request_queue and elevator only when the device is marked as
a request-based device. This avoids unnecessary creation of mempool for
requests. Also we wrongly initialize the elevator even for bio-based devices.
As the /sys/block/dm-*/queue/scheduler is exported for device-mapper devices,
it is possible to confuse with scheduler options for bio-based devices where
scheduler is not at all used.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 8a311ea..8e5a2fd 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1749,22 +1749,21 @@ static struct mapped_device *alloc_dev(int minor)
 	INIT_LIST_HEAD(&md->uevent_list);
 	spin_lock_init(&md->uevent_lock);
 
-	md->queue = blk_init_queue(dm_request_fn, NULL);
+	md->queue = blk_alloc_queue(GFP_KERNEL);
 	if (!md->queue)
 		goto bad_queue;
 
 	/*
 	 * Request-based dm devices cannot be stacked on top of bio-based dm
-	 * devices.  The type of this dm device has not been decided yet,
-	 * although we initialized the queue using blk_init_queue().
+	 * devices. The type of this dm device has not been decided yet.
 	 * The type is decided at the first table loading time.
 	 * To prevent problematic device stacking, clear the queue flag
 	 * for request stacking support until then.
 	 *
 	 * This queue is new, so no concurrency on the queue_flags.
 	 */
+	md->queue->queue_flags = QUEUE_FLAG_DEFAULT;
 	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
-	md->saved_make_request_fn = md->queue->make_request_fn;
 	md->queue->queuedata = md;
 	md->queue->backing_dev_info.congested_fn = dm_any_congested;
 	md->queue->backing_dev_info.congested_data = md;
@@ -1772,9 +1771,6 @@ static struct mapped_device *alloc_dev(int minor)
 	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
 	md->queue->unplug_fn = dm_unplug_all;
 	blk_queue_merge_bvec(md->queue, dm_merge_bvec);
-	blk_queue_softirq_done(md->queue, dm_softirq_done);
-	blk_queue_prep_rq(md->queue, dm_prep_fn);
-	blk_queue_lld_busy(md->queue, dm_lld_busy);
 
 	md->disk = alloc_disk(1);
 	if (!md->disk)
@@ -2203,6 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
 		goto out;
 	}
 
+	/* new device is being marked as request-based */
+	if (!md->map && dm_table_request_based(table)) {
+		/* initialize queue for request-based dm */
+		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (r)
+			goto out;
+
+		/*
+		 * reinitialize make_request_fn as it was reset to the
+		 * default __make_request by blk_init_allocate_queue
+		 */
+		md->saved_make_request_fn = md->queue->make_request_fn;
+		blk_queue_make_request(md->queue, dm_request);
+
+		blk_queue_softirq_done(md->queue, dm_softirq_done);
+		blk_queue_prep_rq(md->queue, dm_prep_fn);
+		blk_queue_lld_busy(md->queue, dm_lld_busy);
+	}
+
 	__unbind(md);
 	r = __bind(md, table, &limits);
 

Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Kiyoshi Ueda]

Hi Nikanth,

On 08/10/2009 07:48 PM +0900, Nikanth Karthikesan wrote:
> Intialize the request_queue and elevator only when the device is marked as
> a request-based device. This avoids unnecessary creation of mempool for
> requests. Also we wrongly initialize the elevator even for bio-based devices.
> As the /sys/block/dm-*/queue/scheduler is exported for device-mapper devices,
> it is possible to confuse with scheduler options for bio-based devices where
> scheduler is not at all used.

Thank you for working on this.
Actually, I had tried this delayed allocation thing before,
but I chose the current implementation since I couldn't solve
some problems, which your patch also has.
Please see my comment below.


> @@ -2203,6 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
>  		goto out;
>  	}
>  
> +	/* new device is being marked as request-based */
> +	if (!md->map && dm_table_request_based(table)) {
> +		/* initialize queue for request-based dm */
> +		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
> +		if (r)
> +			goto out;

Generally, dm must not allocate memory during resume because
it may cause a deadlock in no memory situation.
However, there is no I/O on this device at this point,
so the allocation should be ok for this special case.
I think some comments are needed here to describe that.


> +
> +		/*
> +		 * reinitialize make_request_fn as it was reset to the
> +		 * default __make_request by blk_init_allocate_queue
> +		 */
> +		md->saved_make_request_fn = md->queue->make_request_fn;
> +		blk_queue_make_request(md->queue, dm_request);
> +
> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
> +	}
> +
>  	__unbind(md);
>  	r = __bind(md, table, &limits);

The queue has been registered at the device creation time by
add_disk() in alloc_dev().
Since the queue is reconfigured (elevator is attached), you have to
update the queue registration (e.g. unregister, then re-register).
But it may not be easy.  At least, there is no exported interface to
unregister/re-register queue.

Thanks,
Kiyoshi Ueda

Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Nikanth Karthikesan]

On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
> Hi Nikanth,
>
> On 08/10/2009 07:48 PM +0900, Nikanth Karthikesan wrote:
> > Intialize the request_queue and elevator only when the device is marked
> > as a request-based device. This avoids unnecessary creation of mempool
> > for requests. Also we wrongly initialize the elevator even for bio-based
> > devices. As the /sys/block/dm-*/queue/scheduler is exported for
> > device-mapper devices, it is possible to confuse with scheduler options
> > for bio-based devices where scheduler is not at all used.
>
> Thank you for working on this.
> Actually, I had tried this delayed allocation thing before,
> but I chose the current implementation since I couldn't solve
> some problems, which your patch also has.
> Please see my comment below.
>

Thanks for the review & comments.

> > @@ -2203,6 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct
> > dm_table *table) goto out;
> >  	}
> >
> > +	/* new device is being marked as request-based */
> > +	if (!md->map && dm_table_request_based(table)) {
> > +		/* initialize queue for request-based dm */
> > +		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
> > +		if (r)
> > +			goto out;
>
> Generally, dm must not allocate memory during resume because
> it may cause a deadlock in no memory situation.
> However, there is no I/O on this device at this point,
> so the allocation should be ok for this special case.
> I think some comments are needed here to describe that.
>

Ok. This comment can be added.

> > +
> > +		/*
> > +		 * reinitialize make_request_fn as it was reset to the
> > +		 * default __make_request by blk_init_allocate_queue
> > +		 */
> > +		md->saved_make_request_fn = md->queue->make_request_fn;
> > +		blk_queue_make_request(md->queue, dm_request);
> > +
> > +		blk_queue_softirq_done(md->queue, dm_softirq_done);
> > +		blk_queue_prep_rq(md->queue, dm_prep_fn);
> > +		blk_queue_lld_busy(md->queue, dm_lld_busy);
> > +	}
> > +
> >  	__unbind(md);
> >  	r = __bind(md, table, &limits);
>
> The queue has been registered at the device creation time by
> add_disk() in alloc_dev().
> Since the queue is reconfigured (elevator is attached), you have to
> update the queue registration (e.g. unregister, then re-register).
> But it may not be easy.  At least, there is no exported interface to
> unregister/re-register queue.

Ah, yes. The scheduler attributes will not be exported in 
/sys/block/dm*/queue/iosched. Exporting elv_register_queue() and calling it 
here solves it. Something like..

@@ -2203,6 +2199,29 @@ int dm_swap_table(struct mapped_device *md, struct 
dm_table *table)
 		goto out;
 	}
 
+	/* new device is being marked as request-based */
+	if (!md->map && dm_table_request_based(table)) {
+		/* initialize queue for request-based dm */
+		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (r)
+			goto out;
+
+		r = elv_register_queue(md->queue);
+		/* if (r)
+		 *	 goto out; Better to ignore, just like add_disk does ;-)
+		 */
+		/*
+		 * reinitialize make_request_fn as it was reset to the
+		 * default __make_request by blk_init_allocate_queue
+		 */
+		md->saved_make_request_fn = md->queue->make_request_fn;
+		blk_queue_make_request(md->queue, dm_request);
+
+		blk_queue_softirq_done(md->queue, dm_softirq_done);
+		blk_queue_prep_rq(md->queue, dm_prep_fn);
+		blk_queue_lld_busy(md->queue, dm_lld_busy);
+	}
+
 	__unbind(md);
 	r = __bind(md, table, &limits);
 
I would post the v3 of the patches with this change. Do you see any problems 
in this?

This can also be solved by initializing the queue for new devices and then 
unregistering the elevator, if it is a bio-based device at table load time. 
Like...

diff --git a/block/elevator.c b/block/elevator.c
index 2d511f9..864dd29 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -939,9 +939,10 @@ static void __elv_unregister_queue(struct elevator_queue 
*e)
 
 void elv_unregister_queue(struct request_queue *q)
 {
-	if (q)
+	if (q && q->elevator)
 		__elv_unregister_queue(q->elevator);
 }
+EXPORT_SYMBOL(elv_unregister_queue);
 
 void elv_register(struct elevator_type *e)
 {
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 8a311ea..f6f77ea 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2203,7 +2203,28 @@ int dm_swap_table(struct mapped_device *md, struct 
dm_table *table)
 		goto out;
 	}
 
-	__unbind(md);
+	if (md->map)
+		__unbind(md);
+	else if (!dm_table_request_based(table)) {
+	/*
+	 * This is a new bio-based device, and doesnt use the elevator
+	 * and requests.
+	 */
+		struct request_queue *q;
+		q = md->queue;
+		if (q->elevator) {
+			struct request_list *rl = &q->rq;
+			elevator_exit(q->elevator);
+			elv_unregister_queue(q);
+			q->elevator = 0;
+			if (rl->rq_pool) {
+				mempool_destroy(rl->rq_pool);
+				rl->rq_pool = 0;
+			}
+		}
+
+	}
+
 	r = __bind(md, table, &limits);
 
 out:

But, I think, delaying the initialization, is the best solution.

Thanks
Nikanth

[PATCH-v3 2/2] Initialize mempool and elevator only for request-based dm devices

[Nikanth Karthikesan]

Intialize the request_queue and elevator only when the device is marked as
a request-based device. This avoids unnecessary creation of mempool for
requests. Also we wrongly initialize the elevator even for bio-based devices.
As the /sys/block/dm-*/queue/scheduler is exported for device-mapper devices,
it is possible to confuse with scheduler options for bio-based devices where
scheduler is not at all used.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 8a311ea..40156b0 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1749,22 +1749,21 @@ static struct mapped_device *alloc_dev(int minor)
 	INIT_LIST_HEAD(&md->uevent_list);
 	spin_lock_init(&md->uevent_lock);
 
-	md->queue = blk_init_queue(dm_request_fn, NULL);
+	md->queue = blk_alloc_queue(GFP_KERNEL);
 	if (!md->queue)
 		goto bad_queue;
 
 	/*
 	 * Request-based dm devices cannot be stacked on top of bio-based dm
-	 * devices.  The type of this dm device has not been decided yet,
-	 * although we initialized the queue using blk_init_queue().
+	 * devices. The type of this dm device has not been decided yet.
 	 * The type is decided at the first table loading time.
 	 * To prevent problematic device stacking, clear the queue flag
 	 * for request stacking support until then.
 	 *
 	 * This queue is new, so no concurrency on the queue_flags.
 	 */
+	md->queue->queue_flags = QUEUE_FLAG_DEFAULT;
 	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
-	md->saved_make_request_fn = md->queue->make_request_fn;
 	md->queue->queuedata = md;
 	md->queue->backing_dev_info.congested_fn = dm_any_congested;
 	md->queue->backing_dev_info.congested_data = md;
@@ -1772,9 +1771,6 @@ static struct mapped_device *alloc_dev(int minor)
 	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
 	md->queue->unplug_fn = dm_unplug_all;
 	blk_queue_merge_bvec(md->queue, dm_merge_bvec);
-	blk_queue_softirq_done(md->queue, dm_softirq_done);
-	blk_queue_prep_rq(md->queue, dm_prep_fn);
-	blk_queue_lld_busy(md->queue, dm_lld_busy);
 
 	md->disk = alloc_disk(1);
 	if (!md->disk)
@@ -2203,6 +2199,33 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
 		goto out;
 	}
 
+	/* new device is being marked as request-based */
+	if (!md->map && dm_table_request_based(table)) {
+		/*
+		 * Initialize queue for request-based dm.
+		 * Generally DM must not allocate memory during resume as it
+		 * may cause deadlock during no memory situation. But there is
+		 * no I/O happening on this device. So it is ok to allocate
+		 * memory here.
+		 */
+		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (r)
+			goto out;
+
+		elv_register_queue(md->queue);
+
+		/*
+		 * reinitialize make_request_fn as it was reset to the
+		 * default __make_request by blk_init_allocate_queue
+		 */
+		md->saved_make_request_fn = md->queue->make_request_fn;
+		blk_queue_make_request(md->queue, dm_request);
+
+		blk_queue_softirq_done(md->queue, dm_softirq_done);
+		blk_queue_prep_rq(md->queue, dm_prep_fn);
+		blk_queue_lld_busy(md->queue, dm_lld_busy);
+	}
+
 	__unbind(md);
 	r = __bind(md, table, &limits);
 

Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Kiyoshi Ueda]

Hi Nikanth,

On 08/11/2009 06:05 PM +0900, Nikanth Karthikesan wrote:
> On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
>> On 08/10/2009 07:48 PM +0900, Nikanth Karthikesan wrote:
>>> +
>>> +		/*
>>> +		 * reinitialize make_request_fn as it was reset to the
>>> +		 * default __make_request by blk_init_allocate_queue
>>> +		 */
>>> +		md->saved_make_request_fn = md->queue->make_request_fn;
>>> +		blk_queue_make_request(md->queue, dm_request);
>>> +
>>> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
>>> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
>>> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
>>> +	}
>>> +
>>>  	__unbind(md);
>>>  	r = __bind(md, table, &limits);
>> The queue has been registered at the device creation time by
>> add_disk() in alloc_dev().
>> Since the queue is reconfigured (elevator is attached), you have to
>> update the queue registration (e.g. unregister, then re-register).
>> But it may not be easy.  At least, there is no exported interface to
>> unregister/re-register queue.
> 
> Ah, yes. The scheduler attributes will not be exported in 
> /sys/block/dm*/queue/iosched. Exporting elv_register_queue() and calling it 
> here solves it. Something like..
> 
> @@ -2203,6 +2199,29 @@ int dm_swap_table(struct mapped_device *md, struct 
> dm_table *table)
>  		goto out;
>  	}
>  
> +	/* new device is being marked as request-based */
> +	if (!md->map && dm_table_request_based(table)) {
> +		/* initialize queue for request-based dm */
> +		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
> +		if (r)
> +			goto out;
> +
> +		r = elv_register_queue(md->queue);
> +		/* if (r)
> +		 *	 goto out; Better to ignore, just like add_disk does ;-)
> +		 */
> +		/*
> +		 * reinitialize make_request_fn as it was reset to the
> +		 * default __make_request by blk_init_allocate_queue
> +		 */
> +		md->saved_make_request_fn = md->queue->make_request_fn;
> +		blk_queue_make_request(md->queue, dm_request);
> +
> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
> +	}
> +
>  	__unbind(md);
>  	r = __bind(md, table, &limits);
>  
> I would post the v3 of the patches with this change. Do you see any problems 
> in this?

Humm, it might work for now, but I disagree with that.

Since elevator is block internal and dm doesn't really care
(its initialization is actually hidden in blk_init_allocated_queue()),
directly calling elv_register_queue() from dm seems not right.
It will likely introduce a bug by future changes in block layer.

I think the right approach is to define a proper block layer interface
to reflect the queue configuration change.
That's why I said "Updating the queue registration may not be easy".

Thanks,
Kiyoshi Ueda

Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Nikanth Karthikesan]

Hi Kiyoshi Ueda,

On Wednesday 12 August 2009 07:45:56 Kiyoshi Ueda wrote:
> Hi Nikanth,
>
> On 08/11/2009 06:05 PM +0900, Nikanth Karthikesan wrote:
> > On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
> >> On 08/10/2009 07:48 PM +0900, Nikanth Karthikesan wrote:
> >>> +
> >>> +		/*
> >>> +		 * reinitialize make_request_fn as it was reset to the
> >>> +		 * default __make_request by blk_init_allocate_queue
> >>> +		 */
> >>> +		md->saved_make_request_fn = md->queue->make_request_fn;
> >>> +		blk_queue_make_request(md->queue, dm_request);
> >>> +
> >>> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
> >>> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
> >>> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
> >>> +	}
> >>> +
> >>>  	__unbind(md);
> >>>  	r = __bind(md, table, &limits);
> >>
> >> The queue has been registered at the device creation time by
> >> add_disk() in alloc_dev().
> >> Since the queue is reconfigured (elevator is attached), you have to
> >> update the queue registration (e.g. unregister, then re-register).
> >> But it may not be easy.  At least, there is no exported interface to
> >> unregister/re-register queue.
> >
> > Ah, yes. The scheduler attributes will not be exported in
> > /sys/block/dm*/queue/iosched. Exporting elv_register_queue() and calling
> > it here solves it. Something like..
> >
> > @@ -2203,6 +2199,29 @@ int dm_swap_table(struct mapped_device *md, struct
> > dm_table *table)
> >  		goto out;
> >  	}
> >
> > +	/* new device is being marked as request-based */
> > +	if (!md->map && dm_table_request_based(table)) {
> > +		/* initialize queue for request-based dm */
> > +		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
> > +		if (r)
> > +			goto out;
> > +
> > +		r = elv_register_queue(md->queue);
> > +		/* if (r)
> > +		 *	 goto out; Better to ignore, just like add_disk does ;-)
> > +		 */
> > +		/*
> > +		 * reinitialize make_request_fn as it was reset to the
> > +		 * default __make_request by blk_init_allocate_queue
> > +		 */
> > +		md->saved_make_request_fn = md->queue->make_request_fn;
> > +		blk_queue_make_request(md->queue, dm_request);
> > +
> > +		blk_queue_softirq_done(md->queue, dm_softirq_done);
> > +		blk_queue_prep_rq(md->queue, dm_prep_fn);
> > +		blk_queue_lld_busy(md->queue, dm_lld_busy);
> > +	}
> > +
> >  	__unbind(md);
> >  	r = __bind(md, table, &limits);
> >
> > I would post the v3 of the patches with this change. Do you see any
> > problems in this?
>
> Humm, it might work for now, but I disagree with that.
>
> Since elevator is block internal and dm doesn't really care
> (its initialization is actually hidden in blk_init_allocated_queue()),
> directly calling elv_register_queue() from dm seems not right.
> It will likely introduce a bug by future changes in block layer.
>
> I think the right approach is to define a proper block layer interface
> to reflect the queue configuration change.
> That's why I said "Updating the queue registration may not be easy".
>

I do not see too much of overhead in the future with this approach, atleast no 
more than "proper block layer interface". IMHO, unregistering the queue and 
registering the queue again with the elevator, is basically wasting CPU cycles 
and possibly would confuse the user-space, which may be watching the sysfs... 
Or asking block layer to recheck and find what we have changed in the 
request_queue. It does not sound like the best solution.

It is better to tell the block-layer that we have added a q->request_fn 
function, so initialize the elevator.

If block layer, exports elv_register_queue() and document it, it would become 
a proper block layer interface, right? Device-mapper would always depend on 
internals of block-layer to some extent. ;-)

Thanks
Nikanth

Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Kiyoshi Ueda]

Hi Nikanth,

On 08/12/2009 05:47 PM +0900, Nikanth Karthikesan wrote:
> Hi Kiyoshi Ueda,
> 
> On Wednesday 12 August 2009 07:45:56 Kiyoshi Ueda wrote:
>> Hi Nikanth,
>>
>> On 08/11/2009 06:05 PM +0900, Nikanth Karthikesan wrote:
>>> On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
>>>> On 08/10/2009 07:48 PM +0900, Nikanth Karthikesan wrote:
>>>>> +
>>>>> +		/*
>>>>> +		 * reinitialize make_request_fn as it was reset to the
>>>>> +		 * default __make_request by blk_init_allocate_queue
>>>>> +		 */
>>>>> +		md->saved_make_request_fn = md->queue->make_request_fn;
>>>>> +		blk_queue_make_request(md->queue, dm_request);
>>>>> +
>>>>> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
>>>>> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
>>>>> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
>>>>> +	}
>>>>> +
>>>>>  	__unbind(md);
>>>>>  	r = __bind(md, table, &limits);
>>>> The queue has been registered at the device creation time by
>>>> add_disk() in alloc_dev().
>>>> Since the queue is reconfigured (elevator is attached), you have to
>>>> update the queue registration (e.g. unregister, then re-register).
>>>> But it may not be easy.  At least, there is no exported interface to
>>>> unregister/re-register queue.
>>> Ah, yes. The scheduler attributes will not be exported in
>>> /sys/block/dm*/queue/iosched. Exporting elv_register_queue() and calling
>>> it here solves it. Something like..
>>>
>>> @@ -2203,6 +2199,29 @@ int dm_swap_table(struct mapped_device *md, struct
>>> dm_table *table)
>>>  		goto out;
>>>  	}
>>>
>>> +	/* new device is being marked as request-based */
>>> +	if (!md->map && dm_table_request_based(table)) {
>>> +		/* initialize queue for request-based dm */
>>> +		r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
>>> +		if (r)
>>> +			goto out;
>>> +
>>> +		r = elv_register_queue(md->queue);
>>> +		/* if (r)
>>> +		 *	 goto out; Better to ignore, just like add_disk does ;-)
>>> +		 */
>>> +		/*
>>> +		 * reinitialize make_request_fn as it was reset to the
>>> +		 * default __make_request by blk_init_allocate_queue
>>> +		 */
>>> +		md->saved_make_request_fn = md->queue->make_request_fn;
>>> +		blk_queue_make_request(md->queue, dm_request);
>>> +
>>> +		blk_queue_softirq_done(md->queue, dm_softirq_done);
>>> +		blk_queue_prep_rq(md->queue, dm_prep_fn);
>>> +		blk_queue_lld_busy(md->queue, dm_lld_busy);
>>> +	}
>>> +
>>>  	__unbind(md);
>>>  	r = __bind(md, table, &limits);
>>>
>>> I would post the v3 of the patches with this change. Do you see any
>>> problems in this?
>> Humm, it might work for now, but I disagree with that.
>>
>> Since elevator is block internal and dm doesn't really care
>> (its initialization is actually hidden in blk_init_allocated_queue()),
>> directly calling elv_register_queue() from dm seems not right.
>> It will likely introduce a bug by future changes in block layer.
>>
>> I think the right approach is to define a proper block layer interface
>> to reflect the queue configuration change.
>> That's why I said "Updating the queue registration may not be easy".
> 
> I do not see too much of overhead in the future with this approach,
> atleast no more than "proper block layer interface".

I don't think so.
Just exporting elv_register_queue() will cause some maintenance costs
to request-based dm developers as below.

Although currently only elevator is the queue's feature which is
needed for only request-based dm, such other features may be added
to queue in the future.
Then, the developer who added the feature may not notice that
request-based dm needs to register the feature here, if there
is no critical problem (e.g. compile error or panic) without it.
That causes the lack of such features only in request-based dm.
Therefore, request-based dm developers always have to watch
the change of the block-layer and make the registration related code.
I think it's a sort of big maintenance cost.

So we should make the code as the change of the block-layer becomes
effective automatically in request-based dm, too, as mush as possible.
In this case, you should make/call an interface for the whole queue,
not only for the elevator, since dm can't/shouldn't know how
blk_init_allocated_queue() initializes the queue.
(And the interface should be used in other generic paths (e.g. add_disk()))
That's a proper block-layer interface which I mentioned, and this
approach should have less overhead than your approach from view point
of longer period.


> IMHO, unregistering the queue and registering the queue again with
> the elevator, is basically wasting CPU cycles and possibly would
> confuse the user-space, which may be watching the sysfs... 

Right, so I said "Updating may not be easy."
(By the way, wasting CPU cycles doesn't matter here, since it happens
 only when we initialize the device and it shouldn't too much.)


> Or asking block layer to recheck and find what we have changed
> in the request_queue. It does not sound like the best solution.

I think this is a better solution than exposing a part of queue
internals as I described above.


> It is better to tell the block-layer that we have added a q->request_fn 
> function, so initialize the elevator.

I don't think it's better as I described above.
(dm can't/shouldn't know how blk_init_allocated_queue() initializes
 the queue.)



By the way, another approach to optimizing the memory usage would be
to determine whether the dm device is bio-based or request-based
at the device creation time, instead of the table binding time.
We want the delayed allocation, since kernel can't decide the device
type until the first table is bound because of the auto-detection
mechanism.  The auto-detection is good for keeping compatibility with
existing user-space tools.  But once user-space tools are changed to
specify device type at the device creation time, we can eventually
remove the auto-detection.
Then, kernel can decide device type in alloc_dev(), so
the initialization code in kernel will become very simple.

FYI, actually, I had this approach in a very early stage of
request-based dm development:
    [kernel]     http://marc.info/?l=dm-devel&m=116656637419846&w=2
    [kernel]     http://marc.info/?l=dm-devel&m=116656689701459&w=2
    [kernel]     http://marc.info/?l=dm-devel&m=116656689707043&w=2
    [user-space] http://marc.info/?l=dm-devel&m=116656689906056&w=2
Now, you can change user-space first before kernel, since
request-based dm is already available.

Thanks,
Kiyoshi Ueda

Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices

[Mike Snitzer]

On Fri, Aug 14 2009 at  3:01am -0400,
Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:

> Hi Nikanth,
> 
> On 08/12/2009 05:47 PM +0900, Nikanth Karthikesan wrote:
> > Hi Kiyoshi Ueda,
> > 
> > On Wednesday 12 August 2009 07:45:56 Kiyoshi Ueda wrote:
> >> Hi Nikanth,
...
> >> Humm, it might work for now, but I disagree with that.
> >>
> >> Since elevator is block internal and dm doesn't really care
> >> (its initialization is actually hidden in blk_init_allocated_queue()),
> >> directly calling elv_register_queue() from dm seems not right.
> >> It will likely introduce a bug by future changes in block layer.
> >>
> >> I think the right approach is to define a proper block layer interface
> >> to reflect the queue configuration change.
> >> That's why I said "Updating the queue registration may not be easy".
> > 
> > I do not see too much of overhead in the future with this approach,
> > atleast no more than "proper block layer interface".
> 
> I don't think so.
> Just exporting elv_register_queue() will cause some maintenance costs
> to request-based dm developers as below.
> 
> Although currently only elevator is the queue's feature which is
> needed for only request-based dm, such other features may be added
> to queue in the future.
> Then, the developer who added the feature may not notice that
> request-based dm needs to register the feature here, if there
> is no critical problem (e.g. compile error or panic) without it.
> That causes the lack of such features only in request-based dm.
> Therefore, request-based dm developers always have to watch
> the change of the block-layer and make the registration related code.
> I think it's a sort of big maintenance cost.
> 
> So we should make the code as the change of the block-layer becomes
> effective automatically in request-based dm, too, as mush as possible.
> In this case, you should make/call an interface for the whole queue,
> not only for the elevator, since dm can't/shouldn't know how
> blk_init_allocated_queue() initializes the queue.
> (And the interface should be used in other generic paths (e.g. add_disk()))
> That's a proper block-layer interface which I mentioned, and this
> approach should have less overhead than your approach from view point
> of longer period.

Any future changes must be done with the understanding of how the
current code works (block layer included).  We cannot ignore known
problems because of some future change that has theoretical oversights
on how the existing code works.


> > IMHO, unregistering the queue and registering the queue again with
> > the elevator, is basically wasting CPU cycles and possibly would
> > confuse the user-space, which may be watching the sysfs... 
> 
> Right, so I said "Updating may not be easy."
> (By the way, wasting CPU cycles doesn't matter here, since it happens
>  only when we initialize the device and it shouldn't too much.)

Having DM intelligently build on what the block layer already registered
(with sysfs) seemed less controversial to me.

However, my first private version of dm_init_request_based_queue()
re-registered the entire queue, blk_unregister_queue then
blk_register_queue, but I decided against it -- as I thought it would be
less controversial -- how foolish of me ;)
(I also hadn't read this review you already gave Nikanth...)

My reasoning was that completely unregistering the queue from sysfs and
then re-registering was simply excessive.  We know better so why work
harder?
(and yes I get your point that being wasteful at table load is tolerable
-- but I'm not really seeing the DM design purity gains of
   re-registering the queue).


> > Or asking block layer to recheck and find what we have changed
> > in the request_queue. It does not sound like the best solution.
> 
> I think this is a better solution than exposing a part of queue
> internals as I described above.
> 
> > It is better to tell the block-layer that we have added a q->request_fn 
> > function, so initialize the elevator.
> 
> I don't think it's better as I described above.
> (dm can't/shouldn't know how blk_init_allocated_queue() initializes
>  the queue.)

Taking a step back...

The block layer always deals with exposing the queue's sysfs attributes
as a separate step that is performed _after_ initializing the queue.

With my proposed patch, request-based DM would now build on the
minimalist bio-based request_queue by fully initializing it.  I don't
think it is too big a stretch to say:

If a request_queue is fully initialized either the block layer core or a
subsystem making precise use of block layer interfaces (e.g. DM) should
take care to register the elevator -- especially if said subsystem
already partially registered the minimalist queue.


All this being said, I can easily switch back to my initial version of
dm_init_request_based_queue(), that used blk_unregister_queue() then
blk_register_queue(), if Alasdair and/or Jens would prefer that as a
"better" block interface for DM to use.  But Jens has already accepted
my proposed block change:
http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=01effb0dc1451fad55925873ffbfb88fa4eadce0

We could certainly export blk_unregister_queue and blk_register_queue
too...

Regards,
Mike