Allow delaying initialization of queue after allocation
Subsystems (2)
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
@@ -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
@@ -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 );
Quoted 9 lines · Nikanth Karthikesan
Export a way to delay initializing a
request_queueafter 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.Signed-off-by Nikanth Karthikesan <[email protected]>
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@@ -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
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_noif (!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
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct requeextern 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 );
Quoted 11 lines · Mike Snitzer
Quoted 8 lines · Nikanth Karthikesan
Export a way to delay initializing a
request_queueafter 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
Signed-off-by Nikanth Karthikesan <[email protected]>This patch needed to be refreshed to account for the changes from this recent commit:
a4e7d46407d73f35d217013b363b79a8f8eafcaaI'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
@@ -495,6 +495,8 @@ struct request_queue *blk_alloc_queue_noif (!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 *rfnif (!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
@@ -901,6 +901,8 @@ extern void blk_abort_queue(struct requeextern 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 );
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_queueafter 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
Signed-off-by Nikanth Karthikesan <[email protected]>This patch needed to be refreshed to account for the changes from this recent commit:
a4e7d46407d73f35d217013b363b79a8f8eafcaaI'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@@ -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, intnode_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 calledblk_put_queue()iffelevator_init()failed.
Quoted 4 lines
Why is
blk_init_queue_node()now always callingblk_put_queue()on an error fromblk_init_allocated_queue()? It could be thatblk_init_free_list()was what failed and notelevator_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 ofblk_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 viablk_init_free_list()" would help.
Will add the comment when I resend the patch.
Thanks for reviewing.
Thanks
Nikanth
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
@@ -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
@@ -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 );
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
@@ -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
@@ -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
@@ -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 );