linux-kernel - Re: [PATCH] blk-throttle: check policy bit in blk_throtl

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250903072112.3432190-1-buaajxlj@163.com>
Date: Wed,  3 Sep 2025 15:21:12 +0800
From: Liang Jie <buaajxlj@....com>
To: yukuai1@...weicloud.com
Cc: axboe@...nel.dk,
	fanggeng@...iang.com,
	gj.han@...mail.com,
	hailan@...uai.org.cn,
	hanguangjiang@...iang.com,
	liangjie@...iang.com,
	linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	yangchen11@...iang.com,
	yukuai3@...wei.com
Subject: Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

On Wed, 3 Sep 2025 14:03:37 +0800, Yu Kuai <yukuai1@...weicloud.com> wrote:

>在 2025/09/03 10:55, Han Guangjiang 写道:
>> Hi Kuai,
>> 
>>> Instead of add checking from hot path, do you consider delaying setting q->td
>>> until policy is activated from the slow path? I think this is better solution.
>> 
>> Thank you for your review. You're absolutely right that performance
>> considerations in the hot path are important.
>> 
>> We actually considered delaying the setting of q->td until after policy
>> activation, but we found that q->td is needed by blkcg_activate_policy()
>> during its execution, so it has to be set before calling
>> blkcg_activate_policy().
>
>That's not hard to bypass, q->td is used to initialze tg->td in
>throtl_pd_init(), actually you can just remove it, and add a helper
>tg_to_td() to replace it;
>
>struct throtl_data *tg_to_td(struct throtl_grp *tg)
>{
>	return tg_to_blkg(tg)->q->td;
>}

Hi Kuai,

Thanks for the suggestion. Just a quick note: in throtl_pd_init(), q->td is not
only used to init tg->td, it’s also needed for sq->parent_sq:

 - sq->parent_sq = &td->service_queue;

So if we remove tg->td and delay q->td, throtl_pd_init() won’t have a valid td
to set parent_sq.

>
>Meanwhile, please remove the comment about freeze queue, turns out it
>can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
>yet while issuing bio.

You’re right. We’ll remove that comment in patch v2.

Thanks,
Liang Jie

>
>Thanks,
>Kuai
>
>> 
>> We explored several alternative approaches:
>> 
>> 1) Adding a dedicated flag like 'throttle_ready' to struct request_queue:
>>     - Set this flag at the end of blk_throtl_init()
>>     - Check this flag in blk_throtl_activated() to determine if policy
>>       loading is complete
>>     - However, this requires adding a new bool variable to the struct
>> 
>> 2) Reusing the q->td pointer with low-order bit flags:
>>     - Use pointer low-order bits to mark initialization completion status
>>     - This would avoid adding new fields but requires careful handling
>>       and additional processing
>> 
>> Given these constraints, we chose the current approach of checking the
>> policy bit in blk_throtl_activated() as it:
>> - Doesn't require struct changes
>> - Provides a clean, atomic check
>> - Aligns with the existing policy activation mechanism
>> 
>> We would appreciate your suggestions on how to better handle this
>> initialization race condition.
>> 
>> Thanks,
>> Han Guangjiang
>>