We refcount the request_queue right now towards the end of the
__device_add_disk(), however when we add error handling on this
function we'll want to refcount the request_queue first, to help
make less complicated changes on drivers on their error paths.
For instance, today a driver may call add_disk without error handling
but still handle other errors:
int foo_block_add(...)
{
...
queue = blk_mq_init_queue(...);
...
disk->queue = queue;
disk = alloc_disk(...);
if (!disk)
goto out_free_queue;
...
add_disk(disk);
...
return 0;
out_free_queue:
blk_cleanup_queue(queue);
/* Note: we never call put_disk() as add_disk() never failed */
...
}
We want drivers to cleanup with put_disk() on the error path if
add_disk() fails. However, calling blk_cleanup_queue() will already
put the queue, and so the last put_disk() on the error path will
be extra. This can be simplified later if once error handling is
added to __device_add_disk(), if refcounting the request_queue
fails right away on __device_add_disk() we just return early and
set disk->NULL for the driver. That would ensure driver error
paths chug on with their error paths, and all they'd need to
expand with is the missing put_disk().
The collateral evolution for adding error paths for add_disk() becomes
larger with the alternative of replacing the blk_cleanup_queue() with
a put_disk(). We'd still need to sprinkle then some blk_cleanup_queue()
calls on the driver paths up above prior to add_disk(). And how would
we know we reached a part of add_disk() which did refcount then?
A related commit is 5a0ec388ef0 ("pktcdvd: Fix pkt_setup_dev() error
path") which *had* to take the approach of removing the blk_cleanup_queue()
because otherwise the driver crashes.
Moving this to the top ensure our future error path can easily just
handle this itself. For instance, if it was not able to refcount the
request_queue it can disk->queue to NULL, that way allowing a
blk_cleanup_queue() call followed but a put_disk(). And if the
refcount was incremented, we'd still be able to keep the same error
path of blk_cleanup_queue() followed by put_disk().
Signed-off-by: Luis Chamberlain <[email protected]>
---
block/genhd.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 39ca97b0edc6..51dff87c4756 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -504,6 +504,12 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
dev_t devt;
int retval;
+ /*
+ * Take an extra ref on queue which will be put on disk_release()
+ * so that it sticks around as long as @disk is there.
+ */
+ WARN_ON_ONCE(!blk_get_queue(disk->queue));
+
/*
* The disk queue should now be all set with enough information about
* the device for the elevator code to pick an adequate default
@@ -556,12 +562,6 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
if (register_queue)
blk_register_queue(disk);
- /*
- * Take an extra ref on queue which will be put on disk_release()
- * so that it sticks around as long as @disk is there.
- */
- WARN_ON_ONCE(!blk_get_queue(disk->queue));
-
disk_add_events(disk);
blk_integrity_add(disk);
}
--
2.30.2
On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> We refcount the request_queue right now towards the end of the
> __device_add_disk(), however when we add error handling on this
> function we'll want to refcount the request_queue first, to help
> make less complicated changes on drivers on their error paths.
>
> For instance, today a driver may call add_disk without error handling
> but still handle other errors:
>
> int foo_block_add(...)
> {
> ...
> queue = blk_mq_init_queue(...);
> ...
> disk->queue = queue;
> disk = alloc_disk(...);
> if (!disk)
> goto out_free_queue;
> ...
> add_disk(disk);
> ...
> return 0;
>
> out_free_queue:
> blk_cleanup_queue(queue);
> /* Note: we never call put_disk() as add_disk() never failed */
> ...
> }
>
> We want drivers to cleanup with put_disk() on the error path if
> add_disk() fails. However, calling blk_cleanup_queue() will already
> put the queue, and so the last put_disk() on the error path will
> be extra. This can be simplified later if once error handling is
> added to __device_add_disk(), if refcounting the request_queue
> fails right away on __device_add_disk() we just return early and
> set disk->NULL for the driver. That would ensure driver error
> paths chug on with their error paths, and all they'd need to
> expand with is the missing put_disk().
>
> The collateral evolution for adding error paths for add_disk() becomes
> larger with the alternative of replacing the blk_cleanup_queue() with
> a put_disk(). We'd still need to sprinkle then some blk_cleanup_queue()
> calls on the driver paths up above prior to add_disk(). And how would
> we know we reached a part of add_disk() which did refcount then?
>
> A related commit is 5a0ec388ef0 ("pktcdvd: Fix pkt_setup_dev() error
> path") which *had* to take the approach of removing the blk_cleanup_queue()
> because otherwise the driver crashes.
>
> Moving this to the top ensure our future error path can easily just
> handle this itself. For instance, if it was not able to refcount the
> request_queue it can disk->queue to NULL, that way allowing a
> blk_cleanup_queue() call followed but a put_disk(). And if the
> refcount was incremented, we'd still be able to keep the same error
> path of blk_cleanup_queue() followed by put_disk().
>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> block/genhd.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer