[PATCH] block: Fix lock unbalance caused by lock disconnect

[PATCH] block: Fix lock unbalance caused by lock disconnect

[Asias He]

Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
supplied queue_lock before blk_drain_queue(). This would introduce lock
unbalance because theads which have taken the external lock might unlock
the internal lock in the during the queue drain.

This patch fixes this by disconnecting the lock after the queue drain.

 =====================================
 [ BUG: bad unlock balance detected! ]
 3.4.0+ #288 Not tainted
 -------------------------------------
 fio/17706 is trying to release lock (&(&q->__queue_lock)->rlock) at:
 [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
 but there are no more locks to release!

 other info that might help us debug this:
 1 lock held by fio/17706:
  #0:  (&(&vblk->lock)->rlock){......}, at: [<ffffffff81327f1a>]
 get_request_wait+0x19a/0x250

 stack backtrace:
 Pid: 17706, comm: fio Not tainted 3.4.0+ #288
 Call Trace:
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810dea49>] print_unlock_inbalance_bug+0xf9/0x100
  [<ffffffff810dfe4f>] lock_release_non_nested+0x1df/0x330
  [<ffffffff811dae24>] ? dio_bio_end_aio+0x34/0xc0
  [<ffffffff811d6935>] ? bio_check_pages_dirty+0x85/0xe0
  [<ffffffff811daea1>] ? dio_bio_end_aio+0xb1/0xc0
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810e0079>] lock_release+0xd9/0x250
  [<ffffffff81a74553>] _raw_spin_unlock_irq+0x23/0x40
  [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
  [<ffffffff81328faa>] generic_make_request+0xca/0x100
  [<ffffffff81329056>] submit_bio+0x76/0xf0
  [<ffffffff8115470c>] ? set_page_dirty_lock+0x3c/0x60
  [<ffffffff811d69e1>] ? bio_set_pages_dirty+0x51/0x70
  [<ffffffff811dd1a8>] do_blockdev_direct_IO+0xbf8/0xee0
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811dd4e5>] __blockdev_direct_IO+0x55/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811d92e7>] blkdev_direct_IO+0x57/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff8114c6ae>] generic_file_aio_read+0x70e/0x760
  [<ffffffff810df7c5>] ? __lock_acquire+0x215/0x5a0
  [<ffffffff811e9924>] ? aio_run_iocb+0x54/0x1a0
  [<ffffffff8114bfa0>] ? grab_cache_page_nowait+0xc0/0xc0
  [<ffffffff811e82cc>] aio_rw_vect_retry+0x7c/0x1e0
  [<ffffffff811e8250>] ? aio_fsync+0x30/0x30
  [<ffffffff811e9936>] aio_run_iocb+0x66/0x1a0
  [<ffffffff811ea9b0>] do_io_submit+0x6f0/0xb80
  [<ffffffff8134de2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
  [<ffffffff811eae50>] sys_io_submit+0x10/0x20
  [<ffffffff81a7c9e9>] system_call_fastpath+0x16/0x1b

Signed-off-by: Asias He <asias@redhat.com>
---
 block/blk-core.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..7b4f6fe 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -416,15 +416,10 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* mark @q DEAD, no new request or merges will be allowed afterwards */
 	mutex_lock(&q->sysfs_lock);
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
-
 	spin_lock_irq(lock);
 	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
-
-	if (q->queue_lock != &q->__queue_lock)
-		q->queue_lock = &q->__queue_lock;
-
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
@@ -440,6 +435,11 @@ void blk_cleanup_queue(struct request_queue *q)
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
+	spin_lock_irq(lock);
+	if (q->queue_lock != &q->__queue_lock)
+		q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(lock);
+
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
 }
-- 
1.7.10.2

Re: [PATCH] block: Fix lock unbalance caused by lock disconnect

[Tejun Heo]

On Fri, May 25, 2012 at 10:10:59AM +0800, Asias He wrote:
> Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
> supplied queue_lock before blk_drain_queue(). This would introduce lock
> unbalance because theads which have taken the external lock might unlock
> the internal lock in the during the queue drain.
> 
> This patch fixes this by disconnecting the lock after the queue drain.

