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: <aa813164-9a6a-53bd-405b-ba4cc1f1b656@kernel.dk>
Date:   Mon, 10 Jul 2023 13:47:40 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v5] blk-mq: fix start_time_ns and alloc_time_ns for
 pre-allocated rq
Content-Language: en-US
To:     chengming.zhou@linux.dev, hch@lst.de, tj@kernel.org,
        ming.lei@redhat.com
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhouchengming@bytedance.com
References: <20230710105516.2053478-1-chengming.zhou@linux.dev>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20230710105516.2053478-1-chengming.zhou@linux.dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

-- 
Jens Axboe