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

Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> · Wed, 3 Sep 2025 16:24:45 +0800

Hi,

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

.