I don't think the patch description is correct.  The lock switcihng is
inherently broken and the patch doesn't really fix the problem
although it *might* make the problem less likely.  Trying to switch
locks while there are other accessors of the lock is simply broken, it
can never work without outer synchronization.  Your patch might make
the problem somewhat less likely simply because queue draining makes a
lot of request_queue users go away.

So, can you please update the commit description?  It doesn't really
*fix* anything but I think we're still better off with the change.

Thanks.

-- 
tejun

Re: [PATCH] block: Fix lock unbalance caused by lock disconnect

[Asias He]

On 05/28/2012 08:07 AM, Tejun Heo wrote:
> On Fri, May 25, 2012 at 10:10:59AM +0800, Asias He wrote:
>> Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
>> supplied queue_lock before blk_drain_queue(). This would introduce lock
>> unbalance because theads which have taken the external lock might unlock
>> the internal lock in the during the queue drain.
>>
>> This patch fixes this by disconnecting the lock after the queue drain.
>
> I don't think the patch description is correct.  The lock switcihng is
> inherently broken and the patch doesn't really fix the problem
> although it *might* make the problem less likely.  Trying to switch
> locks while there are other accessors of the lock is simply broken, it
> can never work without outer synchronization.

Since the lock switching is broken, is it a good idea to force all the 
drivers to use the block layer provided lock? i.e. Change the API from
blk_init_queue(rfn, driver_lock) to blk_init_queue(rfn). Any reason not 
to use the block layer provided one.

> Your patch might make
> the problem somewhat less likely simply because queue draining makes a
> lot of request_queue users go away.

Who will use the request_queue after blk_cleanup_queue()?

> So, can you please update the commit description?  It doesn't really
> *fix* anything but I think we're still better off with the change.

Sure. Will send v2.

-- 
Asias

[PATCH v2] block: Mitigate lock unbalance caused by lock disconnect

[Asias He]

Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
supplied queue_lock before blk_drain_queue(). This would introduce lock
unbalance because theads which have taken the external lock might unlock
the internal lock in the during the queue drain.

This patch mitigate this by disconnecting the lock after the queue
draining since queue draining makes a lot of request_queue users go
away.

 =====================================
 [ BUG: bad unlock balance detected! ]
 3.4.0+ #288 Not tainted
 -------------------------------------
 fio/17706 is trying to release lock (&(&q->__queue_lock)->rlock) at:
 [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
 but there are no more locks to release!

 other info that might help us debug this:
 1 lock held by fio/17706:
  #0:  (&(&vblk->lock)->rlock){......}, at: [<ffffffff81327f1a>]
 get_request_wait+0x19a/0x250

 stack backtrace:
 Pid: 17706, comm: fio Not tainted 3.4.0+ #288
 Call Trace:
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810dea49>] print_unlock_inbalance_bug+0xf9/0x100
  [<ffffffff810dfe4f>] lock_release_non_nested+0x1df/0x330
  [<ffffffff811dae24>] ? dio_bio_end_aio+0x34/0xc0
  [<ffffffff811d6935>] ? bio_check_pages_dirty+0x85/0xe0
  [<ffffffff811daea1>] ? dio_bio_end_aio+0xb1/0xc0
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810e0079>] lock_release+0xd9/0x250
  [<ffffffff81a74553>] _raw_spin_unlock_irq+0x23/0x40
  [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
  [<ffffffff81328faa>] generic_make_request+0xca/0x100
  [<ffffffff81329056>] submit_bio+0x76/0xf0
  [<ffffffff8115470c>] ? set_page_dirty_lock+0x3c/0x60
  [<ffffffff811d69e1>] ? bio_set_pages_dirty+0x51/0x70
  [<ffffffff811dd1a8>] do_blockdev_direct_IO+0xbf8/0xee0
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811dd4e5>] __blockdev_direct_IO+0x55/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811d92e7>] blkdev_direct_IO+0x57/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff8114c6ae>] generic_file_aio_read+0x70e/0x760
  [<ffffffff810df7c5>] ? __lock_acquire+0x215/0x5a0
  [<ffffffff811e9924>] ? aio_run_iocb+0x54/0x1a0
  [<ffffffff8114bfa0>] ? grab_cache_page_nowait+0xc0/0xc0
  [<ffffffff811e82cc>] aio_rw_vect_retry+0x7c/0x1e0
  [<ffffffff811e8250>] ? aio_fsync+0x30/0x30
  [<ffffffff811e9936>] aio_run_iocb+0x66/0x1a0
  [<ffffffff811ea9b0>] do_io_submit+0x6f0/0xb80
  [<ffffffff8134de2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
  [<ffffffff811eae50>] sys_io_submit+0x10/0x20
  [<ffffffff81a7c9e9>] system_call_fastpath+0x16/0x1b

Changes since v1: Update commit log as Tejun suggested.

Signed-off-by: Asias He <asias@redhat.com>
---
 block/blk-core.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..7b4f6fe 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -416,15 +416,10 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* mark @q DEAD, no new request or merges will be allowed afterwards */
 	mutex_lock(&q->sysfs_lock);
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
-
 	spin_lock_irq(lock);
 	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
-
-	if (q->queue_lock != &q->__queue_lock)
-		q->queue_lock = &q->__queue_lock;
-
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
@@ -440,6 +435,11 @@ void blk_cleanup_queue(struct request_queue *q)
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
+	spin_lock_irq(lock);
+	if (q->queue_lock != &q->__queue_lock)
+		q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(lock);
+
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
 }
-- 
1.7.10.2

Re: [PATCH] block: Fix lock unbalance caused by lock disconnect

[Tejun Heo]

Hello, Asias.

On Mon, May 28, 2012 at 10:15:18AM +0800, Asias He wrote:
> >I don't think the patch description is correct.  The lock switcihng is
> >inherently broken and the patch doesn't really fix the problem
> >although it *might* make the problem less likely.  Trying to switch
> >locks while there are other accessors of the lock is simply broken, it
> >can never work without outer synchronization.
> 
> Since the lock switching is broken, is it a good idea to force all
> the drivers to use the block layer provided lock? i.e. Change the
> API from
> blk_init_queue(rfn, driver_lock) to blk_init_queue(rfn). Any reason
> not to use the block layer provided one.

I think hch tried to do that a while ago.  Dunno what happened to the
patches.  IIRC, the whole external lock thing was about sharing a
single lock across different request_queues.  Not sure whether it's
actually beneficial enough or just a crazy broken optimization.

> >Your patch might make
> >the problem somewhat less likely simply because queue draining makes a
> >lot of request_queue users go away.
> 
> Who will use the request_queue after blk_cleanup_queue()?

Anyone who still holds a ref might try to issue a new request on a
dead queue.  ie. blkdev with filesystem mounted goes away and the FS
issues a new read request after blk_cleanup_queue() finishes drainig.

Thanks.

-- 
tejun

Re: [PATCH v2] block: Mitigate lock unbalance caused by lock disconnect

[Tejun Heo]

On Mon, May 28, 2012 at 10:20:03AM +0800, Asias He wrote:
> Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
> supplied queue_lock before blk_drain_queue(). This would introduce lock
> unbalance because theads which have taken the external lock might unlock
> the internal lock in the during the queue drain.
> 
> This patch mitigate this by disconnecting the lock after the queue
> draining since queue draining makes a lot of request_queue users go
> away.

Can you please point out how the code is broken and that the code is
still broken after the patch but somewhat less likely to actually
fail?

Thanks.

-- 
tejun

[PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Asias He]

Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
supplied queue_lock before blk_drain_queue(). Switching the lock would
introduce lock unbalance because theads which have taken the external
lock might unlock the internal lock in the during the queue drain. This
patch mitigate this by disconnecting the lock after the queue draining
since queue draining makes a lot of request_queue users go away.

