Re: [PATCH UPDATED 11/11] blkcg: unify blkg's for blkcg policies

[Tejun Heo <tj@kernel.org>]

Hey, Vivek.

On Fri, Feb 03, 2012 at 02:41:05PM -0500, Vivek Goyal wrote:
> On Wed, Feb 01, 2012 at 04:37:30PM -0800, Tejun Heo wrote:
> > As a transitional step to untangle blkg management, elvswitch and
> > policy [de]registration, all blkgs except the root blkg are being shot
> > down during elvswitch and bypass.  This patch adds blkg_root_update()
> > to update root blkg in place on policy change.  This is hacky and racy
> > but should be good enough as interim step until we get locking
> > simplified and switch over to proper in-place update for all blkgs.
> 
> - So we don't shoot down root group over elevator switch and policy
>   changes because we are not sure if we will be able to alloc new
>   group? It is not like elevator where we don't free the old one till
>   we have made sure that new one is allocated and initialized properly.

No, because we policies cache root group and we don't have mechanism
to update them.  I could have added that but root group management
should be moved to blkcg core anyway and in-place update will be
applied to all blkgs, so I just chose a dirty shortcut as an interim
step.

> - I am assuming that we will change  blkg_destroy_all() later to also
>   take policy as argument and only destroy policy data of respective
>   policy and not the whole group. (Well I guess we can destroy the whole
>   group if it was only policy on the group). 

Yeap, that's what's scheduled.

> [..]
> >  static struct blkio_group *blkg_alloc(struct blkio_cgroup *blkcg,
> > -				      struct request_queue *q,
> > -				      struct blkio_policy_type *pol)
> > +				      struct request_queue *q)
> 
> Comment before this function still mentions "pol" as function argument.

Will update.

> [..]
> > @@ -776,43 +786,49 @@ blkiocg_reset_stats(struct cgroup *cgrou
> >  #endif
> >  
> >  	blkcg = cgroup_to_blkio_cgroup(cgroup);
> > +	spin_lock(&blkio_list_lock);
> >  	spin_lock_irq(&blkcg->lock);
> 
> Isn't blkcg lock enough to protect against policy registration/deregistration.
> A policy can not add/delete a group to cgroup list without blkcg list. 

But pol list can change regardless of that, no?

Thanks.

-- 
tejun