Hello, Tejun.
在 2026/10/1 08:20, Tejun Heo 写道:
> 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.
>
The commit message is rewritten to describe the current test,
including the multi-stream model.
>> + 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?
>
Fixed, it returns -errno. The posted runner had not been run against
the posted kernel; it now passes against the v9 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.
>
Done for both models.
>> +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?
>
Done, both maps are ".struct_ops.link".
>> + 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?
>
Done: the seek judgement is now gated on a non-zero IO size, so a
dataless flush is priced as a size-zero sequential write, matching the
builtin model.
Thanks.
Tao
> Thanks.
>
> --
> tejun