However, please note, this patch only makes the problem less likely to
happen. Anyone who still holds a ref might try to issue a new request on
a dead queue after the blk_cleanup_queue() finishes draining, the lock
unbalance might still happen in this case.

 =====================================
 [ BUG: bad unlock balance detected! ]
 3.4.0+ #288 Not tainted
 -------------------------------------
 fio/17706 is trying to release lock (&(&q->__queue_lock)->rlock) at:
 [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
 but there are no more locks to release!

 other info that might help us debug this:
 1 lock held by fio/17706:
  #0:  (&(&vblk->lock)->rlock){......}, at: [<ffffffff81327f1a>]
 get_request_wait+0x19a/0x250

 stack backtrace:
 Pid: 17706, comm: fio Not tainted 3.4.0+ #288
 Call Trace:
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810dea49>] print_unlock_inbalance_bug+0xf9/0x100
  [<ffffffff810dfe4f>] lock_release_non_nested+0x1df/0x330
  [<ffffffff811dae24>] ? dio_bio_end_aio+0x34/0xc0
  [<ffffffff811d6935>] ? bio_check_pages_dirty+0x85/0xe0
  [<ffffffff811daea1>] ? dio_bio_end_aio+0xb1/0xc0
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810e0079>] lock_release+0xd9/0x250
  [<ffffffff81a74553>] _raw_spin_unlock_irq+0x23/0x40
  [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
  [<ffffffff81328faa>] generic_make_request+0xca/0x100
  [<ffffffff81329056>] submit_bio+0x76/0xf0
  [<ffffffff8115470c>] ? set_page_dirty_lock+0x3c/0x60
  [<ffffffff811d69e1>] ? bio_set_pages_dirty+0x51/0x70
  [<ffffffff811dd1a8>] do_blockdev_direct_IO+0xbf8/0xee0
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811dd4e5>] __blockdev_direct_IO+0x55/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811d92e7>] blkdev_direct_IO+0x57/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff8114c6ae>] generic_file_aio_read+0x70e/0x760
  [<ffffffff810df7c5>] ? __lock_acquire+0x215/0x5a0
  [<ffffffff811e9924>] ? aio_run_iocb+0x54/0x1a0
  [<ffffffff8114bfa0>] ? grab_cache_page_nowait+0xc0/0xc0
  [<ffffffff811e82cc>] aio_rw_vect_retry+0x7c/0x1e0
  [<ffffffff811e8250>] ? aio_fsync+0x30/0x30
  [<ffffffff811e9936>] aio_run_iocb+0x66/0x1a0
  [<ffffffff811ea9b0>] do_io_submit+0x6f0/0xb80
  [<ffffffff8134de2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
  [<ffffffff811eae50>] sys_io_submit+0x10/0x20
  [<ffffffff81a7c9e9>] system_call_fastpath+0x16/0x1b

Changes since v2: Update commit log to explain how the code is still
                  broken even if we delay the lock switching after the drain.
Changes since v1: Update commit log as Tejun suggested.

Signed-off-by: Asias He <asias@redhat.com>
---
 block/blk-core.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..7b4f6fe 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -416,15 +416,10 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* mark @q DEAD, no new request or merges will be allowed afterwards */
 	mutex_lock(&q->sysfs_lock);
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
-
 	spin_lock_irq(lock);
 	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
-
-	if (q->queue_lock != &q->__queue_lock)
-		q->queue_lock = &q->__queue_lock;
-
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
@@ -440,6 +435,11 @@ void blk_cleanup_queue(struct request_queue *q)
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
+	spin_lock_irq(lock);
+	if (q->queue_lock != &q->__queue_lock)
+		q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(lock);
+
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
 }
-- 
1.7.10.2

[PATCH] block: Mitigate lock unbalance caused by lock switching

[Asias He]

Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
supplied queue_lock before blk_drain_queue(). Switching the lock would
introduce lock unbalance because theads which have taken the external
lock might unlock the internal lock in the during the queue drain. This
patch mitigate this by disconnecting the lock after the queue draining
since queue draining makes a lot of request_queue users go away.

However, please note, this patch only makes the problem less likely to
happen. Anyone who still holds a ref might try to issue a new request on
a dead queue after the blk_cleanup_queue() finishes draining, the lock
unbalance might still happen in this case.

 =====================================
 [ BUG: bad unlock balance detected! ]
 3.4.0+ #288 Not tainted
 -------------------------------------
 fio/17706 is trying to release lock (&(&q->__queue_lock)->rlock) at:
 [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
 but there are no more locks to release!

 other info that might help us debug this:
 1 lock held by fio/17706:
  #0:  (&(&vblk->lock)->rlock){......}, at: [<ffffffff81327f1a>]
 get_request_wait+0x19a/0x250

 stack backtrace:
 Pid: 17706, comm: fio Not tainted 3.4.0+ #288
 Call Trace:
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810dea49>] print_unlock_inbalance_bug+0xf9/0x100
  [<ffffffff810dfe4f>] lock_release_non_nested+0x1df/0x330
  [<ffffffff811dae24>] ? dio_bio_end_aio+0x34/0xc0
  [<ffffffff811d6935>] ? bio_check_pages_dirty+0x85/0xe0
  [<ffffffff811daea1>] ? dio_bio_end_aio+0xb1/0xc0
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
  [<ffffffff810e0079>] lock_release+0xd9/0x250
  [<ffffffff81a74553>] _raw_spin_unlock_irq+0x23/0x40
  [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
  [<ffffffff81328faa>] generic_make_request+0xca/0x100
  [<ffffffff81329056>] submit_bio+0x76/0xf0
  [<ffffffff8115470c>] ? set_page_dirty_lock+0x3c/0x60
  [<ffffffff811d69e1>] ? bio_set_pages_dirty+0x51/0x70
  [<ffffffff811dd1a8>] do_blockdev_direct_IO+0xbf8/0xee0
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811dd4e5>] __blockdev_direct_IO+0x55/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff811d92e7>] blkdev_direct_IO+0x57/0x60
  [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
  [<ffffffff8114c6ae>] generic_file_aio_read+0x70e/0x760
  [<ffffffff810df7c5>] ? __lock_acquire+0x215/0x5a0
  [<ffffffff811e9924>] ? aio_run_iocb+0x54/0x1a0
  [<ffffffff8114bfa0>] ? grab_cache_page_nowait+0xc0/0xc0
  [<ffffffff811e82cc>] aio_rw_vect_retry+0x7c/0x1e0
  [<ffffffff811e8250>] ? aio_fsync+0x30/0x30
  [<ffffffff811e9936>] aio_run_iocb+0x66/0x1a0
  [<ffffffff811ea9b0>] do_io_submit+0x6f0/0xb80
  [<ffffffff8134de2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
  [<ffffffff811eae50>] sys_io_submit+0x10/0x20
  [<ffffffff81a7c9e9>] system_call_fastpath+0x16/0x1b

Changes since v2: Update commit log to explain how the code is still
                  broken even if we delay the lock switching after the drain.
Changes since v1: Update commit log as Tejun suggested.

Signed-off-by: Asias He <asias@redhat.com>
---
 block/blk-core.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..7b4f6fe 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -416,15 +416,10 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* mark @q DEAD, no new request or merges will be allowed afterwards */
 	mutex_lock(&q->sysfs_lock);
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
-
 	spin_lock_irq(lock);
 	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
-
-	if (q->queue_lock != &q->__queue_lock)
-		q->queue_lock = &q->__queue_lock;
-
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
@@ -440,6 +435,11 @@ void blk_cleanup_queue(struct request_queue *q)
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
+	spin_lock_irq(lock);
+	if (q->queue_lock != &q->__queue_lock)
+		q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(lock);
+
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
 }
-- 
1.7.10.2

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Tejun Heo]

