[PATCH v3 0/3] blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

[PATCH v3 0/3] blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

[chengming.zhou]

From: Chengming Zhou <zhouchengming@bytedance.com>

Hello,

This patchset fix start_time_ns and alloc_time_ns for pre-allocated rq.

patch 1 and 2 is preparation that we ktime_get_ns() outside batched rq init
for batched requests start_time_ns setting.

patch 3 is the fix patch that we set alloc_time_ns and start_time_ns
to now when the pre-allocated rq is actually used.

v3:
 - Skip setting the alloc_time_ns and start_time_ns during pre-allocation,
   which is clearer, as suggested by Tejun.
 - [v2] https://lore.kernel.org/all/20230626050405.781253-1-chengming.zhou@linux.dev/

v2:
 - Let blk_mq_rq_ctx_init() receive start_time_ns for batched time setting.
 - Set alloc_time_ns and start_time_ns when the pre-allocated rq is actually
   used, as suggested by Tejun.
 - [v1] https://lore.kernel.org/all/20230601053919.3639954-1-chengming.zhou@linux.dev/

Chengming Zhou (3):
  blk-mq: always use __blk_mq_alloc_requests() to alloc and init rq
  blk-mq: ktime_get_ns() only once for batched requests init
  blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

 block/blk-mq.c         | 94 ++++++++++++++++++++++++------------------
 include/linux/blk-mq.h |  6 +--
 2 files changed, 56 insertions(+), 44 deletions(-)

-- 
2.39.2

[PATCH v3 1/3] blk-mq: always use __blk_mq_alloc_requests() to alloc and init rq

[chengming.zhou]

From: Chengming Zhou <zhouchengming@bytedance.com>

This patch is preparation for the next patch that ktime_get_ns() only once
for batched pre-allocated requests start_time_ns setting.

1. data->flags is input for blk_mq_rq_ctx_init(), shouldn't update in
   every blk_mq_rq_ctx_init() in batched requests alloc. So put the
   data->flags initialization in the caller.

2. make blk_mq_alloc_request_hctx() to reuse __blk_mq_alloc_requests(),
   instead of directly using blk_mq_rq_ctx_init() by itself, so avoid
   doing the same data->flags initialization in it.

After these cleanup, __blk_mq_alloc_requests() is the only entry to
alloc and init rq.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-mq.c | 46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index decb6ab2d508..c50ef953759f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -349,11 +349,6 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->mq_ctx = ctx;
 	rq->mq_hctx = hctx;
 	rq->cmd_flags = data->cmd_flags;
-
-	if (data->flags & BLK_MQ_REQ_PM)
-		data->rq_flags |= RQF_PM;
-	if (blk_queue_io_stat(q))
-		data->rq_flags |= RQF_IO_STAT;
 	rq->rq_flags = data->rq_flags;
 
 	if (data->rq_flags & RQF_SCHED_TAGS) {
@@ -447,6 +442,15 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 	if (data->cmd_flags & REQ_NOWAIT)
 		data->flags |= BLK_MQ_REQ_NOWAIT;
 
+	if (data->flags & BLK_MQ_REQ_RESERVED)
+		data->rq_flags |= RQF_RESV;
+
+	if (data->flags & BLK_MQ_REQ_PM)
+		data->rq_flags |= RQF_PM;
+
+	if (blk_queue_io_stat(q))
+		data->rq_flags |= RQF_IO_STAT;
+
 	if (q->elevator) {
 		/*
 		 * All requests use scheduler tags when an I/O scheduler is
@@ -471,14 +475,15 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 	}
 
 retry:
-	data->ctx = blk_mq_get_ctx(q);
-	data->hctx = blk_mq_map_queue(q, data->cmd_flags, data->ctx);
+	/* See blk_mq_alloc_request_hctx() for details */
+	if (!data->ctx) {
+		data->ctx = blk_mq_get_ctx(q);
+		data->hctx = blk_mq_map_queue(q, data->cmd_flags, data->ctx);
+	}
+
 	if (!(data->rq_flags & RQF_SCHED_TAGS))
 		blk_mq_tag_busy(data->hctx);
 
-	if (data->flags & BLK_MQ_REQ_RESERVED)
-		data->rq_flags |= RQF_RESV;
-
 	/*
 	 * Try batched alloc if we want more than 1 tag.
 	 */
@@ -505,6 +510,7 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 		 * is going away.
 		 */
 		msleep(3);
+		data->ctx = NULL;
 		goto retry;
 	}
 
@@ -613,16 +619,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 		.cmd_flags	= opf,
 		.nr_tags	= 1,
 	};
-	u64 alloc_time_ns = 0;
 	struct request *rq;
 	unsigned int cpu;
-	unsigned int tag;
 	int ret;
 
-	/* alloc_time includes depth and tag waits */
-	if (blk_queue_rq_alloc_time(q))
-		alloc_time_ns = ktime_get_ns();
-
 	/*
 	 * If the tag allocator sleeps we could get an allocation for a
 	 * different hardware context.  No need to complicate the low level
@@ -653,20 +653,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 		goto out_queue_exit;
 	data.ctx = __blk_mq_get_ctx(q, cpu);
 
-	if (q->elevator)
-		data.rq_flags |= RQF_SCHED_TAGS;
-	else
-		blk_mq_tag_busy(data.hctx);
-
-	if (flags & BLK_MQ_REQ_RESERVED)
-		data.rq_flags |= RQF_RESV;
-
 	ret = -EWOULDBLOCK;
-	tag = blk_mq_get_tag(&data);
-	if (tag == BLK_MQ_NO_TAG)
+	rq = __blk_mq_alloc_requests(&data);
+	if (!rq)
 		goto out_queue_exit;
-	rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
-					alloc_time_ns);
 	rq->__data_len = 0;
 	rq->__sector = (sector_t) -1;
 	rq->bio = rq->biotail = NULL;
-- 
2.39.2

Re: [PATCH v3 1/3] blk-mq: always use __blk_mq_alloc_requests() to alloc and init rq

[Christoph Hellwig]

On Wed, Jun 28, 2023 at 08:45:44PM +0800, chengming.zhou@linux.dev wrote:
> After these cleanup, __blk_mq_alloc_requests() is the only entry to
> alloc and init rq.

I find the code a little hard to follow now, due to the optional
setting of the ctx.  We also introduce really odd behavior here
if the caller for a hctx-specific allocation doesn't have free
tags, as we'll now run into the normal retry path.

Is this really needed for your timestamp changes?  If not I'd prefer
to skip it.

Re: [PATCH v3 1/3] blk-mq: always use __blk_mq_alloc_requests() to alloc and init rq

[Chengming Zhou]

On 2023/6/29 13:28, Christoph Hellwig wrote:
> On Wed, Jun 28, 2023 at 08:45:44PM +0800, chengming.zhou@linux.dev wrote:
>> After these cleanup, __blk_mq_alloc_requests() is the only entry to
>> alloc and init rq.
> 
> I find the code a little hard to follow now, due to the optional
> setting of the ctx.  We also introduce really odd behavior here
> if the caller for a hctx-specific allocation doesn't have free
> tags, as we'll now run into the normal retry path.
> 
> Is this really needed for your timestamp changes?  If not I'd prefer
> to skip it.
> 

Thanks for your review!

Since hctx-specific allocation path always has BLK_MQ_REQ_NOWAIT flag,
it won't retry.

But I agree, this makes the general __blk_mq_alloc_requests() more complex.

The reason is blk_mq_rq_ctx_init() has some data->rq_flags initialization:

```
if (data->flags & BLK_MQ_REQ_PM)
	data->rq_flags |= RQF_PM;
if (blk_queue_io_stat(q))
	data->rq_flags |= RQF_IO_STAT;
rq->rq_flags = data->rq_flags;
```

Because we need this data->rq_flags to tell if we need start_time_ns,
we need to put these initialization in the callers of blk_mq_rq_ctx_init().

Now we basically have two callers, the 1st is general __blk_mq_alloc_requests(),
the 2nd is the special blk_mq_alloc_request_hctx(). So I change the 2nd caller
to reuse the 1st __blk_mq_alloc_requests().

Or we put these data->rq_flags initialization in blk_mq_alloc_request_hctx() too?

Thanks.

Re: [PATCH v3 1/3] blk-mq: always use __blk_mq_alloc_requests() to alloc and init rq

[Christoph Hellwig]

On Thu, Jun 29, 2023 at 03:40:03PM +0800, Chengming Zhou wrote:
> Thanks for your review!
> 
> Since hctx-specific allocation path always has BLK_MQ_REQ_NOWAIT flag,
> it won't retry.
> 
> But I agree, this makes the general __blk_mq_alloc_requests() more complex.

And also very confusing as it pretends to share some code, while almost
nothing of __blk_mq_alloc_requests is actually used.

> The reason is blk_mq_rq_ctx_init() has some data->rq_flags initialization:
> 
> ```
> if (data->flags & BLK_MQ_REQ_PM)
> 	data->rq_flags |= RQF_PM;
> if (blk_queue_io_stat(q))
> 	data->rq_flags |= RQF_IO_STAT;
> rq->rq_flags = data->rq_flags;
> ```
> 
> Because we need this data->rq_flags to tell if we need start_time_ns,
> we need to put these initialization in the callers of blk_mq_rq_ctx_init().

Why can't we just always initialize the time stampts after
blk_mq_rq_ctx_init? Something like this (untested) variant of your
patch 2 from the latest iteration:

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5504719b970d59..55bf1009f3e32a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -328,8 +328,26 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 }
 EXPORT_SYMBOL(blk_rq_init);
 
+/* Set alloc and start time when pre-allocated rq is actually used */
+static inline void blk_mq_rq_time_init(struct request *rq, bool set_alloc_time)
+{
+	if (blk_mq_need_time_stamp(rq)) {
+		u64 now = ktime_get_ns();
+
+#ifdef CONFIG_BLK_RQ_ALLOC_TIME
+		/*
+		 * The alloc time is only used by iocost for now,
+		 * only possible when blk_mq_need_time_stamp().
+		 */
+		if (set_alloc_time)
+			rq->alloc_time_ns = now;
+#endif
+		rq->start_time_ns = now;
+	}
+}
+
 static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
-		struct blk_mq_tags *tags, unsigned int tag, u64 alloc_time_ns)
+		struct blk_mq_tags *tags, unsigned int tag)
 {
 	struct blk_mq_ctx *ctx = data->ctx;
 	struct blk_mq_hw_ctx *hctx = data->hctx;
@@ -356,14 +374,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	}
 	rq->timeout = 0;
 
-	if (blk_mq_need_time_stamp(rq))
-		rq->start_time_ns = ktime_get_ns();
-	else
-		rq->start_time_ns = 0;
 	rq->part = NULL;
-#ifdef CONFIG_BLK_RQ_ALLOC_TIME
-	rq->alloc_time_ns = alloc_time_ns;
-#endif
 	rq->io_start_time_ns = 0;
 	rq->stats_sectors = 0;
 	rq->nr_phys_segments = 0;
@@ -393,8 +404,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 }
 
 static inline struct request *
-__blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
-		u64 alloc_time_ns)
+__blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data)
 {
 	unsigned int tag, tag_offset;
 	struct blk_mq_tags *tags;
@@ -413,7 +423,7 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 		tag = tag_offset + i;
 		prefetch(tags->static_rqs[tag]);
 		tag_mask &= ~(1UL << i);
-		rq = blk_mq_rq_ctx_init(data, tags, tag, alloc_time_ns);
+		rq = blk_mq_rq_ctx_init(data, tags, tag);
 		rq_list_add(data->cached_rq, rq);
 		nr++;
 	}
@@ -427,12 +437,13 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 {
 	struct request_queue *q = data->q;
+	bool set_alloc_time = blk_queue_rq_alloc_time(q);
 	u64 alloc_time_ns = 0;
 	struct request *rq;
 	unsigned int tag;
 
 	/* alloc_time includes depth and tag waits */
-	if (blk_queue_rq_alloc_time(q))
+	if (set_alloc_time)
 		alloc_time_ns = ktime_get_ns();
 
 	if (data->cmd_flags & REQ_NOWAIT)
@@ -474,9 +485,11 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 	 * Try batched alloc if we want more than 1 tag.
 	 */
 	if (data->nr_tags > 1) {
-		rq = __blk_mq_alloc_requests_batch(data, alloc_time_ns);
-		if (rq)
+		rq = __blk_mq_alloc_requests_batch(data);
+		if (rq) {
+			blk_mq_rq_time_init(rq, true);
 			return rq;
+		}
 		data->nr_tags = 1;
 	}
 
@@ -499,8 +512,10 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 		goto retry;
 	}
 
