Re: [RFC PATCH v13 1/4] blk-iocost: add BPF struct_ops cost model support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] blk-iocost: stale autop timestamps in ioc_cost_model_write()
--

commit dbe8a0cfe21679fa0e7f03924d92bf48ea6f4959
Author: Tao Cui <cuitao@kylinos.cn>

blk-iocost: add BPF struct_ops cost model support

This commit adds BPF struct_ops support (iocost_model_ops) to allow custom
per-device IO cost models in blk-iocost.

> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 1c1fe07a96eaf..876f819d2586f 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c

[ ... ]

> @@ -2976,10 +3036,40 @@ static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos)
>  	spin_unlock_irq(&ioc->lock);
>  }
>  
> +/* Called by .unreg and by device removal, whichever is first. */
> +static void ioc_detach_model(struct ioc *ioc)
> +{
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	const struct iocost_model_ops *ops = ioc->attached;
> +
> +	lockdep_assert_held(&ioc->rqos.disk->queue->rq_qos_mutex);
> +
> +	if (!ops)
> +		return;
> +
> +	spin_lock_irq(&ioc->lock);
> +	rcu_assign_pointer(ioc->model, NULL);
> +	ioc->attached = NULL;
> +	ioc->attached_link = NULL;
> +	/*
> +	 * stepping resumes with the timestamps at their pre-attach
> +	 * values, which would immediately satisfy the 10s cooldown;
> +	 * start it fresh instead
> +	 */
> +	ioc->autop_too_fast_at = 0;
> +	ioc->autop_too_slow_at = 0;
> +	spin_unlock_irq(&ioc->lock);
> +
> +	/* calc_cost() can still run. The map is freed after an RCU gp. */
> +	bpf_struct_ops_put(ops);
> +#endif
> +}

[ ... ]

> @@ -3632,7 +3725,11 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  			continue;
>  		case COST_MODEL:
>  			match_strlcpy(buf, &args[0], sizeof(buf));
> -			if (strcmp(buf, "linear"))
> +			if (!strcmp(buf, "linear"))
> +				model = NULL;
> +			else if (!strcmp(buf, "bpf") && ioc->attached)
> +				model = ioc->attached;
> +			else
>  				goto unlock;
>  			continue;
>  		}
> @@ -3652,6 +3749,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  	} else {
>  		ioc->user_cost_model = false;
>  	}
> +	rcu_assign_pointer(ioc->model, model);
>  	ioc_refresh_params(ioc, true);

[Severity: Medium]
Can switching back to model=linear via io.cost.model evaluate stale
ioc->autop_too_fast_at and ioc->autop_too_slow_at timestamps and trigger an
immediate profile step?

ioc_bpf_reg() attaches the BPF model without clearing those timestamps, and
while the BPF model is active ioc_autop_idx() returns early so they remain at
their pre-attach values.  Although ioc_detach_model() zeroes them on detach,
writing model=linear to io.cost.model sets ioc->model to NULL before calling
ioc_refresh_params(ioc, true), where ioc_refresh_params_disk() calls
ioc_autop_idx() before zeroing autop_too_fast_at and autop_too_slow_at:

block/blk-iocost.c:ioc_refresh_params_disk() {
    ...
	idx = ioc_autop_idx(ioc, disk);
	p = &autop[idx];

	if (idx == ioc->autop_idx && !force)
		return false;

	if (idx != ioc->autop_idx) {
		atomic64_set(&ioc->vtime_rate, VTIME_PER_USEC);
		ioc->vtime_base_rate = VTIME_PER_USEC;
	}

	ioc->autop_idx = idx;
	ioc->autop_too_fast_at = 0;
	ioc->autop_too_slow_at = 0;
    ...
}

Because ioc->model is already NULL when ioc_autop_idx() runs, a pre-attach
timestamp older than AUTOP_CYCLE_NSEC (10s) immediately steps ioc->autop_idx
and resets vtime_rate without waiting for the cooldown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106122356.3224638-1-cui.tao@linux.dev?part=1