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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx>
---
  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);





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux