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: <63f93f1c-98da-4c09-b3d8-711f6953d8b7@linux.dev>
Date:   Thu, 13 Jul 2023 20:25:50 +0800
MIME-Version: 1.0
Subject: Re: [PATCH v5] blk-mq: fix start_time_ns and alloc_time_ns for
 pre-allocated rq
Content-Language: en-US
To:     Jens Axboe <axboe@kernel.dk>, tj@kernel.org, hch@lst.de
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        ming.lei@redhat.com, zhouchengming@bytedance.com
References: <20230710105516.2053478-1-chengming.zhou@linux.dev>
 <aa813164-9a6a-53bd-405b-ba4cc1f1b656@kernel.dk>
From:   Chengming Zhou <chengming.zhou@linux.dev>
In-Reply-To: <aa813164-9a6a-53bd-405b-ba4cc1f1b656@kernel.dk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/7/11 03:47, Jens Axboe wrote:
> On 7/10/23 4:55?AM, chengming.zhou@linux.dev wrote:
>> 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().
>>
>> Fix it by setting alloc_time_ns and start_time_ns to now when the
>> allocated request is actually used.
> 
> One of the single most costly things we do in the io path is the crazy
> amount of time stamping that's done or added without attention to what
> else is doing it or where. And this is why we have a ton of them, and
> why the batched time stamping made sense.
> 
> I'd much rather see this just get the saved timestamp updated when
> necessary. If we block, that's certainly one such case.
> 

Ok, this version will only get time stamp once for one request, it's actually
not worse than the current code, which will get start time stamp once for each
request even in the batch allocation.

But yes, maybe we can also set the start time stamp in the batch mode, and only
update the time stamp in the block case, like you said, has better performance.

The first version [1] I posted actually just did this, in which use a nr_flush counter
in plug to indicate that we blocked & flushed plug. Tejun and I think it seems fragile.
So go to this way that only set time stamp once when the request actually used.

[1] https://lore.kernel.org/all/20230601053919.3639954-1-chengming.zhou@linux.dev/

Another way I can think of is to make rq_qos_throttle() return a bool to indicate
if it blocked. Tejun and Jens, how do you think about this way?

Although it's better performance, in case of preemption, the time stamp maybe not accurate.

Thanks.