Hello, Vivek.

On Fri, Apr 27, 2012 at 03:46:54PM -0400, Vivek Goyal wrote:

On Thu, Apr 26, 2012 at 02:59:21PM -0700, Tejun Heo wrote:

[..]

quoted

@@ -926,6 +936,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
 		goto fail_alloc;
 
 	blk_rq_init(q, rq);
+	blk_rq_set_rl(rq, rl);

Given the fact that we have established the rq and blkg releation at
the time of allocation, should we modify CFQ to just use that relation
instread of trying to lookup group again based on bio.

Maybe, given the lookup cache it shouldn't really matter tho.

We avoid one lookup also we avoid duplicate creation of blkg in following
corner case of bio==NULL

	- blkg_get_rl()
	- request allocation fails. sleep, drop queue lock
	- process is moved to a different cgroup. origincal cgroup is
	  deleted. pre_destroy will cleanup all blkg on blkcg.
	- process wakes up, request allocated, set_request sets up new blkg
 	  based on new cgroup. Now a request is queued in one blkg/cgroup and
 	  it has come out of the quota of other blkg/cgroup.

I don't think it really matters as long as the request gets freed to
the right queue on completion.

Well, I have a question. Ideally nobody should be linking any more blkg
to a blkcg once blkg_pre_destroy() has been called? But can it happen
that bio_associate_current() takes are reference to blkcg and bio is
throttled. cgroup associated with bio is deleted resulting in
pre_destroy(). Now bio is submitted to CFQ and it will try to create
a new blkg for blkcg-queue pair and once IO is complete, bio will drop
blkcg reference, which in turn will free up blkcg and associated blkg
is still around and will not be cleaned up.

IOW, looks like we need a mechanism to mark a blkcg dead (set in
pre_destroy() call) and any submissions to blkcg after that should result
in bio being divered to root group?

Don't we already have that with css_tryget()?

Thanks.

-- 
tejun