Messages in this thread | | | Date | Wed, 28 Sep 2011 14:09:05 -0400 | From | Vivek Goyal <> | Subject | Re: [GIT PULL] Queue free fix (was Re: [PATCH] block: Free queue resources at blk_release_queue()) |
| |
On Wed, Sep 28, 2011 at 01:53:04PM -0400, Christoph Hellwig wrote: > On Wed, Sep 28, 2011 at 01:48:59PM -0400, Vivek Goyal wrote: > > I am wondering if we should retain blk_throtl_exit() in blk_cleanup_queue() > > before lock swap and just move elevator cleanup in blk_release_queue(). > > > > A note to myself, I should probably enhance blk_throtl_exit() to look for any > > queued throttled bio and single their completion with error (-ENODEV) or > > something like that. > > The root of this evil is how queue_lock is implemented and (ab)used. > Instead of letting the driver assign a pointer to make the core use > its locks we really need to make the queue_lock a lock embedded directly > into the queue, and drivers may or may not use that lock for their > internal data structures. For high performance drivers they preferable > should use their own locks as queue_lock is far too contended already > for any high IOPS device. The same applies to throtteling btw - > instead of overloading an already highly contended lock it really > should have its own.
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.
So I could do lock nesting.
hold throttling_lock; hold queue_lock;
IIRC, I also had noticed some operations where queue might want to call into throttling with queue lock held and that would have led to lock order problems.
So I had given up on the idea and continued to use queue lock for throttling. Thought it could still probably be done if one could justify additional complexity.
Thanks Vivek
| |