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
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 <[email protected]>
---
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
Replace assocating with associating.
Replace assocaited with associated.
Signed-off-by: Kemeng Shi <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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.
>