Re: [PATCH] io-controller: Add io group reference handling forrequest

[Vivek Goyal]

On Fri, May 15, 2009 at 09:48:40AM +0200, Andrea Righi wrote:
> On Fri, May 15, 2009 at 01:15:24PM +0800, Gui Jianfeng wrote:
> > Vivek Goyal wrote:
> > ...
> > >  }
> > > @@ -1462,20 +1462,27 @@ struct io_cgroup *get_iocg_from_bio(stru
> > >  /*
> > >   * Find the io group bio belongs to.
> > >   * If "create" is set, io group is created if it is not already present.
> > > + * If "curr" is set, io group is information is searched for current
> > > + * task and not with the help of bio.
> > > + *
> > > + * FIXME: Can we assume that if bio is NULL then lookup group for current
> > > + * task and not create extra function parameter ?
> > >   *
> > > - * Note: There is a narrow window of race where a group is being freed
> > > - * by cgroup deletion path and some rq has slipped through in this group.
> > > - * Fix it.
> > >   */
> > > -struct io_group *io_get_io_group_bio(struct request_queue *q, struct bio *bio,
> > > -					int create)
> > > +struct io_group *io_get_io_group(struct request_queue *q, struct bio *bio,
> > > +					int create, int curr)
> > 
> >   Hi Vivek,
> > 
> >   IIUC we can get rid of curr, and just determine iog from bio. If bio is not NULL,
> >   get iog from bio, otherwise get it from current task.
> 
> Consider also that get_cgroup_from_bio() is much more slow than
> task_cgroup() and need to lock/unlock_page_cgroup() in
> get_blkio_cgroup_id(), while task_cgroup() is rcu protected.
> 

True.

> BTW another optimization could be to use the blkio-cgroup functionality
> only for dirty pages and cut out some blkio_set_owner(). For all the
> other cases IO always occurs in the same context of the current task,
> and you can use task_cgroup().
> 

Yes, may be in some cases we can avoid setting page owner. I will get
to it once I have got functionality going well. In the mean time if
you have a patch for it, it will be great.

> However, this is true only for page cache pages, for IO generated by
> anonymous pages (swap) you still need the page tracking functionality
> both for reads and writes.
> 

Right now I am assuming that all the sync IO will belong to task
submitting the bio hence use task_cgroup() for that. Only for async
IO, I am trying to use page tracking functionality to determine the owner.
Look at elv_bio_sync(bio).

You seem to be saying that there are cases where even for sync IO, we
can't use submitting task's context and need to rely on page tracking
functionlity? In case of getting page (read) from swap, will it not happen
in the context of process who will take a page fault and initiate the
swap read?

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/