-	return blk_mq_rq_ctx_init(data, blk_mq_tags_from_data(data), tag,
-					alloc_time_ns);
+	rq = blk_mq_rq_ctx_init(data, blk_mq_tags_from_data(data), tag);
+	if (rq)
+		blk_mq_rq_time_init(rq, set_alloc_time);
+	return rq;
 }
 
 static struct request *blk_mq_rq_cache_fill(struct request_queue *q,
@@ -555,6 +570,7 @@ static struct request *blk_mq_alloc_cached_request(struct request_queue *q,
 			return NULL;
 
 		plug->cached_rq = rq_list_next(rq);
+		blk_mq_rq_time_init(rq, blk_queue_rq_alloc_time(rq->q));
 	}
 
 	rq->cmd_flags = opf;
@@ -656,8 +672,8 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 	tag = blk_mq_get_tag(&data);
 	if (tag == BLK_MQ_NO_TAG)
 		goto out_queue_exit;
-	rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
-					alloc_time_ns);
+	rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag);
+	blk_mq_rq_time_init(rq, blk_queue_rq_alloc_time(rq->q));
 	rq->__data_len = 0;
 	rq->__sector = (sector_t) -1;
 	rq->bio = rq->biotail = NULL;
@@ -2896,6 +2912,7 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
 	plug->cached_rq = rq_list_next(rq);
 	rq_qos_throttle(q, *bio);
 
+	blk_mq_rq_time_init(rq, blk_queue_rq_alloc_time(rq->q));
 	rq->cmd_flags = (*bio)->bi_opf;
 	INIT_LIST_HEAD(&rq->queuelist);
 	return rq;

Re: [PATCH v3 1/3] blk-mq: always use __blk_mq_alloc_requests() to alloc and init rq

[Chengming Zhou]

On 2023/7/10 15:36, Christoph Hellwig wrote:
> On Thu, Jun 29, 2023 at 03:40:03PM +0800, Chengming Zhou wrote:
>> Thanks for your review!
>>
>> Since hctx-specific allocation path always has BLK_MQ_REQ_NOWAIT flag,
>> it won't retry.
>>
>> But I agree, this makes the general __blk_mq_alloc_requests() more complex.
> 
> And also very confusing as it pretends to share some code, while almost
> nothing of __blk_mq_alloc_requests is actually used.

You are right.

I will not mess with reusing __blk_mq_alloc_requests() in the next version.

> 
>> The reason is blk_mq_rq_ctx_init() has some data->rq_flags initialization:
>>
>> ```
>> if (data->flags & BLK_MQ_REQ_PM)
>> 	data->rq_flags |= RQF_PM;
>> if (blk_queue_io_stat(q))
>> 	data->rq_flags |= RQF_IO_STAT;
>> rq->rq_flags = data->rq_flags;
>> ```
>>
>> Because we need this data->rq_flags to tell if we need start_time_ns,
>> we need to put these initialization in the callers of blk_mq_rq_ctx_init().
> 
> Why can't we just always initialize the time stampts after
> blk_mq_rq_ctx_init? Something like this (untested) variant of your
> patch 2 from the latest iteration:

I get what you mean: always initialize the two time stamps after blk_mq_rq_ctx_init(),
so we know whether the time stamps are needed in blk_mq_rq_time_init().

It seems better and clearer indeed, I will try to change as you suggest.

> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 5504719b970d59..55bf1009f3e32a 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -328,8 +328,26 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
>  }
>  EXPORT_SYMBOL(blk_rq_init);
>  
> +/* Set alloc and start time when pre-allocated rq is actually used */
> +static inline void blk_mq_rq_time_init(struct request *rq, bool set_alloc_time)

We need to pass "u64 alloc_time_ns" here, which includes depth and tag waits time by definition.

So:
1. for non-batched request that need alloc_time_ns: passed alloc_time_ns != 0
2. for batched request that need alloc_time_ns: passed alloc_time_ns == 0, will be set to start_time_ns

I have just updated the patch:
https://lore.kernel.org/all/20230710105516.2053478-1-chengming.zhou@linux.dev/

Thanks!


