Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <70ade937-1f94-52a3-7add-b43d3b79e471@linux.dev>
Date:   Sat, 24 Jun 2023 23:24:04 +0800
MIME-Version: 1.0
Subject: Re: [PATCH] blk-mq: fix incorrect rq start_time_ns and alloc_time_ns
 after throttled
Content-Language: en-US
To:     Tejun Heo <tj@kernel.org>
Cc:     axboe@kernel.dk, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Chengming Zhou <zhouchengming@bytedance.com>
References: <20230601053919.3639954-1-chengming.zhou@linux.dev>
 <ZH4p8tqFc57_OYoH@slm.duckdns.org>
 <c82d6bab-36d0-0403-9304-4415f6ffd972@linux.dev>
 <ZIJco7_jaVaxpxSR@slm.duckdns.org>
From:   Chengming Zhou <chengming.zhou@linux.dev>
In-Reply-To: <ZIJco7_jaVaxpxSR@slm.duckdns.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.