[PATCH 0/3] block: Fix external snapshot with VM state

[PATCH 0/3] block: Fix external snapshot with VM state

[Kevin Wolf]

This fixes bdrv_invalidate_cache_all() for some kinds of graphs where
part of the nodes are active and others are inactive. This is a scenario
that happens when libvirt takes an external snapshot with VM state.

This was reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=1781637
('qemu crashed when do mem and disk snapshot')

Based-on: <20191216170857.11880-1-kwolf@redhat.com>
('iotests: Remove duplicated blockdev_create()')

Kevin Wolf (3):
  block: Activate recursively even for already active nodes
  hmp: Allow using qdev ID for qemu-io command
  iotests: Test external snapshot with VM state

 block.c                    | 50 +++++++++++------------
 monitor/hmp-cmds.c         | 28 ++++++++-----
 hmp-commands.hx            |  8 ++--
 tests/qemu-iotests/280     | 83 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/280.out | 50 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 181 insertions(+), 39 deletions(-)
 create mode 100755 tests/qemu-iotests/280
 create mode 100644 tests/qemu-iotests/280.out

-- 
2.20.1

[PATCH 1/3] block: Activate recursively even for already active nodes

[Kevin Wolf]

bdrv_invalidate_cache_all() assumes that all nodes in a given subtree
are either active or inactive when it starts. Therefore, as soon as it
arrives at an already active node, it stops.

However, this assumption is wrong. For example, it's possible to take a
snapshot of an inactive node, which results in an active overlay over an
inactive backing file. The active overlay is probably also the root node
of an inactive BlockBackend (blk->disable_perm == true).

In this case, bdrv_invalidate_cache_all() does not need to do anything
to activate the overlay node, but it still needs to recurse into the
children and the parents to make sure that after returning success,
really everything is activated.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 50 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 73029fad64..1b6f7c86e8 100644
--- a/block.c
+++ b/block.c
@@ -5335,10 +5335,6 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
         return;
     }
 