> +{
> +	if (blk_mq_need_time_stamp(rq)) {
> +		u64 now = ktime_get_ns();
> +
> +#ifdef CONFIG_BLK_RQ_ALLOC_TIME
> +		/*
> +		 * The alloc time is only used by iocost for now,
> +		 * only possible when blk_mq_need_time_stamp().
> +		 */
> +		if (set_alloc_time)
> +			rq->alloc_time_ns = now;
> +#endif
> +		rq->start_time_ns = now;
> +	}
> +}
> +
>  static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
> -		struct blk_mq_tags *tags, unsigned int tag, u64 alloc_time_ns)
> +		struct blk_mq_tags *tags, unsigned int tag)
>  {
>  	struct blk_mq_ctx *ctx = data->ctx;
>  	struct blk_mq_hw_ctx *hctx = data->hctx;
> @@ -356,14 +374,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>  	}
>  	rq->timeout = 0;
>  
> -	if (blk_mq_need_time_stamp(rq))
> -		rq->start_time_ns = ktime_get_ns();
> -	else
> -		rq->start_time_ns = 0;
>  	rq->part = NULL;
> -#ifdef CONFIG_BLK_RQ_ALLOC_TIME
> -	rq->alloc_time_ns = alloc_time_ns;
> -#endif
>  	rq->io_start_time_ns = 0;
>  	rq->stats_sectors = 0;
>  	rq->nr_phys_segments = 0;
> @@ -393,8 +404,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>  }
>  
>  static inline struct request *
> -__blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
> -		u64 alloc_time_ns)
> +__blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data)
>  {
>  	unsigned int tag, tag_offset;
>  	struct blk_mq_tags *tags;
> @@ -413,7 +423,7 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
>  		tag = tag_offset + i;
>  		prefetch(tags->static_rqs[tag]);
>  		tag_mask &= ~(1UL << i);
> -		rq = blk_mq_rq_ctx_init(data, tags, tag, alloc_time_ns);
> +		rq = blk_mq_rq_ctx_init(data, tags, tag);
>  		rq_list_add(data->cached_rq, rq);
>  		nr++;
>  	}
> @@ -427,12 +437,13 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
>  static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
>  {
>  	struct request_queue *q = data->q;
> +	bool set_alloc_time = blk_queue_rq_alloc_time(q);
>  	u64 alloc_time_ns = 0;
>  	struct request *rq;
>  	unsigned int tag;
>  
>  	/* alloc_time includes depth and tag waits */
> -	if (blk_queue_rq_alloc_time(q))
> +	if (set_alloc_time)
>  		alloc_time_ns = ktime_get_ns();
>  
>  	if (data->cmd_flags & REQ_NOWAIT)
> @@ -474,9 +485,11 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
>  	 * Try batched alloc if we want more than 1 tag.
>  	 */
>  	if (data->nr_tags > 1) {
> -		rq = __blk_mq_alloc_requests_batch(data, alloc_time_ns);
> -		if (rq)
> +		rq = __blk_mq_alloc_requests_batch(data);
> +		if (rq) {
> +			blk_mq_rq_time_init(rq, true);
>  			return rq;
> +		}
>  		data->nr_tags = 1;
>  	}
>  
> @@ -499,8 +512,10 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
>  		goto retry;
>  	}
>  
> -	return blk_mq_rq_ctx_init(data, blk_mq_tags_from_data(data), tag,
> -					alloc_time_ns);
> +	rq = blk_mq_rq_ctx_init(data, blk_mq_tags_from_data(data), tag);
> +	if (rq)
> +		blk_mq_rq_time_init(rq, set_alloc_time);
> +	return rq;
>  }
>  
>  static struct request *blk_mq_rq_cache_fill(struct request_queue *q,
> @@ -555,6 +570,7 @@ static struct request *blk_mq_alloc_cached_request(struct request_queue *q,
>  			return NULL;
>  
>  		plug->cached_rq = rq_list_next(rq);
> +		blk_mq_rq_time_init(rq, blk_queue_rq_alloc_time(rq->q));
>  	}
>  
>  	rq->cmd_flags = opf;
> @@ -656,8 +672,8 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
>  	tag = blk_mq_get_tag(&data);
>  	if (tag == BLK_MQ_NO_TAG)
>  		goto out_queue_exit;
> -	rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
> -					alloc_time_ns);
> +	rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag);
> +	blk_mq_rq_time_init(rq, blk_queue_rq_alloc_time(rq->q));
>  	rq->__data_len = 0;
>  	rq->__sector = (sector_t) -1;
>  	rq->bio = rq->biotail = NULL;
> @@ -2896,6 +2912,7 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>  	plug->cached_rq = rq_list_next(rq);
>  	rq_qos_throttle(q, *bio);
>  
> +	blk_mq_rq_time_init(rq, blk_queue_rq_alloc_time(rq->q));
>  	rq->cmd_flags = (*bio)->bi_opf;
>  	INIT_LIST_HEAD(&rq->queuelist);
>  	return rq;

