[PATCHSET 0/2 v3] Misc block cleanups

[PATCHSET 0/2 v3] Misc block cleanups

[Jens Axboe]

Hi,

First patch avoids setting up an io_context for the cases where we don't
need them, and the second patch prunes a huge chunk of memory in the
request_queue and makes it dynamic instead.

Since v2:
- Use cmpxcgh() for poll_stat install
- Move poll_stat alloc + enable into blk-stat
- Remove now dead export

[PATCH 1/2] block: only allocate poll_stats if there's a user of them

[Jens Axboe]

This is essentially never used, yet it's about 1/3rd of the total
queue size. Allocate it when needed, and don't embed it in the queue.

Kill the queue flag for this while at it, since we can just check the
assigned pointer now.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq-debugfs.c |  1 -
 block/blk-mq.c         | 10 ++++------
 block/blk-stat.c       | 18 ++++++++++++++++++
 block/blk-stat.h       |  1 +
 block/blk-sysfs.c      |  3 ++-
 include/linux/blkdev.h |  3 +--
 6 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 4f2cf8399f3d..f4022b198580 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -122,7 +122,6 @@ static const char *const blk_queue_flag_name[] = {
 	QUEUE_FLAG_NAME(FUA),
 	QUEUE_FLAG_NAME(DAX),
 	QUEUE_FLAG_NAME(STATS),
-	QUEUE_FLAG_NAME(POLL_STATS),
 	QUEUE_FLAG_NAME(REGISTERED),
 	QUEUE_FLAG_NAME(QUIESCED),
 	QUEUE_FLAG_NAME(PCI_P2PDMA),
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4c00b22590cc..9d29245511ec 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4580,11 +4580,10 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 /* Enable polling stats and return whether they were already enabled. */
 static bool blk_poll_stats_enable(struct request_queue *q)
 {
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) ||
-	    blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q))
+	if (q->poll_stat)
 		return true;
-	blk_stat_add_callback(q, q->poll_cb);
-	return false;
+
+	return blk_stats_alloc_enable(q);
 }
 
 static void blk_mq_poll_stats_start(struct request_queue *q)
@@ -4593,8 +4592,7 @@ static void blk_mq_poll_stats_start(struct request_queue *q)
 	 * We don't arm the callback if polling stats are not enabled or the
 	 * callback is already active.
 	 */
-	if (!test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) ||
-	    blk_stat_is_active(q->poll_cb))
+	if (!q->poll_stat || blk_stat_is_active(q->poll_cb))
 		return;
 
 	blk_stat_activate_msecs(q->poll_cb, 100);
diff --git a/block/blk-stat.c b/block/blk-stat.c
index ae3dd1fb8e61..1b927b6e49bc 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -219,3 +219,21 @@ void blk_free_queue_stats(struct blk_queue_stats *stats)
 
 	kfree(stats);
 }
