linux-kernel - Re: [PATCH] blk-throttle: support io merge over iops

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z8sZyElaHQQwKqpB@slm.duckdns.org>
Date: Fri, 7 Mar 2025 06:07:36 -1000
From: Tejun Heo <tj@...nel.org>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: ming.lei@...hat.com, axboe@...nel.dk, josef@...icpanda.com,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
	cgroups@...r.kernel.org, yukuai3@...wei.com, yi.zhang@...wei.com,
	yangerkun@...wei.com
Subject: Re: [PATCH] blk-throttle: support io merge over iops_limit

Hello,

On Fri, Mar 07, 2025 at 05:01:52PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@...wei.com>
> 
> Commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
> support to account split IO for iops limit, because block layer provides
> io accounting against split bio.
> 
> However, io merge is still not handled, while block layer doesn't
> account merged io for iops. Fix this problem by decreasing io_disp
> if bio is merged, and following IO can use the extra budget. If io merge
> concurrent with iops throttling, it's not handled if one more or one
> less bio is dispatched, this is fine because as long as new slice is not
> started, blk-throttle already preserve one extra slice for deviation,
> and it's not worth it to handle the case that iops_limit rate is less than
> one per slice.
> 
> A regression test will be added for this case [1], before this patch,
> the test will fail:
> 
> +++ /root/blktests-mainline/results/nodev/throtl/007.out.bad
> @@ -1,4 +1,4 @@
>  Running throtl/007
>  1
> -1
> +11
>  Test complete
> 
> [1] https://lore.kernel.org/all/20250307080318.3860858-2-yukuai1@huaweicloud.com/

For blk-throtl, iops limit has meant the number of bios issued. I'm not
necessarily against this change but this is significantly changing what a
given configuration means. Also, if we're now doing hardware request based
throttling, maybe we should just move this under rq-qos. That has the
problem of not supporting bio-based drivers but maybe we can leave
blk-throtl in deprecation mode and slowly phase it out.

Also, can you please make atomic_t conversion a separate patch and describe
why that's being done?

Thanks.

-- 
tejun