Re: [PATCH 08/15] block: pass a queue_limits argument to blk_alloc_queue

John Garry
2024-01-25T09:45:51Z

On 22/01/2024 17:36, Christoph Hellwig wrote:
> Pass a queue_limits to blk_alloc_queue and apply it if non-NULL. This
> will allow allocating queues with valid queue limits instead of setting
> the values one at a time later.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/blk-core.c | 28 +++++++++++++++++++++-------
> block/blk-mq.c | 6 +++---
> block/blk.h | 2 +-
> block/genhd.c | 4 ++--
> 4 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 09f4a44a4aa3cc..9f1af8fba4dcd2 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -393,9 +393,10 @@ static void blk_timeout_work(struct work_struct *work)
> {
> }
>
> -struct request_queue *blk_alloc_queue(int node_id)
> +struct request_queue *blk_alloc_queue(struct queue_limits *lim, int node_id)
> {
> struct request_queue *q;
> + int error;
>
> q = kmem_cache_alloc_node(blk_requestq_cachep, GFP_KERNEL | __GFP_ZERO,
> node_id);
> @@ -404,13 +405,26 @@ struct request_queue *blk_alloc_queue(int node_id)

Is there actually an issue in that blk_alloc_queue() can return NULL,
and we should be checking IS_ERR_OR_NULL() in the callers?

I don't think that IS_ERR() picks up on NULL pointers, right?

Or make this change:

diff --git a/block/blk-core.c b/block/blk-core.c
index 76cd797d9712..a447b0501e82 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -401,7 +401,7 @@ struct request_queue *blk_alloc_queue(struct
queue_limits *lim, int node_id)
q = kmem_cache_alloc_node(blk_requestq_cachep, GFP_KERNEL |
__GFP_ZERO,
node_id);
if (!q)
- return NULL;
+ return ERR_PTR(-ENOMEM);

q->last_merge = NULL;

>
> q->last_merge = NULL;
>
> + if (lim) {
> + error = blk_validate_limits(lim);

nit: This is only ever going to return -EINVAL or 0 by its very nature,
right? I suppose that it could return a bool and we do the conversion to
EINVAL here. It's a personal taste thing, I suppose.

> + if (error)
> + goto fail_q;
> + q->limits = *lim;

nit: It might be neater to do this in blk_validate_limits()

> + } else {
> + blk_set_default_limits(&q->limits);
> + }
> +
> q->id = ida_alloc(&blk_queue_ida, GFP_KERNEL);
> - if (q->id < 0)
> + if (q->id < 0) {
> + error = q->id;
> goto fail_q;
> + }
>
> q->stats = blk_alloc_queue_stats();
> - if (!q->stats)
> + if (!q->stats) {
> + error = -ENOMEM;
> goto fail_id;
> + }
>
> q->node = node_id;
>
> @@ -435,12 +449,12 @@ struct request_queue *blk_alloc_queue(int node_id)
> * Init percpu_ref in atomic mode so that it's faster to shutdown.
> * See blk_register_queue() for details.
> */
> - if (percpu_ref_init(&q->q_usage_counter,
> + error = percpu_ref_init(&q->q_usage_counter,
> blk_queue_usage_counter_release,
> - PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
> + PERCPU_REF_INIT_ATOMIC, GFP_KERNEL);
> + if (error)
> goto fail_stats;
>
> - blk_set_default_limits(&q->limits);
> q->nr_requests = BLKDEV_DEFAULT_RQ;
>
> return q;
> @@ -451,7 +465,7 @@ struct request_queue *blk_alloc_queue(int node_id)
> ida_free(&blk_queue_ida, q->id);
> fail_q:
> kmem_cache_free(blk_requestq_cachep, q);
> - return NULL;
> + return ERR_PTR(error);
> }
>
> /**
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index aa87fcfda1ecfc..2ddbefdeae93e4 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -4092,9 +4092,9 @@ static struct request_queue *blk_mq_init_queue_data(struct blk_mq_tag_set *set,
> struct request_queue *q;
> int ret;
>
> - q = blk_alloc_queue(set->numa_node);
> - if (!q)
> - return ERR_PTR(-ENOMEM);
> + q = blk_alloc_queue(NULL, set->numa_node);
> + if (IS_ERR(q))
> + return q;
> q->queuedata = queuedata;
> ret = blk_mq_init_allocated_queue(set, q);
> if (ret) {
> diff --git a/block/blk.h b/block/blk.h
> index 58b5dbac2a487d..100c7a02854bfd 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -448,7 +448,7 @@ static inline void bio_release_page(struct bio *bio, struct page *page)
> }
>
> int blk_validate_limits(struct queue_limits *lim);
> -struct request_queue *blk_alloc_queue(int node_id);
> +struct request_queue *blk_alloc_queue(struct queue_limits *lim, int node_id);
>
> int disk_scan_partitions(struct gendisk *disk, blk_mode_t mode);
>
> diff --git a/block/genhd.c b/block/genhd.c
> index d74fb5b4ae6818..defcd35b421bdd 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1396,8 +1396,8 @@ struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass)
> struct request_queue *q;
> struct gendisk *disk;
>
> - q = blk_alloc_queue(node);
> - if (!q)
> + q = blk_alloc_queue(NULL, node);
> + if (IS_ERR(q))
> return NULL;
>
> disk = __alloc_disk_node(q, node, lkclass);