On Tue, May 29, 2012 at 09:39:01AM +0800, Asias He wrote:
> Commit 777eb1bf15b8532c396821774bf6451e563438f5 disconnects externally
> supplied queue_lock before blk_drain_queue(). Switching the lock would
> introduce lock unbalance because theads which have taken the external
> lock might unlock the internal lock in the during the queue drain. This
> patch mitigate this by disconnecting the lock after the queue draining
> since queue draining makes a lot of request_queue users go away.
> 
> However, please note, this patch only makes the problem less likely to
> happen. Anyone who still holds a ref might try to issue a new request on
> a dead queue after the blk_cleanup_queue() finishes draining, the lock
> unbalance might still happen in this case.
> 
>  =====================================
>  [ BUG: bad unlock balance detected! ]
>  3.4.0+ #288 Not tainted
>  -------------------------------------
>  fio/17706 is trying to release lock (&(&q->__queue_lock)->rlock) at:
>  [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
>  but there are no more locks to release!
> 
>  other info that might help us debug this:
>  1 lock held by fio/17706:
>   #0:  (&(&vblk->lock)->rlock){......}, at: [<ffffffff81327f1a>]
>  get_request_wait+0x19a/0x250
> 
>  stack backtrace:
>  Pid: 17706, comm: fio Not tainted 3.4.0+ #288
>  Call Trace:
>   [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
>   [<ffffffff810dea49>] print_unlock_inbalance_bug+0xf9/0x100
>   [<ffffffff810dfe4f>] lock_release_non_nested+0x1df/0x330
>   [<ffffffff811dae24>] ? dio_bio_end_aio+0x34/0xc0
>   [<ffffffff811d6935>] ? bio_check_pages_dirty+0x85/0xe0
>   [<ffffffff811daea1>] ? dio_bio_end_aio+0xb1/0xc0
>   [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
>   [<ffffffff81329372>] ? blk_queue_bio+0x2a2/0x380
>   [<ffffffff810e0079>] lock_release+0xd9/0x250
>   [<ffffffff81a74553>] _raw_spin_unlock_irq+0x23/0x40
>   [<ffffffff81329372>] blk_queue_bio+0x2a2/0x380
>   [<ffffffff81328faa>] generic_make_request+0xca/0x100
>   [<ffffffff81329056>] submit_bio+0x76/0xf0
>   [<ffffffff8115470c>] ? set_page_dirty_lock+0x3c/0x60
>   [<ffffffff811d69e1>] ? bio_set_pages_dirty+0x51/0x70
>   [<ffffffff811dd1a8>] do_blockdev_direct_IO+0xbf8/0xee0
>   [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
>   [<ffffffff811dd4e5>] __blockdev_direct_IO+0x55/0x60
>   [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
>   [<ffffffff811d92e7>] blkdev_direct_IO+0x57/0x60
>   [<ffffffff811d8620>] ? blkdev_get_block+0x80/0x80
>   [<ffffffff8114c6ae>] generic_file_aio_read+0x70e/0x760
>   [<ffffffff810df7c5>] ? __lock_acquire+0x215/0x5a0
>   [<ffffffff811e9924>] ? aio_run_iocb+0x54/0x1a0
>   [<ffffffff8114bfa0>] ? grab_cache_page_nowait+0xc0/0xc0
>   [<ffffffff811e82cc>] aio_rw_vect_retry+0x7c/0x1e0
>   [<ffffffff811e8250>] ? aio_fsync+0x30/0x30
>   [<ffffffff811e9936>] aio_run_iocb+0x66/0x1a0
>   [<ffffffff811ea9b0>] do_io_submit+0x6f0/0xb80
>   [<ffffffff8134de2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>   [<ffffffff811eae50>] sys_io_submit+0x10/0x20
>   [<ffffffff81a7c9e9>] system_call_fastpath+0x16/0x1b
> 
> Changes since v2: Update commit log to explain how the code is still
>                   broken even if we delay the lock switching after the drain.
> Changes since v1: Update commit log as Tejun suggested.
> 
> Signed-off-by: Asias He <asias@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] block: Fix lock unbalance caused by lock disconnect

[Asias He]

On 05/28/2012 06:20 PM, Tejun Heo wrote:
> Hello, Asias.
>
> On Mon, May 28, 2012 at 10:15:18AM +0800, Asias He wrote:
>>> I don't think the patch description is correct.  The lock switcihng is
>>> inherently broken and the patch doesn't really fix the problem
>>> although it *might* make the problem less likely.  Trying to switch
>>> locks while there are other accessors of the lock is simply broken, it
>>> can never work without outer synchronization.
>>
>> Since the lock switching is broken, is it a good idea to force all
>> the drivers to use the block layer provided lock? i.e. Change the
>> API from
>> blk_init_queue(rfn, driver_lock) to blk_init_queue(rfn). Any reason
>> not to use the block layer provided one.
>
> I think hch tried to do that a while ago.  Dunno what happened to the
> patches.  IIRC, the whole external lock thing was about sharing a
> single lock across different request_queues.  Not sure whether it's
> actually beneficial enough or just a crazy broken optimization.

Do we have any existing use case of sharing a single lock across 
different request_queues? What's point of this sharing. Christoph?

If nobody has any objections I'd like to make the patches. Jens, any 
comments?

>>> Your patch might make
>>> the problem somewhat less likely simply because queue draining makes a
>>> lot of request_queue users go away.
>>
>> Who will use the request_queue after blk_cleanup_queue()?
>
> Anyone who still holds a ref might try to issue a new request on a
> dead queue.  ie. blkdev with filesystem mounted goes away and the FS
> issues a new read request after blk_cleanup_queue() finishes drainig.

OK. Thanks for explaining.


-- 
Asias

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Tim Gardner]

On 05/28/2012 07:39 PM, Asias He wrote:

<snip>

> @@ -440,6 +435,11 @@ void blk_cleanup_queue(struct request_queue *q)
>  	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
>  	blk_sync_queue(q);
>  
> +	spin_lock_irq(lock);
> +	if (q->queue_lock != &q->__queue_lock)
> +		q->queue_lock = &q->__queue_lock;
> +	spin_unlock_irq(lock);
> +

Isn't the 'if' clause superfluous ? You could just do the assignment, e.g.,

+	spin_lock_irq(lock);
+	q->queue_lock = &q->__queue_lock;
+	spin_unlock_irq(lock);

rtg
-- 
Tim Gardner tim.gardner@canonical.com

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Asias He]

On 05/29/2012 09:45 PM, Tim Gardner wrote:
> On 05/28/2012 07:39 PM, Asias He wrote:
>
> <snip>
>
>> @@ -440,6 +435,11 @@ void blk_cleanup_queue(struct request_queue *q)
>>   	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
>>   	blk_sync_queue(q);
>>
>> +	spin_lock_irq(lock);
>> +	if (q->queue_lock !=&q->__queue_lock)
>> +		q->queue_lock =&q->__queue_lock;
>> +	spin_unlock_irq(lock);
>> +
>
> Isn't the 'if' clause superfluous ? You could just do the assignment, e.g.,
>
> +	spin_lock_irq(lock);
> +	q->queue_lock =&q->__queue_lock;
> +	spin_unlock_irq(lock);

Well, this saves a if clause but adds an unnecessary assignment if the 
lock is already internal lock.

-- 
Asias

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Tejun Heo]

Hello,

On Wed, May 30, 2012 at 3:28 PM, Asias He <asias@redhat.com> wrote:
>> Isn't the 'if' clause superfluous ? You could just do the assignment,
>> e.g.,
>>
>> +       spin_lock_irq(lock);
>> +       q->queue_lock =&q->__queue_lock;
>> +       spin_unlock_irq(lock);
>
>
> Well, this saves a if clause but adds an unnecessary assignment if the lock
> is already internal lock.

It's not hot path. Dirtying the cacheline there doesn't mean anything.
I don't really care either way but making optimization argument is
pretty silly here.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Asias He]

On 05/30/2012 02:28 PM, Tejun Heo wrote:
> Hello,
>
> On Wed, May 30, 2012 at 3:28 PM, Asias He<asias@redhat.com>  wrote:
>>> Isn't the 'if' clause superfluous ? You could just do the assignment,
>>> e.g.,
>>>
>>> +       spin_lock_irq(lock);
>>> +       q->queue_lock =&q->__queue_lock;
>>> +       spin_unlock_irq(lock);
>>
>>
>> Well, this saves a if clause but adds an unnecessary assignment if the lock
>> is already internal lock.
>
> It's not hot path. Dirtying the cacheline there doesn't mean anything.
> I don't really care either way but making optimization argument is
> pretty silly here.

I don't care this neither ;-)

-- 
Asias

Re: [PATCH] block: Fix lock unbalance caused by lock disconnect

[Michael S. Tsirkin]

On Mon, May 28, 2012 at 07:20:55PM +0900, Tejun Heo wrote:
> Hello, Asias.
> 
> On Mon, May 28, 2012 at 10:15:18AM +0800, Asias He wrote:
> > >I don't think the patch description is correct.  The lock switcihng is
> > >inherently broken and the patch doesn't really fix the problem
> > >although it *might* make the problem less likely.  Trying to switch
> > >locks while there are other accessors of the lock is simply broken, it
> > >can never work without outer synchronization.
> > 
> > Since the lock switching is broken, is it a good idea to force all
> > the drivers to use the block layer provided lock? i.e. Change the
> > API from
> > blk_init_queue(rfn, driver_lock) to blk_init_queue(rfn). Any reason
> > not to use the block layer provided one.
> 
> I think hch tried to do that a while ago.  Dunno what happened to the
> patches.  IIRC, the whole external lock thing was about sharing a
> single lock across different request_queues.  Not sure whether it's
> actually beneficial enough or just a crazy broken optimization.

Looks like almost all drivers get it wrong.  And it's likely
something like a floppy driver doesn't need an optimization:
drivers/block/floppy.c:	disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);