[PATCH v3 2/3] blk-mq: ktime_get_ns() only once for batched requests init

[chengming.zhou]

From: Chengming Zhou <zhouchengming@bytedance.com>

Extend blk_mq_rq_ctx_init() to receive start_time_ns, so we can
ktime_get_ns() only once for batched requests start_time_ns setting.

Since data->rq_flags initialization has been moved to the caller
__blk_mq_alloc_requests(), we can use it to check if time is needed.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-mq.c         | 23 ++++++++++++++---------
 include/linux/blk-mq.h |  6 +++---
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index c50ef953759f..8b981d0a868e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -338,7 +338,8 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 EXPORT_SYMBOL(blk_rq_init);
 
 static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
-		struct blk_mq_tags *tags, unsigned int tag, u64 alloc_time_ns)
+		struct blk_mq_tags *tags, unsigned int tag,
+		u64 alloc_time_ns, u64 start_time_ns)
 {
 	struct blk_mq_ctx *ctx = data->ctx;
 	struct blk_mq_hw_ctx *hctx = data->hctx;
@@ -360,14 +361,11 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	}
 	rq->timeout = 0;
 
-	if (blk_mq_need_time_stamp(rq))
-		rq->start_time_ns = ktime_get_ns();
-	else
-		rq->start_time_ns = 0;
 	rq->part = NULL;
 #ifdef CONFIG_BLK_RQ_ALLOC_TIME
 	rq->alloc_time_ns = alloc_time_ns;
 #endif
+	rq->start_time_ns = start_time_ns;
 	rq->io_start_time_ns = 0;
 	rq->stats_sectors = 0;
 	rq->nr_phys_segments = 0;
@@ -405,11 +403,15 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 	struct request *rq;
 	unsigned long tag_mask;
 	int i, nr = 0;
+	u64 start_time_ns = 0;
 
 	tag_mask = blk_mq_get_tags(data, data->nr_tags, &tag_offset);
 	if (unlikely(!tag_mask))
 		return NULL;
 
+	if (blk_mq_need_time_stamp(data->rq_flags))
+		start_time_ns = ktime_get_ns();
+
 	tags = blk_mq_tags_from_data(data);
 	for (i = 0; tag_mask; i++) {
 		if (!(tag_mask & (1UL << i)))
@@ -417,7 +419,7 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 		tag = tag_offset + i;
 		prefetch(tags->static_rqs[tag]);
 		tag_mask &= ~(1UL << i);
-		rq = blk_mq_rq_ctx_init(data, tags, tag, alloc_time_ns);
+		rq = blk_mq_rq_ctx_init(data, tags, tag, alloc_time_ns, start_time_ns);
 		rq_list_add(data->cached_rq, rq);
 		nr++;
 	}
@@ -431,7 +433,7 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 {
 	struct request_queue *q = data->q;
-	u64 alloc_time_ns = 0;
+	u64 alloc_time_ns = 0, start_time_ns = 0;
 	struct request *rq;
 	unsigned int tag;
 
@@ -514,8 +516,11 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 		goto retry;
 	}
 
