bdrv_unref() is called by a lot of places that need to hold the graph
lock (it naturally happens in the context of operations that change the
graph). However, bdrv_unref() takes the graph writer lock internally, so
it can't actually be called while already holding a graph lock without
causing a deadlock.
bdrv_unref() also can't just become GRAPH_WRLOCK because it drains the
node before closing it, and draining requires that the graph is
unlocked.
The solution is to defer deleting the node until we don't hold the lock
any more and draining is possible again.
Note that keeping images open for longer than necessary can create
problems, too: You can't open an image again before it is really closed
(if image locking didn't prevent it, it would cause corruption).
Reopening an image immediately happens at least during bdrv_open() and
bdrv_co_create().
In order to solve this problem, make sure to run the deferred unref in
bdrv_graph_wrunlock(), i.e. the first possible place where we can drain
again. This is also why bdrv_schedule_unref() is marked GRAPH_WRLOCK.
The output of iotest 051 is updated because the additional polling
changes the order of HMP output, resulting in a new "(qemu)" prompt in
the test output that was previously on a separate line and filtered out.
Signed-off-by: Kevin Wolf <kw...@redhat.com>
---
include/block/block-global-state.h | 1 +
block.c | 9 +++++++++
block/graph-lock.c | 23 ++++++++++++++++-------
tests/qemu-iotests/051.pc.out | 6 +++---
4 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/include/block/block-global-state.h
b/include/block/block-global-state.h
index f347199bff..e570799f85 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -224,6 +224,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
void bdrv_ref(BlockDriverState *bs);
void no_coroutine_fn bdrv_unref(BlockDriverState *bs);
void coroutine_fn no_co_wrapper bdrv_co_unref(BlockDriverState *bs);
+void GRAPH_WRLOCK bdrv_schedule_unref(BlockDriverState *bs);
void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
BlockDriverState *child_bs,
diff --git a/block.c b/block.c
index 6376452768..9c4f24f4b9 100644
--- a/block.c
+++ b/block.c
@@ -7033,6 +7033,15 @@ void bdrv_unref(BlockDriverState *bs)
}
}
+void bdrv_schedule_unref(BlockDriverState *bs)
+{
+ if (!bs) {
+ return;
+ }
+ aio_bh_schedule_oneshot(qemu_get_aio_context(),
+ (QEMUBHFunc *) bdrv_unref, bs);
+}
+
struct BdrvOpBlocker {
Error *reason;
QLIST_ENTRY(BdrvOpBlocker) list;
diff --git a/block/graph-lock.c b/block/graph-lock.c
index 5e66f01ae8..395d387651 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -163,17 +163,26 @@ void bdrv_graph_wrlock(BlockDriverState *bs)
void bdrv_graph_wrunlock(void)
{
GLOBAL_STATE_CODE();
- QEMU_LOCK_GUARD(&aio_context_list_lock);
assert(qatomic_read(&has_writer));
+ WITH_QEMU_LOCK_GUARD(&aio_context_list_lock) {
+ /*
+ * No need for memory barriers, this works in pair with
+ * the slow path of rdlock() and both take the lock.
+ */
+ qatomic_store_release(&has_writer, 0);
+
+ /* Wake up all coroutine that are waiting to read the graph */
+ qemu_co_enter_all(&reader_queue, &aio_context_list_lock);
+ }
+
/*
- * No need for memory barriers, this works in pair with
- * the slow path of rdlock() and both take the lock.
+ * Run any BHs that were scheduled during the wrlock section and that
+ * callers might expect to have finished (e.g. bdrv_unref() calls). Do this
+ * only after restarting coroutines so that nested event loops in BHs don't
+ * deadlock if their condition relies on the coroutine making progress.
*/
- qatomic_store_release(&has_writer, 0);
-
- /* Wake up all coroutine that are waiting to read the graph */
- qemu_co_enter_all(&reader_queue, &aio_context_list_lock);
+ aio_bh_poll(qemu_get_aio_context());
}
void coroutine_fn bdrv_graph_co_rdlock(void)
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 4d4af5a486..7e10c5fa1b 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -169,11 +169,11 @@ QEMU_PROG: -device scsi-hd,drive=disk: Device needs
media, but drive is empty
Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device
ide-hd,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
-QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of
active block backend
+(qemu) QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change
iothread of active block backend
Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device
virtio-blk-pci,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
-QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change
iothread of active block backend
+(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change
iothread of active block backend
Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device
lsi53c895a,id=lsi0 -device scsi-hd,bus=lsi0.0,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
@@ -185,7 +185,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
-QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on:
Cannot change iothread of active block backend
+(qemu) QEMU_PROG: -device
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread
of active block backend
Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device
virtio-scsi,id=virtio-scsi1,iothread=thread0 -device
scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
--
2.41.0
