Initialize mempool and elevator only for request-based dm devices

PATCH Superseded Nikanth Karthikesan 3 participants 11 messages Aug 8, 2009 – May 11, 2010

Subsystems (2)

Block layer Device mapper

lore

Share & tools

lore

Nikanth Karthikesan

author

2009-08-08 04:54 UTC #0 lore raw

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

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 <[email protected]>

---

drivers/md/dm.c +22 -8

index 8a311ea..b01dfbe 100644
@@ -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:

Mike Snitzer

2009-08-08 16:21 UTC #1 lore raw

Quoted 8 lines · 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 <[email protected]>

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

More comments below.

Quoted 11 lines

drivers/md/dm.c +2 -1

index 8a311ea..b01dfbe 100644
@@ -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);

drivers/md/dm.c int dm_swap_table(struct mapped_device *md, struct dm_table *table) · line 2213

Quoted 10 lines

drivers/md/dm.c +10 -0

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

drivers/md/dm.c +10 ~~-17~~

index b01dfbe..1b8aa43 100644
@@ -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:

Nikanth Karthikesan

author

2009-08-10 10:19 UTC #2 lore raw

Quoted 10 lines · On Saturday 08 August 2009 21:51:59 Mike Snitzer wrote:

On Sat, Aug 08 2009 at 12:56am -0400,
Quoted 7 lines · 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.

Quoted 4 lines

Quoted 1 line
Signed-off-by Nikanth Karthikesan <[email protected]>
I like how this clearly splits out request-based DM queue initialization.

Thanks.

Quoted 20 lines

More comments below.

Quoted 18 lines

drivers/md/dm.c +9 -1

index 8a311ea..b01dfbe 100644
@@ -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)

Quoted 9 lines

drivers/md/dm.c int dm_swap_table(struct mapped_device *md, struct dm_table *table)
Quoted 3 lines
drivers/md/dm.c +3 -0
+				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.

Quoted 32 lines

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

drivers/md/dm.c +5 ~~-17~~

index b01dfbe..1b8aa43 100644
@@ -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;

drivers/md/dm.c int dm_swap_table(struct mapped_device *md, struct · line 2216

Quoted 10 lines

drivers/md/dm.c +5 -0

+		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

Nikanth Karthikesan

author

2009-08-10 10:47 UTC #3 lore raw

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

Signed-off-by Nikanth Karthikesan <[email protected]>

---

drivers/md/dm.c +22 -7

index 8a311ea..8e5a2fd 100644
@@ -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);

Kiyoshi Ueda

2009-08-11 08:06 UTC #4 lore raw

Hi Nikanth,

Quoted 6 lines · 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.

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.

drivers/md/dm.c int dm_swap_table(struct mapped_device *md, struct dm_table *table) · line 2207

Quoted 10 lines

drivers/md/dm.c +6 -0

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

Quoted 5 lines

drivers/md/dm.c +5 -0

+
+		/*
+		 * reinitialize make_request_fn as it was reset to the
+		 * default __make_request by blk_init_allocate_queue
+		 */

drivers/md/dm.c int dm_swap_table(struct mapped_device *md, struct dm_table *table) · line 2222

Quoted 10 lines

drivers/md/dm.c +8 -0

+		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

Nikanth Karthikesan

author

2009-08-11 09:05 UTC #5 lore raw

Quoted 10 lines · On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:

Hi Nikanth,
Quoted 6 lines · 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.

Quoted 6 lines

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.

Quoted 4 lines

(quoted diff) +0 -0

@@ -2203,6 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct
 dm_table *table) goto out;
 	}

Quoted 13 lines

Review int dm_swap_table(struct mapped_device *md, struct · line 2207
Quoted 6 lines
(quoted diff) +6 -0
+	/* 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.

Quoted 14 lines

(quoted diff) +13 -0

+
+		/*
+		 * 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);

Quoted 8 lines

Review int dm_swap_table(struct mapped_device *md, struct · line 2222
Quoted 1 line
(quoted diff) +0 -0
 	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...

block/elevator.c +0 -0

index 2d511f9..864dd29 100644
@@ -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)
{

drivers/md/dm.c +0 -0

index 8a311ea..f6f77ea 100644
@@ -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

Nikanth Karthikesan

author

2009-08-11 09:32 UTC #6 lore raw

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

Signed-off-by Nikanth Karthikesan <[email protected]>

---

drivers/md/dm.c +30 -7

index 8a311ea..40156b0 100644
@@ -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);

Kiyoshi Ueda

2009-08-12 02:15 UTC #7 lore raw

In reply to Nikanth Karthikesan

Hi Nikanth,

Quoted 59 lines · Nikanth Karthikesan

Quoted 14 lines · On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
Quoted 13 lines · Nikanth Karthikesan
(quoted diff) +13 -0
+
+		/*
+		 * 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);
+	}
+
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
 	__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..
(quoted diff) +23 -0
@@ -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);

Quoted 2 lines

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

Nikanth Karthikesan

author

2009-08-12 08:45 UTC #8 lore raw

Hi Kiyoshi Ueda,

Quoted 68 lines · On Wednesday 12 August 2009 07:45:56 Kiyoshi Ueda wrote:

Hi Nikanth,
Quoted 60 lines · Nikanth Karthikesan
Quoted 15 lines · On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
Quoted 14 lines · Nikanth Karthikesan
(quoted diff) +13 -0
+
+		/*
+		 * 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);
Quoted 8 lines
Quoted 1 line
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..
(quoted diff) +23 -0
@@ -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);
Quoted 2 lines
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.

Quoted 9 lines

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

Kiyoshi Ueda

2009-08-14 07:01 UTC #9 lore raw

Hi Nikanth,

Quoted 73 lines · Nikanth Karthikesan

Hi Kiyoshi Ueda,
Quoted 70 lines · On Wednesday 12 August 2009 07:45:56 Kiyoshi Ueda wrote:
Hi Nikanth,
Quoted 58 lines · Nikanth Karthikesan
Quoted 14 lines · On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote:
Quoted 13 lines · Nikanth Karthikesan
(quoted diff) +13 -0
+
+		/*
+		 * 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);
+	}
+
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
 	__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..
(quoted diff) +23 -0
@@ -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);
Quoted 2 lines
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 2 lines

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

Mike Snitzer

2010-05-11 16:23 UTC #10 lore raw

Quoted 7 lines · Kiyoshi Ueda

Hi Nikanth,
Quoted 4 lines · Nikanth Karthikesan
Hi Kiyoshi Ueda,
Quoted 1 line · On Wednesday 12 August 2009 07:45:56 Kiyoshi Ueda wrote:
Hi Nikanth,

...

Quoted 29 lines

Quoted 7 lines
Quoted 7 lines
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 9 lines

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.

Quoted 7 lines

Quoted 3 lines
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).

Quoted 6 lines

Quoted 2 lines
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.

Quoted 6 lines

Quoted 2 lines
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