[PATCH v2 0/3] blk-ioprio: remove per-disk structure

[PATCH v2 0/3] blk-ioprio: remove per-disk structure

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v2:
 - add patch 1;

The idea is that ioprio doesn't need to access blkg, all it needs is
blkcg, hence blk_register_policy() is enough, and blk_activate_policy()
is not needed.

Yu Kuai (3):
  blk-cgroup: check for pd_(alloc|free)_fn in blkcg_activate_policy()
  blk-ioprio: remove ioprio_blkcg_from_bio()
  blk-ioprio: remove per-disk structure

 block/blk-cgroup.c | 21 +++++++++--------
 block/blk-ioprio.c | 57 +---------------------------------------------
 block/blk-ioprio.h |  9 --------
 3 files changed, 13 insertions(+), 74 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] blk-cgroup: check for pd_(alloc|free)_fn in blkcg_activate_policy()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently all policies implement pd_(alloc|free)_fn, however, this is
not necessary for ioprio that only works for blkcg, not blkg.

There are no functional changes, prepare to cleanup activating ioprio
policy.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 37e6cc91d576..dcd5e857650a 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1554,6 +1554,14 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 	if (blkcg_policy_enabled(q, pol))
 		return 0;
 
+	/*
+	 * Policy is allowed to be registered without pd_alloc_fn/pd_free_fn,
+	 * for example, ioprio. Such policy will work on blkcg level, not disk
+	 * level, and don't need to be activated.
+	 */
+	if (WARN_ON_ONCE(!pol->pd_alloc_fn || !pol->pd_free_fn))
+		return -EINVAL;
+
 	if (queue_is_mq(q))
 		blk_mq_freeze_queue(q);
 retry:
@@ -1733,7 +1741,10 @@ int blkcg_policy_register(struct blkcg_policy *pol)
 		goto err_unlock;
 	}
 
-	/* Make sure cpd/pd_alloc_fn and cpd/pd_free_fn in pairs */
+	/*
+	 * Make sure cpd/pd_alloc_fn and cpd/pd_free_fn in pairs, and policy
+	 * without pd_alloc_fn/pd_free_fn can't be activated.
+	 */
 	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
 		(!pol->pd_alloc_fn ^ !pol->pd_free_fn))
 		goto err_unlock;
-- 
2.39.2

Re: [PATCH v2 1/3] blk-cgroup: check for pd_(alloc|free)_fn in blkcg_activate_policy()

[Christoph Hellwig]

On Fri, Jul 19, 2024 at 10:34:29AM +0800, Yu Kuai wrote:
> +	/*
> +	 * Make sure cpd/pd_alloc_fn and cpd/pd_free_fn in pairs, and policy
> +	 * without pd_alloc_fn/pd_free_fn can't be activated.
> +	 */
>  	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>  		(!pol->pd_alloc_fn ^ !pol->pd_free_fn))
>  		goto err_unlock;
> -- 

I know this is existing code, but can you fix up the incorrect
indentation while you touch this:

 	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
 	    (!pol->pd_alloc_fn ^ !pol->pd_free_fn))

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/3] blk-cgroup: check for pd_(alloc|free)_fn in blkcg_activate_policy()

[Yu Kuai]

Hi!

在 2024/07/19 13:29, Christoph Hellwig 写道:
> On Fri, Jul 19, 2024 at 10:34:29AM +0800, Yu Kuai wrote:
>> +	/*
>> +	 * Make sure cpd/pd_alloc_fn and cpd/pd_free_fn in pairs, and policy
>> +	 * without pd_alloc_fn/pd_free_fn can't be activated.
>> +	 */
>>   	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>>   		(!pol->pd_alloc_fn ^ !pol->pd_free_fn))
>>   		goto err_unlock;
>> -- 
> 
> I know this is existing code, but can you fix up the incorrect
> indentation while you touch this:

Yes, and thanks for the review.

Kuai

> 
>   	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>   	    (!pol->pd_alloc_fn ^ !pol->pd_free_fn))
> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> .
> 

Re: [PATCH v2 1/3] blk-cgroup: check for pd_(alloc|free)_fn in blkcg_activate_policy()

[Bart Van Assche]

On 7/18/24 10:29 PM, Christoph Hellwig wrote:
> On Fri, Jul 19, 2024 at 10:34:29AM +0800, Yu Kuai wrote:
>> +	/*
>> +	 * Make sure cpd/pd_alloc_fn and cpd/pd_free_fn in pairs, and policy
>> +	 * without pd_alloc_fn/pd_free_fn can't be activated.
>> +	 */
>>   	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>>   		(!pol->pd_alloc_fn ^ !pol->pd_free_fn))
>>   		goto err_unlock;
>> -- 
> 
> I know this is existing code, but can you fix up the incorrect
> indentation while you touch this:
> 
>   	if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>   	    (!pol->pd_alloc_fn ^ !pol->pd_free_fn))

