[PATCH 0/4] A few cleanup patches for blk-cgroup

[PATCH 0/4] A few cleanup patches for blk-cgroup

[Kemeng Shi]

This series contains a few cleanup patches to correct comment, remove
unnecessary exit and add NULL check.

Kemeng Shi (4):
  blk-cgroup: Remove unnecessary blk_ioprio_exit in blkcg_init_queue
  blk-cgroup: correct comment for blk_alloc_queue and blk_exit_queue
  blk-cgroup: Add NULL check of pd_alloc_fn in blkcg_activate_policy
  blk-cgroup: Fix typo in comment

 block/blk-cgroup.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.30.0

[PATCH 1/4] blk-cgroup: Remove unnecessary blk_ioprio_exit in blkcg_init_queue

[Kemeng Shi]

Function blk_ioprio_init only alloc blkg_policy_data which will be freed
in blkg_destroy_all, so no blk_ioprio_exit is called when blk_throtl_init
is failed in blkcg_init_queue. Also blk_ioprio_exit is not called in
blkcg_exit_queue for the same reason. Just remove blk_ioprio_exit to
keep behavior consistent.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-cgroup.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 869af9d72bcf..bc4dec705572 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1302,7 +1302,6 @@ int blkcg_init_queue(struct request_queue *q)
 	ret = blk_iolatency_init(q);
 	if (ret) {
 		blk_throtl_exit(q);
-		blk_ioprio_exit(q);
 		goto err_destroy_all;
 	}
 
-- 
2.30.0

Re: [PATCH 1/4] blk-cgroup: Remove unnecessary blk_ioprio_exit in blkcg_init_queue

[Christoph Hellwig]

On Mon, Oct 10, 2022 at 10:38:56AM +0800, Kemeng Shi wrote:
> Function blk_ioprio_init only alloc blkg_policy_data which will be freed
> in blkg_destroy_all, so no blk_ioprio_exit is called when blk_throtl_init
> is failed in blkcg_init_queue. Also blk_ioprio_exit is not called in
> blkcg_exit_queue for the same reason. Just remove blk_ioprio_exit to
> keep behavior consistent.

This code looks very different in current mainline.

Re: [PATCH 1/4] blk-cgroup: Remove unnecessary blk_ioprio_exit in blkcg_init_queue

[Kemeng Shi]

on 10/10/2022 3:37 PM, Christoph Hellwig wrote:
> On Mon, Oct 10, 2022 at 10:38:56AM +0800, Kemeng Shi wrote:
>> Function blk_ioprio_init only alloc blkg_policy_data which will be freed
>> in blkg_destroy_all, so no blk_ioprio_exit is called when blk_throtl_init
>> is failed in blkcg_init_queue. Also blk_ioprio_exit is not called in
>> blkcg_exit_queue for the same reason. Just remove blk_ioprio_exit to
>> keep behavior consistent.
> 
> This code looks very different in current mainline.
Thanks for review. I will remove this patch and make new patches based on
mainline code.
-- 
Best wishes
Kemeng Shi

[PATCH 2/4] blk-cgroup: correct comment for blk_alloc_queue and blk_exit_queue

[Kemeng Shi]

