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

[Vivek Goyal <vgoyal@redhat.com>]

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
>