[PATCHSET 0/3 v2] Misc block cleanups

[PATCHSET 0/3 v2] Misc block cleanups

[Jens Axboe]

Hi,

First patch avoids setting up an io_context for the cases where we don't
need them, second patch optimizes re-writing to the bio if not needed, and
prunes a huge chunk of memory in the request_queue and makes it dynamic
instead.

Since v1:
- Get rid of QUEUE_FLAG_POLL_STATS
- Use kcalloc() for poll_stats
- Move io_context creation into blk-mq-sched helper

-- 
Jens Axboe

[PATCH 1/3] 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-ioc.c      | 1 +
 block/blk-mq-sched.c | 5 +++++
 block/blk-mq.c       | 3 ---
 5 files changed, 8 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-ioc.c b/block/blk-ioc.c
index 57299f860d41..736e0280d76f 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -286,6 +286,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(create_task_io_context);
 
 /**
  * get_task_io_context - get io_context of a task
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 4c00b22590cc..20a6445f6a01 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

[PATCH 2/3] blk-ioprio: don't set bio priority if not needed

[Jens Axboe]

We don't need to write to the bio if:

1) No ioprio value has ever been assigned to the blkcg
2) We wouldn't anyway, depending on bio and blkcg IO priority

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-ioprio.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 332a07761bf8..2e7f10e1c03f 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -62,6 +62,7 @@ struct ioprio_blkg {
 struct ioprio_blkcg {
 	struct blkcg_policy_data cpd;
 	enum prio_policy	 prio_policy;
+	bool			 prio_set;
 };
 
 static inline struct ioprio_blkg *pd_to_ioprio(struct blkg_policy_data *pd)
@@ -112,7 +113,7 @@ static ssize_t ioprio_set_prio_policy(struct kernfs_open_file *of, char *buf,
 	if (ret < 0)
 		return ret;
 	blkcg->prio_policy = ret;
-
+	blkcg->prio_set = true;
 	return nbytes;
 }
 
@@ -190,6 +191,10 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
 			       struct bio *bio)
 {
 	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
+	u16 prio;
+
+	if (!blkcg->prio_set)
+		return;
 
 	/*
 	 * Except for IOPRIO_CLASS_NONE, higher I/O priority numbers
@@ -199,8 +204,10 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
 	 * bio I/O priority is not modified. If the bio I/O priority equals
 	 * IOPRIO_CLASS_NONE, the cgroup I/O priority is assigned to the bio.
 	 */
-	bio->bi_ioprio = max_t(u16, bio->bi_ioprio,
-			       IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));
+	prio = max_t(u16, bio->bi_ioprio,
+			IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));
+	if (prio > bio->bi_ioprio)
+		bio->bi_ioprio = prio;
 }
 
 static void blkcg_ioprio_exit(struct rq_qos *rqos)
-- 
2.34.0

[PATCH 3/3] 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         | 23 +++++++++++++++++++----
 block/blk-stat.c       |  6 ------
 block/blk-sysfs.c      |  3 ++-
 include/linux/blkdev.h | 10 +++++++---
 5 files changed, 28 insertions(+), 15 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 20a6445f6a01..4c59b24690d7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4577,9 +4577,25 @@ 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))
+	struct blk_rq_stat *poll_stat;
+
+	if (q->poll_stat)
 		return true;
+
+	poll_stat = kcalloc(BLK_MQ_POLL_STATS_BKTS, sizeof(*poll_stat),
+				GFP_ATOMIC);
+	if (!poll_stat)
+		return false;
+
+	spin_lock_irq(&q->stats->lock);
+	if (q->poll_stat) {
+		spin_unlock_irq(&q->stats->lock);
+		kfree(poll_stat);
+		return true;
+	}
+	q->poll_stat = poll_stat;
+	spin_unlock_irq(&q->stats->lock);
+
 	blk_stat_add_callback(q, q->poll_cb);
 	return false;
 }
@@ -4590,8 +4606,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..7ba504166d1b 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -12,12 +12,6 @@
 #include "blk-mq.h"
 #include "blk.h"
 
-struct blk_queue_stats {
-	struct list_head callbacks;
-	spinlock_t lock;
-	bool enable_accounting;
-};
-
 void blk_rq_stat_init(struct blk_rq_stat *stat)
 {
 	stat->min = -1ULL;
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..90dac4a67cfc 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -28,10 +28,15 @@ struct blk_flush_queue;
 struct kiocb;
 struct pr_ops;
 struct rq_qos;
-struct blk_queue_stats;
 struct blk_stat_callback;
 struct blk_crypto_profile;
 
+struct blk_queue_stats {
+	struct list_head callbacks;
+	spinlock_t lock;
+	bool enable_accounting;
+};
+
 /* Must be consistent with blk_mq_poll_stats_bkt() */
 #define BLK_MQ_POLL_STATS_BKTS 16
 
@@ -267,7 +272,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 +402,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/3] block: move io_context creation into where it's needed

