Skip to contentSkip to conversation

Allow delaying initialization of queue after allocation

PATCH Superseded Nikanth Karthikesan 2 participants 6 messages Aug 8, 2009 – Aug 11, 2009

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/2 Allow delaying initialization of queue after allocation

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

---

block/blk-core.c +29 -9
index 4b45435..5db0772 100644
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	if (!q)
 		return NULL;
 
+	q->node = node_id;
+
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
@@ -569,12 +571,25 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	if (!q)
 		return NULL;
 
-	q->node = node_id;
-	if (blk_init_free_list(q)) {
+	if (blk_init_allocated_queue(q, rfn, lock)) {
+		blk_put_queue(q);
 		kmem_cache_free(blk_requestq_cachep, q);
 		return NULL;
 	}
 
+	return q;
+}
+EXPORT_SYMBOL(blk_init_queue_node);
+
+int blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
+							 spinlock_t *lock)
+{
+	int err = 0;
+
+	err = blk_init_free_list(q);
+	if (err)
+		goto out;
+
 	/*
 	 * if caller didn't supply a lock, they get per-queue locking with
 	 * our embedded lock
@@ -598,15 +613,20 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	/*
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+
+free_and_out:
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {
include/linux/blkdev.h +2 -0
index e7cb5db..9552961 100644
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct request_queue *);
 extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
 					spinlock_t *lock, int node_id);
 extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
+extern int blk_init_allocated_queue(struct request_queue *q,
+				request_fn_proc *rfn, spinlock_t *lock);
 extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);
#1 lore raw
Quoted 9 lines · Nikanth Karthikesan

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

This patch needed to be refreshed to account for the changes from this recent commit: a4e7d46407d73f35d217013b363b79a8f8eafcaa

I've attached a refreshed patch.

Though I still have questions/feedback below.

Quoted 50 lines
block/blk-core.c +22 -8
index 4b45435..5db0772 100644
@@ -569,12 +571,25 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	if (!q)
 		return NULL;
 
-	q->node = node_id;
-	if (blk_init_free_list(q)) {
+	if (blk_init_allocated_queue(q, rfn, lock)) {
+		blk_put_queue(q);
 		kmem_cache_free(blk_requestq_cachep, q);
 		return NULL;
 	}
 
+	return q;
+}
+EXPORT_SYMBOL(blk_init_queue_node);
+
+int blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
+							 spinlock_t *lock)
+{
+	int err = 0;
+
+	err = blk_init_free_list(q);
+	if (err)
+		goto out;
+
 	/*
 	 * if caller didn't supply a lock, they get per-queue locking with
 	 * our embedded lock
@@ -598,15 +613,20 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	/*
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+

block/blk-core.c blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id) · line 632

Quoted 10 lines
block/blk-core.c +5 -1
+free_and_out:
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {

In the previous code blk_init_queue_node() only called blk_put_queue() iff elevator_init() failed.

Why is blk_init_queue_node() now always calling blk_put_queue() on an error from blk_init_allocated_queue()? It could be that blk_init_free_list() was what failed and not elevator_init().

I'd imagine it is because some callers of blk_init_allocated_queue(), e.g. DM, must not have the queue's refcount dropped on failure? A comment on _why_ would really help set the caller's expectations. Maybe at the top of blk_init_allocated_queue()? E.g.:

"It is up to the caller to manage the allocated queue's lifecycle relative to blk_init_allocated_queue() failure". I guess that is obvious after having reviewed this but...

Also, a comment that blk_init_allocated_queue()'s mempool_destroy() is to "cleanup the mempool allocated via blk_init_free_list()" would help.

Thanks,
Mike

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

---

block/blk-core.c +14 -7
Index: linux-2.6/block/blk-core.c
===================================================================
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_no
 	if (!q)
 		return NULL;
 
+	q->node = node_id;
+
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
@@ -604,15 +606,20 @@ int blk_init_allocated_queue(struct requ
 	/*
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+
+free_and_out:
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {
include/linux/blkdev.h +2 -0
Index: linux-2.6/include/linux/blkdev.h
===================================================================
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct reque
 extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
 					spinlock_t *lock, int node_id);
 extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
+extern int blk_init_allocated_queue(struct request_queue *q,
+				request_fn_proc *rfn, spinlock_t *lock);
 extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);
#2 lore raw
Quoted 11 lines · Mike Snitzer
Quoted 8 lines · Nikanth Karthikesan

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

Quoted 6 lines
Quoted 1 line

This patch needed to be refreshed to account for the changes from this recent commit: a4e7d46407d73f35d217013b363b79a8f8eafcaa

I've attached a refreshed patch.

Err, I dropped a hunk in the process... here is the refreshed patch inlined. Figured I'd save you the busy-work for when you get V2 together.

Quoted 1 line

Though I still have questions/feedback below.

Hopefully V2 will address my questions/feedback.

Regards,
Mike

---

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

---

block/blk-core.c +29 -9
Index: linux-2.6/block/blk-core.c
===================================================================
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_no
 	if (!q)
 		return NULL;
 
+	q->node = node_id;
+
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
@@ -569,12 +571,25 @@ blk_init_queue_node(request_fn_proc *rfn
 	if (!q)
 		return NULL;
 
-	q->node = node_id;
-	if (blk_init_free_list(q)) {
+	if (blk_init_allocated_queue(q, rfn, lock)) {
+		blk_put_queue(q);
 		kmem_cache_free(blk_requestq_cachep, q);
 		return NULL;
 	}
 
+	return q;
+}
+EXPORT_SYMBOL(blk_init_queue_node);
+
+int blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
+			     spinlock_t *lock)
+{
+	int err = 0;
+
+	err = blk_init_free_list(q);
+	if (err)
+		goto out;
+
 	q->request_fn		= rfn;
 	q->prep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
@@ -591,15 +606,20 @@ blk_init_queue_node(request_fn_proc *rfn
 	/*
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+
+free_and_out:
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {
include/linux/blkdev.h +2 -0
Index: linux-2.6/include/linux/blkdev.h
===================================================================
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct reque
 extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
 					spinlock_t *lock, int node_id);
 extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
+extern int blk_init_allocated_queue(struct request_queue *q,
+				request_fn_proc *rfn, spinlock_t *lock);
 extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);
#3 lore raw
In reply to Mike Snitzer
Quoted 11 lines · On Saturday 08 August 2009 21:12:40 Mike Snitzer wrote:

On Sat, Aug 08 2009 at 12:55am -0400,

Quoted 8 lines · Nikanth Karthikesan

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

Quoted 7 lines
Quoted 1 line

This patch needed to be refreshed to account for the changes from this recent commit: a4e7d46407d73f35d217013b363b79a8f8eafcaa

I've attached a refreshed patch.

Thanks.

Quoted 66 lines

Though I still have questions/feedback below.

Quoted 50 lines
block/blk-core.c +14 -2
index 4b45435..5db0772 100644
@@ -569,12 +571,25 @@ blk_init_queue_node(request_fn_proc *rfn,
 spinlock_t *lock, int node_id) if (!q)
 		return NULL;
 
-	q->node = node_id;
-	if (blk_init_free_list(q)) {
+	if (blk_init_allocated_queue(q, rfn, lock)) {
+		blk_put_queue(q);
 		kmem_cache_free(blk_requestq_cachep, q);
 		return NULL;
 	}
 
+	return q;
+}
+EXPORT_SYMBOL(blk_init_queue_node);
+
+int blk_init_allocated_queue(struct request_queue *q, request_fn_proc
 *rfn, +							 spinlock_t *lock)
+{
+	int err = 0;
+
+	err = blk_init_free_list(q);
+	if (err)
+		goto out;
+
 	/*
 	 * if caller didn't supply a lock, they get per-queue locking with
 	 * our embedded lock
@@ -598,15 +613,20 @@ blk_init_queue_node(request_fn_proc *rfn,

spinlock_t *lock, int node_id) /*

block/blk-core.c +7 -6
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+

block/blk-core.c int blk_init_allocated_queue(struct requ · line 625

Quoted 10 lines
block/blk-core.c +5 -1
+free_and_out:
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {

In the previous code blk_init_queue_node() only called blk_put_queue() iff elevator_init() failed.

Quoted 4 lines

Why is blk_init_queue_node() now always calling blk_put_queue() on an error from blk_init_allocated_queue()? It could be that blk_init_free_list() was what failed and not elevator_init().

I think, it was a bug on not calling blk_put_queue() even when blk_init_free_list() failed which would be fixed now.

Quoted 5 lines

I'd imagine it is because some callers of blk_init_allocated_queue(), e.g. DM, must not have the queue's refcount dropped on failure? A comment on _why_ would really help set the caller's expectations. Maybe at the top of blk_init_allocated_queue()? E.g.:

Quoted 7 lines

"It is up to the caller to manage the allocated queue's lifecycle relative to blk_init_allocated_queue() failure". I guess that is obvious after having reviewed this but...

Also, a comment that blk_init_allocated_queue()'s mempool_destroy() is to "cleanup the mempool allocated via blk_init_free_list()" would help.

Will add the comment when I resend the patch.

Thanks for reviewing.

Thanks
Nikanth

#4 lore raw

PATCH v2 1/2 Allow delaying initialization of queue after allocation

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

This slightly changes the behaviour of block_init_queue_node() such that blk_put_queue() would be called, even if blk_init_free_list() fails.

---

block/blk-core.c +32 -9
index e3299a7..8b05b3b 100644
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	if (!q)
 		return NULL;
 
+	q->node = node_id;
+
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
@@ -569,12 +571,25 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	if (!q)
 		return NULL;
 
-	q->node = node_id;
-	if (blk_init_free_list(q)) {
+	if (blk_init_allocated_queue(q, rfn, lock)) {
+		blk_put_queue(q);
 		kmem_cache_free(blk_requestq_cachep, q);
 		return NULL;
 	}
 
+	return q;
+}
+EXPORT_SYMBOL(blk_init_queue_node);
+
+int blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
+							spinlock_t *lock)
+{
+	int err = 0;
+
+	err = blk_init_free_list(q);
+	if (err)
+		goto out;
+
 	q->request_fn		= rfn;
 	q->prep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
@@ -591,15 +606,23 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	/*
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+
+free_and_out:
+	/*
+	 * Cleanup mempool allocated by blk_init_free_list
+	 */
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {
include/linux/blkdev.h +2 -0
index 69103e0..4a26fc1 100644
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct request_queue *);
 extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
 					spinlock_t *lock, int node_id);
 extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
