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

[Tao Cui]

Hello, Tejun.

在 2026/9/29 08:42, Tejun Heo 写道:
> Hello, Tao.
> 
> On Thu, 24 Sep 2026 13:45:46 +0800, Tao Cui wrote:
> 
>> -	/* if user is overriding anything, maintain what was there */
>> -	if (ioc->user_qos_params || ioc->user_cost_model)
>> +	/* if user is overriding anything, maintain what was there; the
>> +	 * same while a BPF model is attached: the builtin coefficients
>> +	 * are inert then, so stepping the profile is pointless
>> +	 */
>> +	if (ioc->user_qos_params || ioc->user_cost_model
>> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
>> +	    || rcu_dereference_protected(ioc->model,
>> +					 lockdep_is_held(&ioc->lock))
>> +#endif
>> +	   )
> 
> Can you add a helper which returns the model in use with a stub returning
> NULL for !CONFIG_BLK_CGROUP_IOCOST_BPF? That'd remove most of the #ifdefs
> including the one in this condition and the duplicated seq_printf() in
> ioc_cost_model_prfill().
Done.  ioc_model_in_use() and a locked variant return the model in
use, with NULL stubs for !CONFIG_BLK_CGROUP_IOCOST_BPF; the autop
condition, both calc paths, the pd callbacks and the prfill now call
the helpers, and the duplicated seq_printf() is unified.

> 
>> +		/* sub-page IO: nothing to transfer-price */
>> +		if (!pages)
>> +			return 0;
>> +		/* zero transfer cost is a legal model; guard the division */
>> +		if (!coeff)
>> +			return 0;
>> +		/* pages * coeff can wrap and dodge the clamp below */
>> +		if (coeff > VTIME_PER_SEC || pages > VTIME_PER_SEC / coeff)
>> +			return VTIME_PER_SEC;
>> +		return min(pages * coeff, VTIME_PER_SEC);
> 
> This can just be pages * coeff like the builtin. An overflow only skews
> the met/missed accounting, same as a user-set linear coefficient, and it
> also gets rid of the 64-bit division.
> 
Done; the guards and the division are gone and the completion-time
sizing is back to a plain pages * coeff, like the builtin.

>> +	bdevf = bdev_file_open_by_dev(new_decode_dev(ops->dev),
>> +				      BLK_OPEN_READ, NULL, NULL);
> 
> When the disk goes away, the model should be ejected completely.
> ioc_rqos_exit() unbinds it but the open bdev file keeps the dead disk and
> the driver module pinned until the link is destroyed. Can you drop all
> device references on removal like hid_bpf_destroy_device() does and look
> up the device like blkg_conf_open_bdev() does, with blkdev_get_no_open()
> and disk_live() checked under rq_qos_mutex? The two attach issues bpf-ci
> reported, the missing re-attach check in .reg and the missing disk_live()
> check, are real.
> 
Done.  The struct file is gone: the attach looks the device up with
blkdev_get_no_open() and checks disk_live() under rq_qos_mutex like
blkg_conf_open_bdev(), .reg rejects re-attach via ops->q, and
ioc_rqos_exit() ejects the model completely on removal, clearing the
queue pointer.

To keep the queue alive across detach, the attach now holds a
no_open bdev reference, similar to hid_bpf's per-ops device
reference but without a struct file or driver-module pin; the
reference is dropped by whichever path detaches the model first,
either ioc_rqos_exit() during removal or .unreg, and .unreg
re-checks ops->q under rq_qos_mutex.

That leaves one race: .unreg may observe a non-NULL ops->q before
ioc_rqos_exit() clears it, then block on rq_qos_mutex while the
ejection drops the last bdev reference.  This looks analogous to
hid_bpf's .unreg vs. destroy_device synchronization.

Does that seem acceptable here too, or would you rather have .unreg
own the final reference unconditionally?

Thanks.
Tao

> Thanks.
> 
> --
> tejun