linux-kernel - Re: [PATCH v5] blk-mq: fix start_time_ns and alloc_time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aa813164-9a6a-53bd-405b-ba4cc1f1b656@kernel.dk>
Date:   Mon, 10 Jul 2023 13:47:40 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     chengming.zhou@...ux.dev, hch@....de, tj@...nel.org,
        ming.lei@...hat.com
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        zhouchengming@...edance.com
Subject: Re: [PATCH v5] blk-mq: fix start_time_ns and alloc_time_ns for
 pre-allocated rq

On 7/10/23 4:55?AM, chengming.zhou@...ux.dev wrote:
> From: Chengming Zhou <zhouchengming@...edance.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