[PATCH 1/2] blk-throttle: Free up policy node associated with deleted rule

[PATCH 1/2] blk-throttle: Free up policy node associated with deleted rule

[Vivek Goyal]

If a rule is being deleted, free up associated policy node. Otherwise
that memory is leaked.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-cgroup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index b596e54..4ddf11f 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -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;
 	}
-- 
1.7.4.4

[PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list

[Vivek Goyal]

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.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-cgroup.c |   54 ++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4ddf11f..1b5d789 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -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 */
-- 
1.7.4.4

Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list

[Tejun Heo]

Hello,

On Thu, Oct 20, 2011 at 05:08:26PM -0400, Vivek Goyal wrote:
> 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.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

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

And yeap, some of rcu usages in cfq/iocg seem either incorrect or
unnecessary.  Trying to clean up that now too.

Thank you.

-- 
tejun

Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list

[Vivek Goyal]

On Thu, Oct 20, 2011 at 02:11:40PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Thu, Oct 20, 2011 at 05:08:26PM -0400, Vivek Goyal wrote:
> > 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.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
>  Acked-by: Tejun Heo <tj@kernel.org>
> 
> 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

Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list

[Tejun Heo]

Hello,

On Thu, Oct 20, 2011 at 05:20:21PM -0400, Vivek Goyal wrote:
> 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.

-- 
tejun

Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list

[Vivek Goyal]

On Thu, Oct 20, 2011 at 02:29:58PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Thu, Oct 20, 2011 at 05:20:21PM -0400, Vivek Goyal wrote:
> > 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.

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

[Jens Axboe]

On 2011-10-21 14:10, Vivek Goyal wrote:
> On Thu, Oct 20, 2011 at 02:29:58PM -0700, Tejun Heo wrote:
>> Hello,
>>
>> On Thu, Oct 20, 2011 at 05:20:21PM -0400, Vivek Goyal wrote:
>>> 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.
> 
> 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.

-- 
Jens Axboe

Re: [PATCH 2/2] blk-throttle: Take blkcg->lock while traversing blkcg->policy_list

[Vivek Goyal]

On Tue, Oct 25, 2011 at 04:13:11PM +0200, Jens Axboe wrote:
> On 2011-10-21 14:10, Vivek Goyal wrote:
> > On Thu, Oct 20, 2011 at 02:29:58PM -0700, Tejun Heo wrote:
> >> Hello,
> >>
> >> On Thu, Oct 20, 2011 at 05:20:21PM -0400, Vivek Goyal wrote:
> >>> 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.
> > 
> > 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.

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

Re: [PATCH 1/2] blk-throttle: Free up policy node associated with deleted rule

[Tejun Heo]

Hello,

On Thu, Oct 20, 2011 at 05:08:25PM -0400, Vivek Goyal wrote:
> If a rule is being deleted, free up associated policy node. Otherwise
> that memory is leaked.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Heh, yeah, have the exactly same patch in my queue.

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

Thanks.

-- 
tejun

Re: [PATCH 1/2] blk-throttle: Free up policy node associated with deleted rule

[Vivek Goyal]

On Thu, Oct 20, 2011 at 05:08:25PM -0400, Vivek Goyal wrote:
> If a rule is being deleted, free up associated policy node. Otherwise
> that memory is leaked.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Hi Jens,

Do you see any issues with the two patches. Can you please consider these
for merging.

Thanks
Vivek

> ---
>  block/blk-cgroup.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index b596e54..4ddf11f 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -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;
>  	}
> -- 
> 1.7.4.4

Re: [PATCH 1/2] blk-throttle: Free up policy node associated with deleted rule

[Jens Axboe]

On 2011-10-25 15:36, Vivek Goyal wrote:
> On Thu, Oct 20, 2011 at 05:08:25PM -0400, Vivek Goyal wrote:
>> If a rule is being deleted, free up associated policy node. Otherwise
>> that memory is leaked.
>>
>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Hi Jens,
> 
> Do you see any issues with the two patches. Can you please consider these
> for merging.

They are in, thanks.

-- 
Jens Axboe