| Message ID | 20230103112833.2013432-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-cgroup: fix missing pd_online_fn() while activating policy | expand |
Hello. On Tue, Jan 03, 2023 at 07:28:33PM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > If the policy defines pd_online_fn(), it should be called after > pd_init_fn(), like blkg_create(). Is this based only on code review or has it some negative effects? I assume this would affect hot-plugged (read after cgroup creation) devices. I took a cursory look at: blkcg_init_disk blkg_create pol->pd_init_fn(blkg->pd[i]); pol->pd_online_fn(blkg->pd[i]); blk_throtl_init blkcg_activate_policy pol->pd_init_fn(blkg->pd[i]); ?? pol->pd_online_fn(blkg->pd[i]); I.e. the pd_online_fn is already called and pd_init_fn is called 2nd time? Thanks, Michal
Hi, Michal 在 2023/01/04 23:12, Michal Koutný 写道: > Hello. > > On Tue, Jan 03, 2023 at 07:28:33PM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: >> If the policy defines pd_online_fn(), it should be called after >> pd_init_fn(), like blkg_create(). > > Is this based only on code review or has it some negative effects? This is based only on code review, currently the only negative effects is that root blkg from blk-throtl won't call pd_online_fn(). > > I assume this would affect hot-plugged (read after cgroup creation) devices. > > I took a cursory look at: > > blkcg_init_disk > blkg_create > pol->pd_init_fn(blkg->pd[i]); > pol->pd_online_fn(blkg->pd[i]); > blk_throtl_init > blkcg_activate_policy > pol->pd_init_fn(blkg->pd[i]); > ?? pol->pd_online_fn(blkg->pd[i]); > > I.e. the pd_online_fn is already called and pd_init_fn is called 2nd > time? No, this is not true, before blkcg_activate_policy() is called, blkg_create() won't see this policy, hence pd_init_fn/pd_online_fn won't be called from blkg_create(). Thanks, Kuai > > Thanks, > Michal >
On Thu, Jan 05, 2023 at 09:43:02AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > This is based only on code review, currently the only negative effects > is that root blkg from blk-throtl won't call pd_online_fn(). Good, that's a NOP and there are no other uses of pd_online_fn. I wonder are the separate pd_init_fn and pd_online_fn callbacks necessary today? (IOW your fixup is a good catch and looks correct to me; I'd suggest more of a clean up. Shall I look into that?) > No, this is not true, before blkcg_activate_policy() is called, > blkg_create() won't see this policy, hence pd_init_fn/pd_online_fn won't > be called from blkg_create(). Thanks, I missed the q->blkcg_pols bit. Michal
Hi, 在 2023/01/05 18:45, Michal Koutný 写道: > On Thu, Jan 05, 2023 at 09:43:02AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: >> This is based only on code review, currently the only negative effects >> is that root blkg from blk-throtl won't call pd_online_fn(). > > Good, that's a NOP and there are no other uses of pd_online_fn. > > I wonder are the separate pd_init_fn and pd_online_fn callbacks > necessary today? I think online can combine to init, consider that only blk-throttle implement pd_online_fn(), but I'm not sure... It seems to me the policies(bfq, iocost...) seem don't honor how pd apis works: alloc->init->online->offline->free, bfq combines online to init, iocost combines offline to free, ... Thanks, Kuai
On Thu, Jan 05, 2023 at 09:52:29PM +0800, Yu Kuai wrote: > Hi, > > 在 2023/01/05 18:45, Michal Koutný 写道: > > On Thu, Jan 05, 2023 at 09:43:02AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > > > This is based only on code review, currently the only negative effects > > > is that root blkg from blk-throtl won't call pd_online_fn(). > > > > Good, that's a NOP and there are no other uses of pd_online_fn. > > > > I wonder are the separate pd_init_fn and pd_online_fn callbacks > > necessary today? > > I think online can combine to init, consider that only blk-throttle > implement pd_online_fn(), but I'm not sure... > > It seems to me the policies(bfq, iocost...) seem don't honor how pd > apis works: alloc->init->online->offline->free, bfq combines online to > init, iocost combines offline to free, ... So, the distinction between alloc and online is that a pd which gets allocated may be freed without ever going online if later allocations fail. This is following cgroup init/exit pattern. Maybe it's a bit too elaborate but the distinction is meaningful, at least in principle. What seems truly spurious is pd_init_fn(). All that pd_init_fn() can do should be achievable between pd_alloc_fn() and pd_online_fn(). The overlap seems at least partially historical and we used to have pd_exit_fn() too. So, yeah, getting rid of pd_init_fn() would be a nice first step. Thanks.
On Tue, Jan 03, 2023 at 07:28:33PM +0800, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > If the policy defines pd_online_fn(), it should be called after > pd_init_fn(), like blkg_create(). > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> Acked-by: Tejun Heo <tj@kernel.org> However, it'd be useful to note the practical implication of the bug in the patch description, which seems like not much as discussed in the thread. Thanks.
Hi, Jens 在 2023/01/03 19:28, Yu Kuai 写道: > From: Yu Kuai <yukuai3@huawei.com> > > If the policy defines pd_online_fn(), it should be called after > pd_init_fn(), like blkg_create(). > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> Can you apply this patch? Thanks, Kuai > --- > block/blk-cgroup.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > index ce6a2b7d3dfb..4c94a6560f62 100644 > --- a/block/blk-cgroup.c > +++ b/block/blk-cgroup.c > @@ -1455,6 +1455,10 @@ int blkcg_activate_policy(struct request_queue *q, > list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) > pol->pd_init_fn(blkg->pd[pol->plid]); > > + if (pol->pd_online_fn) > + list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) > + pol->pd_online_fn(blkg->pd[pol->plid]); > + > __set_bit(pol->plid, q->blkcg_pols); > ret = 0; > >
On Tue, 03 Jan 2023 19:28:33 +0800, Yu Kuai wrote: > If the policy defines pd_online_fn(), it should be called after > pd_init_fn(), like blkg_create(). > > Applied, thanks! [1/1] blk-cgroup: fix missing pd_online_fn() while activating policy commit: e3ff8887e7db757360f97634e0d6f4b8e27a8c46 Best regards,
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index ce6a2b7d3dfb..4c94a6560f62 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -1455,6 +1455,10 @@ int blkcg_activate_policy(struct request_queue *q, list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) pol->pd_init_fn(blkg->pd[pol->plid]); + if (pol->pd_online_fn) + list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) + pol->pd_online_fn(blkg->pd[pol->plid]); + __set_bit(pol->plid, q->blkcg_pols); ret = 0;