[Qemu-devel] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads

[Qemu-devel] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads

[Stefan Hajnoczi]

v2:
 * Use StrOrNull for x-blockdev-set-iothread iothread argument [eblake]

(This is for QEMU 2.12 because this bug is not -rc4 critical)

Previously AioContext was held across QMP 'transaction' in an attempt to
achieve bdrv_drained_begin/end() semantics.  Nowadays we have
bdrv_drained_begin/end() and the AioContext lock just protects state.
Therefore there is no reason to hold AioContext across
.prepare/.commit/.abort/.clean() anymore.

Besides being cleanup-worthy, holding AioContext also exposes a bug:
BDRV_POLL_WHILE() doesn't support recursive AioContext locking and will hang if
depth > 1.  This is easy to trigger by submitting a transaction with 2 actions
that touch two drives assigned to an IOThread.  The IOThread's AioContext will
be locked twice and BDRV_POLL_WHILE() will hang.

BDRV_POLL_WHILE() is best fixed by eliminating the AioContext lock entirely in
favor of fine-grained locking.  I discussed some fixes for BDRV_POLL_WHILE()
with Paolo but we came to the conclusion that it will just add complexity when
we really want to stop using AioContext locking.

Summary:

 * Patch 1 fixes missing AioContext lock protection

 * Patches 2-6 clean up excessive AioContext locked regions in QMP
   'transaction' to solve the hang

 * Patches 7-9 add a qemu-iotests test case and the necessary infrastructure

Stefan Hajnoczi (9):
  blockdev: hold AioContext for bdrv_unref() in
    external_snapshot_clean()
  block: don't keep AioContext acquired after
    external_snapshot_prepare()
  block: don't keep AioContext acquired after drive_backup_prepare()
  block: don't keep AioContext acquired after blockdev_backup_prepare()
  block: don't keep AioContext acquired after
    internal_snapshot_prepare()
  block: drop unused BlockDirtyBitmapState->aio_context field
  iothread: add iothread_by_id() API
  blockdev: add x-blockdev-set-iothread testing command
  qemu-iotests: add 202 external snapshots IOThread test

 qapi/block-core.json       |  36 +++++++
 include/sysemu/iothread.h  |   1 +
 blockdev.c                 | 258 +++++++++++++++++++++++++++++++++------------
 iothread.c                 |   7 ++
 tests/qemu-iotests/202     |  95 +++++++++++++++++
 tests/qemu-iotests/202.out |  11 ++
 tests/qemu-iotests/group   |   1 +
 7 files changed, 339 insertions(+), 70 deletions(-)
 create mode 100755 tests/qemu-iotests/202
 create mode 100644 tests/qemu-iotests/202.out

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/9] blockdev: hold AioContext for bdrv_unref() in external_snapshot_clean()

[Stefan Hajnoczi]

bdrv_unref() requires the AioContext lock because bdrv_flush() uses
BDRV_POLL_WHILE(), which assumes the AioContext is currently held.  If
BDRV_POLL_WHILE() runs without AioContext held the
pthread_mutex_unlock() call in aio_context_release() fails.

This patch moves bdrv_unref() into the AioContext locked region to solve
the following pthread_mutex_unlock() failure:

  #0  0x00007f566181969b in raise () at /lib64/libc.so.6
  #1  0x00007f566181b3b1 in abort () at /lib64/libc.so.6
  #2  0x00005592cd590458 in error_exit (err=<optimized out>, msg=msg@entry=0x5592cdaf6d60 <__func__.23977> "qemu_mutex_unlock") at util/qemu-thread-posix.c:36
  #3  0x00005592cd96e738 in qemu_mutex_unlock (mutex=mutex@entry=0x5592ce9505e0) at util/qemu-thread-posix.c:96
  #4  0x00005592cd969b69 in aio_context_release (ctx=ctx@entry=0x5592ce950580) at util/async.c:507
  #5  0x00005592cd8ead78 in bdrv_flush (bs=bs@entry=0x5592cfa87210) at block/io.c:2478
  #6  0x00005592cd89df30 in bdrv_close (bs=0x5592cfa87210) at block.c:3207
  #7  0x00005592cd89df30 in bdrv_delete (bs=0x5592cfa87210) at block.c:3395
  #8  0x00005592cd89df30 in bdrv_unref (bs=0x5592cfa87210) at block.c:4418
  #9  0x00005592cd6b7f86 in qmp_transaction (dev_list=<optimized out>, has_props=<optimized out>, props=<optimized out>, errp=errp@entry=0x7ffe4a1fc9d8) at blockdev.c:2308

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 56a6b24a0b..3c8d994ced 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1812,8 +1812,8 @@ static void external_snapshot_clean(BlkActionState *common)
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->aio_context) {
         bdrv_drained_end(state->old_bs);
-        aio_context_release(state->aio_context);
         bdrv_unref(state->new_bs);
+        aio_context_release(state->aio_context);
     }
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/9] block: don't keep AioContext acquired after external_snapshot_prepare()

[Stefan Hajnoczi]

It is not necessary to hold AioContext across transactions anymore since
bdrv_drained_begin/end() is used to keep the nodes quiesced.  In fact,
using the AioContext lock for this purpose was always buggy.

This patch reduces the scope of AioContext locked regions.  This is not
just a cleanup but also fixes hangs that occur in BDRV_POLL_WHILE()
because it is unware of recursive locking and does not release the
AioContext the necessary number of times to allow progress to be made.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 71 ++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3c8d994ced..3b598f8f0e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1606,7 +1606,6 @@ typedef struct ExternalSnapshotState {
     BlkActionState common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
-    AioContext *aio_context;
     bool overlay_appended;
 } ExternalSnapshotState;
 
@@ -1626,6 +1625,7 @@ static void external_snapshot_prepare(BlkActionState *common,
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
+    AioContext *aio_context;
 
     /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
      * purpose but a different set of parameters */
@@ -1662,31 +1662,32 @@ static void external_snapshot_prepare(BlkActionState *common,
         return;
     }
 
-    /* Acquire AioContext now so any threads operating on old_bs stop */
-    state->aio_context = bdrv_get_aio_context(state->old_bs);
-    aio_context_acquire(state->aio_context);
+    aio_context = bdrv_get_aio_context(state->old_bs);
+    aio_context_acquire(aio_context);
+
+    /* Paired with .clean() */
     bdrv_drained_begin(state->old_bs);
 
     if (!bdrv_is_inserted(state->old_bs)) {
         error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        return;
+        goto out;
     }
 
     if (bdrv_op_is_blocked(state->old_bs,
                            BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
-        return;
+        goto out;
     }
 
     if (!bdrv_is_read_only(state->old_bs)) {
         if (bdrv_flush(state->old_bs)) {
             error_setg(errp, QERR_IO_ERROR);
-            return;
+            goto out;
         }
     }
 
     if (!bdrv_is_first_non_filter(state->old_bs)) {
         error_setg(errp, QERR_FEATURE_DISABLED, "snapshot");
-        return;
+        goto out;
     }
 
     if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
@@ -1698,13 +1699,13 @@ static void external_snapshot_prepare(BlkActionState *common,
 
         if (node_name && !snapshot_node_name) {
             error_setg(errp, "New snapshot node name missing");
-            return;
+            goto out;
         }
 
         if (snapshot_node_name &&
             bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
             error_setg(errp, "New snapshot node name already in use");
-            return;
+            goto out;
         }
 
         flags = state->old_bs->open_flags;
@@ -1717,7 +1718,7 @@ static void external_snapshot_prepare(BlkActionState *common,
             int64_t size = bdrv_getlength(state->old_bs);
             if (size < 0) {
                 error_setg_errno(errp, -size, "bdrv_getlength failed");
-                return;
+                goto out;
             }
             bdrv_img_create(new_image_file, format,
                             state->old_bs->filename,
@@ -1725,7 +1726,7 @@ static void external_snapshot_prepare(BlkActionState *common,
                             NULL, size, flags, false, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
-                return;
+                goto out;
             }
         }
 
@@ -1740,30 +1741,30 @@ static void external_snapshot_prepare(BlkActionState *common,
                               errp);
     /* We will manually add the backing_hd field to the bs later */
     if (!state->new_bs) {
-        return;
+        goto out;
     }
 
     if (bdrv_has_blk(state->new_bs)) {
         error_setg(errp, "The snapshot is already in use");
-        return;
+        goto out;
     }
 
     if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
                            errp)) {
-        return;
+        goto out;
     }
 
     if (state->new_bs->backing != NULL) {
         error_setg(errp, "The snapshot already has a backing image");
-        return;
+        goto out;
     }
 
     if (!state->new_bs->drv->supports_backing) {
         error_setg(errp, "The snapshot does not support backing images");
-        return;
+        goto out;
     }
 
-    bdrv_set_aio_context(state->new_bs, state->aio_context);
+    bdrv_set_aio_context(state->new_bs, aio_context);
 
     /* This removes our old bs and adds the new bs. This is an operation that
      * can fail, so we need to do it in .prepare; undoing it for abort is
@@ -1772,15 +1773,22 @@ static void external_snapshot_prepare(BlkActionState *common,
     bdrv_append(state->new_bs, state->old_bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
     state->overlay_appended = true;
+
+out:
+    aio_context_release(aio_context);
 }
 
 static void external_snapshot_commit(BlkActionState *common)
 {
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
+    AioContext *aio_context;
+
+    aio_context = bdrv_get_aio_context(state->old_bs);
+    aio_context_acquire(aio_context);
 
     /* We don't need (or want) to use the transactional
      * bdrv_reopen_multiple() across all the entries at once, because we
@@ -1789,6 +1797,8 @@ static void external_snapshot_commit(BlkActionState *common)
         bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
                     NULL);
     }
+
+    aio_context_release(aio_context);
 }
 
 static void external_snapshot_abort(BlkActionState *common)
@@ -1797,11 +1807,18 @@ static void external_snapshot_abort(BlkActionState *common)
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
         if (state->overlay_appended) {
+            AioContext *aio_context;
+
+            aio_context = bdrv_get_aio_context(state->old_bs);
+            aio_context_acquire(aio_context);
+
             bdrv_ref(state->old_bs);   /* we can't let bdrv_set_backind_hd()
                                           close state->old_bs; we need it */
             bdrv_set_backing_hd(state->new_bs, NULL, &error_abort);
             bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
             bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */
+
+            aio_context_release(aio_context);
         }
     }
 }
@@ -1810,11 +1827,19 @@ static void external_snapshot_clean(BlkActionState *common)
 {
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
-    if (state->aio_context) {
-        bdrv_drained_end(state->old_bs);
-        bdrv_unref(state->new_bs);
-        aio_context_release(state->aio_context);
+    AioContext *aio_context;
+
+    if (!state->old_bs) {
+        return;
     }
+
+    aio_context = bdrv_get_aio_context(state->old_bs);
+    aio_context_acquire(aio_context);
+
+    bdrv_drained_end(state->old_bs);
+    bdrv_unref(state->new_bs);
+
+    aio_context_release(aio_context);
 }
 
 typedef struct DriveBackupState {
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/9] block: don't keep AioContext acquired after drive_backup_prepare()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3b598f8f0e..5a56a1abf2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1845,7 +1845,6 @@ static void external_snapshot_clean(BlkActionState *common)
 typedef struct DriveBackupState {
     BlkActionState common;
     BlockDriverState *bs;
-    AioContext *aio_context;
     BlockJob *job;
 } DriveBackupState;
 
@@ -1857,6 +1856,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs;
     DriveBackup *backup;
+    AioContext *aio_context;
     Error *local_err = NULL;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
@@ -1867,24 +1867,36 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
-    /* AioContext is released in .clean() */
-    state->aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(state->aio_context);
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
+    /* Paired with .clean() */
     bdrv_drained_begin(bs);
+
     state->bs = bs;
 
     state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
 static void drive_backup_commit(BlkActionState *common)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+    AioContext *aio_context;
+
+    aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(aio_context);
+
     assert(state->job);
     block_job_start(state->job);
+
+    aio_context_release(aio_context);
 }
 
 static void drive_backup_abort(BlkActionState *common)
@@ -1892,18 +1904,32 @@ static void drive_backup_abort(BlkActionState *common)
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
 
     if (state->job) {
+        AioContext *aio_context;
+
+        aio_context = bdrv_get_aio_context(state->bs);
+        aio_context_acquire(aio_context);
+
         block_job_cancel_sync(state->job);
+
+        aio_context_release(aio_context);
     }
 }
 
 static void drive_backup_clean(BlkActionState *common)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+    AioContext *aio_context;
 
-    if (state->aio_context) {
-        bdrv_drained_end(state->bs);
-        aio_context_release(state->aio_context);
+    if (!state->bs) {
+        return;
     }
+
+    aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(aio_context);
+
+    bdrv_drained_end(state->bs);
+
+    aio_context_release(aio_context);
 }
 
 typedef struct BlockdevBackupState {
-- 
2.14.3

[Qemu-devel] [PATCH v2 4/9] block: don't keep AioContext acquired after blockdev_backup_prepare()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5a56a1abf2..d7ad76416e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1936,7 +1936,6 @@ typedef struct BlockdevBackupState {
     BlkActionState common;
     BlockDriverState *bs;
     BlockJob *job;
-    AioContext *aio_context;
 } BlockdevBackupState;
 
 static BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
@@ -1947,6 +1946,7 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
     BlockDriverState *bs, *target;
+    AioContext *aio_context;
     Error *local_err = NULL;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
@@ -1962,29 +1962,39 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
-    /* AioContext is released in .clean() */
-    state->aio_context = bdrv_get_aio_context(bs);
-    if (state->aio_context != bdrv_get_aio_context(target)) {
-        state->aio_context = NULL;
+    aio_context = bdrv_get_aio_context(bs);
+    if (aio_context != bdrv_get_aio_context(target)) {
         error_setg(errp, "Backup between two IO threads is not implemented");
         return;
     }
-    aio_context_acquire(state->aio_context);
+    aio_context_acquire(aio_context);
     state->bs = bs;
+
+    /* Paired with .clean() */
     bdrv_drained_begin(state->bs);
 
     state->job = do_blockdev_backup(backup, common->block_job_txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
 static void blockdev_backup_commit(BlkActionState *common)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+    AioContext *aio_context;
+
+    aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(aio_context);
+
     assert(state->job);
     block_job_start(state->job);
+
+    aio_context_release(aio_context);
 }
 
 static void blockdev_backup_abort(BlkActionState *common)
@@ -1992,18 +2002,32 @@ static void blockdev_backup_abort(BlkActionState *common)
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
 
     if (state->job) {
+        AioContext *aio_context;
+
+        aio_context = bdrv_get_aio_context(state->bs);
+        aio_context_acquire(aio_context);
+
         block_job_cancel_sync(state->job);
+
+        aio_context_release(aio_context);
     }
 }
 
 static void blockdev_backup_clean(BlkActionState *common)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+    AioContext *aio_context;
 
-    if (state->aio_context) {
-        bdrv_drained_end(state->bs);
-        aio_context_release(state->aio_context);
+    if (!state->bs) {
+        return;
     }
+
+    aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(aio_context);
+
+    bdrv_drained_end(state->bs);
+
+    aio_context_release(aio_context);
 }
 
 typedef struct BlockDirtyBitmapState {
-- 
2.14.3

[Qemu-devel] [PATCH v2 5/9] block: don't keep AioContext acquired after internal_snapshot_prepare()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d7ad76416e..6332a249ea 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1454,7 +1454,6 @@ struct BlkActionState {
 typedef struct InternalSnapshotState {
     BlkActionState common;
     BlockDriverState *bs;
-    AioContext *aio_context;
     QEMUSnapshotInfo sn;
     bool created;
 } InternalSnapshotState;
@@ -1485,6 +1484,7 @@ static void internal_snapshot_prepare(BlkActionState *common,
     qemu_timeval tv;
     BlockdevSnapshotInternal *internal;
     InternalSnapshotState *state;
+    AioContext *aio_context;
     int ret1;
 
     g_assert(common->action->type ==
@@ -1506,32 +1506,33 @@ static void internal_snapshot_prepare(BlkActionState *common,
         return;
     }
 
-    /* AioContext is released in .clean() */
-    state->aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(state->aio_context);
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
 
     state->bs = bs;
+
+    /* Paired with .clean() */
     bdrv_drained_begin(bs);
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
-        return;
+        goto out;
     }
 
     if (bdrv_is_read_only(bs)) {
         error_setg(errp, "Device '%s' is read only", device);
-        return;
+        goto out;
     }
 
     if (!bdrv_can_snapshot(bs)) {
         error_setg(errp, "Block format '%s' used by device '%s' "
                    "does not support internal snapshots",
                    bs->drv->format_name, device);
-        return;
+        goto out;
     }
 
     if (!strlen(name)) {
         error_setg(errp, "Name is empty");
-        return;
+        goto out;
     }
 
     /* check whether a snapshot with name exist */
@@ -1539,12 +1540,12 @@ static void internal_snapshot_prepare(BlkActionState *common,
                                             &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     } else if (ret) {
         error_setg(errp,
                    "Snapshot with name '%s' already exists on device '%s'",
                    name, device);
-        return;
+        goto out;
     }
 
     /* 3. take the snapshot */
@@ -1560,11 +1561,14 @@ static void internal_snapshot_prepare(BlkActionState *common,
         error_setg_errno(errp, -ret1,
                          "Failed to create snapshot '%s' on device '%s'",
                          name, device);
-        return;
+        goto out;
     }
 
     /* 4. succeed, mark a snapshot is created */
     state->created = true;
+
+out:
+    aio_context_release(aio_context);
 }
 
 static void internal_snapshot_abort(BlkActionState *common)
@@ -1573,12 +1577,16 @@ static void internal_snapshot_abort(BlkActionState *common)
                              DO_UPCAST(InternalSnapshotState, common, common);
     BlockDriverState *bs = state->bs;
     QEMUSnapshotInfo *sn = &state->sn;
+    AioContext *aio_context;
     Error *local_error = NULL;
 
     if (!state->created) {
         return;
     }
 
+    aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(aio_context);
+
     if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
         error_reportf_err(local_error,
                           "Failed to delete snapshot with id '%s' and "
@@ -1586,19 +1594,26 @@ static void internal_snapshot_abort(BlkActionState *common)
                           sn->id_str, sn->name,
                           bdrv_get_device_name(bs));
     }
+
+    aio_context_release(aio_context);
 }
 
 static void internal_snapshot_clean(BlkActionState *common)
 {
     InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
                                              common, common);
+    AioContext *aio_context;
 
-    if (state->aio_context) {
-        if (state->bs) {
-            bdrv_drained_end(state->bs);
-        }
-        aio_context_release(state->aio_context);
+    if (!state->bs) {
+        return;
     }
+
+    aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(aio_context);
+
+    bdrv_drained_end(state->bs);
+
+    aio_context_release(aio_context);
 }
 
 /* external snapshot private data */
-- 
2.14.3

[Qemu-devel] [PATCH v2 6/9] block: drop unused BlockDirtyBitmapState->aio_context field

[Stefan Hajnoczi]

The dirty bitmap actions in qmp_transaction have not used AioContext
since the dirty bitmap locking discipline was introduced in commit
2119882c7eb7e2c612b24fc0c8d86f5887d6f1c3 ("block: introduce
dirty_bitmap_mutex").  Remove the unused field.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6332a249ea..e865ae4873 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2049,7 +2049,6 @@ typedef struct BlockDirtyBitmapState {
     BlkActionState common;
     BdrvDirtyBitmap *bitmap;
     BlockDriverState *bs;
-    AioContext *aio_context;
     HBitmap *backup;
     bool prepared;
 } BlockDirtyBitmapState;
@@ -2128,7 +2127,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
     }
 
     bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
-    /* AioContext is released in .clean() */
 }
 
 static void block_dirty_bitmap_clear_abort(BlkActionState *common)
@@ -2149,16 +2147,6 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
     hbitmap_free(state->backup);
 }
 
-static void block_dirty_bitmap_clear_clean(BlkActionState *common)
-{
-    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
-                                             common, common);
-
-    if (state->aio_context) {
-        aio_context_release(state->aio_context);
-    }
-}
-
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -2219,7 +2207,6 @@ static const BlkActionOps actions[] = {
         .prepare = block_dirty_bitmap_clear_prepare,
         .commit = block_dirty_bitmap_clear_commit,
         .abort = block_dirty_bitmap_clear_abort,
-        .clean = block_dirty_bitmap_clear_clean,
     }
 };
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 7/9] iothread: add iothread_by_id() API

[Stefan Hajnoczi]

Encapsulate IOThread QOM object lookup so that callers don't need to
know how and where IOThread objects live.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/sysemu/iothread.h | 1 +
 iothread.c                | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 110329b2b4..55de1715c7 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -42,6 +42,7 @@ typedef struct {
    OBJECT_CHECK(IOThread, obj, TYPE_IOTHREAD)
 
 char *iothread_get_id(IOThread *iothread);
+IOThread *iothread_by_id(const char *id);
 AioContext *iothread_get_aio_context(IOThread *iothread);
 void iothread_stop_all(void);
 GMainContext *iothread_get_g_main_context(IOThread *iothread);
diff --git a/iothread.c b/iothread.c
index 27a4288578..e7b93e02a3 100644
--- a/iothread.c
+++ b/iothread.c
@@ -380,3 +380,10 @@ void iothread_destroy(IOThread *iothread)
 {
     object_unparent(OBJECT(iothread));
 }
+
+/* Lookup IOThread by its id.  Only finds user-created objects, not internal
+ * iothread_create() objects. */
+IOThread *iothread_by_id(const char *id)
+{
+    return IOTHREAD(object_resolve_path_type(id, TYPE_IOTHREAD, NULL));
+}
-- 
2.14.3

[Qemu-devel] [PATCH v2 8/9] blockdev: add x-blockdev-set-iothread testing command

[Stefan Hajnoczi]

Currently there is no easy way for iotests to ensure that a BDS is bound
to a particular IOThread.  Normally the virtio-blk device calls
blk_set_aio_context() when dataplane is enabled during guest driver
initialization.  This never happens in iotests since -machine
accel=qtest means there is no guest activity (including device driver
initialization).

This patch adds a QMP command to explicitly assign IOThreads in test
cases.  See qapi/block-core.json for a description of the command.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/block-core.json | 36 ++++++++++++++++++++++++++++++++++++
 blockdev.c           | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index dd763dcf87..741d6c4367 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3949,3 +3949,39 @@
   'data' : { 'parent': 'str',
              '*child': 'str',
              '*node': 'str' } }
+
+##
+# @x-blockdev-set-iothread:
+#
+# Move @node and its children into the @iothread.  If @iothread is null then
+# move @node and its children into the main loop.
+#
+# The node must not be attached to a BlockBackend.
+#
+# @node-name: the name of the block driver node
+#
+# @iothread: the name of the IOThread object or null for the main loop
+#
+# Note: this command is experimental and intended for test cases that need
+# control over IOThreads only.
+#
+# Since: 2.12
+#
+# Example:
+#
+# 1. Move a node into an IOThread
+# -> { "execute": "x-blockdev-set-iothread",
+#      "arguments": { "node-name": "disk1",
+#                     "iothread": "iothread0" } }
+# <- { "return": {} }
+#
+# 2. Move a node into the main loop
+# -> { "execute": "x-blockdev-set-iothread",
+#      "arguments": { "node-name": "disk1",
+#                     "iothread": null } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'x-blockdev-set-iothread',
+  'data' : { 'node-name': 'str',
+             'iothread': 'StrOrNull' } }
diff --git a/blockdev.c b/blockdev.c
index e865ae4873..f75c01f664 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -45,6 +45,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qobject-output-visitor.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/iothread.h"
 #include "block/block_int.h"
 #include "qmp-commands.h"
 #include "block/trace.h"
@@ -4129,6 +4130,46 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
     return head;
 }
 
+void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
+                                 Error **errp)
+{
+    AioContext *old_context;
+    AioContext *new_context;
+    BlockDriverState *bs;
+
+    bs = bdrv_find_node(node_name);
+    if (!bs) {
+        error_setg(errp, "Cannot find node %s", node_name);
+        return;
+    }
+
+    /* If we want to allow more extreme test scenarios this guard could be
+     * removed.  For now it protects against accidents. */
+    if (bdrv_has_blk(bs)) {
+        error_setg(errp, "Node %s is in use", node_name);
+        return;
+    }
+
+    if (iothread->type == QTYPE_QSTRING) {
+        IOThread *obj = iothread_by_id(iothread->u.s);
+        if (!obj) {
+            error_setg(errp, "Cannot find iothread %s", iothread->u.s);
+            return;
+        }
+
+        new_context = iothread_get_aio_context(obj);
+    } else {
+        new_context = qemu_get_aio_context();
+    }
+
+    old_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(old_context);
+
+    bdrv_set_aio_context(bs, new_context);
+
+    aio_context_release(old_context);
+}
+
 QemuOptsList qemu_common_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
-- 
2.14.3

[Qemu-devel] [PATCH v2 9/9] qemu-iotests: add 202 external snapshots IOThread test

[Stefan Hajnoczi]

QMP 'transaction' blockdev-snapshot-sync with multiple disks in an
IOThread is an untested code path.  Several bugs have been found in
connection with this command.  This patch adds a test case to prevent
future regressions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/202     | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/202.out | 11 ++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 107 insertions(+)
 create mode 100755 tests/qemu-iotests/202
 create mode 100644 tests/qemu-iotests/202.out

diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
new file mode 100755
index 0000000000..581ca34d79
--- /dev/null
+++ b/tests/qemu-iotests/202
@@ -0,0 +1,95 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>
+#
+# Check that QMP 'transaction' blockdev-snapshot-sync with multiple drives on a
+# single IOThread completes successfully.  This particular command triggered a
+# hang due to recursive AioContext locking and BDRV_POLL_WHILE().  Protect
+# against regressions.
+
+import iotests
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('disk0.img') as disk0_img_path, \
+     iotests.FilePath('disk1.img') as disk1_img_path, \
+     iotests.FilePath('disk0-snap.img') as disk0_snap_img_path, \
+     iotests.FilePath('disk1-snap.img') as disk1_snap_img_path, \
+     iotests.VM() as vm:
+
+    img_size = '10M'
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size)
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size)
+
+    iotests.log('Launching VM...')
+    vm.launch()
+
+    iotests.log('Adding IOThread...')
+    iotests.log(vm.qmp('object-add',
+                       qom_type='iothread',
+                       id='iothread0'))
+
+    iotests.log('Adding blockdevs...')
+    iotests.log(vm.qmp('blockdev-add',
+                       driver=iotests.imgfmt,
+                       node_name='disk0',
+                       file={
+                           'driver': 'file',
+                           'filename': disk0_img_path,
+                       }))
+    iotests.log(vm.qmp('blockdev-add',
+                       driver=iotests.imgfmt,
+                       node_name='disk1',
+                       file={
+                           'driver': 'file',
+                           'filename': disk1_img_path,
+                       }))
+
+    iotests.log('Setting iothread...')
+    iotests.log(vm.qmp('x-blockdev-set-iothread',
+                       node_name='disk0',
+                       iothread='iothread0'))
+    iotests.log(vm.qmp('x-blockdev-set-iothread',
+                       node_name='disk1',
+                       iothread='iothread0'))
+
+    iotests.log('Creating external snapshots...')
+    iotests.log(vm.qmp(
+                  'transaction',
+                  actions=[
+                      {
+                          'data': {
+                              'node-name': 'disk0',
+                              'snapshot-file': disk0_snap_img_path,
+                              'snapshot-node-name': 'disk0-snap',
+                              'mode': 'absolute-paths',
+                              'format': iotests.imgfmt,
+                          },
+                          'type': 'blockdev-snapshot-sync'
+                      }, {
+                          'data': {
+                              'node-name': 'disk1',
+                              'snapshot-file': disk1_snap_img_path,
+                              'snapshot-node-name': 'disk1-snap',
+                              'mode': 'absolute-paths',
+                              'format': iotests.imgfmt
+                          },
+                          'type': 'blockdev-snapshot-sync'
+                      }
+                  ]))
diff --git a/tests/qemu-iotests/202.out b/tests/qemu-iotests/202.out
new file mode 100644
index 0000000000..d5ea374e17
--- /dev/null
+++ b/tests/qemu-iotests/202.out
@@ -0,0 +1,11 @@
+Launching VM...
+Adding IOThread...
+{u'return': {}}
+Adding blockdevs...
+{u'return': {}}
+{u'return': {}}
+Setting iothread...
+{u'return': {}}
+{u'return': {}}
+Creating external snapshots...
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3e688678dd..d0ee1e2e55 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -197,3 +197,4 @@
 197 rw auto quick
 198 rw auto
 200 rw auto
+202 rw auto quick
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 1/9] blockdev: hold AioContext for bdrv_unref() in external_snapshot_clean()

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> bdrv_unref() requires the AioContext lock because bdrv_flush() uses
> BDRV_POLL_WHILE(), which assumes the AioContext is currently held.  If
> BDRV_POLL_WHILE() runs without AioContext held the
> pthread_mutex_unlock() call in aio_context_release() fails.
> 
> This patch moves bdrv_unref() into the AioContext locked region to solve
> the following pthread_mutex_unlock() failure:
> 

> diff --git a/blockdev.c b/blockdev.c
> index 56a6b24a0b..3c8d994ced 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1812,8 +1812,8 @@ static void external_snapshot_clean(BlkActionState *common)
>                               DO_UPCAST(ExternalSnapshotState, common, common);
>      if (state->aio_context) {
>          bdrv_drained_end(state->old_bs);
> -        aio_context_release(state->aio_context);
>          bdrv_unref(state->new_bs);
> +        aio_context_release(state->aio_context);

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 2/9] block: don't keep AioContext acquired after external_snapshot_prepare()

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> It is not necessary to hold AioContext across transactions anymore since
> bdrv_drained_begin/end() is used to keep the nodes quiesced.  In fact,
> using the AioContext lock for this purpose was always buggy.
> 
> This patch reduces the scope of AioContext locked regions.  This is not
> just a cleanup but also fixes hangs that occur in BDRV_POLL_WHILE()
> because it is unware of recursive locking and does not release the

s/unware/unaware/

> AioContext the necessary number of times to allow progress to be made.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 71 ++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 48 insertions(+), 23 deletions(-)
> 

> @@ -1662,31 +1662,32 @@ static void external_snapshot_prepare(BlkActionState *common,
>          return;
>      }
>  
> -    /* Acquire AioContext now so any threads operating on old_bs stop */
> -    state->aio_context = bdrv_get_aio_context(state->old_bs);
> -    aio_context_acquire(state->aio_context);
> +    aio_context = bdrv_get_aio_context(state->old_bs);
> +    aio_context_acquire(aio_context);
> +
> +    /* Paired with .clean() */
>      bdrv_drained_begin(state->old_bs);
>  
>      if (!bdrv_is_inserted(state->old_bs)) {
>          error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> -        return;
> +        goto out;
>      }

Using gcc/clang's __attribute__((cleanup)) would make this so much nicer ;)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 3/9] block: don't keep AioContext acquired after drive_backup_prepare()

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 

> @@ -1867,24 +1867,36 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>          return;
>      }
>  
> -    /* AioContext is released in .clean() */
> -    state->aio_context = bdrv_get_aio_context(bs);
> -    aio_context_acquire(state->aio_context);
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +
> +    /* Paired with .clean() */
>      bdrv_drained_begin(bs);
> +
>      state->bs = bs;
>  
>      state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return;
> +        goto out;
>      }
> +
> +out:
> +    aio_context_release(aio_context);

Looks a bit funny to have a label with a single use that would fall
through to the same location anyways.  But it's fine from the future
maintenance point of view, so no need to change it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 4/9] block: don't keep AioContext acquired after blockdev_backup_prepare()

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 44 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 

>      state->job = do_blockdev_backup(backup, common->block_job_txn, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return;
> +        goto out;
>      }
> +
> +out:
> +    aio_context_release(aio_context);
>  }

Again a weird one-shot label where fallthrough would do the same.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 5/9] block: don't keep AioContext acquired after internal_snapshot_prepare()

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 47 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 6/9] block: drop unused BlockDirtyBitmapState->aio_context field

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> The dirty bitmap actions in qmp_transaction have not used AioContext
> since the dirty bitmap locking discipline was introduced in commit
> 2119882c7eb7e2c612b24fc0c8d86f5887d6f1c3 ("block: introduce
> dirty_bitmap_mutex").  Remove the unused field.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 13 -------------
>  1 file changed, 13 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 7/9] iothread: add iothread_by_id() API

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Encapsulate IOThread QOM object lookup so that callers don't need to
> know how and where IOThread objects live.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/sysemu/iothread.h | 1 +
>  iothread.c                | 7 +++++++
>  2 files changed, 8 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 8/9] blockdev: add x-blockdev-set-iothread testing command

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Currently there is no easy way for iotests to ensure that a BDS is bound
> to a particular IOThread.  Normally the virtio-blk device calls
> blk_set_aio_context() when dataplane is enabled during guest driver
> initialization.  This never happens in iotests since -machine
> accel=qtest means there is no guest activity (including device driver
> initialization).
> 
> This patch adds a QMP command to explicitly assign IOThreads in test
> cases.  See qapi/block-core.json for a description of the command.

The x- prefix is perfect for this.

> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qapi/block-core.json | 36 ++++++++++++++++++++++++++++++++++++
>  blockdev.c           | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 

> +##
> +# @x-blockdev-set-iothread:
> +#
> +# Move @node and its children into the @iothread.  If @iothread is null then
> +# move @node and its children into the main loop.
> +#
> +# The node must not be attached to a BlockBackend.
> +#
> +# @node-name: the name of the block driver node
> +#
> +# @iothread: the name of the IOThread object or null for the main loop
> +#
> +# Note: this command is experimental and intended for test cases that need
> +# control over IOThreads only.

I'd place 'only' sooner; it fits better as 'intended only for ...'.

As a wording tweak is minor,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 9/9] qemu-iotests: add 202 external snapshots IOThread test

[Eric Blake]

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

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> QMP 'transaction' blockdev-snapshot-sync with multiple disks in an
> IOThread is an untested code path.  Several bugs have been found in
> connection with this command.  This patch adds a test case to prevent
> future regressions.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/202     | 95 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/202.out | 11 ++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 107 insertions(+)
>  create mode 100755 tests/qemu-iotests/202
>  create mode 100644 tests/qemu-iotests/202.out
> 

> +# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>

There's another cleanup series posted that drops these sorts of lines in
favor of git history.

> +++ b/tests/qemu-iotests/202.out
> @@ -0,0 +1,11 @@
> +Launching VM...
> +Adding IOThread...
> +{u'return': {}}
> +Adding blockdevs...
> +{u'return': {}}
> +{u'return': {}}
> +Setting iothread...
> +{u'return': {}}
> +{u'return': {}}
> +Creating external snapshots...
> +{u'return': {}}

Yay - a python test with sane output - if I had to, I could actually
debug this test without too much effort!

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads

[Stefan Hajnoczi]

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

On Wed, Dec 06, 2017 at 02:45:41PM +0000, Stefan Hajnoczi wrote:
> v2:
>  * Use StrOrNull for x-blockdev-set-iothread iothread argument [eblake]

Thanks for the reviews, Eric.

I will wait for one more block layer person (Kevin, Paolo, or Fam?) to
review before merging.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads

[Kevin Wolf]

Am 06.12.2017 um 15:45 hat Stefan Hajnoczi geschrieben:
> v2:
>  * Use StrOrNull for x-blockdev-set-iothread iothread argument [eblake]
> 
> (This is for QEMU 2.12 because this bug is not -rc4 critical)
> 
> Previously AioContext was held across QMP 'transaction' in an attempt to
> achieve bdrv_drained_begin/end() semantics.  Nowadays we have
> bdrv_drained_begin/end() and the AioContext lock just protects state.
> Therefore there is no reason to hold AioContext across
> .prepare/.commit/.abort/.clean() anymore.
> 
> Besides being cleanup-worthy, holding AioContext also exposes a bug:
> BDRV_POLL_WHILE() doesn't support recursive AioContext locking and will hang if
> depth > 1.  This is easy to trigger by submitting a transaction with 2 actions
> that touch two drives assigned to an IOThread.  The IOThread's AioContext will
> be locked twice and BDRV_POLL_WHILE() will hang.
> 
> BDRV_POLL_WHILE() is best fixed by eliminating the AioContext lock entirely in
> favor of fine-grained locking.  I discussed some fixes for BDRV_POLL_WHILE()
> with Paolo but we came to the conclusion that it will just add complexity when
> we really want to stop using AioContext locking.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

The only thing that I see that could use some improvements is the test
case, which tests only a small part of the bugs that are fixed by this
series. This doesn't invalidate any of the review, but I think without
full test coverage, it's incomplete.

Kevin

Re: [Qemu-devel] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads

[Stefan Hajnoczi]

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

