Re: [PATCH 2/5] blk-iocost: add BPF struct_ops cost model support

[Tejun Heo <tj@kernel.org>]

Hello, Tao.

On Wed, Oct 28, 2026 at 10:35:31PM +0800, Tao Cui wrote:
> +/*
> +/*
> + * Deliver iocg_init()/iocg_free() to the cgroups already online on the

The comment opens with "/*" twice. clang warns "'/*' within block comment
[-Wcomment]" and CONFIG_WERROR builds break.

> +			/*
> +			 * A blkg which never went online never reached the
> +			 * radix tree, so it must not receive an init either;
> ...
> +			if (!iocg || !iocg->pd.online)
> +				continue;

blkg_create() adds a blkg to q->blkg_list only after radix_tree_insert()
succeeds, so the blkgs skipped here are the ones blkg_destroy() already
took offline, not ones that never reached the radix tree.

> +	if (!disk_live(disk)) {
> +		mutex_unlock(&q->rq_qos_mutex);
> +		blk_mq_unfreeze_queue(q, memflags);
> +		blkdev_put_no_open(bdev);
> +		return -ENODEV;
> +	}

The guard() suggestion from v10 was only partly applied. This unlock,
unfreeze and put sequence is repeated on three error paths.

> +	/*
> +	 * Scope the mutex so the tail bdev put runs with blkcg_mutex
> +	 * already released: dropping the last device reference can
> +	 * reach disk_release() -> blkcg_exit_disk() ->
> +	 * blkg_destroy_all(), which takes q->blkcg_mutex.
> +	 */
> +	{
> +		guard(mutex)(&q->blkcg_mutex);

The bare { } block isn't necessary, and ioc_bpf_detach() has the same
block with the same comment. Can you drop them?

> +	/*
> +	 * The attachment is checked under rq_qos_mutex first, so an unmatched
> +	 * detach exits without freezing.
> +	 */
> +	mutex_lock(&q->rq_qos_mutex);
> +	spin_lock_irq(&q->queue_lock);
> +	ioc = q_to_ioc(q);
> +	if (ioc) {
> +		spin_lock(&ioc->lock);
> +		matched = ioc_attached_model_locked(ioc) == ops &&
> +			  ioc->bpf_link == link;
> ...

The same 11 lines check the attachment again after the freeze. The check
under rq_qos_mutex after the freeze is the one that matters.

> +/*
> + * Pluggable cost model interface for blk-iocost.
> + *
> + * A BPF struct_ops implementation can take over the cost model of one device.
> ...
> + * offline and its free. Both callbacks are called with ioc->lock held and must
> + * not sleep; the delivery points nest inside queue_lock except for iocg_free()
> + * from ioc_pd_free() in blkg_free_workfn().
> + */

This restates the attach mechanics and lists the call sites. The comments
and descriptions in general are still long and narrate mechanics.

Thanks.

--
tejun