Messages in this thread | | | Date | Sat, 24 Jun 2023 23:24:04 +0800 | Subject | Re: [PATCH] blk-mq: fix incorrect rq start_time_ns and alloc_time_ns after throttled | From | Chengming Zhou <> |
| |
Hello,
On 2023/6/9 06:56, Tejun Heo wrote: > Hello, > > On Tue, Jun 06, 2023 at 06:22:28PM +0800, Chengming Zhou wrote: > ... >> 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 block in any qos ->throttle(). > > Ah, okay, that's problematic. >
Thanks for your review!
Sorry for my delay, I was out of the office.
>>>> This patch add nr_flush counter in blk_plug, so we can tell if the task >>>> has throttled in any qos ->throttle(), in which case we need to correct >>>> the rq start_time_ns and alloc_time_ns. >>>> >>>> Another solution may be make rq_qos_throttle() return bool to indicate >>>> if it has throttled in any qos ->throttle(). But this need more changes. >>>> >>>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >>> >>> Depending on the flush behavior and adjusting alloc_time_ns seems fragile to >>> me and will likely confuse other users of alloc_time_ns too. >> >> I agree with you, this code is not good. My basic idea is to adjust the cached >> request alloc_time_ns and start_time_ns when throttled. > > Would it make sense to skip setting the alloc_time_ns during pre-allocation > and set it later when the pre-allocated rq is actually used? That should > jive better. >
Ok, I think it's much clearer that we set the alloc_time_ns and start_time_ns to "now" when the pre-allocated rq is actually used.
I will send an updated version later.
Thanks.
| |