Re: [PATCH 06/20] blkio: Introduce cgroup interface

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Nov 04, 2009 at 10:23:16AM -0500, Jeff Moyer wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > +void blkiocg_add_blkio_group(struct blkio_cgroup *blkcg,
> > +				struct blkio_group *blkg, void *key)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&blkcg->lock, flags);
> > +	rcu_assign_pointer(blkg->key, key);
> > +	hlist_add_head_rcu(&blkg->blkcg_node, &blkcg->blkg_list);
> > +	spin_unlock_irqrestore(&blkcg->lock, flags);
> > +}
> 
> I took a look at the rcu stuff, and it seems to be in order.
> 
> > +/*
> > + * We cannot support shared io contexts, as we have no mean to support
> > + * two tasks with the same ioc in two different groups without major rework
> > + * of the main cic data structures.  For now we allow a task to change
> > + * its cgroup only if it's the only owner of its ioc.
> > + */
> 
> Interesting.  So is there no way at all to set the cgroup for a set of
> processes that are cloned using CLONE_IO?
> 

In the current patchset "no". This is bad and should be fixed. Thinking of
following.

- In case of CLONE_IO, when a tread moves, drop the reference to the sync
  queue and allow movement of the thread to a different group. Now once
  a new request comes in, a new queue will be setup again. Because two
  threads sharing the IO context are in two different groups, A sync queue
  will be setup for the group from which request comes first. So for some
  time we will have a situation where a thread is one group but its IO
  is going into a queue of a different group. This will be only temporary
  situation and correct itself once all the threads sharing io context
  move to same group.

The downside is that user might not know exactly which threads are sharing
IO context and might end up with some threads in one group and others in
different group.


> > +static int blkiocg_can_attach(struct cgroup_subsys *subsys,
> > +				struct cgroup *cgroup, struct task_struct *tsk,
> > +				bool threadgroup)
> > +{
> > +	struct io_context *ioc;
> > +	int ret = 0;
> > +
> > +	/* task_lock() is needed to avoid races with exit_io_context() */
> > +	task_lock(tsk);
> > +	ioc = tsk->io_context;
> > +	if (ioc && atomic_read(&ioc->nr_tasks) > 1)
> > +		ret = -EINVAL;
> > +	task_unlock(tsk);
> > +
> > +	return ret;
> > +}
> 
> This function's name implies that it returns a boolean.

Yes. Will change from int to bool. Thanks.

Vivek