The obvious use of this API is wrong. So how about introducing
a correct one, deprecating the broken one so we can start
slowly converting users?

Then if someone sees a real reason for the internal lock,
he will complain.




> > >Your patch might make
> > >the problem somewhat less likely simply because queue draining makes a
> > >lot of request_queue users go away.
> > 
> > Who will use the request_queue after blk_cleanup_queue()?
> 
> Anyone who still holds a ref might try to issue a new request on a
> dead queue.  ie. blkdev with filesystem mounted goes away and the FS
> issues a new read request after blk_cleanup_queue() finishes drainig.
> 
> Thanks.
> 
> -- 
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Jens Axboe]

On 05/30/2012 08:28 AM, Tejun Heo wrote:
> Hello,
> 
> On Wed, May 30, 2012 at 3:28 PM, Asias He <asias@redhat.com> wrote:
>>> Isn't the 'if' clause superfluous ? You could just do the assignment,
>>> e.g.,
>>>
>>> +       spin_lock_irq(lock);
>>> +       q->queue_lock =&q->__queue_lock;
>>> +       spin_unlock_irq(lock);
>>
>>
>> Well, this saves a if clause but adds an unnecessary assignment if the lock
>> is already internal lock.
> 
> It's not hot path. Dirtying the cacheline there doesn't mean anything.
> I don't really care either way but making optimization argument is
> pretty silly here.

