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

[Tejun Heo <tj@kernel.org>]

Hello, Tao.

The following is a Claude-generated review.

On Wed, 30 Sep 2026 15:51:51 +0800, Tao Cui wrote:
> Add the iocost_model_ops struct_ops.  Attachment follows the
> hid_bpf_ops model: the struct_ops instance is per-device, the target
> device is set in the dev member from userspace before load, .reg
> attaches the model to that device and switches it away from the
> builtin linear model, and .unreg detaches it and restores the builtin
> model.  The struct_ops core owns the program lifetime, so there is no
> name registry and no bound-state bookkeeping.
...
> The cgroup callbacks are bound to the iocg policy init/free paths,
> one (cgroup, device) pair per invocation, matching the builtin
> cursor's lifetime, instead of the blkcg css lifecycle, which also
> drops the mutex from the cgroup online/offline paths.

The description is written against earlier versions. There is no name
registry, css lifecycle binding or mutex in the tree, so a reader of the
commit can't follow it. Can you open with why a pluggable model is wanted
and then describe what the patch does: attach creates the ioc and
switches the model under the same freeze as io.cost.model writes,
model=bpf and model=linear select between the attached and builtin
models, device removal ejects the model, and so on? Also, the model
prices every charged IO, not every IO. Root cgroup IOs and IOs while the
controller is disabled never reach it.

> +bool blk_get_queue_rcu(struct request_queue *q)
> +{
> +	return refcount_inc_not_zero(&q->refs);
> +}
> +EXPORT_SYMBOL(blk_get_queue_rcu);
...
> +struct block_device *blkdev_get_no_open(dev_t dev, bool autoload);
> +void blkdev_put_no_open(struct block_device *bdev);
> +bool blk_get_queue_rcu(struct request_queue *q);
> +void blk_put_queue(struct request_queue *q);

blk_get_queue_rcu() has no prototype in any header, so every build warns
on it. Can you declare it in block/blk.h, include "blk.h" from
blk-iocost-bpf.c and drop these local prototypes? blkdev_get_no_open()
and blkdev_put_no_open() are already in blk.h and blk_put_queue() in
blkdev.h. The export isn't needed either as the only user is built-in.

> +	case offsetof(struct iocost_model_ops, bdev):
...
> +	case offsetof(struct iocost_model_ops, q):

The struct_ops core rejects non-zero non-function members that
init_member doesn't claim and kvalue starts zeroed, so these two cases
can go. Same for the name lookup in .init, the core has already found the
struct by then.

> +	rcu_read_lock();
> +	q = rcu_dereference(ops->q);

ops->q isn't __rcu annotated, so sparse will complain here. Either
annotate it and use RCU_INIT_POINTER() for the stores, or READ_ONCE() it.
The RCU section protects the queue, not the ops pointer.

> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	{
> +		struct iocost_model_ops *ops;
> +
> +		spin_lock_irq(&ioc->lock);
> +		ops = (struct iocost_model_ops *)rcu_dereference_protected(
> +				ioc->attached, lockdep_is_held(&ioc->lock));
> +		rcu_assign_pointer(ioc->attached, NULL);
> +		rcu_assign_pointer(ioc->model, NULL);
> +		spin_unlock_irq(&ioc->lock);
> +
> +		/* eject the model completely on device removal */
> +		if (ops) {
> +			WRITE_ONCE(ops->q, NULL);
> +			blkdev_put_no_open(ops->bdev);
> +		}
> +	}
> +#endif

Once ops->q is NULL, .unreg returns without taking rq_qos_mutex, the link
can go away and the struct_ops map which holds ops can be freed after a
grace period. This path isn't in an RCU read section, so reading
ops->bdev after the store is a use-after-free window. Read bdev first and
make the NULL store the last access.

This is also a block scoping a variable. Can you make it an
ioc_bpf_eject() helper next to attach and detach, with an empty stub for
!CONFIG_BLK_CGROUP_IOCOST_BPF? That removes the #ifdef and the const
cast. The remaining #ifdefs in ioc_cost_model_write() would go away too
if the two pointers in struct ioc were unconditional.

> + * Deliver iocg_init()/iocg_free() to the cgroups which already have a
> + * blkg on the queue, the same q->blkg_list walk the blkcg policy
> + * teardown uses.  The queue is frozen and quiesced and blkcg_mutex
> + * serializes against blkg creation and destruction.  Cgroups without
> + * a blkg on the device yet are not missed: their blkg is created
> + * later and ioc_pd_init()/ioc_pd_free() deliver the callbacks then.
> + */

blkcg_mutex doesn't serialize blkg creation. The IO path creates blkgs
from bio_associate_blkg() through blkg_lookup_create(), before
bio_queue_enter(), so the freeze doesn't stop it either, and
blkg_create() runs pd_init and then list_add() to q->blkg_list under
queue_lock only. Against the attach:

1. ioc->attached is published, blkg_create() runs ioc_pd_init() which
   delivers iocg_init(), then list_add(), then the walk delivers
   iocg_init() again.

2. blkg_create() runs ioc_pd_init() while attached is NULL, the walk runs
   before list_add(), and the blkg never gets iocg_init() but gets
   iocg_free() from ioc_pd_free() or the detach walk.

Detach has the mirror cases. The walk is also the only q->blkg_list
walker without queue_lock, and a plain list_for_each_entry() against a
concurrent list_add() isn't safe. The teardown walk this mirrors holds
blkcg_mutex and queue_lock. Can you publish and clear ioc->attached and
walk under queue_lock as well? ioc_pd_init() already calls iocg_init()
under queue_lock, so nothing changes for the BPF side.

While at it, the walk goes newest first, so children usually get
iocg_init() before their parents. blkcg_activate_policy() walks in
reverse for that reason.

> +	/* prevent multiple attach of the same struct_ops */
> +	if (ops->q)
> +		return -EINVAL;

On the link path the map stays READY after .unreg, so a map can be
attached again through a new link. If the device goes away, the ejection
clears ops->q while the old link is still open, a device with the same
dev_t comes back and a new link attaches the same map, closing the old
link then detaches the new attachment. Recording the owning link in the
ops and having .unreg detach only when it matches would close that.

> +	/*
> +	 * check liveness and create the ioc under rq_qos_mutex, like
> +	 * blkg_conf_open_bdev() does; enabling stays with io.cost.qos
> +	 *
> +	 * the queue reference is held across the unlocked window below:
> +	 * the bdev reference does not pin the queue, bdev only holds a
> +	 * raw bd_queue pointer, and concurrent device removal may eject
> +	 * the model and free the ioc while we are off the mutex, so
> +	 * without our own reference the second mutex_lock() would touch
> +	 * a freed queue
> +	 */
> +	mutex_lock(&q->rq_qos_mutex);
> +	if (!disk_live(disk) || !blk_get_queue(q)) {
...
> +	ioc = q_to_ioc(q);
> +	if (!ioc) {
> +		ret = blk_iocost_init(disk);
> +		mutex_unlock(&q->rq_qos_mutex);

Why drop rq_qos_mutex here? ioc_cost_model_write() holds it from
blkg_conf_open_bdev() across blk_iocost_init() and its own freeze and
quiesce, so attach can hold it from the disk_live() check through the
unfreeze. Then the ioc can't be freed under us and the queue reference,
the second q_to_ioc() check and this comment go away. The comment is also
wrong: a whole-disk no-open bdev reference holds the disk device, and
disk_release() is what drops the queue reference.

Thanks.

--
tejun