[Devel] Re: [PATCH] io-controller: Add io group reference handling for request

[Vivek Goyal]

On Fri, May 15, 2009 at 04:16:38PM +0800, Gui Jianfeng wrote:
> 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.
> > 
> > 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().
> > 
> > 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.
> 
>   Hi Andrea,
> 
>   Thanks for pointing this out. Yes, i think we can determine io group in
>   terms of bio->bi_rw. If bio is a READ bio, just taking io group by 
>   task_cgroup(). If it's a WRITE bio, getting it from blkio_cgroup.
> 

Gui, we are already doing it. page tracking functionality is used only
for async IO and for all sync IO, we are using submitting tasks's group
to determine io group bio belongs to.

	if (elv_bio_sync(bio)) {
		/* sync io. Determine cgroup from submitting task
		 * context.*/
                cgroup = task_cgroup(current, io_subsys_id);
                return cgroup;
        }

Thanks
Vivek
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers