Messages in this thread | | | Date | Tue, 22 Feb 2011 15:20:03 +1100 | From | NeilBrown <> | Subject | Re: [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() |
| |
On Mon, 21 Feb 2011 22:53:34 -0500 Vivek Goyal <vgoyal@redhat.com> wrote:
> Hi All, > > Currently it is not clear what's the status of ->queue_lock when > blk_cleanup_queue() is called. Now block throttle code (blk_throtl_exit()) > requires queue lock to be initialized as it takes the spin lock, so we need > some kind of clear convention that when is it safe to assume that queue lock > is initiliazed and blk_throtle_exit() can be called safely. > > We recently ran into issues with loop driver which was trying to free > up queue almost immediately after block_alloc_queue() due to some internal > errors. Also NeilBrown complained that in current form, md provides its > own spinlock and zaps that before blk_cleanup_queue() call and that will > have issues with throttling code. Neil has not fixed the issue with > md driver and queued up a patch in this tree. > > So there is a need to make sure blk_throtl_exit() can be called safely > and to also catch any errors if some drivers are not doing so. This ^^^ "now" !! I make that typo all the time too :-(
> patch series puts a WARN_ON(!q->queue_lock) in blk_cleanup_queue() and > moves the queue lock initialization in queue allocation code. Also it > move blk_throtl_exit() from release_queue() to cleanup_queue().
I'm not sure there is a lot of point in the WARN_ON. Modules that replace ->queue_lock are unlikely to set it to NULL when done, they will just free the structure that it points into. This will already be caught if you compile with DEBUG_PAGEALLOC and DEBUG_SPINLOCK (I think) and there is not much else you can do to catch these.
Thanks, NeilBrown
> > These pathes are only boot tested on one machine. Before I do more > extensive testing, I wanted to throw it out there and figure out > if it makes sense or not. > > Thanks > Vivek > > Vivek Goyal (3): > block: Initialize ->queue_lock to internal lock at queue allocation > time > loop: No need to initialize ->queue_lock explicitly before calling > blk_cleanup_queue() > block: Move blk_throtl_exit() call to blk_cleanup_queue() > > block/blk-core.c | 26 ++++++++++++++++++++++++-- > block/blk-settings.c | 7 ------- > block/blk-sysfs.c | 2 -- > block/blk-throttle.c | 2 +- > drivers/block/loop.c | 3 --- > include/linux/blkdev.h | 2 -- > 6 files changed, 25 insertions(+), 17 deletions(-)
| |