diff mbox series

[3/3] blockdev: mirror: avoid potential deadlock when using iothread

Message ID 20231019131936.414246-4-f.ebner@proxmox.com (mailing list archive)
State New, archived
Headers show
Series fix a few blockjob-related deadlocks when using iothread | expand

Commit Message

Fiona Ebner Oct. 19, 2023, 1:19 p.m. UTC 
The bdrv_getlength() function is a generated co-wrapper and uses
AIO_WAIT_WHILE() to wait for the spawned coroutine. AIO_WAIT_WHILE()
expects the lock to be acquired exactly once.

This can happen when the source node is explicitly specified as the
@replaces parameter or if there is a filter on top of the source node.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 blockdev.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Fiona Ebner Oct. 20, 2023, 8:46 a.m. UTC | #1 
Am 19.10.23 um 15:19 schrieb Fiona Ebner:
> The bdrv_getlength() function is a generated co-wrapper and uses
> AIO_WAIT_WHILE() to wait for the spawned coroutine. AIO_WAIT_WHILE()
> expects the lock to be acquired exactly once.
> 
> This can happen when the source node is explicitly specified as the
> @replaces parameter or if there is a filter on top of the source node.

Correction: this should read "or if the source node is a filter node".
diff mbox series

Patch

diff --git a/blockdev.c b/blockdev.c
index a01c62596b..877e3a26d4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2968,6 +2968,7 @@  static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
 
     if (replaces) {
         BlockDriverState *to_replace_bs;
+        AioContext *aio_context;
         AioContext *replace_aio_context;
         int64_t bs_size, replace_size;
 
@@ -2982,10 +2983,19 @@  static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
             return;
         }
 
+        aio_context = bdrv_get_aio_context(bs);
         replace_aio_context = bdrv_get_aio_context(to_replace_bs);
-        aio_context_acquire(replace_aio_context);
+        /*
+         * bdrv_getlength() is a co-wrapper and uses AIO_WAIT_WHILE. Be sure not
+         * to acquire the same AioContext twice.
+         */
+        if (replace_aio_context != aio_context) {
+            aio_context_acquire(replace_aio_context);
+        }
         replace_size = bdrv_getlength(to_replace_bs);
-        aio_context_release(replace_aio_context);
+        if (replace_aio_context != aio_context) {
+            aio_context_release(replace_aio_context);
+        }
 
         if (replace_size < 0) {
             error_setg_errno(errp, -replace_size,