Using xor (^) for booleans seems weird to me. Is there any preference in
Linux kernel code whether to use ^ or != to check whether to booleans
are different?

Thanks,

Bart.

Re: [PATCH v2 1/3] blk-cgroup: check for pd_(alloc|free)_fn in blkcg_activate_policy()

[Yu Kuai]

Hi,

在 2024/07/20 0:25, Bart Van Assche 写道:
> On 7/18/24 10:29 PM, Christoph Hellwig wrote:
>> On Fri, Jul 19, 2024 at 10:34:29AM +0800, Yu Kuai wrote:
>>> +    /*
>>> +     * Make sure cpd/pd_alloc_fn and cpd/pd_free_fn in pairs, and 
>>> policy
>>> +     * without pd_alloc_fn/pd_free_fn can't be activated.
>>> +     */
>>>       if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>>>           (!pol->pd_alloc_fn ^ !pol->pd_free_fn))
>>>           goto err_unlock;
>>> -- 
>>
>> I know this is existing code, but can you fix up the incorrect
>> indentation while you touch this:
>>
>>       if ((!pol->cpd_alloc_fn ^ !pol->cpd_free_fn) ||
>>           (!pol->pd_alloc_fn ^ !pol->pd_free_fn))
> 
> Using xor (^) for booleans seems weird to me. Is there any preference in
> Linux kernel code whether to use ^ or != to check whether to booleans
> are different?

I don't know, but I feel more comfortable to use '!=' myself.

Thanks,
Kuai

> 
> Thanks,
> 
> Bart.
> 
> .
> 

[PATCH v2 2/3] blk-ioprio: remove ioprio_blkcg_from_bio()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, if config is enabled, then ioprio is always enabled by
default from blkcg_init_disk(), hence there is no point to check if
the policy is enabled from blkg in ioprio_blkcg_from_bio(). Hence remove
it and get blkcg directly from bio.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-ioprio.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 4051fada01f1..ae52b418e984 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -84,16 +84,6 @@ ioprio_blkcg_from_css(struct cgroup_subsys_state *css)
 	return blkcg_to_ioprio_blkcg(css_to_blkcg(css));
 }
 
