On Fri, May 8, 2009 at 11:56 AM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Fri, May 08, 2009 at 10:41:01AM -0700, Nauman Rafique wrote: >> On Fri, May 8, 2009 at 6:57 AM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> > On Fri, May 08, 2009 at 05:45:32PM +0800, Gui Jianfeng wrote: >> >> Hi Vivek, >> >> >> >> This patch adds io group reference handling when allocating >> >> and removing a request. >> >> >> > >> > Hi Gui, >> > >> > Thanks for the patch. We were thinking that requests can take a reference >> > on io queues and io queues can take a reference on io groups. That should >> > make sure that io groups don't go away as long as active requests are >> > present. >> > >> > But there seems to be a small window while allocating the new request >> > where request gets allocated from a group first and then later it is >> > mapped to that group and queue is created. IOW, in get_request_wait(), >> > we allocate a request from a particular group and set rq->rl, then >> > drop the queue lock and later call elv_set_request() which again maps >> > the request to the group saves rq->iog and creates new queue. This window >> > is troublesome because request can be mapped to a particular group at the >> > time of allocation and during set_request() it can go to a different >> > group as queue lock was dropped and group might have disappeared. >> > >> > In this case probably it might make sense that request also takes a >> > reference on groups. At the same time it looks too much that request takes >> > a reference on queue as well as group object. Ideas are welcome on how >> > to handle it... >> >> IMHO a request being allocated on the wrong cgroup should not be a big >> problem as such. All it means is that the request descriptor was >> accounted to the wrong cgroup in this particular corner case. Please >> correct me if I am wrong. > > I think you are right. We just need to be little careful while freeing > the request that associated request list might have gone away (rq->rl). > > Or we probably can think of getting rid of (rq->rl) also and while > freeing request determine io queue and group from rq->ioq. But somehow > I remember that I had to introduce rq->rl otherwise I was running into > issues of request being mapped to different groups at different point > of time hence different request list etc. Will check again.. >> >> We can also get rid of rq->iog pointer too. What that means is that >> request is associated with ioq (rq->ioq), and we can use >> ioq_to_io_group() function to get the io_group. So the request would >> only be indirectly associated with an io_group i.e. the request is >> associated with an io_queue and the io_group for the request is the >> io_group associated with io_queue. Do you see any problems with that >> approach? > > Looks like this is also doable. Good idea. Can't think of why can't > we get rid of rq->iog and manage with rq->ioq. There are only 1-2 > places where ioq is not setup yet and rq has been mapped to the group. > There we shall have to carry group information or carry bio information > and map it again to get group info. > > Will try to implement it and see how does it go. I tried it, and it seems to work. I passed io_group around as function arguments, and return values before ioq was set. > > Thanks > Vivek > -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
