[PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[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>
---
 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

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Yu Kuai]

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
> 

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Yu Kuai]

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

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Tejun Heo]

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-XF6JlduFytWkHkcT6e4Xnw@public.gmane.org> 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

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Tejun Heo]

On Tue, Jan 03, 2023 at 07:28:33PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> If the policy defines pd_online_fn(), it should be called after
> pd_init_fn(), like blkg_create().
> 
> Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

-- 
tejun

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Yu Kuai]

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;
>   
> 

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Yu Kuai]

Hi, Jens

ÔÚ 2023/01/03 19:28, Yu Kuai дµÀ:
> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> If the policy defines pd_online_fn(), it should be called after
> pd_init_fn(), like blkg_create().
> 
> Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

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;
>   
> 

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Jens Axboe]

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

Re: [PATCH] blk-cgroup: fix missing pd_online_fn() while activating policy

[Jens Axboe]

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