+
+bool blk_stats_alloc_enable(struct request_queue *q)
+{
+	struct blk_rq_stat *poll_stat;
+
+	poll_stat = kcalloc(BLK_MQ_POLL_STATS_BKTS, sizeof(*poll_stat),
+				GFP_ATOMIC);
+	if (!poll_stat)
+		return false;
+
+	if (cmpxchg(&q->poll_stat, poll_stat, NULL) != poll_stat) {
+		kfree(poll_stat);
+		return true;
+	}
+
+	blk_stat_add_callback(q, q->poll_cb);
+	return false;
+}
diff --git a/block/blk-stat.h b/block/blk-stat.h
index 17b47a86eefb..58f029af49e5 100644
--- a/block/blk-stat.h
+++ b/block/blk-stat.h
@@ -64,6 +64,7 @@ struct blk_stat_callback {
 
 struct blk_queue_stats *blk_alloc_queue_stats(void);
 void blk_free_queue_stats(struct blk_queue_stats *);
+bool blk_stats_alloc_enable(struct request_queue *q);
 
 void blk_stat_add(struct request *rq, u64 now);
 
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index cd75b0f73dc6..c079be1c58a3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -785,11 +785,12 @@ static void blk_release_queue(struct kobject *kobj)
 
 	might_sleep();
 
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
+	if (q->poll_stat)
 		blk_stat_remove_callback(q, q->poll_cb);
 	blk_stat_free_callback(q->poll_cb);
 
 	blk_free_queue_stats(q->stats);
+	kfree(q->poll_stat);
 
 	blk_exit_queue(q);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bd4370baccca..74118e67f649 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -267,7 +267,7 @@ struct request_queue {
 	int			poll_nsec;
 
 	struct blk_stat_callback	*poll_cb;
-	struct blk_rq_stat	poll_stat[BLK_MQ_POLL_STATS_BKTS];
+	struct blk_rq_stat	*poll_stat;
 
 	struct timer_list	timeout;
 	struct work_struct	timeout_work;
@@ -397,7 +397,6 @@ struct request_queue {
 #define QUEUE_FLAG_FUA		18	/* device supports FUA writes */
 #define QUEUE_FLAG_DAX		19	/* device supports DAX */
 #define QUEUE_FLAG_STATS	20	/* track IO start and completion times */
-#define QUEUE_FLAG_POLL_STATS	21	/* collecting stats for hybrid polling */
 #define QUEUE_FLAG_REGISTERED	22	/* queue has been registered to a disk */
 #define QUEUE_FLAG_QUIESCED	24	/* queue has been quiesced */
 #define QUEUE_FLAG_PCI_P2PDMA	25	/* device supports PCI p2p requests */
-- 
2.34.0

Re: [PATCH 1/2] block: only allocate poll_stats if there's a user of them

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] block: only allocate poll_stats if there's a user of them

[Pankaj Raghav]

Hi Jens,

On Tue, Nov 23, 2021 at 12:15:18PM -0700, Jens Axboe wrote:
> +bool blk_stats_alloc_enable(struct request_queue *q)
> +{
> +	struct blk_rq_stat *poll_stat;
> +
> +	poll_stat = kcalloc(BLK_MQ_POLL_STATS_BKTS, sizeof(*poll_stat),
> +				GFP_ATOMIC);
> +	if (!poll_stat)
> +		return false;
> +
> +	if (cmpxchg(&q->poll_stat, poll_stat, NULL) != poll_stat) {
Isn't the logic inverted here? As we already check for non-NULL
q->poll_stat at the caller side, shouldn't it be:

if (cmpxchg(&q->poll_stat, NULL, poll_stat) != NULL) {

> +		kfree(poll_stat);
> +		return true;
> +	}
> +
> -- 
> 2.34.0
> 
Regards,
Pankaj

Re: [PATCH 1/2] block: only allocate poll_stats if there's a user of them

[Jens Axboe]

On 11/24/21 5:52 AM, Pankaj Raghav wrote:
> Hi Jens,
> 
> On Tue, Nov 23, 2021 at 12:15:18PM -0700, Jens Axboe wrote:
>> +bool blk_stats_alloc_enable(struct request_queue *q)
>> +{
>> +	struct blk_rq_stat *poll_stat;
>> +
>> +	poll_stat = kcalloc(BLK_MQ_POLL_STATS_BKTS, sizeof(*poll_stat),
>> +				GFP_ATOMIC);
>> +	if (!poll_stat)
>> +		return false;
>> +
>> +	if (cmpxchg(&q->poll_stat, poll_stat, NULL) != poll_stat) {
> Isn't the logic inverted here? As we already check for non-NULL
> q->poll_stat at the caller side, shouldn't it be:
> 
> if (cmpxchg(&q->poll_stat, NULL, poll_stat) != NULL) {

Yes, I did fix that one up right after sending this out, here's the
patch:

https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.17/block&id=987567fece249eabb14406e4e52f427e679d8461

-- 
Jens Axboe

[PATCH 2/2] block: move io_context creation into where it's needed

[Jens Axboe]

The only user of the io_context for IO is BFQ, yet we put the checking
and logic of it into the normal IO path.

Put the creation into blk_mq_sched_assign_ioc(), and have BFQ use that
helper.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/bfq-iosched.c  | 2 ++
 block/blk-core.c     | 9 ---------
 block/blk-mq-sched.c | 5 +++++
 block/blk-mq.c       | 3 ---
 4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index fec18118dc30..1ce1a99a7160 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6573,6 +6573,8 @@ static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd,
  */
 static void bfq_prepare_request(struct request *rq)
 {
+	blk_mq_sched_assign_ioc(rq);
+
 	/*
 	 * Regardless of whether we have an icq attached, we have to
 	 * clear the scheduler pointers, as they might point to
diff --git a/block/blk-core.c b/block/blk-core.c
index 6443f2dfe43e..6ae8297b033f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -750,15 +750,6 @@ noinline_for_stack bool submit_bio_checks(struct bio *bio)
 		break;
 	}
 
-	/*
-	 * Various block parts want %current->io_context, so allocate it up
-	 * front rather than dealing with lots of pain to allocate it only
-	 * where needed. This may fail and the block layer knows how to live
-	 * with it.
-	 */
-	if (unlikely(!current->io_context))
-		create_task_io_context(current, GFP_ATOMIC, q->node);
-
 	if (blk_throtl_bio(bio))
 		return false;
 
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index ba21449439cc..b942b38000e5 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -24,6 +24,10 @@ void blk_mq_sched_assign_ioc(struct request *rq)
 	struct io_context *ioc;
 	struct io_cq *icq;
 
+	/* create task io_context, if we don't have one already */
+	if (unlikely(!current->io_context))
+		create_task_io_context(current, GFP_ATOMIC, q->node);
+
 	/*
 	 * May not have an IO context if it's a passthrough request
 	 */
@@ -43,6 +47,7 @@ void blk_mq_sched_assign_ioc(struct request *rq)
 	get_io_context(icq->ioc);
 	rq->elv.icq = icq;
 }
+EXPORT_SYMBOL_GPL(blk_mq_sched_assign_ioc);
 
 /*
  * Mark a hardware queue as needing a restart. For shared queues, maintain
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9d29245511ec..b9ca76fb1a03 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -406,9 +406,6 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 
 		if (!op_is_flush(data->cmd_flags) &&
 		    e->type->ops.prepare_request) {
-			if (e->type->icq_cache)
-				blk_mq_sched_assign_ioc(rq);
-
 			e->type->ops.prepare_request(rq);
 			rq->rq_flags |= RQF_ELVPRIV;
 		}
-- 
2.34.0

Re: [PATCH 2/2] block: move io_context creation into where it's needed

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHSET 0/2 v3] Misc block cleanups

[Johannes Thumshirn]

On 23/11/2021 20:15, Jens Axboe wrote:
> Hi,
> 
> First patch avoids setting up an io_context for the cases where we don't
> need them, and the second patch prunes a huge chunk of memory in the
> request_queue and makes it dynamic instead.
> 
> Since v2:
> - Use cmpxcgh() for poll_stat install
> - Move poll_stat alloc + enable into blk-stat
> - Remove now dead export
> 
> 
> 

For the whole series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>