Since commit 1059699f87eb("block: move blkcg initialization/destroy into
disk allocation/release handler"), blk_alloc_queue and blk_exit_queue is
called directly from gendisk. Update the corresponding comment.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-cgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bc4dec705572..463c568d3e86 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1259,7 +1259,7 @@ static int blkcg_css_online(struct cgroup_subsys_state *css)
  * blkcg_init_queue - initialize blkcg part of request queue
  * @q: request_queue to initialize
  *
- * Called from blk_alloc_queue(). Responsible for initializing blkcg
+ * Called from gendisk. Responsible for initializing blkcg
  * part of new request_queue @q.
  *
  * RETURNS:
@@ -1321,7 +1321,7 @@ int blkcg_init_queue(struct request_queue *q)
  * blkcg_exit_queue - exit and release blkcg part of request_queue
  * @q: request_queue being released
  *
- * Called from blk_exit_queue().  Responsible for exiting blkcg part.
+ * Called from gendisk.  Responsible for exiting blkcg part.
  */
 void blkcg_exit_queue(struct request_queue *q)
 {
-- 
2.30.0

Re: [PATCH 2/4] blk-cgroup: correct comment for blk_alloc_queue and blk_exit_queue

[Tejun Heo]

On Mon, Oct 10, 2022 at 10:38:57AM +0800, Kemeng Shi wrote:
> Since commit 1059699f87eb("block: move blkcg initialization/destroy into
> disk allocation/release handler"), blk_alloc_queue and blk_exit_queue is
> called directly from gendisk. Update the corresponding comment.
> 
> Signed-off-by: Kemeng Shi <shikemeng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  block/blk-cgroup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bc4dec705572..463c568d3e86 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1259,7 +1259,7 @@ static int blkcg_css_online(struct cgroup_subsys_state *css)
>   * blkcg_init_queue - initialize blkcg part of request queue
>   * @q: request_queue to initialize
>   *
> - * Called from blk_alloc_queue(). Responsible for initializing blkcg
> + * Called from gendisk. Responsible for initializing blkcg

Maybe be a bit more specific and say blk_alloc_disk()?

>   * part of new request_queue @q.
>   *
>   * RETURNS:
> @@ -1321,7 +1321,7 @@ int blkcg_init_queue(struct request_queue *q)
>   * blkcg_exit_queue - exit and release blkcg part of request_queue
>   * @q: request_queue being released
>   *
> - * Called from blk_exit_queue().  Responsible for exiting blkcg part.
> + * Called from gendisk.  Responsible for exiting blkcg part.

Ditto.

-- 
tejun

Re: [PATCH 2/4] blk-cgroup: correct comment for blk_alloc_queue and blk_exit_queue

[Kemeng Shi]

on 10/11/2022 4:26 AM, Tejun Heo wrote:
> On Mon, Oct 10, 2022 at 10:38:57AM +0800, Kemeng Shi wrote:
>> Since commit 1059699f87eb("block: move blkcg initialization/destroy into
>> disk allocation/release handler"), blk_alloc_queue and blk_exit_queue is
>> called directly from gendisk. Update the corresponding comment.
>>
>> Signed-off-by: Kemeng Shi <shikemeng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  block/blk-cgroup.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index bc4dec705572..463c568d3e86 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1259,7 +1259,7 @@ static int blkcg_css_online(struct cgroup_subsys_state *css)
>>   * blkcg_init_queue - initialize blkcg part of request queue
>>   * @q: request_queue to initialize
>>   *
>> - * Called from blk_alloc_queue(). Responsible for initializing blkcg
>> + * Called from gendisk. Responsible for initializing blkcg
> 
> Maybe be a bit more specific and say blk_alloc_disk()?
Thanks for review. I will update this in next version.
> 
>>   * part of new request_queue @q.
>>   *
>>   * RETURNS:
>> @@ -1321,7 +1321,7 @@ int blkcg_init_queue(struct request_queue *q)
>>   * blkcg_exit_queue - exit and release blkcg part of request_queue
>>   * @q: request_queue being released
>>   *
>> - * Called from blk_exit_queue().  Responsible for exiting blkcg part.
>> + * Called from gendisk.  Responsible for exiting blkcg part.
> 
> Ditto.
> 

-- 
Best wishes
Kemeng Shi

[PATCH 3/4] blk-cgroup: Add NULL check of pd_alloc_fn in blkcg_activate_policy

[Kemeng Shi]

Function blkcg_policy_register only make sure pd_alloc_fn and pd_free_fn in
pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL
before use for pd_alloc_fn in blkcg_activate_policy to avoid protential
NULL dereference.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-cgroup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 463c568d3e86..fc083c35dc42 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q,
 	if (blkcg_policy_enabled(q, pol))
 		return 0;
 
+	if (pol->pd_alloc_fn == NULL)
+		return -EINVAL;
+
 	if (queue_is_mq(q))
 		blk_mq_freeze_queue(q);
 retry:
-- 
2.30.0

Re: [PATCH 3/4] blk-cgroup: Add NULL check of pd_alloc_fn in blkcg_activate_policy

[Tejun Heo]

On Mon, Oct 10, 2022 at 10:38:58AM +0800, Kemeng Shi wrote:
> Function blkcg_policy_register only make sure pd_alloc_fn and pd_free_fn in
> pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL
> before use for pd_alloc_fn in blkcg_activate_policy to avoid protential
> NULL dereference.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
> ---
>  block/blk-cgroup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 463c568d3e86..fc083c35dc42 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q,
>  	if (blkcg_policy_enabled(q, pol))
>  		return 0;
>  
> +	if (pol->pd_alloc_fn == NULL)
> +		return -EINVAL;

This isn't the only place this function is called, so the above won't
achieve much. Given that this is rather trivially noticeable and all the
current users do implement pd_alloc_fn, I'm not sure we need to update this
now.

Thanks.

-- 
tejun

Re: [PATCH 3/4] blk-cgroup: Add NULL check of pd_alloc_fn in blkcg_activate_policy

[Kemeng Shi]

on 10/11/2022 4:29 AM, Tejun Heo wrote:
> On Mon, Oct 10, 2022 at 10:38:58AM +0800, Kemeng Shi wrote:
>> Function blkcg_policy_register only make sure pd_alloc_fn and pd_free_fn in
>> pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL
>> before use for pd_alloc_fn in blkcg_activate_policy to avoid protential
>> NULL dereference.
>>
>> Signed-off-by: Kemeng Shi <shikemeng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  block/blk-cgroup.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 463c568d3e86..fc083c35dc42 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q,
>>  	if (blkcg_policy_enabled(q, pol))
>>  		return 0;
>>  
>> +	if (pol->pd_alloc_fn == NULL)
>> +		return -EINVAL;
> 
> This isn't the only place this function is called, so the above won't
> achieve much. Given that this is rather trivially noticeable and all the
> current users do implement pd_alloc_fn, I'm not sure we need to update this
> now.
Thanks for review. The rest call of this function will always protect by
blkcg_policy_enabled while policy only can be enabled if new added NULL
check is passed. So the new added NULL check enough.

By the way, the policy enable/disable work is direct call to
__set_bit(pol->plid, q->blkcg_pols) in blkcg_policy_enabled
and __clear_bit(pol->plid, q->blkcg_pols) in blkcg_deactivate_policy
which is not intuitive. Is it a good idea to add function
blkcg_policy_enable and blkcg_policy_disable to improve readability?

> 
> Thanks.
> 

-- 
Best wishes
Kemeng Shi

Re: [PATCH 3/4] blk-cgroup: Add NULL check of pd_alloc_fn in blkcg_activate_policy

[Yu Kuai]

Hi, Tejun

ÔÚ 2022/10/11 4:29, Tejun Heo дµÀ:
> On Mon, Oct 10, 2022 at 10:38:58AM +0800, Kemeng Shi wrote:
>> Function only make sure pd_alloc_fn and pd_free_fn in
>> pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL
>> before use for pd_alloc_fn in blkcg_activate_policy to avoid protential
>> NULL dereference.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
>> ---
>>   block/blk-cgroup.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 463c568d3e86..fc083c35dc42 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q,
>>   	if (blkcg_policy_enabled(q, pol))
>>   		return 0;
>>   
>> +	if (pol->pd_alloc_fn == NULL)
>> +		return -EINVAL;
> 
> This isn't the only place this function is called, so the above won't
> achieve much. Given that this is rather trivially noticeable and all the
> current users do implement pd_alloc_fn, I'm not sure we need to update this
> now.

It's right all the current users implement pd_alloc_fn, can we check
pd_alloc/free_fn NULL instead in blkcg_policy_register()?

Thanks,
Kuai
> 
> Thanks.
> 

[PATCH 4/4] blk-cgroup: Fix typo in comment

[Kemeng Shi]

Replace assocating with associating.
Replace assocaited with associated.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-cgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index fc083c35dc42..f723901ef9b9 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -205,7 +205,7 @@ static inline struct blkcg *blkcg_parent(struct blkcg *blkcg)
  * @q: request_queue the new blkg is associated with
  * @gfp_mask: allocation mask to use
  *
- * Allocate a new blkg assocating @blkcg and @q.
+ * Allocate a new blkg associating @blkcg and @q.
  */
 static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct request_queue *q,
 				   gfp_t gfp_mask)
@@ -602,7 +602,7 @@ EXPORT_SYMBOL_GPL(blkcg_print_blkgs);
  * @pd: policy private data of interest
  * @v: value to print
  *
- * Print @v to @sf for the device assocaited with @pd.
+ * Print @v to @sf for the device associated with @pd.
  */
 u64 __blkg_prfill_u64(struct seq_file *sf, struct blkg_policy_data *pd, u64 v)
 {
@@ -802,7 +802,7 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep);
 
 /**
  * blkg_conf_finish - finish up per-blkg config update
- * @ctx: blkg_conf_ctx intiailized by blkg_conf_prep()
+ * @ctx: blkg_conf_ctx initialized by blkg_conf_prep()
  *
  * Finish up after per-blkg config update.  This function must be paired
  * with blkg_conf_prep().
-- 
2.30.0

Re: [PATCH 4/4] blk-cgroup: Fix typo in comment

[Tejun Heo]

On Mon, Oct 10, 2022 at 10:38:59AM +0800, Kemeng Shi wrote:
> Replace assocating with associating.
> Replace assocaited with associated.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun