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

[Tao Cui]

Hello Tejun,

在 2026/10/29 04:09, Tejun Heo 写道:
> 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 stray line was left over from an earlier edit. Sorry about that.
>> +			/*
>> +			 * 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.
> 
You're right, the comment described the wrong blkgs. It goes away with
the walk, which I'll drop in v12 together with the callbacks.

>> +	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.
> 
I'll fold the reg error path into a single out label. Detach no longer
needs the freeze after the v12 rework.

>> +	/*
>> +	 * 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?
> 
I'll drop both.
>> +	/*
>> +	 * 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.
> 
This goes away with the freeze removal: detach becomes a single
mutex-critical section.
>> +/*
>> + * 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.
> 
v12 folds everything back into blk-iocost.c, so the private header and
the long interface comment both go away.

Thanks,
Tao

> Thanks.
> 
> --
> tejun