lkml.org 
[lkml]   [2011]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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(-)



\
 
 \ /
  Last update: 2011-02-22 05:23    [W:0.051 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site