From: Yu Kuai <[email protected]>
If the policy defines pd_online_fn(), it should be called after
pd_init_fn(), like blkg_create().
Signed-off-by: Yu Kuai <[email protected]>
---
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;
--
2.31.1
Hello.
On Tue, Jan 03, 2023 at 07:28:33PM +0800, Yu Kuai <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 Tue, Jan 03, 2023 at 07:28:33PM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> If the policy defines pd_online_fn(), it should be called after
> pd_init_fn(), like blkg_create().
>
> Signed-off-by: Yu Kuai <[email protected]>
Acked-by: Tejun Heo <[email protected]>
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.
--
tejun
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 <[email protected]> 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.
--
tejun
Hi, Jens
?? 2023/01/03 19:28, Yu Kuai д??:
> From: Yu Kuai <[email protected]>
>
> If the policy defines pd_online_fn(), it should be called after
> pd_init_fn(), like blkg_create().
>
> Signed-off-by: Yu Kuai <[email protected]>
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,
--
Jens Axboe