blk-throttle: Free up policy node associated with deleted rule
Subsystems (2)
PATCH 1/2 blk-throttle: Free up policy node associated with deleted rule
If a rule is being deleted, free up associated policy node. Otherwise that memory is leaked.
--- block/blk-cgroup.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
block/blk-cgroup.c +1 -0
@@ -1085,6 +1085,7 @@ static int blkiocg_file_write(struct cgroup *cgrp, struct cftype *cft,if (blkio_delete_rule_command (newpn)) {blkio_policy_delete_node (pn); +kfree (pn);spin_unlock_irq (&blkcg->lock); goto update_io_group; }
Signature · 1 line
1.7.4.4
PATCH 2/2 blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
blkcg->policy_list is protected by blkcg->lock. Its not rcu protected list. So even for readers, they need to take blkcg->lock. There are few functions which were reading the list without taking lock. Fix it.
--- block/blk-cgroup.c | 54 ++++++++++++++++++++++++++++++++++++++------------- 1 files changed, 40 insertions(+), 14 deletions(-)
block/blk-cgroup.c +40 -14
@@ -879,60 +879,86 @@ unsigned int blkcg_get_weight(struct blkio_cgroup *blkcg, dev_t dev) {struct blkio_policy_node *pn; +unsigned long flags; +unsigned int weight; + +spin_lock_irqsave (&blkcg->lock, flags); pn =blkio_policy_search_node (blkcg, dev,BLKIO_POLICY_PROP , BLKIO_PROP_weight_device);if (pn) -return pn->val.weight; + weight = pn->val.weight;else -return blkcg->weight; + weight = blkcg->weight; + +spin_unlock_irqrestore (&blkcg->lock, flags); + +return weight; }EXPORT_SYMBOL_GPL (blkcg_get_weight);uint64_t blkcg_get_read_bps (struct blkio_cgroup *blkcg,dev_t dev) {struct blkio_policy_node *pn; +unsigned long flags; +uint64_t bps =-1 ; +spin_lock_irqsave (&blkcg->lock, flags); pn =blkio_policy_search_node (blkcg, dev,BLKIO_POLICY_THROTL , BLKIO_THROTL_read_bps_device);if (pn) -return pn->val.bps; -else -return -1 ; + bps = pn->val.bps; +spin_unlock_irqrestore (&blkcg->lock, flags); + +return bps; }uint64_t blkcg_get_write_bps (struct blkio_cgroup *blkcg,dev_t dev) {struct blkio_policy_node *pn; +unsigned long flags; +uint64_t bps =-1 ; + +spin_lock_irqsave (&blkcg->lock, flags); pn =blkio_policy_search_node (blkcg, dev,BLKIO_POLICY_THROTL , BLKIO_THROTL_write_bps_device);if (pn) -return pn->val.bps; -else -return -1 ; + bps = pn->val.bps; +spin_unlock_irqrestore (&blkcg->lock, flags); + +return bps; }unsigned int blkcg_get_read_iops (struct blkio_cgroup *blkcg,dev_t dev) {struct blkio_policy_node *pn; +unsigned long flags; +unsigned int iops =-1 ; +spin_lock_irqsave (&blkcg->lock, flags); pn =blkio_policy_search_node (blkcg, dev,BLKIO_POLICY_THROTL , BLKIO_THROTL_read_iops_device);if (pn) -return pn->val.iops; -else -return -1 ; + iops = pn->val.iops; +spin_unlock_irqrestore (&blkcg->lock, flags); + +return iops; }unsigned int blkcg_get_write_iops (struct blkio_cgroup *blkcg,dev_t dev) {struct blkio_policy_node *pn; +unsigned long flags; +unsigned int iops =-1 ; + +spin_lock_irqsave (&blkcg->lock, flags); pn =blkio_policy_search_node (blkcg, dev,BLKIO_POLICY_THROTL , BLKIO_THROTL_write_iops_device);if (pn) -return pn->val.iops; -else -return -1 ; + iops = pn->val.iops; +spin_unlock_irqrestore (&blkcg->lock, flags); + +return iops; }/* Checks whether user asked for deleting a policy rule */
Signature · 1 line
1.7.4.4
Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
Hello,
Quoted 5 lines · Vivek Goyal
blkcg->policy_listis protected byblkcg->lock. Its not rcu protected list. So even for readers, they need to takeblkcg->lock. There are few functions which were reading the list without taking lock. Fix it.Signed-off-by Vivek Goyal <[email protected]>
And yeap, some of rcu usages in cfq/iocg seem either incorrect or unnecessary. Trying to clean up that now too.
Thank you.
Signature · 1 line
tejun
Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
Quoted 7 lines · Tejun Heo
Hello,
Quoted 4 lines · Vivek Goyal
blkcg->policy_listis protected byblkcg->lock. Its not rcu protected list. So even for readers, they need to takeblkcg->lock. There are few functions which were reading the list without taking lock. Fix it.
Quoted 6 lines
Quoted 1 line
Signed-off-by Vivek Goyal <[email protected]>Acked-by Tejun Heo <[email protected]>And yeap, some of rcu usages in cfq/iocg seem either incorrect or unnecessary. Trying to clean up that now too.
Cool. I am waiting for the patches as I am curious to know what's incorrect or unnecessary.
I noticed above two problems while trying to figure out how can we cleanup the device rules when devices goes away. One place could be.
blk_throtl_exit() {
blkiocg_del_blkio_group() {
delete_any_policy_nodes_associated_with_this device;
}
}The only problem with this approach is that it will cleanup per device weight rules also at elevator_exit() time which is not same as device removal and one might device to bring CFQ back on device and we will need the rules again.
cfq_exit_queue()
cfq_release_cfq_groups
cfq_blkiocg_del_blkio_group()
blkiocg_del_blkio_group()
Thanks
Vivek
Tejun Heo
maintainerRe: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
Hello,
Quoted 4 lines · Vivek Goyal
The only problem with this approach is that it will cleanup per device weight rules also at
elevator_exit()time which is not same as device removal and one might device to bring CFQ back on device and we will need the rules again.
I actually think removoing those rules on elevator detach would be the right thing to do. We don't try to keep cfq setting across elevator switch. When we're switching from cfq, we're detaching iocg policy too. The settings going away is perfectly fine. I actually think it's a pretty bad idea to implement ad-hoc setting persistence in kernel. Just making sure that userland is notified is far better approach. Userland has all the facilities to deal with this type of situations.
When switching from cfq to deadline, we lose the whole proportional io control. It's way more confusing to have lingering settings which don't do anything.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
Quoted 17 lines · Tejun Heo
Hello,
Quoted 4 lines · Vivek Goyal
The only problem with this approach is that it will cleanup per device weight rules also at
elevator_exit()time which is not same as device removal and one might device to bring CFQ back on device and we will need the rules again.I actually think removoing those rules on elevator detach would be the right thing to do. We don't try to keep cfq setting across elevator switch. When we're switching from cfq, we're detaching iocg policy too. The settings going away is perfectly fine. I actually think it's a pretty bad idea to implement ad-hoc setting persistence in kernel. Just making sure that userland is notified is far better approach. Userland has all the facilities to deal with this type of situations.
Quoted 3 lines
When switching from cfq to deadline, we lose the whole proportional io control. It's way more confusing to have lingering settings which don't do anything.
I am not so sure about this. Suppose tomorrow another IO sheduler starts taking into account the cgroup gloabl weight or cgroup per device weight to do some kind of IO prioritization, then removing the rules upon changing the IO schduler will not make sense.
IOW, rules are per cgroup per device and not per cgroup per IO scheduler and more than one IO scheduler should be able to share the rules.
Thanks
Vivek
Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
Quoted 22 lines · Vivek Goyal
Quoted 17 lines · Tejun Heo
Hello,
Quoted 4 lines · Vivek Goyal
The only problem with this approach is that it will cleanup per device weight rules also at
elevator_exit()time which is not same as device removal and one might device to bring CFQ back on device and we will need the rules again.I actually think removoing those rules on elevator detach would be the right thing to do. We don't try to keep cfq setting across elevator switch. When we're switching from cfq, we're detaching iocg policy too. The settings going away is perfectly fine. I actually think it's a pretty bad idea to implement ad-hoc setting persistence in kernel. Just making sure that userland is notified is far better approach. Userland has all the facilities to deal with this type of situations.
Quoted 3 lines
When switching from cfq to deadline, we lose the whole proportional io control. It's way more confusing to have lingering settings which don't do anything.
Quoted 7 lines
I am not so sure about this. Suppose tomorrow another IO sheduler starts taking into account the cgroup gloabl weight or cgroup per device weight to do some kind of IO prioritization, then removing the rules upon changing the IO schduler will not make sense.
IOW, rules are per cgroup per device and not per cgroup per IO scheduler and more than one IO scheduler should be able to share the rules.
FWIW, I agree with Tejun here. A switch operation is a reset, start from scratch. We don't preserve other per IO-scheduler settings on a switch, preserving _some_ settings is just confusing.
Signature · 1 line
Jens Axboe
Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list
Quoted 28 lines · Jens Axboe
Quoted 27 lines · Vivek Goyal
Quoted 17 lines · Tejun Heo
Hello,
Quoted 4 lines · Vivek Goyal
The only problem with this approach is that it will cleanup per device weight rules also at
elevator_exit()time which is not same as device removal and one might device to bring CFQ back on device and we will need the rules again.I actually think removoing those rules on elevator detach would be the right thing to do. We don't try to keep cfq setting across elevator switch. When we're switching from cfq, we're detaching iocg policy too. The settings going away is perfectly fine. I actually think it's a pretty bad idea to implement ad-hoc setting persistence in kernel. Just making sure that userland is notified is far better approach. Userland has all the facilities to deal with this type of situations.
Quoted 3 lines
When switching from cfq to deadline, we lose the whole proportional io control. It's way more confusing to have lingering settings which don't do anything.
I am not so sure about this. Suppose tomorrow another IO sheduler starts taking into account the cgroup gloabl weight or cgroup per device weight to do some kind of IO prioritization, then removing the rules upon changing the IO schduler will not make sense.
Quoted 6 lines
Quoted 2 lines
IOW, rules are per cgroup per device and not per cgroup per IO scheduler and more than one IO scheduler should be able to share the rules.
FWIW, I agree with Tejun here. A switch operation is a reset, start from scratch. We don't preserve other per IO-scheduler settings on a switch, preserving _some_ settings is just confusing.
Ok. But this is more of a per queue setting (per cgroup, per device) and not per IO scheduler one. That's a different thing that currently only CFQ makes use of it.
If we start looking at them just as CFQ specific weigths, then it is a different story. My thought process about these files was per cgroup per device weights.
Thanks
Vivek
Hello,
Quoted 4 lines · Vivek Goyal
If a rule is being deleted, free up associated policy node. Otherwise that memory is leaked.
Signed-off-by Vivek Goyal <[email protected]>
Heh, yeah, have the exactly same patch in my queue.
Thanks.
Signature · 1 line
tejun
Quoted 4 lines · Vivek Goyal
If a rule is being deleted, free up associated policy node. Otherwise that memory is leaked.
Signed-off-by Vivek Goyal <[email protected]>
Hi Jens,
Do you see any issues with the two patches. Can you please consider these for merging.
Thanks
Vivek
Quoted 18 lines
--- block/blk-cgroup.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)block/blk-cgroup.c +1
-0@@ -1085,6 +1085,7 @@ static int blkiocg_file_write(struct cgroup *cgrp, struct cftype *cft,if (blkio_delete_rule_command (newpn)) {blkio_policy_delete_node (pn); +kfree (pn);spin_unlock_irq (&blkcg->lock); goto update_io_group; }Signature · 1 line
1.7.4.4
Jens Axboe
maintainerQuoted 10 lines · Vivek Goyal
Quoted 4 lines · Vivek Goyal
If a rule is being deleted, free up associated policy node. Otherwise that memory is leaked.
Signed-off-by Vivek Goyal <[email protected]>Hi Jens,
Do you see any issues with the two patches. Can you please consider these for merging.
They are in, thanks.
Signature · 1 line
Jens Axboe