Re: [PATCH 1/4] blkio_group key change: void * -> request_queue *

[Chad Talbott]

On Thu, Mar 25, 2010 at 4:25 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> human-readable disk names probably are more convenient. I got few general
> concerns.

Thanks for the quick and detailed review.

> - Can we change the sysfs interface now. In 2.6.33 kernel we released the
>  code to export blkio.time and blkio.sectors using device numbers. We
>  shall have to change those interfaces also to reflect device stats in
>  terms of device names.

I wasn't aware of those interface, but I'm all for consistency.  I'll
look into it.

> - I had kept void* as the key in blkg object so that it does not make any
>  assumption about the key. This allowed that any xyz code in kernel can
>  register with blkio code and implement some kind of IO control policy
>  and it does not have to instanciate a request queue.
>
>  But I am not sure if there will be any subsystems which will really do
>  that. I am assuming that all the device mapper and md code do
>  instanciate the request queue? (In case we implement max bw policy
>  there).

I understand your preference to keep the key generic, but I wonder if
there is *any* device that won't have a request_queue?  The penalty of
keeping it very generic might be the complexity of another auxiliary
structure to lookup gendisks or the next thing.  I wish the device
mapper folks would speak up and be involved in these discussions.

> - How do you make sure that request queue does not go away and while
>  somebody is accessing blkg->queue under rcu read lock? This can happen
>  while we call unlink code.

Here I'll admit to being very new to RCU, and I'll defer to your
worries.  More homework needed on my part.

>  blkio_unlink_group_fn().
>
>  Because we store the pointer to cfqd as key, we make sure cfqd does not
>  get deleted before one rcu grace period. (call_rcu). But this gurantees
>  only that cfqd object is around and not cfqd->queue. So this is a
>  problem with even today's code.
>
> ...
> - How do you make sure that gendisk does not go away while q->disk is
>  being accessed under rcu lock. (Already asked in other thread too.)

On locking between gendisk deletion and stats printing: I suppose that
you've already considered and discarded using the queue_lock to
protect a  gendisk pointer in request_queue?

> These are some quick thoughs. More later...

Thanks again!
Chad
--
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/