+	if (blk_mq_need_time_stamp(data->rq_flags))
+		start_time_ns = ktime_get_ns();
+
 	return blk_mq_rq_ctx_init(data, blk_mq_tags_from_data(data), tag,
-					alloc_time_ns);
+				  alloc_time_ns, start_time_ns);
 }
 
 static struct request *blk_mq_rq_cache_fill(struct request_queue *q,
@@ -1004,7 +1009,7 @@ static inline void __blk_mq_end_request_acct(struct request *rq, u64 now)
 
 inline void __blk_mq_end_request(struct request *rq, blk_status_t error)
 {
-	if (blk_mq_need_time_stamp(rq))
+	if (blk_mq_need_time_stamp(rq->rq_flags))
 		__blk_mq_end_request_acct(rq, ktime_get_ns());
 
 	if (rq->end_io) {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index f401067ac03a..e8366e9c3388 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -834,9 +834,9 @@ void blk_mq_end_request_batch(struct io_comp_batch *ib);
  * Only need start/end time stamping if we have iostat or
  * blk stats enabled, or using an IO scheduler.
  */
-static inline bool blk_mq_need_time_stamp(struct request *rq)
+static inline bool blk_mq_need_time_stamp(req_flags_t rq_flags)
 {
-	return (rq->rq_flags & (RQF_IO_STAT | RQF_STATS | RQF_USE_SCHED));
+	return (rq_flags & (RQF_IO_STAT | RQF_STATS | RQF_USE_SCHED));
 }
 
 static inline bool blk_mq_is_reserved_rq(struct request *rq)
@@ -860,7 +860,7 @@ static inline bool blk_mq_add_to_batch(struct request *req,
 		iob->complete = complete;
 	else if (iob->complete != complete)
 		return false;
-	iob->need_ts |= blk_mq_need_time_stamp(req);
+	iob->need_ts |= blk_mq_need_time_stamp(req->rq_flags);
 	rq_list_add(&iob->req_list, req);
 	return true;
 }
-- 
2.39.2

Re: [PATCH v3 2/3] blk-mq: ktime_get_ns() only once for batched requests init

[Christoph Hellwig]

Can we just stash the start time into blk_mq_alloc_data instead of
passing down yet another parameter?

Re: [PATCH v3 2/3] blk-mq: ktime_get_ns() only once for batched requests init

[Chengming Zhou]

On 2023/6/29 13:30, Christoph Hellwig wrote:
> Can we just stash the start time into blk_mq_alloc_data instead of
> passing down yet another parameter?
> 

Yes, it's much better.

Thanks.

[PATCH v3 3/3] blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

[chengming.zhou]

From: Chengming Zhou <zhouchengming@bytedance.com>

The iocost rely on rq start_time_ns and alloc_time_ns to tell saturation
state of the block device. Most of the time request is allocated after
rq_qos_throttle() and its alloc_time_ns or start_time_ns won't be affected.

But for plug batched allocation introduced by the commit 47c122e35d7e
("block: pre-allocate requests if plug is started and is a batch"), we can
rq_qos_throttle() after the allocation of the request. This is what the
blk_mq_get_cached_request() does.

In this case, the cached request alloc_time_ns or start_time_ns is much
ahead if blocked in any qos ->throttle().

This patch fix it by setting alloc_time_ns and start_time_ns to now
when the pre-allocated rq is actually used. And we skip setting the
alloc_time_ns and start_time_ns during pre-allocation, so just pass 0
in __blk_mq_alloc_requests_batch().

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-mq.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8b981d0a868e..55a2e600f943 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -337,6 +337,24 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 }
 EXPORT_SYMBOL(blk_rq_init);
 
+/* Set rq alloc and start time when pre-allocated rq is actually used */
+static inline void blk_mq_rq_time_init(struct request_queue *q, struct request *rq)
+{
+	if (blk_mq_need_time_stamp(rq->rq_flags)) {
+		u64 now = ktime_get_ns();
+
+#ifdef CONFIG_BLK_RQ_ALLOC_TIME
+		/*
+		 * alloc time is only used by iocost for now,
+		 * only possible when blk_mq_need_time_stamp().
+		 */
+		if (blk_queue_rq_alloc_time(q))
+			rq->alloc_time_ns = now;
+#endif
+		rq->start_time_ns = now;
+	}
+}
+
 static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		struct blk_mq_tags *tags, unsigned int tag,
 		u64 alloc_time_ns, u64 start_time_ns)
@@ -395,23 +413,18 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 }
 
 static inline struct request *