+extern int blk_init_allocated_queue(struct request_queue *q,
+				request_fn_proc *rfn, spinlock_t *lock);
 extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);
#5 lore raw

PATCH v3 1/2 Allow delaying initialization of queue after allocation

Export a way to delay initializing a request_queue after allocating it. This is needed by device-mapper devices, as they create the queue on device creation time, but they decide whether it would use the elevator and requests only after first successful table load. Only request-based dm-devices use the elevator and requests. Without this either one needs to initialize and free the mempool and elevator, if it was a bio-based dm-device or leave it allocated, as it is currently done.

This slightly changes the behaviour of block_init_queue_node() such that blk_put_queue() would be called, even if blk_init_free_list() fails.

Also export elevator_register_queue() to modules.

---

block/blk-core.c +32 -9
index e3299a7..8b05b3b 100644
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	if (!q)
 		return NULL;
 
+	q->node = node_id;
+
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
@@ -569,12 +571,25 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	if (!q)
 		return NULL;
 
-	q->node = node_id;
-	if (blk_init_free_list(q)) {
+	if (blk_init_allocated_queue(q, rfn, lock)) {
+		blk_put_queue(q);
 		kmem_cache_free(blk_requestq_cachep, q);
 		return NULL;
 	}
 
+	return q;
+}
+EXPORT_SYMBOL(blk_init_queue_node);
+
+int blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
+							spinlock_t *lock)
+{
+	int err = 0;
+
+	err = blk_init_free_list(q);
+	if (err)
+		goto out;
+
 	q->request_fn		= rfn;
 	q->prep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
@@ -591,15 +606,23 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
 	/*
 	 * all done
 	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	err = elevator_init(q, NULL);
+	if (err)
+		goto free_and_out;
 
-	blk_put_queue(q);
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	return 0;
+
+free_and_out:
+	/*
+	 * Cleanup mempool allocated by blk_init_free_list
+	 */
+	mempool_destroy(q->rq.rq_pool);
+out:
+	return err;
 }
-EXPORT_SYMBOL(blk_init_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
 
 int blk_get_queue(struct request_queue *q)
 {
block/elevator.c +1 -0
index 2d511f9..0827cd3 100644
@@ -930,6 +930,7 @@ int elv_register_queue(struct request_queue *q)
 	}
 	return error;
 }
+EXPORT_SYMBOL(elv_register_queue);
 
 static void __elv_unregister_queue(struct elevator_queue *e)
 {
include/linux/blkdev.h +2 -0
index 69103e0..4a26fc1 100644
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct request_queue *);
 extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
 					spinlock_t *lock, int node_id);
 extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
+extern int blk_init_allocated_queue(struct request_queue *q,
+				request_fn_proc *rfn, spinlock_t *lock);
 extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);