[PATCH] cfq: Remove useless css reference get

[PATCH] cfq: Remove useless css reference get

[Gui Jianfeng]

There's no need to take css reference here, for the caller
has already called rcu_read_lock() to prevent cgroup from 
being removed.

Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
---
 block/blk-cgroup.c  |   14 --------------
 block/blk-cgroup.h  |    3 ---
 block/cfq-iosched.c |    5 -----
 3 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1fa2654..cba28f4 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -23,20 +23,6 @@ static LIST_HEAD(blkio_list);
 struct blkio_cgroup blkio_root_cgroup = { .weight = 2*BLKIO_WEIGHT_DEFAULT };
 EXPORT_SYMBOL_GPL(blkio_root_cgroup);
 
-bool blkiocg_css_tryget(struct blkio_cgroup *blkcg)
-{
-	if (!css_tryget(&blkcg->css))
-		return false;
-	return true;
-}
-EXPORT_SYMBOL_GPL(blkiocg_css_tryget);
-
-void blkiocg_css_put(struct blkio_cgroup *blkcg)
-{
-	css_put(&blkcg->css);
-}
-EXPORT_SYMBOL_GPL(blkiocg_css_put);
-
 struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
 {
 	return container_of(cgroup_subsys_state(cgroup, blkio_subsys_id),
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 4d316df..84bf745 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -43,9 +43,6 @@ struct blkio_group {
 	unsigned long sectors;
 };
 
-extern bool blkiocg_css_tryget(struct blkio_cgroup *blkcg);
-extern void blkiocg_css_put(struct blkio_cgroup *blkcg);
-
 typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
 typedef void (blkio_update_group_weight_fn) (struct blkio_group *blkg,
 						unsigned int weight);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index e2f8046..5d6b427 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -958,10 +958,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
 	struct backing_dev_info *bdi = &cfqd->queue->backing_dev_info;
 	unsigned int major, minor;
 
-	/* Do we need to take this reference */
-	if (!blkiocg_css_tryget(blkcg))
-		return NULL;;
-
 	cfqg = cfqg_of_blkg(blkiocg_lookup_group(blkcg, key));
 	if (cfqg || !create)
 		goto done;
@@ -992,7 +988,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
 	hlist_add_head(&cfqg->cfqd_node, &cfqd->cfqg_list);
 
 done:
-	blkiocg_css_put(blkcg);
 	return cfqg;
 }
 
-- 
1.5.4.rc3

Re: [PATCH] cfq: Remove useless css reference get

[Corrado Zoccolo]

Hi Gui,
maybe you can add a comment that cfq_find_alloc_cfqg can now be called
only under rcu_read_lock().

On Wed, Dec 16, 2009 at 9:38 AM, Gui Jianfeng
<guijianfeng@cn.fujitsu.com> wrote:
> There's no need to take css reference here, for the caller
> has already called rcu_read_lock() to prevent cgroup from
> being removed.
>
> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  block/blk-cgroup.c  |   14 --------------
>  block/blk-cgroup.h  |    3 ---
>  block/cfq-iosched.c |    5 -----
>  3 files changed, 0 insertions(+), 22 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 1fa2654..cba28f4 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -23,20 +23,6 @@ static LIST_HEAD(blkio_list);
>  struct blkio_cgroup blkio_root_cgroup = { .weight = 2*BLKIO_WEIGHT_DEFAULT };
>  EXPORT_SYMBOL_GPL(blkio_root_cgroup);
>
> -bool blkiocg_css_tryget(struct blkio_cgroup *blkcg)
> -{
> -       if (!css_tryget(&blkcg->css))
> -               return false;
> -       return true;
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_css_tryget);
> -
> -void blkiocg_css_put(struct blkio_cgroup *blkcg)
> -{
> -       css_put(&blkcg->css);
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_css_put);
> -
>  struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
>  {
>        return container_of(cgroup_subsys_state(cgroup, blkio_subsys_id),
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index 4d316df..84bf745 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -43,9 +43,6 @@ struct blkio_group {
>        unsigned long sectors;
>  };
>
> -extern bool blkiocg_css_tryget(struct blkio_cgroup *blkcg);
> -extern void blkiocg_css_put(struct blkio_cgroup *blkcg);
> -
>  typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
>  typedef void (blkio_update_group_weight_fn) (struct blkio_group *blkg,
>                                                unsigned int weight);
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index e2f8046..5d6b427 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -958,10 +958,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>        struct backing_dev_info *bdi = &cfqd->queue->backing_dev_info;
>        unsigned int major, minor;
>
> -       /* Do we need to take this reference */
> -       if (!blkiocg_css_tryget(blkcg))
> -               return NULL;;
> -
>        cfqg = cfqg_of_blkg(blkiocg_lookup_group(blkcg, key));
>        if (cfqg || !create)
>                goto done;
> @@ -992,7 +988,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>        hlist_add_head(&cfqg->cfqd_node, &cfqd->cfqg_list);
>
>  done:
> -       blkiocg_css_put(blkcg);
>        return cfqg;
>  }
>
> --
> 1.5.4.rc3

Thanks,
Corrado

Re: [PATCH] cfq: Remove useless css reference get

[Jens Axboe]

On Wed, Dec 16 2009, Corrado Zoccolo wrote:
> Hi Gui,
> maybe you can add a comment that cfq_find_alloc_cfqg can now be called
> only under rcu_read_lock().

I'll add that while committing the patch.

-- 
Jens Axboe

Re: [PATCH] cfq: Remove useless css reference get

[Vivek Goyal]

On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
> There's no need to take css reference here, for the caller
> has already called rcu_read_lock() to prevent cgroup from 
> being removed.
> 
> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

Hi Gui,

How would an rcu lock protect against the possibility that blkiocg_destroy()
has not already been called on another cpu? rcu lock will make sure that
cgroup and blkio_cgroup object should still be around as long as I am
holding rcu lock but will not protect against deletion path being executed
on another cpu? So I don't want to end up in following situation.

     cpu1                      cpu2

rcu_read_lock()
			      blkiocg_destroy()

blkiocg_add_blkio_group()
rcu_read_unlock()

I don't want to add blkg object on a potentially dead blkio_cgroup object
which will go away. Does this protection is provided by generic cgroup
code where blkiocg_destroy() will not be called if I have got cgroup
pointer under rcu lock?

Currently we are deriving cgroup information from task context so task is
still inside cgroup, so cgroup can't be deleted anyway. What if I was 
deriving cgroup information from bio, will taking css object reference be
necessary in that case or just cgroup pointer under rcu lock is sufficient
to preclude the race against cgroup deletion path?

Thanks
Vivek

> ---
>  block/blk-cgroup.c  |   14 --------------
>  block/blk-cgroup.h  |    3 ---
>  block/cfq-iosched.c |    5 -----
>  3 files changed, 0 insertions(+), 22 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 1fa2654..cba28f4 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -23,20 +23,6 @@ static LIST_HEAD(blkio_list);
>  struct blkio_cgroup blkio_root_cgroup = { .weight = 2*BLKIO_WEIGHT_DEFAULT };
>  EXPORT_SYMBOL_GPL(blkio_root_cgroup);
>  
> -bool blkiocg_css_tryget(struct blkio_cgroup *blkcg)
> -{
> -	if (!css_tryget(&blkcg->css))
> -		return false;
> -	return true;
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_css_tryget);
> -
> -void blkiocg_css_put(struct blkio_cgroup *blkcg)
> -{
> -	css_put(&blkcg->css);
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_css_put);
> -
>  struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
>  {
>  	return container_of(cgroup_subsys_state(cgroup, blkio_subsys_id),
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index 4d316df..84bf745 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -43,9 +43,6 @@ struct blkio_group {
>  	unsigned long sectors;
>  };
>  
> -extern bool blkiocg_css_tryget(struct blkio_cgroup *blkcg);
> -extern void blkiocg_css_put(struct blkio_cgroup *blkcg);
> -
>  typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
>  typedef void (blkio_update_group_weight_fn) (struct blkio_group *blkg,
>  						unsigned int weight);
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index e2f8046..5d6b427 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -958,10 +958,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>  	struct backing_dev_info *bdi = &cfqd->queue->backing_dev_info;
>  	unsigned int major, minor;
>  
> -	/* Do we need to take this reference */
> -	if (!blkiocg_css_tryget(blkcg))
> -		return NULL;;
> -
>  	cfqg = cfqg_of_blkg(blkiocg_lookup_group(blkcg, key));
>  	if (cfqg || !create)
>  		goto done;
> @@ -992,7 +988,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>  	hlist_add_head(&cfqg->cfqd_node, &cfqd->cfqg_list);
>  
>  done:
> -	blkiocg_css_put(blkcg);
>  	return cfqg;
>  }
>  
> -- 
> 1.5.4.rc3
> 

Re: [PATCH] cfq: Remove useless css reference get

[Jens Axboe]

On Wed, Dec 16 2009, Vivek Goyal wrote:
> On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
> > There's no need to take css reference here, for the caller
> > has already called rcu_read_lock() to prevent cgroup from 
> > being removed.
> > 
> > Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> > Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> Hi Gui,
> 
> How would an rcu lock protect against the possibility that blkiocg_destroy()
> has not already been called on another cpu? rcu lock will make sure that
> cgroup and blkio_cgroup object should still be around as long as I am
> holding rcu lock but will not protect against deletion path being executed
> on another cpu? So I don't want to end up in following situation.
> 
>      cpu1                      cpu2
> 
> rcu_read_lock()
> 			      blkiocg_destroy()
> 
> blkiocg_add_blkio_group()
> rcu_read_unlock()
> 
> I don't want to add blkg object on a potentially dead blkio_cgroup object
> which will go away. Does this protection is provided by generic cgroup
> code where blkiocg_destroy() will not be called if I have got cgroup
> pointer under rcu lock?

Hmm, looking at blkiocg_destroy(), it seems to free blkcg directly. You
need an RCU grace period to pass in between, if you expect blkcg to be
valid under rcu_read_lock() always. Or is ->destroy() called from an RCU
callback? Checks... OK, so it looks like cgroup guarantees
synchronize_rcu() before calling ->destroy(), so that should be safe.

Why is it doing both rcu_read_lock() and grabbing the update lock?

On the real topic, it'll of course only guarentee that blkcg is valid
inside the rcu read lock. If it needs to be valid afterwards, the
reference must be grabbed.

> Currently we are deriving cgroup information from task context so task is
> still inside cgroup, so cgroup can't be deleted anyway. What if I was 
> deriving cgroup information from bio, will taking css object reference be
> necessary in that case or just cgroup pointer under rcu lock is sufficient
> to preclude the race against cgroup deletion path?

It's guarenteed that ->destroy() cannot be called while someone is under
rcu_read_lock().

-- 
Jens Axboe

Re: [PATCH] cfq: Remove useless css reference get

[Vivek Goyal]

On Wed, Dec 16, 2009 at 07:38:17PM +0100, Jens Axboe wrote:
> On Wed, Dec 16 2009, Vivek Goyal wrote:
> > On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
> > > There's no need to take css reference here, for the caller
> > > has already called rcu_read_lock() to prevent cgroup from 
> > > being removed.
> > > 
> > > Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> > > Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> > 
> > Hi Gui,
> > 
> > How would an rcu lock protect against the possibility that blkiocg_destroy()
> > has not already been called on another cpu? rcu lock will make sure that
> > cgroup and blkio_cgroup object should still be around as long as I am
> > holding rcu lock but will not protect against deletion path being executed
> > on another cpu? So I don't want to end up in following situation.
> > 
> >      cpu1                      cpu2
> > 
> > rcu_read_lock()
> > 			      blkiocg_destroy()
> > 
> > blkiocg_add_blkio_group()
> > rcu_read_unlock()
> > 
> > I don't want to add blkg object on a potentially dead blkio_cgroup object
> > which will go away. Does this protection is provided by generic cgroup
> > code where blkiocg_destroy() will not be called if I have got cgroup
> > pointer under rcu lock?
> 
> Hmm, looking at blkiocg_destroy(), it seems to free blkcg directly. You
> need an RCU grace period to pass in between, if you expect blkcg to be
> valid under rcu_read_lock() always. Or is ->destroy() called from an RCU
> callback? Checks... OK, so it looks like cgroup guarantees
> synchronize_rcu() before calling ->destroy(), so that should be safe.

Ok. Thanks Jens. So then I don't have to worry about ->destroy() being called
and blkcg being freed while I am trying to access this blkcg from CFQ under
rcu.

> 
> Why is it doing both rcu_read_lock() and grabbing the update lock?
> 

Are you referring to "blkio_list_lock"?

rcu_read_lock() is taken to protect "key". When cfq adds a blkio_group,
to the hash list in blkio_cgroup, it passes "cfqd" as the key. If somebody
is removing cgroup, we call back into CFQ to notify that cgroup is going
away so destroy associated cfq_group object. Now it can happen that both
elevator switch and cgroup removal is happening at same time and they both
can try to remove destroy cfq_group/blkio_group objects.

blkio_group list is protected by blkcg->lock and cfqd->cfqg_list is
protected by request queue lock. We always take request queue lock first
and then blkcg->lock to add or remove a blkio_group.

In cgroup removal path, we take blkcg->lock first and now can't take
request queue lock as it can lead to deadlock. So in cgroup removal path
we rely on accessing key (cfqd) under rcu and call CFQ to remove group
after dropping blkcg->lock. CFQ will take cfqd->queue lock and destroy
the group.

Because cfqd is rcu protected, we should do synchronize_rcu() in
cfq_exit_queue() to make sure that cfqd, hence cfqd->queue hence
cfqd->queue->lock object are still valid. But as we discussed in private
mail, synchronize_rcu() unconditionally was intorducing boot delays,
especially for megaraid driver. Replacing synchronize_rcu() with
call_rcu() will not help as cfqd will be around but cfqd->queue and
cfqd->queue->lock might have disappeared. So this is one problem which
needs to be fixed.

One solution is to do synchronize_rcu() only if we have some cfq_groups
alive. So during boot there will be only one group which will always be
cleaned up by cfq exit (cgroup userspace is not around yet). And we will
avoid delays in boot path as well as get rid of above race condition.

About taking blkio_list_lock, under rcu, may be we can drop it and make 
blkio_list rcu protected. But then we need to make sure that cfq does not
go away until one rcu grace period is over and blkio_unlink_group_fn()
function and key (cfqd, cfqd->queue)  objects are valid.

I guess, if we fix synchronize_rcu() issue in cfq_exit_queue(), it will
make sure that under rcu, cfqd and cfqd->queue objects are valid, and that
means cfq is still around and blkio_unlink_group_fn() is valid and we
don't need to take blkio_list_lock. Having said that, taking this lock was
not fixing the problem anyway. I was taking this lock here to make sure
cfq can't exit and unregister the io control policy while a cgroup is
going away. 

> On the real topic, it'll of course only guarentee that blkcg is valid
> inside the rcu read lock. If it needs to be valid afterwards, the
> reference must be grabbed.
> 

I guess for the time being we are fine. Because, after rcu read lock is
dropped, if blkcg is going away (due to cgroup deletion), it will cleanup
all the blkio_group (hence cfq_group) objects first on blkcg->blkg_list
and that will make sure all associated cfq_group objects for that cgroup are
gone and then we don't have to access blkcg anymore.


> > Currently we are deriving cgroup information from task context so task is
> > still inside cgroup, so cgroup can't be deleted anyway. What if I was 
> > deriving cgroup information from bio, will taking css object reference be
> > necessary in that case or just cgroup pointer under rcu lock is sufficient
> > to preclude the race against cgroup deletion path?
> 
> It's guarenteed that ->destroy() cannot be called while someone is under
> rcu_read_lock().
> 

Thanks
Vivek

Re: [PATCH] cfq: Remove useless css reference get

[Li Zefan]

于 2009年12月17日 00:39, Vivek Goyal 写道:
> On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
>> There's no need to take css reference here, for the caller
>> has already called rcu_read_lock() to prevent cgroup from 
>> being removed.
>>
>> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
>> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> Hi Gui,
> 
> How would an rcu lock protect against the possibility that blkiocg_destroy()
> has not already been called on another cpu? rcu lock will make sure that
> cgroup and blkio_cgroup object should still be around as long as I am
> holding rcu lock but will not protect against deletion path being executed
> on another cpu? So I don't want to end up in following situation.
> 
>      cpu1                      cpu2
> 
> rcu_read_lock()
> 			      blkiocg_destroy()
> 
> blkiocg_add_blkio_group()
> rcu_read_unlock()
> 
> I don't want to add blkg object on a potentially dead blkio_cgroup object
> which will go away. Does this protection is provided by generic cgroup
> code where blkiocg_destroy() will not be called if I have got cgroup
> pointer under rcu lock?
> 
> Currently we are deriving cgroup information from task context so task is
> still inside cgroup, so cgroup can't be deleted anyway. What if I was 
> deriving cgroup information from bio, will taking css object reference be
> necessary in that case or just cgroup pointer under rcu lock is sufficient
> to preclude the race against cgroup deletion path?
> 

We pass a cgroup ptr to cfq_find_alloc_cfqg(), which means the cgroup is
valid. As long as it's safe to access the cgroup, it's safe to access the
corresponding blkio_cgroup.

But if you still want blkio_cgroup to be valid after dropping rcu_read_lock
(or cgroup_mutex), you need to call css_get() first. Note you don't need
to call css_tryget(). css_tryget() is only needed in mem_cgroup, because
mem_cgroup is a bit special, in that a mem_cgroup can remain valid after
a cgroup is destroyed.

If a user tries to rmdir a cgroup when its css refcnt > 0, EBUSY will be
returned.

Re: [PATCH] cfq: Remove useless css reference get

[Vivek Goyal]

On Thu, Dec 17, 2009 at 01:57:08PM +0800, Li Zefan wrote:
> 于 2009年12月17日 00:39, Vivek Goyal 写道:
> > On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
> >> There's no need to take css reference here, for the caller
> >> has already called rcu_read_lock() to prevent cgroup from 
> >> being removed.
> >>
> >> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> >> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> > 
> > Hi Gui,
> > 
> > How would an rcu lock protect against the possibility that blkiocg_destroy()
> > has not already been called on another cpu? rcu lock will make sure that
> > cgroup and blkio_cgroup object should still be around as long as I am
> > holding rcu lock but will not protect against deletion path being executed
> > on another cpu? So I don't want to end up in following situation.
> > 
> >      cpu1                      cpu2
> > 
> > rcu_read_lock()
> > 			      blkiocg_destroy()
> > 
> > blkiocg_add_blkio_group()
> > rcu_read_unlock()
> > 
> > I don't want to add blkg object on a potentially dead blkio_cgroup object
> > which will go away. Does this protection is provided by generic cgroup
> > code where blkiocg_destroy() will not be called if I have got cgroup
> > pointer under rcu lock?
> > 
> > Currently we are deriving cgroup information from task context so task is
> > still inside cgroup, so cgroup can't be deleted anyway. What if I was 
> > deriving cgroup information from bio, will taking css object reference be
> > necessary in that case or just cgroup pointer under rcu lock is sufficient
> > to preclude the race against cgroup deletion path?
> > 
> 
> We pass a cgroup ptr to cfq_find_alloc_cfqg(), which means the cgroup is
> valid. As long as it's safe to access the cgroup, it's safe to access the
> corresponding blkio_cgroup.
> 

Ok.

> But if you still want blkio_cgroup to be valid after dropping rcu_read_lock
> (or cgroup_mutex), you need to call css_get() first. Note you don't need
> to call css_tryget(). css_tryget() is only needed in mem_cgroup, because
> mem_cgroup is a bit special, in that a mem_cgroup can remain valid after
> a cgroup is destroyed.
> 
> If a user tries to rmdir a cgroup when its css refcnt > 0, EBUSY will be
> returned.

Ok. I was not taking a reference to css object because I don't want to
stop deletion of cgroup just because there might be some pending IO from 
that group which has not finished yet. So if some task issues some IO and
it exits or moves to a different cgroup, then user should be able to
delete the cgroup even if there is pending IO in that cgroup.

That's the reason I don't take a reference to css object and once cgroup
deletion call comes (->destroy()), I just call CFQ to unlink the group and let
cgroup and blkio_cgroup go away. cfq_group and blkio_group objects will still
be around till all the pending IO in that group is finished and cfq_group
will be freed after that automatically (all rq and cfqq references gone).

Thanks
Vivek

Re: [PATCH] cfq: Remove useless css reference get

[Vivek Goyal]

On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
> There's no need to take css reference here, for the caller
> has already called rcu_read_lock() to prevent cgroup from 
> being removed.
> 
> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

> ---
>  block/blk-cgroup.c  |   14 --------------
>  block/blk-cgroup.h  |    3 ---
>  block/cfq-iosched.c |    5 -----
>  3 files changed, 0 insertions(+), 22 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 1fa2654..cba28f4 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -23,20 +23,6 @@ static LIST_HEAD(blkio_list);
>  struct blkio_cgroup blkio_root_cgroup = { .weight = 2*BLKIO_WEIGHT_DEFAULT };
>  EXPORT_SYMBOL_GPL(blkio_root_cgroup);
>  
> -bool blkiocg_css_tryget(struct blkio_cgroup *blkcg)
> -{
> -	if (!css_tryget(&blkcg->css))
> -		return false;
> -	return true;
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_css_tryget);
> -
> -void blkiocg_css_put(struct blkio_cgroup *blkcg)
> -{
> -	css_put(&blkcg->css);
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_css_put);
> -
>  struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
>  {
>  	return container_of(cgroup_subsys_state(cgroup, blkio_subsys_id),
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index 4d316df..84bf745 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -43,9 +43,6 @@ struct blkio_group {
>  	unsigned long sectors;
>  };
>  
> -extern bool blkiocg_css_tryget(struct blkio_cgroup *blkcg);
> -extern void blkiocg_css_put(struct blkio_cgroup *blkcg);
> -
>  typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
>  typedef void (blkio_update_group_weight_fn) (struct blkio_group *blkg,
>  						unsigned int weight);
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index e2f8046..5d6b427 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -958,10 +958,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>  	struct backing_dev_info *bdi = &cfqd->queue->backing_dev_info;
>  	unsigned int major, minor;
>  
> -	/* Do we need to take this reference */
> -	if (!blkiocg_css_tryget(blkcg))
> -		return NULL;;
> -
>  	cfqg = cfqg_of_blkg(blkiocg_lookup_group(blkcg, key));
>  	if (cfqg || !create)
>  		goto done;
> @@ -992,7 +988,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>  	hlist_add_head(&cfqg->cfqd_node, &cfqd->cfqg_list);
>  
>  done:
> -	blkiocg_css_put(blkcg);
>  	return cfqg;
>  }
>  
> -- 
> 1.5.4.rc3
> 

Re: [PATCH] cfq: Remove useless css reference get

[Gui Jianfeng]

Vivek Goyal wrote:
> On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
>> There's no need to take css reference here, for the caller
>> has already called rcu_read_lock() to prevent cgroup from 
>> being removed.
>>
>> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
>> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> Acked-by: Vivek Goyal <vgoyal@redhat.com>

Hi Jens

Would you pick this patch up?

> 
> Thanks
> Vivek
> 
>> ---
>>  block/blk-cgroup.c  |   14 --------------
>>  block/blk-cgroup.h  |    3 ---
>>  block/cfq-iosched.c |    5 -----
>>  3 files changed, 0 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 1fa2654..cba28f4 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -23,20 +23,6 @@ static LIST_HEAD(blkio_list);
>>  struct blkio_cgroup blkio_root_cgroup = { .weight = 2*BLKIO_WEIGHT_DEFAULT };
>>  EXPORT_SYMBOL_GPL(blkio_root_cgroup);
>>  
>> -bool blkiocg_css_tryget(struct blkio_cgroup *blkcg)
>> -{
>> -	if (!css_tryget(&blkcg->css))
>> -		return false;
>> -	return true;
>> -}
>> -EXPORT_SYMBOL_GPL(blkiocg_css_tryget);
>> -
>> -void blkiocg_css_put(struct blkio_cgroup *blkcg)
>> -{
>> -	css_put(&blkcg->css);
>> -}
>> -EXPORT_SYMBOL_GPL(blkiocg_css_put);
>> -
>>  struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
>>  {
>>  	return container_of(cgroup_subsys_state(cgroup, blkio_subsys_id),
>> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
>> index 4d316df..84bf745 100644
>> --- a/block/blk-cgroup.h
>> +++ b/block/blk-cgroup.h
>> @@ -43,9 +43,6 @@ struct blkio_group {
>>  	unsigned long sectors;
>>  };
>>  
>> -extern bool blkiocg_css_tryget(struct blkio_cgroup *blkcg);
>> -extern void blkiocg_css_put(struct blkio_cgroup *blkcg);
>> -
>>  typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
>>  typedef void (blkio_update_group_weight_fn) (struct blkio_group *blkg,
>>  						unsigned int weight);
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index e2f8046..5d6b427 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -958,10 +958,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>>  	struct backing_dev_info *bdi = &cfqd->queue->backing_dev_info;
>>  	unsigned int major, minor;
>>  
>> -	/* Do we need to take this reference */
>> -	if (!blkiocg_css_tryget(blkcg))
>> -		return NULL;;
>> -
>>  	cfqg = cfqg_of_blkg(blkiocg_lookup_group(blkcg, key));
>>  	if (cfqg || !create)
>>  		goto done;
>> @@ -992,7 +988,6 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>>  	hlist_add_head(&cfqg->cfqd_node, &cfqd->cfqg_list);
>>  
>>  done:
>> -	blkiocg_css_put(blkcg);
>>  	return cfqg;
>>  }
>>  
>> -- 
>> 1.5.4.rc3
>>
> 
> 
> 

Re: [PATCH] cfq: Remove useless css reference get

[Jens Axboe]

On Fri, Feb 26 2010, Gui Jianfeng wrote:
> Vivek Goyal wrote:
> > On Wed, Dec 16, 2009 at 04:38:43PM +0800, Gui Jianfeng wrote:
> >> There's no need to take css reference here, for the caller
> >> has already called rcu_read_lock() to prevent cgroup from 
> >> being removed.
> >>
> >> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> >> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> > 
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Hi Jens
> 
> Would you pick this patch up?

Yep, it's added.

-- 
Jens Axboe