Re: [RFC PATCH v8 2/4] selftests/bpf: add iocost cost model test

[Tejun Heo <tj@kernel.org>]

Hello, Tao.

The following is a Claude-generated review.

On Wed, 30 Sep 2026 15:51:52 +0800, Tao Cui wrote:
> Add an example cost model implementing the full builtin linear HDD
> formula at double cost, and a test which attaches it to one device:
> the dev member of the struct_ops is written through the map's
> initial value before load, as hid_bpf tests do with hid_id, and
> loading the struct_ops attaches the model to the device.  The test
> verifies the ctrl=bpf readback while attached, that a second model
> on the same device fails with -EBUSY, and that detaching restores
> the builtin model.
...
> cgroup that comes back starts fresh.  opf carries the full bio->bi_opf
> including REQ_* flag bits, so the operation must be extracted with a
> mask, not compared for equality.

The description is out of date. The test reads back model=bpf and checks
that ctrl=bpf is rejected, calc_cost() takes the bio so there is no opf
argument, attaching rather than loading binds the model, and the
multi-stream model and its test aren't mentioned at all.

> +	if (fwrite(buf, 1, strlen(buf), fp) != strlen(buf))
> +		err = ferror(fp) ? errno : EIO;
> +	if (fclose(fp) && !err)
> +		err = errno;
> +	return err;
...
> +		err = write_cost_model(dev, "ctrl=bpf");
> +		ASSERT_ERR(err, "ctrl_bpf_rejected");

write_cost_model() returns a positive errno while ASSERT_ERR() wants a
negative value, so this and model_bpf_after_detach fail with "unexpected
success: 22" on a correct kernel. Return -errno. Can you double check
that the posted runner passes against the posted kernel?

> +	/* dev is the first member of struct iocost_model_ops */
> +	ops_dev = bpf_map__initial_value(skel->maps.iocost_2x, NULL);

The skeleton exposes the struct_ops shadow type, so
skel->struct_ops.iocost_2x->dev = ... is type checked and drops the
layout assumption. Same for iocost_ms.

> +SEC(".struct_ops")
> +struct iocost_model_ops iocost_2x = {

With a plain struct_ops map, a test that dies between attach and detach
leaves the model attached until something deletes the map element. The
hid and sched_ext selftests use ".struct_ops.link" so that closing the fd
detaches. Can you use that here too?

> +	cur = *cursor;
> +	if (cur && priced) {
> +		seek_pages = sector > cur ? sector - cur
> +					   : cur - sector;

A dataless flush is REQ_OP_WRITE|REQ_PREFLUSH at sector 0 with bi_size 0,
so priced is set here. Once a cgroup's cursor is past 16MB, every fsync
is judged a random write and charged 2 * (WRANDIO + WPAGE), about 5ms,
not the one-page write the header comment describes, and iocost_ms.c
prices the same bio with the sequential base. Can you gate the seek
judgement on a non-zero size?

Thanks.

--
tejun