[Christoph Hellwig]

On Tue, Nov 23, 2021 at 10:10:56AM -0700, Jens Axboe wrote:
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -286,6 +286,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(create_task_io_context);

No need to export this now.

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

[Christoph Hellwig]

> +	spin_lock_irq(&q->stats->lock);
> +	if (q->poll_stat) {
> +		spin_unlock_irq(&q->stats->lock);
> +		kfree(poll_stat);
> +		return true;
> +	}
> +	q->poll_stat = poll_stat;
> +	spin_unlock_irq(&q->stats->lock);

If we'd use a cmpxchg to install the pointer we could keep the
blk_queue_stats definition private.

Re: [PATCH 2/3] blk-ioprio: don't set bio priority if not needed

[Christoph Hellwig]

Looks fine:

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

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

[Jens Axboe]

On 11/23/21 11:50 AM, Christoph Hellwig wrote:
>> +	spin_lock_irq(&q->stats->lock);
>> +	if (q->poll_stat) {
>> +		spin_unlock_irq(&q->stats->lock);
>> +		kfree(poll_stat);
>> +		return true;
>> +	}
>> +	q->poll_stat = poll_stat;
>> +	spin_unlock_irq(&q->stats->lock);
> 
> If we'd use a cmpxchg to install the pointer we could keep the
> blk_queue_stats definition private.

How about we just move this alloc+enable logic into blk-stat.c instead?

-- 
Jens Axboe

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

[Jens Axboe]

On 11/23/21 11:46 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 10:10:56AM -0700, Jens Axboe wrote:
>> --- a/block/blk-ioc.c
>> +++ b/block/blk-ioc.c
>> @@ -286,6 +286,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
>>  
>>  	return ret;
>>  }
>> +EXPORT_SYMBOL_GPL(create_task_io_context);
> 
> No need to export this now.

Indeed, killed.

-- 
Jens Axboe

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

[Christoph Hellwig]

On Tue, Nov 23, 2021 at 11:58:42AM -0700, Jens Axboe wrote:
> On 11/23/21 11:50 AM, Christoph Hellwig wrote:
> >> +	spin_lock_irq(&q->stats->lock);
> >> +	if (q->poll_stat) {
> >> +		spin_unlock_irq(&q->stats->lock);
> >> +		kfree(poll_stat);
> >> +		return true;
> >> +	}
> >> +	q->poll_stat = poll_stat;
> >> +	spin_unlock_irq(&q->stats->lock);
> > 
> > If we'd use a cmpxchg to install the pointer we could keep the
> > blk_queue_stats definition private.
> 
> How about we just move this alloc+enable logic into blk-stat.c instead?

That's a good idea either way.  But I think cmpxchg is much better
for installing a pointer than an unrelated lock.

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

[Jens Axboe]

On 11/23/21 11:59 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 11:58:42AM -0700, Jens Axboe wrote:
>> On 11/23/21 11:50 AM, Christoph Hellwig wrote:
>>>> +	spin_lock_irq(&q->stats->lock);
>>>> +	if (q->poll_stat) {
>>>> +		spin_unlock_irq(&q->stats->lock);
>>>> +		kfree(poll_stat);
>>>> +		return true;
>>>> +	}
>>>> +	q->poll_stat = poll_stat;
>>>> +	spin_unlock_irq(&q->stats->lock);
>>>
>>> If we'd use a cmpxchg to install the pointer we could keep the
>>> blk_queue_stats definition private.
>>
>> How about we just move this alloc+enable logic into blk-stat.c instead?
> 
> That's a good idea either way.  But I think cmpxchg is much better
> for installing a pointer than an unrelated lock.

True, I'll do that while moving it.

-- 
Jens Axboe

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

[Jens Axboe]

On 11/23/21 10:05 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 09:44:18AM -0700, Jens Axboe wrote:
>> I think so:
> 
> I think my eternal enemy in blk-mq-debugfs.c will need an update for
> the flag removal, but otherwise this looks good.

Heh yes, it always evades grep.

-- 
Jens Axboe

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

[Christoph Hellwig]

On Tue, Nov 23, 2021 at 09:44:18AM -0700, Jens Axboe wrote:
> I think so:

I think my eternal enemy in blk-mq-debugfs.c will need an update for
the flag removal, but otherwise this looks good.

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

[Jens Axboe]

