Hi,
> >>> static inline bool blk_throtl_activated(struct request_queue *q)
> >>> {
> >>> - return q->td != NULL;
> >>> + return q->td != NULL &&
> >>> + test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
> >>> }
> >> You can just remove the fist checking, p->td must be set if policy is
> >> enabled. And please make blkcg_policy_enabled() inline function in
> >> blk-cgroup.h and use it here.
> > We intentionally kept the q->td != NULL check because we cannot guarantee
> > that the policy module is fully loaded when this function is called.
> > If the policy module is not loaded yet, blkcg_policy_throtl.plid might not be
> > assigned, which could cause the test_bit() check to be incorrect.
> >
> > By keeping this check, we ensure that we have at least reached the cgroup
> > configuration flow, indicating that the policy loading is complete.
> >
> > I'm wondering if there are any risks here and whether we should remove
> > the q->td != NULL check?
>
> I think there is none. blk-throttle can't be build as module, if config is n,
> blk_throtl_bio() is a non-function, if config is y, policy is registered during
> init. And throtl_init() failure will panic, noted blkcg_policy_register() will
> never fail for blk-throttle. BTW, policy pid is not a dynamic value at runtime.
I understand your point, but I think there's still a potential race
condition during kernel initialization. While blk-throttle is built as a
built-in module, block devices can also be built as built-in modules, and
at the same initcall level, the loading order may be unpredictable.
Additionally, the policy plid is allocated during blk-throttle module
initialization, so we need to verify this timing issue.
I conducted an experiment on the qemu platform by adding a BUG() statement
in the blk_throtl_activated() function:
------------[ cut here ]------------
kernel BUG at block/blk-throttle.h:157!
Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT_RT SMP
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.17-g69f6c99f1c48 #2
Hardware name: linux,dummy-virt (DT)
pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : submit_bio_noacct+0xec/0x388
lr : submit_bio_noacct+0x40/0x388
sp : ffff80008135b530
x29: ffff80008135b530 x28: ffff00000180f000 x27: fffffdffc00876c0
x26: 0000000000000040 x25: ffff800080a405f0 x24: 0000000000000004
x23: ffff800080393128 x22: ffff000002720000 x21: ffff0000018c0700
x20: 0000000000000000 x19: ffff00000207b180 x18: 0000000000000020
x17: 0000000000000002 x16: 0000000000000002 x15: ffffffffffffffff
x14: 0000000000000000 x13: ffff800080dff350 x12: ffffffffffffffff
x11: 0000000000000000 x10: 0000000000000020 x9 : ffff8000804cd088
x8 : ffff00000180f088 x7 : 0000000000000000 x6 : ffff00000207b1f8
x5 : 0000000000000008 x4 : ffff0000018c0700 x3 : ffff7fffdee71000
x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
submit_bio_noacct+0xec/0x388
submit_bio+0xb4/0x150
submit_bh_wbc+0x14c/0x1d0
block_read_full_folio+0x25c/0x398
blkdev_read_folio+0x24/0x38
filemap_read_folio+0x34/0xf0
do_read_cache_folio+0x150/0x290
read_cache_folio+0x1c/0x30
read_part_sector+0x48/0xd0
read_lba+0x9c/0x180
efi_partition+0xa0/0x780
bdev_disk_changed+0x238/0x608
blkdev_get_whole+0xac/0x150
bdev_open+0x28c/0x418
bdev_file_open_by_dev+0xe0/0x150
disk_scan_partitions+0x74/0x160
device_add_disk+0x3f4/0x448
virtblk_probe+0x74c/0x920
virtio_dev_probe+0x1a4/0x250
really_probe+0xb4/0x2b0
__driver_probe_device+0x80/0x140
driver_probe_device+0xdc/0x178
__driver_attach+0x9c/0x1b8
bus_for_each_dev+0x7c/0xe8
driver_attach+0x2c/0x40
bus_add_driver+0xec/0x218
driver_register+0x68/0x138
__register_virtio_driver+0x2c/0x50
virtio_blk_init+0x74/0xd0
do_one_initcall+0x64/0x290
kernel_init_freeable+0x214/0x408
kernel_init+0x2c/0x1f0
ret_from_fork+0x10/0x20
>From the experimental results, we can see that virtio block device is
executing blk_throtl_activated() during initialization in do_one_initcall()
when loading virio-blk module_init().
Combined with the information that blk-throttle is also loaded as a
built-in module, there exists a scenario where the block device probes
first, at which point the plid has not been allocated yet, and
blk_throtl_activated() is executed, followed by the loading of the
blk-throttle built-in module.
Given this experimental evidence, I'm wondering if we should consider
keeping the q->td != NULL check as a safeguard against this initialization
race condition. I'd appreciate your thoughts on this matter.
Thanks,
Han Guangjiang
