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