On 11/23/21 9:41 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 09:18:13AM -0700, Jens Axboe wrote:
>> 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.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  block/blk-mq.c         | 20 ++++++++++++++++++--
>>  block/blk-stat.c       |  6 ------
>>  block/blk-sysfs.c      |  1 +
>>  include/linux/blkdev.h |  9 +++++++--
>>  4 files changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 20a6445f6a01..cb41c441aa8f 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -4577,9 +4577,25 @@ 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))
>> +	struct blk_rq_stat *poll_stat;
>> +
>> +	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
>>  		return true;
> 
> Can't we replace the checks for QUEUE_FLAG_POLL_STATS with checks for
> q->poll_stat now?

I think so:


diff --git a/block/blk-mq.c b/block/blk-mq.c
index f011fa3ebcc7..af4580bdf931 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4593,7 +4593,7 @@ static bool blk_poll_stats_enable(struct request_queue *q)
 {
 	struct blk_rq_stat *poll_stat;
 
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
+	if (q->poll_stat)
 		return true;
 
 	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
@@ -4602,7 +4602,7 @@ static bool blk_poll_stats_enable(struct request_queue *q)
 		return false;
 
 	spin_lock_irq(&q->stats->lock);
-	if (blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) {
+	if (q->poll_stat) {
 		spin_unlock_irq(&q->stats->lock);
 		kfree(poll_stat);
 		return true;
@@ -4620,8 +4620,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-sysfs.c b/block/blk-sysfs.c
index e1b846ec58cb..c079be1c58a3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -785,7 +785,7 @@ 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);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3d558cb397d5..20cf877d6627 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -414,7 +414,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 */

-- 
Jens Axboe

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

[Christoph Hellwig]

On Tue, Nov 23, 2021 at 09:18:13AM -0700, Jens Axboe wrote:
> 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.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/blk-mq.c         | 20 ++++++++++++++++++--
>  block/blk-stat.c       |  6 ------
>  block/blk-sysfs.c      |  1 +
>  include/linux/blkdev.h |  9 +++++++--
>  4 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 20a6445f6a01..cb41c441aa8f 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -4577,9 +4577,25 @@ 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))
> +	struct blk_rq_stat *poll_stat;
> +
> +	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
>  		return true;

Can't we replace the checks for QUEUE_FLAG_POLL_STATS with checks for
q->poll_stat now?

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

[Jens Axboe]

On 11/23/21 9:21 AM, Johannes Thumshirn wrote:
> On 23/11/2021 17:18, Jens Axboe wrote:
>> +	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
>> +				GFP_ATOMIC);
> 
> Why not kcalloc()?

Sure, can do.

-- 
Jens Axboe

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

[Johannes Thumshirn]

On 23/11/2021 17:18, Jens Axboe wrote:
> +	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
> +				GFP_ATOMIC);

Why not kcalloc()?

[PATCH 3/3] 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.

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

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 20a6445f6a01..cb41c441aa8f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4577,9 +4577,25 @@ 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))
+	struct blk_rq_stat *poll_stat;
+
+	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
 		return true;
+
+	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
+				GFP_ATOMIC);
+	if (!poll_stat)
+		return false;
+
+	spin_lock_irq(&q->stats->lock);
+	if (blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) {
+		spin_unlock_irq(&q->stats->lock);
+		kfree(poll_stat);
+		return true;
+	}
+	q->poll_stat = poll_stat;
+	spin_unlock_irq(&q->stats->lock);
+
 	blk_stat_add_callback(q, q->poll_cb);
 	return false;
 }
diff --git a/block/blk-stat.c b/block/blk-stat.c
index ae3dd1fb8e61..7ba504166d1b 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -12,12 +12,6 @@
 #include "blk-mq.h"
 #include "blk.h"
 
-struct blk_queue_stats {
-	struct list_head callbacks;
-	spinlock_t lock;
-	bool enable_accounting;
-};
-
 void blk_rq_stat_init(struct blk_rq_stat *stat)
 {
 	stat->min = -1ULL;
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index cd75b0f73dc6..e1b846ec58cb 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -790,6 +790,7 @@ static void blk_release_queue(struct kobject *kobj)
 	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..b46fd2a80062 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -28,10 +28,15 @@ struct blk_flush_queue;
 struct kiocb;
 struct pr_ops;
 struct rq_qos;
-struct blk_queue_stats;
 struct blk_stat_callback;
 struct blk_crypto_profile;
 
+struct blk_queue_stats {
+	struct list_head callbacks;
+	spinlock_t lock;
+	bool enable_accounting;
+};
+
 /* Must be consistent with blk_mq_poll_stats_bkt() */
 #define BLK_MQ_POLL_STATS_BKTS 16
 
@@ -267,7 +272,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;
-- 
2.34.0