On Wed, Dec 06, 2017 at 02:45:41PM +0000, Stefan Hajnoczi wrote:
> v2:
>  * Use StrOrNull for x-blockdev-set-iothread iothread argument [eblake]
> 
> (This is for QEMU 2.12 because this bug is not -rc4 critical)
> 
> Previously AioContext was held across QMP 'transaction' in an attempt to
> achieve bdrv_drained_begin/end() semantics.  Nowadays we have
> bdrv_drained_begin/end() and the AioContext lock just protects state.
> Therefore there is no reason to hold AioContext across
> .prepare/.commit/.abort/.clean() anymore.
> 
> Besides being cleanup-worthy, holding AioContext also exposes a bug:
> BDRV_POLL_WHILE() doesn't support recursive AioContext locking and will hang if
> depth > 1.  This is easy to trigger by submitting a transaction with 2 actions
> that touch two drives assigned to an IOThread.  The IOThread's AioContext will
> be locked twice and BDRV_POLL_WHILE() will hang.
> 
> BDRV_POLL_WHILE() is best fixed by eliminating the AioContext lock entirely in
> favor of fine-grained locking.  I discussed some fixes for BDRV_POLL_WHILE()
> with Paolo but we came to the conclusion that it will just add complexity when
> we really want to stop using AioContext locking.
> 
> Summary:
> 
>  * Patch 1 fixes missing AioContext lock protection
> 
>  * Patches 2-6 clean up excessive AioContext locked regions in QMP
>    'transaction' to solve the hang
> 
>  * Patches 7-9 add a qemu-iotests test case and the necessary infrastructure
> 
> Stefan Hajnoczi (9):
>   blockdev: hold AioContext for bdrv_unref() in
>     external_snapshot_clean()
>   block: don't keep AioContext acquired after
>     external_snapshot_prepare()
>   block: don't keep AioContext acquired after drive_backup_prepare()
>   block: don't keep AioContext acquired after blockdev_backup_prepare()
>   block: don't keep AioContext acquired after
>     internal_snapshot_prepare()
>   block: drop unused BlockDirtyBitmapState->aio_context field
>   iothread: add iothread_by_id() API
>   blockdev: add x-blockdev-set-iothread testing command
>   qemu-iotests: add 202 external snapshots IOThread test
> 
>  qapi/block-core.json       |  36 +++++++
>  include/sysemu/iothread.h  |   1 +
>  blockdev.c                 | 258 +++++++++++++++++++++++++++++++++------------
>  iothread.c                 |   7 ++
>  tests/qemu-iotests/202     |  95 +++++++++++++++++
>  tests/qemu-iotests/202.out |  11 ++
>  tests/qemu-iotests/group   |   1 +
>  7 files changed, 339 insertions(+), 70 deletions(-)
>  create mode 100755 tests/qemu-iotests/202
>  create mode 100644 tests/qemu-iotests/202.out
> 
> -- 
> 2.14.3
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads

[Paolo Bonzini]

On 06/12/2017 15:45, Stefan Hajnoczi wrote:
> v2:
>  * Use StrOrNull for x-blockdev-set-iothread iothread argument [eblake]
> 
> (This is for QEMU 2.12 because this bug is not -rc4 critical)
> 
> Previously AioContext was held across QMP 'transaction' in an attempt to
> achieve bdrv_drained_begin/end() semantics.  Nowadays we have
> bdrv_drained_begin/end() and the AioContext lock just protects state.
> Therefore there is no reason to hold AioContext across
> .prepare/.commit/.abort/.clean() anymore.
> 
> Besides being cleanup-worthy, holding AioContext also exposes a bug:
> BDRV_POLL_WHILE() doesn't support recursive AioContext locking and will hang if
> depth > 1.  This is easy to trigger by submitting a transaction with 2 actions
> that touch two drives assigned to an IOThread.  The IOThread's AioContext will
> be locked twice and BDRV_POLL_WHILE() will hang.
> 
> BDRV_POLL_WHILE() is best fixed by eliminating the AioContext lock entirely in
> favor of fine-grained locking.  I discussed some fixes for BDRV_POLL_WHILE()
> with Paolo but we came to the conclusion that it will just add complexity when
> we really want to stop using AioContext locking.
> 
> Summary:
> 
>  * Patch 1 fixes missing AioContext lock protection
> 
>  * Patches 2-6 clean up excessive AioContext locked regions in QMP
>    'transaction' to solve the hang
> 
>  * Patches 7-9 add a qemu-iotests test case and the necessary infrastructure
> 
> Stefan Hajnoczi (9):
>   blockdev: hold AioContext for bdrv_unref() in
>     external_snapshot_clean()
>   block: don't keep AioContext acquired after
>     external_snapshot_prepare()
>   block: don't keep AioContext acquired after drive_backup_prepare()
>   block: don't keep AioContext acquired after blockdev_backup_prepare()
>   block: don't keep AioContext acquired after
>     internal_snapshot_prepare()
>   block: drop unused BlockDirtyBitmapState->aio_context field
>   iothread: add iothread_by_id() API
>   blockdev: add x-blockdev-set-iothread testing command
>   qemu-iotests: add 202 external snapshots IOThread test
> 
>  qapi/block-core.json       |  36 +++++++
>  include/sysemu/iothread.h  |   1 +
>  blockdev.c                 | 258 +++++++++++++++++++++++++++++++++------------
>  iothread.c                 |   7 ++
>  tests/qemu-iotests/202     |  95 +++++++++++++++++
>  tests/qemu-iotests/202.out |  11 ++
>  tests/qemu-iotests/group   |   1 +
>  7 files changed, 339 insertions(+), 70 deletions(-)
>  create mode 100755 tests/qemu-iotests/202
>  create mode 100644 tests/qemu-iotests/202.out
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>