Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/09/03 15:21, Liang Jie 写道:
> On Wed, 3 Sep 2025 14:03:37 +0800, Yu Kuai <yukuai1@huaweicloud.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.

Yes, however, this can be fixed very similar:

Set sq->parent_sq to NULL here, and add a helper parent_sq(q, sq):

if (sq->parent_sq)
	return sq->parent_sq;

td_sq = &q->td->service_queue;
return sq == td_sq ? NULL : td_sq;

And sq_to_tg() need to be changed as well. So far, I'm not sure how many
code changes are required this way. We of course want a simple fix for
stable backport, but we definitely still want this kind of fix in future
release.

Thanks,
Kuai

> 
>>
>> 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
>>>
> 
> .
>