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 <[email protected]>
---
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
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
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
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 <[email protected]>
---
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
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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
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 [email protected]
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
Hello,
On Wed, May 30, 2012 at 3:28 PM, Asias He <[email protected]> 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
On 05/30/2012 02:28 PM, Tejun Heo wrote:
> Hello,
>
> On Wed, May 30, 2012 at 3:28 PM, Asias He<[email protected]> 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
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 05/30/2012 08:28 AM, Tejun Heo wrote:
> Hello,
>
> On Wed, May 30, 2012 at 3:28 PM, Asias He <[email protected]> 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
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<[email protected]> 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