All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue()
@ 2011-02-22  3:53 Vivek Goyal
  2011-02-22  3:53 ` [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time Vivek Goyal
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Vivek Goyal @ 2011-02-22  3:53 UTC (permalink / raw)
  To: linux-kernel, jaxboe; +Cc: neilb, sergey.senozhatsky, tj, jmoyer, snitzer

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
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().

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(-)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time
  2011-02-22  3:53 [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() Vivek Goyal
@ 2011-02-22  3:53 ` Vivek Goyal
  2011-02-22  3:53 ` [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue() Vivek Goyal
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Vivek Goyal @ 2011-02-22  3:53 UTC (permalink / raw)
  To: linux-kernel, jaxboe; +Cc: neilb, sergey.senozhatsky, tj, jmoyer, snitzer

o There does not seem to be a clear convention whether q->queue_lock is
  initialized or not when blk_cleanup_queue() is called. In the past
  it was not necessary but now blk_throtl_exit() takes up queue lock
  by default and needs queue lock to be available.

  In fact elevator_exit() code also has similar requirement just that
  it is less stringent in the sense that elevator_exit() is called only
  if elevator is initialized.

o Two problems have been noticed because of ambiguity about spin lock
  status.

	- If a driver calls blk_alloc_queue() and then soon calls
	  blk_cleanup_queue() almost immediately, (because some other driver
	  structure allocation failed or some other error happened) then
	  blk_throtl_exit() will run into issues as queue lock is not
	  initialized. Loop driver ran into this issue recently and I noticed
	  error paths in md driver too. Similar error paths should exist in
	  other drivers too.

	- If some driver provided external spin lock and zapped the lock
	  before blk_cleanup_queue(), then it can lead to issues.

o So this patch does two things.

	- Initialize the default queue lock at queue allocation time.
	  block throttling code is one of the users of queue lock and
	  it is initialized at the queue allocation time, so it makes
	  sense to initialize ->queue_lock also to internal lock. A
	  driver can overide that lock later. This will take care of
	  first issue where a driver does not have to worry about
	  initializing the queue lock to default before calling
	  blk_cleanup_queue()

	- Put a WARN_ON() in blk_cleanup_queue() to make sure ->queue_lock
	  is initialized and it will catch the cases if there is a bad
	  driver which provides an external queue lock but zaps it
	  unexpectedly. So this WARN_ON() will catch that and we can
	  fix the driver.

	  Recently NeilBrown noted that md is doing something similar and
	  he has queued a fix in this tree. This patch relies on that
	  patch otherwise it might WARN and then crash in blk_throtl_exit().

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-core.c     |   19 ++++++++++++++++++-
 block/blk-settings.c |    7 -------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2f4002f..9a8e256 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -459,6 +459,14 @@ void blk_put_queue(struct request_queue *q)
 void blk_cleanup_queue(struct request_queue *q)
 {
 	/*
+	 * If a driver supplied the queue lock, it should not zap that
+	 * unexpectedly as some queue cleanup components like
+	 * elevator_exit() and blk_throtl_exit() need queue lock. Hence
+	 * warn if queue lock is NULL
+	 */
+	WARN_ON(!q->queue_lock);
+
+	/*
 	 * We know we have process context here, so we can be a little
 	 * cautious and ensure that pending block actions on this device
 	 * are done before moving on. Going into this function, we should
@@ -548,6 +556,12 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	mutex_init(&q->sysfs_lock);
 	spin_lock_init(&q->__queue_lock);
 
+	/*
+	 * By default initialize queue_lock to internal lock and driver can
+	 * override it later if need be.
+	 */
+	q->queue_lock = &q->__queue_lock;
+
 	return q;
 }
 EXPORT_SYMBOL(blk_alloc_queue_node);
@@ -632,7 +646,10 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
 	q->unprep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
 	q->queue_flags		= QUEUE_FLAG_DEFAULT;
-	q->queue_lock		= lock;
+
+	/* Override internal queue lock with supplied lock pointer */
+	if (lock)
+		q->queue_lock		= lock;
 
 	/*
 	 * This also sets hw/phys segments, boundary and size
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 36c8c1f..df649fa 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -176,13 +176,6 @@ void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
 	blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
 
 	/*
-	 * If the caller didn't supply a lock, fall back to our embedded
-	 * per-queue locks
-	 */
-	if (!q->queue_lock)
-		q->queue_lock = &q->__queue_lock;
-
-	/*
 	 * by default assume old behaviour and bounce for any highmem page
 	 */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_HIGH);
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue()
  2011-02-22  3:53 [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() Vivek Goyal
  2011-02-22  3:53 ` [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time Vivek Goyal
@ 2011-02-22  3:53 ` Vivek Goyal
  2011-02-22  7:30   ` Sergey Senozhatsky
  2011-02-22  3:53 ` [PATCH 3/3] block: Move blk_throtl_exit() call to blk_cleanup_queue() Vivek Goyal
  2011-02-22  4:20 ` [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during " NeilBrown
  3 siblings, 1 reply; 9+ messages in thread
From: Vivek Goyal @ 2011-02-22  3:53 UTC (permalink / raw)
  To: linux-kernel, jaxboe; +Cc: neilb, sergey.senozhatsky, tj, jmoyer, snitzer

o Now we initialize ->queue_lock at queue allocation time so driver does
  not have to worry about initializing it before calling blk_cleanup_queue().

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/block/loop.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 49e6a54..44e18c0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1641,9 +1641,6 @@ out:
 
 static void loop_free(struct loop_device *lo)
 {
-	if (!lo->lo_queue->queue_lock)
-		lo->lo_queue->queue_lock = &lo->lo_queue->__queue_lock;
-
 	blk_cleanup_queue(lo->lo_queue);
 	put_disk(lo->lo_disk);
 	list_del(&lo->lo_list);
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] block: Move blk_throtl_exit() call to blk_cleanup_queue()
  2011-02-22  3:53 [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() Vivek Goyal
  2011-02-22  3:53 ` [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time Vivek Goyal
  2011-02-22  3:53 ` [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue() Vivek Goyal
@ 2011-02-22  3:53 ` Vivek Goyal
  2011-02-22  4:20 ` [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during " NeilBrown
  3 siblings, 0 replies; 9+ messages in thread
From: Vivek Goyal @ 2011-02-22  3:53 UTC (permalink / raw)
  To: linux-kernel, jaxboe; +Cc: neilb, sergey.senozhatsky, tj, jmoyer, snitzer

o Move blk_throtl_exit() in blk_cleanup_queue() as blk_throtl_exit() is
  written in such a way that it needs queue lock. In blk_release_queue()
  there is no gurantee that ->queue_lock is still around.

o Initially blk_throtl_exit() was in blk_cleanup_queue() but Ingo reported
  one problem.

  https://lkml.org/lkml/2010/10/23/86

  And a quick fix moved blk_throtl_exit() to blk_release_queue().

        commit 7ad58c028652753814054f4e3ac58f925e7343f4
        Author: Jens Axboe <jaxboe@fusionio.com>
        Date:   Sat Oct 23 20:40:26 2010 +0200

        block: fix use-after-free bug in blk throttle code

o This patch reverts above change and does not try to shutdown the
  throtl timer or throtl work in blk_sync_queue.

o blk_sync_queue() seems to be used only by md driver and it seems to be
  using it to make sure q->unplug_fn is not called as md registers its
  own unplug functions and it is about to free up the data structures
  used by unplug_fn(). Block throttle does not call back into unplug_fn()
  or into md. So there is no need to cancel blk throttle work.

  In fact I think cancelling block throttle work is bad because it might
  happen that some bios are throttled and scheduled to be dispatched later
  with the help of pending work and if work is cancelled, these bios might
  never be dispatched.

  Block layer also uses blk_sync_queue() during blk_cleanup_queue() and
  blk_release_queue() time. That should be safe as we are also calling
  blk_throtl_exit() which should make sure all the throttling related
  data structures are cleaned up.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-core.c       |    7 ++++++-
 block/blk-sysfs.c      |    2 --
 block/blk-throttle.c   |    2 +-
 include/linux/blkdev.h |    2 --
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9a8e256..e8bf051 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -390,13 +390,16 @@ EXPORT_SYMBOL(blk_stop_queue);
  *     that its ->make_request_fn will not re-add plugging prior to calling
  *     this function.
  *
+ *     This function does not cancel any asynchronous activity arising
+ *     out of elevator or throttling code. That would require elevaotor_exit()
+ *     and blk_throtl_exit() to be called with queue lock initialized.
+ *
  */
 void blk_sync_queue(struct request_queue *q)
 {
 	del_timer_sync(&q->unplug_timer);
 	del_timer_sync(&q->timeout);
 	cancel_work_sync(&q->unplug_work);
-	throtl_shutdown_timer_wq(q);
 }
 EXPORT_SYMBOL(blk_sync_queue);
 
@@ -482,6 +485,8 @@ void blk_cleanup_queue(struct request_queue *q)
 	if (q->elevator)
 		elevator_exit(q->elevator);
 
+	blk_throtl_exit(q);
+
 	blk_put_queue(q);
 }
 EXPORT_SYMBOL(blk_cleanup_queue);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 41fb691..261c75c 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -471,8 +471,6 @@ static void blk_release_queue(struct kobject *kobj)
 
 	blk_sync_queue(q);
 
-	blk_throtl_exit(q);
-
 	if (rl->rq_pool)
 		mempool_destroy(rl->rq_pool);
 
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index bb7d21b..db41167 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -947,7 +947,7 @@ static void throtl_update_blkio_group_write_iops(void *key,
 	throtl_update_blkio_group_common(td, tg);
 }
 
-void throtl_shutdown_timer_wq(struct request_queue *q)
+static void throtl_shutdown_timer_wq(struct request_queue *q)
 {
 	struct throtl_data *td = q->td;
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4d18ff3..3f6b17b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1137,7 +1137,6 @@ extern int blk_throtl_init(struct request_queue *q);
 extern void blk_throtl_exit(struct request_queue *q);
 extern int blk_throtl_bio(struct request_queue *q, struct bio **bio);
 extern void throtl_schedule_delayed_work(struct request_queue *q, unsigned long delay);
-extern void throtl_shutdown_timer_wq(struct request_queue *q);
 #else /* CONFIG_BLK_DEV_THROTTLING */
 static inline int blk_throtl_bio(struct request_queue *q, struct bio **bio)
 {
@@ -1147,7 +1146,6 @@ static inline int blk_throtl_bio(struct request_queue *q, struct bio **bio)
 static inline int blk_throtl_init(struct request_queue *q) { return 0; }
 static inline int blk_throtl_exit(struct request_queue *q) { return 0; }
 static inline void throtl_schedule_delayed_work(struct request_queue *q, unsigned long delay) {}
-static inline void throtl_shutdown_timer_wq(struct request_queue *q) {}
 #endif /* CONFIG_BLK_DEV_THROTTLING */
 
 #define MODULE_ALIAS_BLOCKDEV(major,minor) \
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue()
  2011-02-22  3:53 [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() Vivek Goyal
                   ` (2 preceding siblings ...)
  2011-02-22  3:53 ` [PATCH 3/3] block: Move blk_throtl_exit() call to blk_cleanup_queue() Vivek Goyal
@ 2011-02-22  4:20 ` NeilBrown
  2011-02-22 14:17   ` Vivek Goyal
  3 siblings, 1 reply; 9+ messages in thread
From: NeilBrown @ 2011-02-22  4:20 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: linux-kernel, jaxboe, sergey.senozhatsky, tj, jmoyer, snitzer

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(-)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue()
  2011-02-22  3:53 ` [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue() Vivek Goyal
@ 2011-02-22  7:30   ` Sergey Senozhatsky
  2011-02-22 14:20     ` Vivek Goyal
  0 siblings, 1 reply; 9+ messages in thread
From: Sergey Senozhatsky @ 2011-02-22  7:30 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: linux-kernel, jaxboe, neilb, sergey.senozhatsky, tj, jmoyer, snitzer

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On (02/21/11 22:53), Vivek Goyal wrote:
> o Now we initialize ->queue_lock at queue allocation time so driver does
>   not have to worry about initializing it before calling blk_cleanup_queue().
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  drivers/block/loop.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 49e6a54..44e18c0 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1641,9 +1641,6 @@ out:
>  
>  static void loop_free(struct loop_device *lo)
>  {
> -	if (!lo->lo_queue->queue_lock)
> -		lo->lo_queue->queue_lock = &lo->lo_queue->__queue_lock;
> -
>  	blk_cleanup_queue(lo->lo_queue);
>  	put_disk(lo->lo_disk);
>  	list_del(&lo->lo_list);

Hi,

(just for note) 
There is an incremental patch fixing this case in Andrew's mm tree: 
https://lkml.org/lkml/2011/2/11/165
                                   
(block-fix-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v4.patch
added to -mm tree).


        Sergey

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue()
  2011-02-22  4:20 ` [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during " NeilBrown
@ 2011-02-22 14:17   ` Vivek Goyal
  0 siblings, 0 replies; 9+ messages in thread
From: Vivek Goyal @ 2011-02-22 14:17 UTC (permalink / raw)
  To: NeilBrown; +Cc: linux-kernel, jaxboe, sergey.senozhatsky, tj, jmoyer, snitzer

On Tue, Feb 22, 2011 at 03:20:03PM +1100, NeilBrown wrote:
> 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.
> 

Ok, that makes sense. So I will drop the WARN_ON() part.

Patch3 also removes throtl_shutdown_timer_wq() call from blk_sync_queue().
The only driver user of blk_sync_queue() seems to be md. IIUC, md needs
it because md provides its own unplug function and it does not want
that function to be called any more as it accesses some "conf" data
structures which are going away.

If that's the case, then we don't have to worry about quiescing throtl
work as it does not call unplug function. It just calls
generic_make_request(bio) and by that time md queue should be either 
DEAD or it should be able to handle bios coming in and returning error
while it is cleaning up its data structures.

In fact cancelling the pending throtl work will hang disptch of throttled
bios so it is not a good idea. blk_throtl_exit() is the right way to
cleanup all the throtl related state.

Removing this call from blk_sync_queue() enables me to move
blk_throtl_exit() in blk_cleanup_queue() and that makes sure ->queue_lock
is valid hence blk_throtl_exit() can execute safely.

Thanks
Vivek


> 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 --

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue()
  2011-02-22  7:30   ` Sergey Senozhatsky
@ 2011-02-22 14:20     ` Vivek Goyal
  2011-02-22 14:48       ` Sergey Senozhatsky
  0 siblings, 1 reply; 9+ messages in thread
From: Vivek Goyal @ 2011-02-22 14:20 UTC (permalink / raw)
  To: Sergey Senozhatsky; +Cc: linux-kernel, jaxboe, neilb, tj, jmoyer, snitzer

On Tue, Feb 22, 2011 at 09:30:32AM +0200, Sergey Senozhatsky wrote:
> On (02/21/11 22:53), Vivek Goyal wrote:
> > o Now we initialize ->queue_lock at queue allocation time so driver does
> >   not have to worry about initializing it before calling blk_cleanup_queue().
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  drivers/block/loop.c |    3 ---
> >  1 files changed, 0 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 49e6a54..44e18c0 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1641,9 +1641,6 @@ out:
> >  
> >  static void loop_free(struct loop_device *lo)
> >  {
> > -	if (!lo->lo_queue->queue_lock)
> > -		lo->lo_queue->queue_lock = &lo->lo_queue->__queue_lock;
> > -
> >  	blk_cleanup_queue(lo->lo_queue);
> >  	put_disk(lo->lo_disk);
> >  	list_del(&lo->lo_list);
> 
> Hi,
> 
> (just for note) 
> There is an incremental patch fixing this case in Andrew's mm tree: 
> https://lkml.org/lkml/2011/2/11/165
>                                    
> (block-fix-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v4.patch
> added to -mm tree).

Hi Sergey,

Thinking more about it, initializing queue lock in blk_alloc_queue() seems
to be even more cleaner to me instead of initializing it to internal lock
during blk_cleanup_queue(). If others like the idea, then we can either
ask Andrew to drop the patch or I can generate one on top of it.

Thanks
Vivek

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue()
  2011-02-22 14:20     ` Vivek Goyal
@ 2011-02-22 14:48       ` Sergey Senozhatsky
  0 siblings, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2011-02-22 14:48 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: linux-kernel, jaxboe, neilb, tj, jmoyer, snitzer

[-- Attachment #1: Type: text/plain, Size: 2049 bytes --]

On (02/22/11 09:20), Vivek Goyal wrote:
> On Tue, Feb 22, 2011 at 09:30:32AM +0200, Sergey Senozhatsky wrote:
> > On (02/21/11 22:53), Vivek Goyal wrote:
> > > o Now we initialize ->queue_lock at queue allocation time so driver does
> > >   not have to worry about initializing it before calling blk_cleanup_queue().
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  drivers/block/loop.c |    3 ---
> > >  1 files changed, 0 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > > index 49e6a54..44e18c0 100644
> > > --- a/drivers/block/loop.c
> > > +++ b/drivers/block/loop.c
> > > @@ -1641,9 +1641,6 @@ out:
> > >  
> > >  static void loop_free(struct loop_device *lo)
> > >  {
> > > -	if (!lo->lo_queue->queue_lock)
> > > -		lo->lo_queue->queue_lock = &lo->lo_queue->__queue_lock;
> > > -
> > >  	blk_cleanup_queue(lo->lo_queue);
> > >  	put_disk(lo->lo_disk);
> > >  	list_del(&lo->lo_list);
> > 
> > Hi,
> > 
> > (just for note) 
> > There is an incremental patch fixing this case in Andrew's mm tree: 
> > https://lkml.org/lkml/2011/2/11/165
> >                                    
> > (block-fix-queue_lock-null-pointer-derefence-in-blk_throtl_exit-v4.patch
> > added to -mm tree).
> 
> Hi Sergey,
> 
> Thinking more about it, initializing queue lock in blk_alloc_queue() seems
> to be even more cleaner to me instead of initializing it to internal lock
> during blk_cleanup_queue(). If others like the idea, then we can either
> ask Andrew to drop the patch or I can generate one on top of it.
> 

Hi Vivek,

Sure, fixing the even probability of NULL queue_lock is better (correct/sane/etc.) 
then fixing NULL queue_lock in `different places'. I'm presonally OK with dropping 
my patch.

By the way, I guess we should Cc stable when we decide which one [fix] will land 
on the mainline (taking in account that we're on -rc6 already).

Side Note: Need to check if .37 is affected (I suspect it is).


	Sergey

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-02-22 14:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-22  3:53 [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() Vivek Goyal
2011-02-22  3:53 ` [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time Vivek Goyal
2011-02-22  3:53 ` [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue() Vivek Goyal
2011-02-22  7:30   ` Sergey Senozhatsky
2011-02-22 14:20     ` Vivek Goyal
2011-02-22 14:48       ` Sergey Senozhatsky
2011-02-22  3:53 ` [PATCH 3/3] block: Move blk_throtl_exit() call to blk_cleanup_queue() Vivek Goyal
2011-02-22  4:20 ` [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during " NeilBrown
2011-02-22 14:17   ` Vivek Goyal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.