Re: [GIT PULL] Queue free fix (was Re: [PATCH] block: Free queue resources at blk_release_queue())

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Sep 28, 2011 at 02:16:49PM -0400, Christoph Hellwig wrote:
> On Wed, Sep 28, 2011 at 02:09:05PM -0400, Vivek Goyal wrote:
> > I had thought of implementing a separate lock for throttling. Then I
> > noticed few operations like checking for queue flags where I would
> > be required to hold queue locks.
> 
> Can you do a writeup of these issues?  E.g. if the flags are throtteling
> specific we can move them to a separate flags field, if not we can
> see if we can make them atomic or similar.

Sure. I am going through the code and trying to list down some of the
dependencies.

But before that I would say that tyring to optimize the throttling code
to reduce the queue lock contention should be last on the priority list.
The simple reason being that in common case it does not incur any locking
cost. One needs to have IO cgroup configured and needs to have some
throttling rules put in place only then queue lock cost will come into
picture. And for most of the people, I don't think that's the common
case.

Now coming back to question of dependencies. We take some services from
request queue which needs to happen under queue lock.

- Looks like in current code I am only accessing QUEUE_FLAG_DEAD. I see
  that flag is not synchronized using queue lock. So it probably is not
  a concern.

- I am using tracing functionality. blk_add_trace_msg(). I guess it is
  not required to take queue lock for this. But I am not sure.

- I am accessing q->backing_dev_info to get to associated devices's
  major and minor number.

That's it I guess. That's what a quick look tells me. So looks like
it is not too hard to convert existing blk throttle code to use its
own lock. That will also get rid of dependency on queue lock and
in turn uncertainity associated with queue lock being valid (if
driver provided the lock).

Thanks
Vivek