| Message ID | 20240711090059.3998565-3-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-ioprio: remove per-disk structure | expand |
On 7/11/24 2:00 AM, 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. As one can see in the output of git grep -nHEB1 '>pd_(alloc|free)_fn\(', none of the pd_alloc_fn / pd_free_fn callers checks whether or not these pointers are NULL. Hence my question why this patch does not trigger any NULL pointer dereferences? Thanks, Bart.
Hi, 在 2024/07/12 2:03, Bart Van Assche 写道: > On 7/11/24 2:00 AM, 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. > > As one can see in the output of git grep -nHEB1 '>pd_(alloc|free)_fn\(', > none of the pd_alloc_fn / pd_free_fn callers checks whether or not these > pointers are NULL. Hence my question why this patch does not trigger any > NULL pointer dereferences? Because the blkcg_deactivate_policy() is removed as well, there are no callers now... blkcg_policy_register() is still called to make sure cpd_(alloc|free)_fn will still be called. Thanks, Kuai > > Thanks, > > Bart. > . >
Hi, 在 2024/07/12 9:10, Yu Kuai 写道: > Hi, > > 在 2024/07/12 2:03, Bart Van Assche 写道: >> On 7/11/24 2:00 AM, 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. >> >> As one can see in the output of git grep -nHEB1 '>pd_(alloc|free)_fn\(', >> none of the pd_alloc_fn / pd_free_fn callers checks whether or not these >> pointers are NULL. Hence my question why this patch does not trigger any >> NULL pointer dereferences? > > Because the blkcg_deactivate_policy() is removed as well, there are no Sorry about the typo, blkcg_activate_policy() is removed, all the places of pd_(alloc|free)_fn can't be reached. Thanks, Kuai > callers now... blkcg_policy_register() is still called to make sure > cpd_(alloc|free)_fn will still be called. > > Thanks, > Kuai > >> >> Thanks, >> >> Bart. >> . >> > > . >
On Fri, Jul 12, 2024 at 09:10:30AM +0800, Yu Kuai wrote: > > As one can see in the output of git grep -nHEB1 '>pd_(alloc|free)_fn\(', > > none of the pd_alloc_fn / pd_free_fn callers checks whether or not these > > pointers are NULL. Hence my question why this patch does not trigger any > > NULL pointer dereferences? > > Because the blkcg_deactivate_policy() is removed as well, there are no > callers now... blkcg_policy_register() is still called to make sure > cpd_(alloc|free)_fn will still be called. Can you throw in a patch documenting this? Any maybe add a check that pd_alloc_fn / pd_free_fn exist in blkcg_activate_policy and WARN and return an error otherwise?
Hi, 在 2024/07/12 13:22, Christoph Hellwig 写道: > On Fri, Jul 12, 2024 at 09:10:30AM +0800, Yu Kuai wrote: >>> As one can see in the output of git grep -nHEB1 '>pd_(alloc|free)_fn\(', >>> none of the pd_alloc_fn / pd_free_fn callers checks whether or not these >>> pointers are NULL. Hence my question why this patch does not trigger any >>> NULL pointer dereferences? >> >> Because the blkcg_deactivate_policy() is removed as well, there are no >> callers now... blkcg_policy_register() is still called to make sure >> cpd_(alloc|free)_fn will still be called. > > Can you throw in a patch documenting this? Any maybe add a check > that pd_alloc_fn / pd_free_fn exist in blkcg_activate_policy and > WARN and return an error otherwise? Of course, I realized now that just mention don't activate the policy from commit message is too little explanation. Thanks, Kuai > > . >
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 37e6cc91d576..c7d00d95d499 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) { }