Re: [PATCH 2/3] blk-iocost: clear delay state when freeing policy data

[Usama Arif <usama.arif@linux.dev>]

On 06/08/2026 19:37, Usama Arif wrote:
> iocg_kick_delay() turns sufficiently large debt into an explicit
> block-cgroup delay with blkcg_set_delay(), setting blkg->use_delay to
> -1 and incrementing blkcg->congestion_count.  Clearing it again depends
> on iocg_kick_delay() running from the period timer, the waitq timer or
> the issue path.
> 
> ioc_pd_free() removes the iocg from active_iocgs and cancels its waitq
> timer, and no further bios can arrive, so once it has run nothing is
> left which can reduce the debt and clear the delay.  The blkcg stays
> marked congested for the rest of its life.
> 
> blk_cgroup_congested() then returns true for every task in that cgroup
> and its descendants: page_cache_sync_ra() cuts readahead to a single
> page, page_cache_async_ra() skips it altogether, and
> __folio_throttle_swaprate() takes swap_avail_lock and schedules a
> throttle on anonymous folio allocation.
> 
> Clear it explicitly, after the list removal and the synchronous
> hrtimer_cancel() so that neither timer processing nor an I/O path can
> re-arm it.  The free callback can also see policy data which was never
> attached to a blkg, hence the pd->blkg check.
> 
> Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
> Signed-off-by: Usama Arif <usama.arif@linux.dev>
> ---
>  block/blk-iocost.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index b60625613e095..64b92aa3e5d48 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -3063,6 +3063,7 @@ static void iocg_release(struct rcu_head *rcu)
>  static void ioc_pd_free(struct blkg_policy_data *pd)
>  {
>  	struct ioc_gq *iocg = pd_to_iocg(pd);
> +	struct blkcg_gq *blkg = pd_to_blkg(pd);
>  	struct ioc *ioc = iocg->ioc;
>  	unsigned long flags;
>  
> @@ -3085,6 +3086,12 @@ static void ioc_pd_free(struct blkg_policy_data *pd)
>  		hrtimer_cancel(&iocg->waitq_timer);
>  	}
>  
> +	/* off ->active_iocgs and timer gone, so nothing can re-arm the delay */
> +	iocg->delay = 0;
> +	iocg->indelay_since = 0;
> +	if (blkg)
> +		blkcg_clear_delay(blkg);
> +


Sashiko reported:

---
[Severity: High]
Does this unconditional call to blkcg_clear_delay() overwrite state managed
by other policies?

Since use_delay is shared among block cgroup policies, if iocost never
actually applied a delay, this might forcefully zero out the delay state
managed by other active policies like blk-iolatency. 

Would it be safer to check if iocost had actually set a delay before clearing
it here?
---


My understanding is that this requires both policies to coexist on the same
device/blkg?

I believe the main problematic sequence would be:

  1. io.latency owns positive use_delay.
  2. io.cost policy data exists but may never have delayed anything.
  3. io.cost policy data is freed while the blkg survives.
  4. Its unconditional clear removes io.latency’s state.

With only one policy, the terminal clear is correct and clearing zero is harmless.

The concern is narrower still: ordinary disk/cgroup destruction removes the whole
blkg, so the main observable case is one-policy activation rollback or deactivation
while the other remains active.

I am not sure if this is something that is done by users? So might not need
to be addressed?

Thanks,
Usama


>  	call_rcu(&pd->rcu_head, iocg_release);
>  }
>