And more importantly, dropping the if loses information as well. That's
a lot more important than any misguided optimization attempts. So I
agree, the if stays.

-- 
Jens Axboe

Re: [PATCH V3] block: Mitigate lock unbalance caused by lock switching

[Asias He]

Hello, Jens

On 06/01/2012 05:31 PM, Jens Axboe wrote:
> On 05/30/2012 08:28 AM, Tejun Heo wrote:
>> Hello,
>>
>> On Wed, May 30, 2012 at 3:28 PM, Asias He<asias@redhat.com>  wrote:
>>>> Isn't the 'if' clause superfluous ? You could just do the assignment,
>>>> e.g.,
>>>>
>>>> +       spin_lock_irq(lock);
>>>> +       q->queue_lock =&q->__queue_lock;
>>>> +       spin_unlock_irq(lock);
>>>
>>>
>>> Well, this saves a if clause but adds an unnecessary assignment if the lock
>>> is already internal lock.
>>
>> It's not hot path. Dirtying the cacheline there doesn't mean anything.
>> I don't really care either way but making optimization argument is
>> pretty silly here.
>
> And more importantly, dropping the if loses information as well. That's
> a lot more important than any misguided optimization attempts. So I
> agree, the if stays.

Could you pick this patch in your tree?

-- 
Asias