-static struct ioprio_blkcg *ioprio_blkcg_from_bio(struct bio *bio)
-{
-	struct blkg_policy_data *pd = blkg_to_pd(bio->bi_blkg, &ioprio_policy);
-
-	if (!pd)
-		return NULL;
-
-	return blkcg_to_ioprio_blkcg(pd->blkg->blkcg);
-}
-
 static int ioprio_show_prio_policy(struct seq_file *sf, void *v)
 {
 	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_css(seq_css(sf));
@@ -186,7 +176,7 @@ static struct blkcg_policy ioprio_policy = {
 
 void blkcg_set_ioprio(struct bio *bio)
 {
-	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
+	struct ioprio_blkcg *blkcg = blkcg_to_ioprio_blkcg(bio->bi_blkg->blkcg);
 	u16 prio;
 
 	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
-- 
2.39.2

Re: [PATCH v2 2/3] blk-ioprio: remove ioprio_blkcg_from_bio()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/3] blk-ioprio: remove ioprio_blkcg_from_bio()

[Bart Van Assche]

On 7/18/24 7:34 PM, Yu Kuai wrote:
> Currently, if config is enabled, then ioprio is always enabled by
> default from blkcg_init_disk(), hence there is no point to check if
> the policy is enabled from blkg in ioprio_blkcg_from_bio(). Hence remove
> it and get blkcg directly from bio.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 3/3] blk-ioprio: remove per-disk structure

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

ioprio works on the blk-cgroup level, all disks in the same cgroup
are the same, and the struct ioprio_blkg doesn't have anything in it.
Hence register the policy is enough, because cpd_alloc/free_fn will
be handled for each blk-cgroup, and there is no need to activate the
policy for disk. Hence remove blk_ioprio_init/exit and
ioprio_alloc/free_pd.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup.c |  8 --------
 block/blk-ioprio.c | 45 ---------------------------------------------
 block/blk-ioprio.h |  9 ---------
 3 files changed, 62 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index dcd5e857650a..425dd1b33a4d 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1458,7 +1458,6 @@ int blkcg_init_disk(struct gendisk *disk)
 	struct request_queue *q = disk->queue;
 	struct blkcg_gq *new_blkg, *blkg;
 	bool preloaded;
-	int ret;
 
 	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
 	if (!new_blkg)
@@ -1478,15 +1477,8 @@ int blkcg_init_disk(struct gendisk *disk)
 	if (preloaded)
 		radix_tree_preload_end();
 
-	ret = blk_ioprio_init(disk);
-	if (ret)
-		goto err_destroy_all;
-
 	return 0;
 
-err_destroy_all:
-	blkg_destroy_all(disk);
-	return ret;
 err_unlock:
 	spin_unlock_irq(&q->queue_lock);
 	if (preloaded)
diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index ae52b418e984..8fff7ccc0ac7 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -49,14 +49,6 @@ static const char *policy_name[] = {
 
 static struct blkcg_policy ioprio_policy;
 
-/**
- * struct ioprio_blkg - Per (cgroup, request queue) data.
- * @pd: blkg_policy_data structure.
- */
-struct ioprio_blkg {
-	struct blkg_policy_data pd;
-};
-
 /**
  * struct ioprio_blkcg - Per cgroup data.
  * @cpd: blkcg_policy_data structure.
@@ -67,11 +59,6 @@ struct ioprio_blkcg {
 	enum prio_policy	 prio_policy;
 };
 
-static inline struct ioprio_blkg *pd_to_ioprio(struct blkg_policy_data *pd)
-{
-	return pd ? container_of(pd, struct ioprio_blkg, pd) : NULL;
-}
-
 static struct ioprio_blkcg *blkcg_to_ioprio_blkcg(struct blkcg *blkcg)
 {
 	return container_of(blkcg_to_cpd(blkcg, &ioprio_policy),
@@ -108,25 +95,6 @@ static ssize_t ioprio_set_prio_policy(struct kernfs_open_file *of, char *buf,
 	return nbytes;
 }
 
-static struct blkg_policy_data *
-ioprio_alloc_pd(struct gendisk *disk, struct blkcg *blkcg, gfp_t gfp)
-{
-	struct ioprio_blkg *ioprio_blkg;
-
-	ioprio_blkg = kzalloc(sizeof(*ioprio_blkg), gfp);
-	if (!ioprio_blkg)
-		return NULL;
-
-	return &ioprio_blkg->pd;
-}
-
-static void ioprio_free_pd(struct blkg_policy_data *pd)
-{
-	struct ioprio_blkg *ioprio_blkg = pd_to_ioprio(pd);
-
-	kfree(ioprio_blkg);
-}
-
 static struct blkcg_policy_data *ioprio_alloc_cpd(gfp_t gfp)
 {
 	struct ioprio_blkcg *blkcg;
@@ -169,9 +137,6 @@ static struct blkcg_policy ioprio_policy = {
 
 	.cpd_alloc_fn	= ioprio_alloc_cpd,
 	.cpd_free_fn	= ioprio_free_cpd,
-
-	.pd_alloc_fn	= ioprio_alloc_pd,
-	.pd_free_fn	= ioprio_free_pd,
 };
 
 void blkcg_set_ioprio(struct bio *bio)
@@ -209,16 +174,6 @@ void blkcg_set_ioprio(struct bio *bio)
 		bio->bi_ioprio = prio;
 }
 
-void blk_ioprio_exit(struct gendisk *disk)
-{
-	blkcg_deactivate_policy(disk, &ioprio_policy);
-}
-
-int blk_ioprio_init(struct gendisk *disk)
-{
-	return blkcg_activate_policy(disk, &ioprio_policy);
-}
-
 static int __init ioprio_init(void)
 {
 	return blkcg_policy_register(&ioprio_policy);
diff --git a/block/blk-ioprio.h b/block/blk-ioprio.h
index b6afb8e80de0..9265143f9bc9 100644
--- a/block/blk-ioprio.h
+++ b/block/blk-ioprio.h
@@ -9,17 +9,8 @@ struct request_queue;
 struct bio;
 
 #ifdef CONFIG_BLK_CGROUP_IOPRIO
-int blk_ioprio_init(struct gendisk *disk);
-void blk_ioprio_exit(struct gendisk *disk);
 void blkcg_set_ioprio(struct bio *bio);
 #else
-static inline int blk_ioprio_init(struct gendisk *disk)
-{
-	return 0;
-}
-static inline void blk_ioprio_exit(struct gendisk *disk)
-{
-}
 static inline void blkcg_set_ioprio(struct bio *bio)
 {
 }
-- 
2.39.2

Re: [PATCH v2 3/3] blk-ioprio: remove per-disk structure

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/3] blk-ioprio: remove per-disk structure

[Bart Van Assche]

On 7/18/24 7:34 PM, Yu Kuai wrote:
> ioprio works on the blk-cgroup level, all disks in the same cgroup
> are the same, and the struct ioprio_blkg doesn't have anything in it.
> Hence register the policy is enough, because cpd_alloc/free_fn will
> be handled for each blk-cgroup, and there is no need to activate the
> policy for disk. Hence remove blk_ioprio_init/exit and
> ioprio_alloc/free_pd.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>