-__blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
-		u64 alloc_time_ns)
+__blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data)
 {
 	unsigned int tag, tag_offset;
 	struct blk_mq_tags *tags;
 	struct request *rq;
 	unsigned long tag_mask;
 	int i, nr = 0;
-	u64 start_time_ns = 0;
 
 	tag_mask = blk_mq_get_tags(data, data->nr_tags, &tag_offset);
 	if (unlikely(!tag_mask))
 		return NULL;
 
-	if (blk_mq_need_time_stamp(data->rq_flags))
-		start_time_ns = ktime_get_ns();
-
 	tags = blk_mq_tags_from_data(data);
 	for (i = 0; tag_mask; i++) {
 		if (!(tag_mask & (1UL << i)))
@@ -419,7 +432,7 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 		tag = tag_offset + i;
 		prefetch(tags->static_rqs[tag]);
 		tag_mask &= ~(1UL << i);
-		rq = blk_mq_rq_ctx_init(data, tags, tag, alloc_time_ns, start_time_ns);
+		rq = blk_mq_rq_ctx_init(data, tags, tag, 0, 0);
 		rq_list_add(data->cached_rq, rq);
 		nr++;
 	}
@@ -490,9 +503,11 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 	 * Try batched alloc if we want more than 1 tag.
 	 */
 	if (data->nr_tags > 1) {
-		rq = __blk_mq_alloc_requests_batch(data, alloc_time_ns);
-		if (rq)
+		rq = __blk_mq_alloc_requests_batch(data);
+		if (rq) {
+			blk_mq_rq_time_init(q, rq);
 			return rq;
+		}
 		data->nr_tags = 1;
 	}
 
@@ -575,6 +590,7 @@ static struct request *blk_mq_alloc_cached_request(struct request_queue *q,
 			return NULL;
 
 		plug->cached_rq = rq_list_next(rq);
+		blk_mq_rq_time_init(q, rq);
 	}
 
 	rq->cmd_flags = opf;
@@ -2896,6 +2912,7 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
 	plug->cached_rq = rq_list_next(rq);
 	rq_qos_throttle(q, *bio);
 
+	blk_mq_rq_time_init(q, rq);
 	rq->cmd_flags = (*bio)->bi_opf;
 	INIT_LIST_HEAD(&rq->queuelist);
 	return rq;
-- 
2.39.2

Re: [PATCH v3 3/3] blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

[Christoph Hellwig]

On Wed, Jun 28, 2023 at 08:56:30PM +0200, Markus Elfring wrote:
> …
> > This patch fix it by setting alloc_time_ns and start_time_ns to now
> …
> 
> Please choose another imperative change suggestion.

Please stop bothering our contributors.

Re: [PATCH v3 3/3] blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

[Christoph Hellwig]

> +/* Set rq alloc and start time when pre-allocated rq is actually used */
> +static inline void blk_mq_rq_time_init(struct request_queue *q, struct request *rq)
> +{
> +	if (blk_mq_need_time_stamp(rq->rq_flags)) {
> +		u64 now = ktime_get_ns();
> +
> +#ifdef CONFIG_BLK_RQ_ALLOC_TIME
> +		/*
> +		 * alloc time is only used by iocost for now,
> +		 * only possible when blk_mq_need_time_stamp().
> +		 */
> +		if (blk_queue_rq_alloc_time(q))
> +			rq->alloc_time_ns = now;
> +#endif
> +		rq->start_time_ns = now;
> +	}
> +}

No need to pass q separately here, you can just use rq->q.

While you're at it please capitalize the first letter of block comments.

Re: [PATCH v3 3/3] blk-mq: fix start_time_ns and alloc_time_ns for pre-allocated rq

[Chengming Zhou]

On 2023/6/29 13:32, Christoph Hellwig wrote:
>> +/* Set rq alloc and start time when pre-allocated rq is actually used */
>> +static inline void blk_mq_rq_time_init(struct request_queue *q, struct request *rq)
>> +{
>> +	if (blk_mq_need_time_stamp(rq->rq_flags)) {
>> +		u64 now = ktime_get_ns();
>> +
>> +#ifdef CONFIG_BLK_RQ_ALLOC_TIME
>> +		/*
>> +		 * alloc time is only used by iocost for now,
>> +		 * only possible when blk_mq_need_time_stamp().
>> +		 */
>> +		if (blk_queue_rq_alloc_time(q))
>> +			rq->alloc_time_ns = now;
>> +#endif
>> +		rq->start_time_ns = now;
>> +	}
>> +}
> 
> No need to pass q separately here, you can just use rq->q.
> 
> While you're at it please capitalize the first letter of block comments.
> 

Ok, I will use rq->q and fix the comments in the next version.

Thanks.