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

[Nauman Rafique <nauman@google.com>]

On Fri, May 8, 2009 at 6:57 AM, Vivek Goyal <vgoyal@redhat.com> 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.

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?

Thanks.
--
Nauman


>
> Thanks
> Vivek
>
>> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
>> ---
>>  elevator-fq.c |   15 ++++++++++++++-
>>  elevator-fq.h |    5 +++++
>>  elevator.c    |    2 ++
>>  3 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/elevator-fq.c b/block/elevator-fq.c
>> index 9500619..e6d6712 100644
>> --- a/block/elevator-fq.c
>> +++ b/block/elevator-fq.c
>> @@ -1968,11 +1968,24 @@ void elv_fq_set_request_io_group(struct request_queue *q, struct request *rq,
>>       spin_unlock_irqrestore(q->queue_lock, flags);
>>       BUG_ON(!iog);
>>
>> -     /* Store iog in rq. TODO: take care of referencing */
>> +     elv_get_iog(iog);
>>       rq->iog = iog;
>>  }
>>
>>  /*
>> + * This request has been serviced. Clean up iog info and drop the reference.
>> + */
>> +void elv_fq_unset_request_io_group(struct request *rq)
>> +{
>> +     struct io_group *iog = rq->iog;
>> +
>> +     if (iog) {
>> +             rq->iog = NULL;
>> +             elv_put_iog(iog);
>> +     }
>> +}
>> +
>> +/*
>>   * Find/Create the io queue the rq should go in. This is an optimization
>>   * for the io schedulers (noop, deadline and AS) which maintain only single
>>   * io queue per cgroup. In this case common layer can just maintain a
>> diff --git a/block/elevator-fq.h b/block/elevator-fq.h
>> index db3a347..96a28e9 100644
>> --- a/block/elevator-fq.h
>> +++ b/block/elevator-fq.h
>> @@ -512,6 +512,7 @@ static inline struct io_group *ioq_to_io_group(struct io_queue *ioq)
>>  extern int io_group_allow_merge(struct request *rq, struct bio *bio);
>>  extern void elv_fq_set_request_io_group(struct request_queue *q,
>>                                       struct request *rq, struct bio *bio);
>> +extern void elv_fq_unset_request_io_group(struct request *rq);
>>  static inline bfq_weight_t iog_weight(struct io_group *iog)
>>  {
>>       return iog->entity.weight;
>> @@ -571,6 +572,10 @@ static inline void elv_fq_set_request_io_group(struct request_queue *q,
>>  {
>>  }
>>
>> +static inline void elv_fq_unset_request_io_group(struct request *rq)
>> +{
>> +}
>> +
>>  static inline bfq_weight_t iog_weight(struct io_group *iog)
>>  {
>>       /* Just root group is present and weight is immaterial. */
>> diff --git a/block/elevator.c b/block/elevator.c
>> index 44c9fad..d75eec7 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -992,6 +992,8 @@ void elv_put_request(struct request_queue *q, struct request *rq)
>>  {
>>       struct elevator_queue *e = q->elevator;
>>
>> +     elv_fq_unset_request_io_group(rq);
>> +
>>       /*
>>        * Optimization for noop, deadline and AS which maintain only single
>>        * ioq per io group
>>
>