-    if (!(bs->open_flags & BDRV_O_INACTIVE)) {
-        return;
-    }
-
     QLIST_FOREACH(child, &bs->children, next) {
         bdrv_co_invalidate_cache(child->bs, &local_err);
         if (local_err) {
@@ -5360,34 +5356,36 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
      * just keep the extended permissions for the next time that an activation
      * of the image is tried.
      */
-    bs->open_flags &= ~BDRV_O_INACTIVE;
-    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
-    if (ret < 0) {
-        bs->open_flags |= BDRV_O_INACTIVE;
-        error_propagate(errp, local_err);
-        return;
-    }
-    bdrv_set_perm(bs, perm, shared_perm);
-
-    if (bs->drv->bdrv_co_invalidate_cache) {
-        bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
-        if (local_err) {
+    if (bs->open_flags & BDRV_O_INACTIVE) {
+        bs->open_flags &= ~BDRV_O_INACTIVE;
+        bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+        ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
+        if (ret < 0) {
             bs->open_flags |= BDRV_O_INACTIVE;
             error_propagate(errp, local_err);
             return;
         }
-    }
+        bdrv_set_perm(bs, perm, shared_perm);
 
-    FOR_EACH_DIRTY_BITMAP(bs, bm) {
-        bdrv_dirty_bitmap_skip_store(bm, false);
-    }
+        if (bs->drv->bdrv_co_invalidate_cache) {
+            bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
+            if (local_err) {
+                bs->open_flags |= BDRV_O_INACTIVE;
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
 
-    ret = refresh_total_sectors(bs, bs->total_sectors);
-    if (ret < 0) {
-        bs->open_flags |= BDRV_O_INACTIVE;
-        error_setg_errno(errp, -ret, "Could not refresh total sector count");
-        return;
+        FOR_EACH_DIRTY_BITMAP(bs, bm) {
+            bdrv_dirty_bitmap_skip_store(bm, false);
+        }
+
+        ret = refresh_total_sectors(bs, bs->total_sectors);
+        if (ret < 0) {
+            bs->open_flags |= BDRV_O_INACTIVE;
+            error_setg_errno(errp, -ret, "Could not refresh total sector count");
+            return;
+        }
     }
 
     QLIST_FOREACH(parent, &bs->parents, next_parent) {
-- 
2.20.1

Re: [PATCH 1/3] block: Activate recursively even for already active nodes

[Kevin Wolf]

Am 17.12.2019 um 15:59 hat Kevin Wolf geschrieben:
> bdrv_invalidate_cache_all() assumes that all nodes in a given subtree
> are either active or inactive when it starts. Therefore, as soon as it
> arrives at an already active node, it stops.
> 
> However, this assumption is wrong. For example, it's possible to take a
> snapshot of an inactive node, which results in an active overlay over an
> inactive backing file. The active overlay is probably also the root node
> of an inactive BlockBackend (blk->disable_perm == true).
> 
> In this case, bdrv_invalidate_cache_all() does not need to do anything
> to activate the overlay node, but it still needs to recurse into the
> children and the parents to make sure that after returning success,
> really everything is activated.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Actually:
Cc: qemu-stable@nongnu.org

Re: [PATCH 1/3] block: Activate recursively even for already active nodes

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1241 bytes --]

On 17.12.19 15:59, Kevin Wolf wrote:
> bdrv_invalidate_cache_all() assumes that all nodes in a given subtree
> are either active or inactive when it starts. Therefore, as soon as it
> arrives at an already active node, it stops.
> 
> However, this assumption is wrong. For example, it's possible to take a
> snapshot of an inactive node, which results in an active overlay over an
> inactive backing file. The active overlay is probably also the root node
> of an inactive BlockBackend (blk->disable_perm == true).
> 
> In this case, bdrv_invalidate_cache_all() does not need to do anything
> to activate the overlay node, but it still needs to recurse into the
> children and the parents to make sure that after returning success,
> really everything is activated.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 50 ++++++++++++++++++++++++--------------------------
>  1 file changed, 24 insertions(+), 26 deletions(-)

Basically the only change is to not skip the whole function when the
node is already active but only the part that actually activates the
node.  blk_root_activate() is a no-op for already-active BBs, so this
looks good to me.

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[PATCH 2/3] hmp: Allow using qdev ID for qemu-io command

[Kevin Wolf]

In order to issue requests on an existing BlockBackend with the
'qemu-io' HMP command, allow specifying the BlockBackend not only with a
BlockBackend name, but also with a qdev ID/QOM path for a device that
owns the (possibly anonymous) BlockBackend.

Because qdev names could be conflicting with BlockBackend and node
names, introduce a -d option to explicitly address a device. If the
option is not given, a BlockBackend or a node is addressed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/hmp-cmds.c | 28 ++++++++++++++++++----------
 hmp-commands.hx    |  8 +++++---
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..5f8941d298 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2468,23 +2468,31 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)
 {
     BlockBackend *blk;
     BlockBackend *local_blk = NULL;
+    bool qdev = qdict_get_try_bool(qdict, "qdev", false);
     const char* device = qdict_get_str(qdict, "device");
     const char* command = qdict_get_str(qdict, "command");
     Error *err = NULL;
     int ret;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);
-        if (bs) {
-            blk = local_blk = blk_new(bdrv_get_aio_context(bs),
-                                      0, BLK_PERM_ALL);
-            ret = blk_insert_bs(blk, bs, &err);
-            if (ret < 0) {
+    if (qdev) {
+        blk = blk_by_qdev_id(device, &err);
+        if (!blk) {
+            goto fail;
+        }
+    } else {
+        blk = blk_by_name(device);
+        if (!blk) {
+            BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);
+            if (bs) {
+                blk = local_blk = blk_new(bdrv_get_aio_context(bs),
+                                          0, BLK_PERM_ALL);
+                ret = blk_insert_bs(blk, bs, &err);
+                if (ret < 0) {
+                    goto fail;
+                }
+            } else {
                 goto fail;
             }
-        } else {
-            goto fail;
         }
     }
 
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cfcc044ce4..dc23185de4 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1875,9 +1875,11 @@ ETEXI
 
     {
         .name       = "qemu-io",
-        .args_type  = "device:B,command:s",
-        .params     = "[device] \"[command]\"",
-        .help       = "run a qemu-io command on a block device",
+        .args_type  = "qdev:-d,device:B,command:s",
+        .params     = "[-d] [device] \"[command]\"",
+        .help       = "run a qemu-io command on a block device\n\t\t\t"
+                      "-d: [device] is a device ID rather than a "
+                      "drive ID or node name",
         .cmd        = hmp_qemu_io,
     },
 
-- 
2.20.1

[PATCH 3/3] iotests: Test external snapshot with VM state

[Kevin Wolf]

This tests creating an external snapshot with VM state (which results in
an active overlay over an inactive backing file, which is also the root
node of an inactive BlockBackend), re-activating the images and
performing some operations to test that the re-activation worked as
intended.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/280     | 83 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/280.out | 50 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 134 insertions(+)
 create mode 100755 tests/qemu-iotests/280
 create mode 100644 tests/qemu-iotests/280.out

diff --git a/tests/qemu-iotests/280 b/tests/qemu-iotests/280
new file mode 100755
index 0000000000..0b1fa8e1d8
--- /dev/null
+++ b/tests/qemu-iotests/280
@@ -0,0 +1,83 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2019 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: Kevin Wolf <kwolf@redhat.com>
+#
+# Test migration to file for taking an external snapshot with VM state.
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_protocol(supported=['file'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('base') as base_path , \
+     iotests.FilePath('top') as top_path, \
+     iotests.VM() as vm:
+
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base_path, '64M')
+
+    iotests.log('=== Launch VM ===')
+    vm.add_object('iothread,id=iothread0')
+    vm.add_blockdev('file,filename=%s,node-name=base-file' % (base_path))
+    vm.add_blockdev('%s,file=base-file,node-name=base-fmt' % (iotests.imgfmt))
+    vm.add_device('virtio-blk,drive=base-fmt,iothread=iothread0,id=vda')
+    vm.launch()
+
+    vm.enable_migration_events('VM')
+
+    iotests.log('\n=== Migrate to file ===')
+    vm.qmp_log('migrate', uri='exec:cat > /dev/null')
+
+    with iotests.Timeout(3, 'Migration does not complete'):
+        vm.wait_migration()
+
+    iotests.log('\nVM is now stopped:')
+    iotests.log(vm.qmp('query-migrate')['return']['status'])
+    vm.qmp_log('query-status')
+
+    iotests.log('\n=== Create a snapshot of the disk image ===')
+    vm.blockdev_create({
+        'driver': 'file',
+        'filename': top_path,
+        'size': 0,
+    })
+    vm.qmp_log('blockdev-add', node_name='top-file',
+               driver='file', filename=top_path,
+               filters=[iotests.filter_qmp_testfiles])
+
+    vm.blockdev_create({
+        'driver': iotests.imgfmt,
+        'file': 'top-file',
+        'size': 1024 * 1024,
+    })
+    vm.qmp_log('blockdev-add', node_name='top-fmt',
+               driver=iotests.imgfmt, file='top-file')
+
+    vm.qmp_log('blockdev-snapshot', node='base-fmt', overlay='top-fmt')
+
+    iotests.log('\n=== Resume the VM and simulate a write request ===')
+    vm.qmp_log('cont')
+    iotests.log(vm.hmp_qemu_io('-d vda/virtio-backend', 'write 4k 4k'))
+
+    iotests.log('\n=== Commit it to the backing file ===')
+    result = vm.qmp_log('block-commit', job_id='job0', auto_dismiss=False,
+                        device='top-fmt', top_node='top-fmt',
+                        filters=[iotests.filter_qmp_testfiles])
+    if 'return' in result:
+        vm.run_job('job0')
diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
new file mode 100644
index 0000000000..5d382faaa8
--- /dev/null
+++ b/tests/qemu-iotests/280.out
@@ -0,0 +1,50 @@
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+=== Launch VM ===
+Enabling migration QMP events on VM...
+{"return": {}}
+
+=== Migrate to file ===
+{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
+{"return": {}}
+{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+VM is now stopped:
+completed
+{"execute": "query-status", "arguments": {}}
+{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}
+
+=== Create a snapshot of the disk image ===
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-top", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-top", "node-name": "top-file"}}
+{"return": {}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "qcow2", "file": "top-file", "size": 1048576}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": "top-file", "node-name": "top-fmt"}}
+{"return": {}}
+{"execute": "blockdev-snapshot", "arguments": {"node": "base-fmt", "overlay": "top-fmt"}}
+{"return": {}}
+
+=== Resume the VM and simulate a write request ===
+{"execute": "cont", "arguments": {}}
+{"return": {}}
+{"return": ""}
+
+=== Commit it to the backing file ===
+{"execute": "block-commit", "arguments": {"auto-dismiss": false, "device": "top-fmt", "job-id": "job0", "top-node": "top-fmt"}}
+{"return": {}}
+{"execute": "job-complete", "arguments": {"id": "job0"}}
+{"return": {}}
+{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index eb57ddc72c..cb2b789e44 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -287,3 +287,4 @@
 273 backing quick
 277 rw quick
 279 rw backing quick
+280 rw migration quick
-- 
2.20.1

Re: [PATCH 3/3] iotests: Test external snapshot with VM state

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1837 bytes --]

On 17.12.19 15:59, Kevin Wolf wrote:
> This tests creating an external snapshot with VM state (which results in
> an active overlay over an inactive backing file, which is also the root
> node of an inactive BlockBackend), re-activating the images and
> performing some operations to test that the re-activation worked as
> intended.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/280     | 83 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/280.out | 50 +++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 134 insertions(+)
>  create mode 100755 tests/qemu-iotests/280
>  create mode 100644 tests/qemu-iotests/280.out

[...]

> diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
> new file mode 100644
> index 0000000000..5d382faaa8
> --- /dev/null
> +++ b/tests/qemu-iotests/280.out
> @@ -0,0 +1,50 @@
> +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +=== Launch VM ===
> +Enabling migration QMP events on VM...
> +{"return": {}}
> +
> +=== Migrate to file ===
> +{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
> +{"return": {}}
> +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +
> +VM is now stopped:
> +completed
> +{"execute": "query-status", "arguments": {}}
> +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}

Hmmm, I get a finish-migrate status here (on tmpfs)...

Max


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

Re: [PATCH 3/3] iotests: Test external snapshot with VM state

[Kevin Wolf]

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

Am 19.12.2019 um 15:26 hat Max Reitz geschrieben:
> On 17.12.19 15:59, Kevin Wolf wrote:
> > This tests creating an external snapshot with VM state (which results in
> > an active overlay over an inactive backing file, which is also the root
> > node of an inactive BlockBackend), re-activating the images and
> > performing some operations to test that the re-activation worked as
> > intended.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> [...]
> 
> > diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
> > new file mode 100644
> > index 0000000000..5d382faaa8
> > --- /dev/null
> > +++ b/tests/qemu-iotests/280.out
> > @@ -0,0 +1,50 @@
> > +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > +
> > +=== Launch VM ===
> > +Enabling migration QMP events on VM...
> > +{"return": {}}
> > +
> > +=== Migrate to file ===
> > +{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
> > +{"return": {}}
> > +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > +
> > +VM is now stopped:
> > +completed
> > +{"execute": "query-status", "arguments": {}}
> > +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}
> 
> Hmmm, I get a finish-migrate status here (on tmpfs)...

Dave, is it intentional that the "completed" migration event is emitted
while we are still in finish-migration rather than postmigrate?

I guess we could change wait_migration() in qemu-iotests to wait for the
postmigrate state rather than the "completed" event, but maybe it would
be better to change the migration code to avoid similar races in other
QMP clients.

Kevin

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

Re: [PATCH 3/3] iotests: Test external snapshot with VM state

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 19.12.2019 um 15:26 hat Max Reitz geschrieben:
> > On 17.12.19 15:59, Kevin Wolf wrote:
> > > This tests creating an external snapshot with VM state (which results in
> > > an active overlay over an inactive backing file, which is also the root
> > > node of an inactive BlockBackend), re-activating the images and
> > > performing some operations to test that the re-activation worked as
> > > intended.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > 
> > [...]
> > 
> > > diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
> > > new file mode 100644
> > > index 0000000000..5d382faaa8
> > > --- /dev/null
> > > +++ b/tests/qemu-iotests/280.out
> > > @@ -0,0 +1,50 @@
> > > +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > > +
> > > +=== Launch VM ===
> > > +Enabling migration QMP events on VM...
> > > +{"return": {}}
> > > +
> > > +=== Migrate to file ===
> > > +{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
> > > +{"return": {}}
> > > +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > +
> > > +VM is now stopped:
> > > +completed
> > > +{"execute": "query-status", "arguments": {}}
> > > +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}
> > 
> > Hmmm, I get a finish-migrate status here (on tmpfs)...
> 
> Dave, is it intentional that the "completed" migration event is emitted
> while we are still in finish-migration rather than postmigrate?

Yes it looks like it;  it's that the migration state machine hits
COMPLETED that then _causes_ the runstate transitition to POSTMIGRATE.

static void migration_iteration_finish(MigrationState *s)
{
    /* If we enabled cpu throttling for auto-converge, turn it off. */
    cpu_throttle_stop();

    qemu_mutex_lock_iothread();
    switch (s->state) {
    case MIGRATION_STATUS_COMPLETED:
        migration_calculate_complete(s);
        runstate_set(RUN_STATE_POSTMIGRATE);
        break;

then there are a bunch of error cases where if it landed in
FAILED/CANCELLED etc then we either restart the VM or also go to
POSTMIGRATE.

> I guess we could change wait_migration() in qemu-iotests to wait for the
> postmigrate state rather than the "completed" event, but maybe it would
> be better to change the migration code to avoid similar races in other
> QMP clients.

Given that the migration state machine is driving the runstate state
machine I think it currently makes sense internally;  (although I don't
think it's documented to be in that order or tested to be, which we
might want to fix).

Looking at 234 and 262, it looks like you're calling wait_migration on
both the source and dest; I don't think the dest will see the
POSTMIGRATE.  Also note that depending what you're trying to do, with
postcopy you'll be running on the destination before you see COMPLETED.

Waiting for the destination to leave 'inmigrate' state is probably
the best strategy; then wait for the source to be in postmigrate.
You can cause early exits if you see transitions to 'FAILED' - but
actually the destination will likely quit in that case; so it should
be much rarer for you to hit a timeout on a failed migration.

Dave


> Kevin


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] iotests: Test external snapshot with VM state

[Kevin Wolf]

Am 02.01.2020 um 14:25 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > Am 19.12.2019 um 15:26 hat Max Reitz geschrieben:
> > > On 17.12.19 15:59, Kevin Wolf wrote:
> > > > This tests creating an external snapshot with VM state (which results in
> > > > an active overlay over an inactive backing file, which is also the root
> > > > node of an inactive BlockBackend), re-activating the images and
> > > > performing some operations to test that the re-activation worked as
> > > > intended.
> > > > 
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > 
> > > [...]
> > > 
> > > > diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
> > > > new file mode 100644
> > > > index 0000000000..5d382faaa8
> > > > --- /dev/null
> > > > +++ b/tests/qemu-iotests/280.out
> > > > @@ -0,0 +1,50 @@
> > > > +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > > > +
> > > > +=== Launch VM ===
> > > > +Enabling migration QMP events on VM...
> > > > +{"return": {}}
> > > > +
> > > > +=== Migrate to file ===
> > > > +{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
> > > > +{"return": {}}
> > > > +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > +
> > > > +VM is now stopped:
> > > > +completed
> > > > +{"execute": "query-status", "arguments": {}}
> > > > +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}
> > > 
> > > Hmmm, I get a finish-migrate status here (on tmpfs)...
> > 
> > Dave, is it intentional that the "completed" migration event is emitted
> > while we are still in finish-migration rather than postmigrate?
> 
> Yes it looks like it;  it's that the migration state machine hits
> COMPLETED that then _causes_ the runstate transitition to POSTMIGRATE.
> 
> static void migration_iteration_finish(MigrationState *s)
> {
>     /* If we enabled cpu throttling for auto-converge, turn it off. */
>     cpu_throttle_stop();
> 
>     qemu_mutex_lock_iothread();
>     switch (s->state) {
>     case MIGRATION_STATUS_COMPLETED:
>         migration_calculate_complete(s);
>         runstate_set(RUN_STATE_POSTMIGRATE);
>         break;
> 
> then there are a bunch of error cases where if it landed in
> FAILED/CANCELLED etc then we either restart the VM or also go to
> POSTMIGRATE.

Yes, I read the code. My question was more if there is a reason why we
want things to look like this in the external interface.

I just thought that it was confusing that migration is already called
completed when it will still change the runstate. But I guess the
opposite could be confusing as well (if we're in postmigrate, why should
the migration status still change?)

> > I guess we could change wait_migration() in qemu-iotests to wait for the
> > postmigrate state rather than the "completed" event, but maybe it would
> > be better to change the migration code to avoid similar races in other
> > QMP clients.
> 
> Given that the migration state machine is driving the runstate state
> machine I think it currently makes sense internally;  (although I don't
> think it's documented to be in that order or tested to be, which we
> might want to fix).

In any case, I seem to remember that it's inconsistent between source
and destination. On one side, the migration status is updated first, on
the other side the runstate is updated first.

> Looking at 234 and 262, it looks like you're calling wait_migration on
> both the source and dest; I don't think the dest will see the
> POSTMIGRATE.  Also note that depending what you're trying to do, with
> postcopy you'll be running on the destination before you see COMPLETED.
> 
> Waiting for the destination to leave 'inmigrate' state is probably
> the best strategy; then wait for the source to be in postmigrate.
> You can cause early exits if you see transitions to 'FAILED' - but
> actually the destination will likely quit in that case; so it should
> be much rarer for you to hit a timeout on a failed migration.

Commit 37ff7d70 changed it to wait for "postmigrate" on the source and
"running" on the destination, which I guess is good enough for a test
case that doesn't expect failure.

Kevin

Re: [PATCH 3/3] iotests: Test external snapshot with VM state

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 02.01.2020 um 14:25 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > Am 19.12.2019 um 15:26 hat Max Reitz geschrieben:
> > > > On 17.12.19 15:59, Kevin Wolf wrote:
> > > > > This tests creating an external snapshot with VM state (which results in
> > > > > an active overlay over an inactive backing file, which is also the root
> > > > > node of an inactive BlockBackend), re-activating the images and
> > > > > performing some operations to test that the re-activation worked as
> > > > > intended.
> > > > > 
> > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
> > > > > new file mode 100644
> > > > > index 0000000000..5d382faaa8
> > > > > --- /dev/null
> > > > > +++ b/tests/qemu-iotests/280.out
> > > > > @@ -0,0 +1,50 @@
> > > > > +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > > > > +
> > > > > +=== Launch VM ===
> > > > > +Enabling migration QMP events on VM...
> > > > > +{"return": {}}
> > > > > +
> > > > > +=== Migrate to file ===
> > > > > +{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
> > > > > +{"return": {}}
> > > > > +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > > +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > > +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > > +
> > > > > +VM is now stopped:
> > > > > +completed
> > > > > +{"execute": "query-status", "arguments": {}}
> > > > > +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}
> > > > 
> > > > Hmmm, I get a finish-migrate status here (on tmpfs)...
> > > 
> > > Dave, is it intentional that the "completed" migration event is emitted
> > > while we are still in finish-migration rather than postmigrate?
> > 
> > Yes it looks like it;  it's that the migration state machine hits
> > COMPLETED that then _causes_ the runstate transitition to POSTMIGRATE.
> > 
> > static void migration_iteration_finish(MigrationState *s)
> > {
> >     /* If we enabled cpu throttling for auto-converge, turn it off. */
> >     cpu_throttle_stop();
> > 
> >     qemu_mutex_lock_iothread();
> >     switch (s->state) {
> >     case MIGRATION_STATUS_COMPLETED:
> >         migration_calculate_complete(s);
> >         runstate_set(RUN_STATE_POSTMIGRATE);
> >         break;
> > 
> > then there are a bunch of error cases where if it landed in
> > FAILED/CANCELLED etc then we either restart the VM or also go to
> > POSTMIGRATE.
> 
> Yes, I read the code. My question was more if there is a reason why we
> want things to look like this in the external interface.
> 
> I just thought that it was confusing that migration is already called
> completed when it will still change the runstate. But I guess the
> opposite could be confusing as well (if we're in postmigrate, why should
> the migration status still change?)
> 
> > > I guess we could change wait_migration() in qemu-iotests to wait for the
> > > postmigrate state rather than the "completed" event, but maybe it would
> > > be better to change the migration code to avoid similar races in other
> > > QMP clients.
> > 
> > Given that the migration state machine is driving the runstate state
> > machine I think it currently makes sense internally;  (although I don't
> > think it's documented to be in that order or tested to be, which we
> > might want to fix).
> 
> In any case, I seem to remember that it's inconsistent between source
> and destination. On one side, the migration status is updated first, on
> the other side the runstate is updated first.

(Digging through old mails)

That might be partially due to my ed1f30 from 2015 where I move the
COMPLETED event later - prior to that it was much too early; before
the network announce and before the bdrv_invalidate_cache_all, and I
ended up moving it right to the end - it might have been better to leave
it before the runstate change.



> > Looking at 234 and 262, it looks like you're calling wait_migration on
> > both the source and dest; I don't think the dest will see the
> > POSTMIGRATE.  Also note that depending what you're trying to do, with
> > postcopy you'll be running on the destination before you see COMPLETED.
> > 
> > Waiting for the destination to leave 'inmigrate' state is probably
> > the best strategy; then wait for the source to be in postmigrate.
> > You can cause early exits if you see transitions to 'FAILED' - but
> > actually the destination will likely quit in that case; so it should
> > be much rarer for you to hit a timeout on a failed migration.
> 
> Commit 37ff7d70 changed it to wait for "postmigrate" on the source and
> "running" on the destination, which I guess is good enough for a test
> case that doesn't expect failure.

Dave

> Kevin
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] iotests: Test external snapshot with VM state

[Kevin Wolf]

Am 10.02.2020 um 13:31 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > Am 02.01.2020 um 14:25 hat Dr. David Alan Gilbert geschrieben:
> > > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > > Am 19.12.2019 um 15:26 hat Max Reitz geschrieben:
> > > > > On 17.12.19 15:59, Kevin Wolf wrote:
> > > > > > This tests creating an external snapshot with VM state (which results in
> > > > > > an active overlay over an inactive backing file, which is also the root
> > > > > > node of an inactive BlockBackend), re-activating the images and
> > > > > > performing some operations to test that the re-activation worked as
> > > > > > intended.
> > > > > > 
> > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
> > > > > > new file mode 100644
> > > > > > index 0000000000..5d382faaa8
> > > > > > --- /dev/null
> > > > > > +++ b/tests/qemu-iotests/280.out
> > > > > > @@ -0,0 +1,50 @@
> > > > > > +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > > > > > +
> > > > > > +=== Launch VM ===
> > > > > > +Enabling migration QMP events on VM...
> > > > > > +{"return": {}}
> > > > > > +
> > > > > > +=== Migrate to file ===
> > > > > > +{"execute": "migrate", "arguments": {"uri": "exec:cat > /dev/null"}}
> > > > > > +{"return": {}}
> > > > > > +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > > > +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > > > +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> > > > > > +
> > > > > > +VM is now stopped:
> > > > > > +completed
> > > > > > +{"execute": "query-status", "arguments": {}}
> > > > > > +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}}
> > > > > 
> > > > > Hmmm, I get a finish-migrate status here (on tmpfs)...
> > > > 
> > > > Dave, is it intentional that the "completed" migration event is emitted
> > > > while we are still in finish-migration rather than postmigrate?
> > > 
> > > Yes it looks like it;  it's that the migration state machine hits
> > > COMPLETED that then _causes_ the runstate transitition to POSTMIGRATE.
> > > 
> > > static void migration_iteration_finish(MigrationState *s)
> > > {
> > >     /* If we enabled cpu throttling for auto-converge, turn it off. */
> > >     cpu_throttle_stop();
> > > 
> > >     qemu_mutex_lock_iothread();
> > >     switch (s->state) {
> > >     case MIGRATION_STATUS_COMPLETED:
> > >         migration_calculate_complete(s);
> > >         runstate_set(RUN_STATE_POSTMIGRATE);
> > >         break;
> > > 
> > > then there are a bunch of error cases where if it landed in
> > > FAILED/CANCELLED etc then we either restart the VM or also go to
> > > POSTMIGRATE.
> > 
> > Yes, I read the code. My question was more if there is a reason why we
> > want things to look like this in the external interface.
> > 
> > I just thought that it was confusing that migration is already called
> > completed when it will still change the runstate. But I guess the
> > opposite could be confusing as well (if we're in postmigrate, why should
> > the migration status still change?)
> > 
> > > > I guess we could change wait_migration() in qemu-iotests to wait for the
> > > > postmigrate state rather than the "completed" event, but maybe it would
> > > > be better to change the migration code to avoid similar races in other
> > > > QMP clients.
> > > 
> > > Given that the migration state machine is driving the runstate state
> > > machine I think it currently makes sense internally;  (although I don't
> > > think it's documented to be in that order or tested to be, which we
> > > might want to fix).
> > 
> > In any case, I seem to remember that it's inconsistent between source
> > and destination. On one side, the migration status is updated first, on
> > the other side the runstate is updated first.
> 
> (Digging through old mails)
> 
> That might be partially due to my ed1f30 from 2015 where I move the
> COMPLETED event later - prior to that it was much too early; before
> the network announce and before the bdrv_invalidate_cache_all, and I
> ended up moving it right to the end - it might have been better to leave
> it before the runstate change.

We are working around this in the qemu-iotests now, so I guess I don't
have a pressing need for a consistent interface any more at the moment.
But if having this kind of inconsistency bothers you, feel free to do
something about it anyway. :-)

Kevin