[PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

When QMP was first introduced some 10+ years ago now, the snapshot
related commands (savevm/loadvm/delvm) were not converted. This was
primarily because their implementation causes blocking of the thread
running the monitor commands. This was (and still is) considered
undesirable behaviour both in HMP and QMP.

In theory someone was supposed to fix this flaw at some point in the
past 10 years and bring them into the QMP world. Sadly, thus far it
hasn't happened as people always had more important things to work
on. Enterprise apps were much more interested in external snapshots
than internal snapshots as they have many more features.

Meanwhile users still want to use internal snapshots as there is
a certainly simplicity in having everything self-contained in one
image, even though it has limitations. Thus the apps that end up
executing the savevm/loadvm/delvm via the "human-monitor-command"
QMP command.


IOW, the problematic blocking behaviour that was one of the reasons
for not having savevm/loadvm/delvm in QMP is experienced by applications
regardless. By not portting the commands to QMP due to one design flaw,
we've forced apps and users to suffer from other design flaws of HMP (
bad error reporting, strong type checking of args, no introspection) for
an additional 10 years. This feels rather sub-optimal :-(

In practice users don't appear to care strongly about the fact that these
commands block the VM while they run. I might have seen one bug report
about it, but it certainly isn't something that comes up as a frequent
topic except among us QEMU maintainers. Users do care about having
access to the snapshot feature.

Where I am seeing frequent complaints is wrt the use of OVMF combined
with snapshots which has some serious pain points. This is getting worse
as the push to ditch legacy BIOS in favour of UEFI gain momentum both
across OS vendors and mgmt apps. Solving it requires new parameters to
the commands, but doing this in HMP is super unappealing.



After 10 years, I think it is time for us to be a little pragmatic about
our handling of snapshots commands. My desire is that libvirt should never
use "human-monitor-command" under any circumstances, because of the
inherant flaws in HMP as a protocol for machine consumption. If there
are flaws in QMP commands that's fine. If we fix them in future, we can
deprecate the current QMP commands and remove them not too long after,
without being locked in forever.


Thus in this series I'm proposing a direct 1-1 mapping of the existing
HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
not solve the blocking thread problem, but it does eliminate the error
reporting, type checking and introspection problems inherant to HMP.
We're winning on 3 out of the 4 long term problems.

If someone can suggest a easy way to fix the thread blocking problem
too, I'd be interested to hear it. If it involves a major refactoring
then I think user are better served by unlocking what look like easy
wins today.

With a QMP variant, we reasonably deal with the problems related to OVMF:

 - The logic to pick which disk to store the vmstate in is not
   satsifactory.

   The first block driver state cannot be assumed to be the root disk
   image, it might be OVMF varstore and we don't want to store vmstate
   in there.

 - The logic to decide which disks must be snapshotted is hardwired
   to all disks which are writable

   Again with OVMF there might be a writable varstore, but this can be
   raw rather than qcow2 format, and thus unable to be snapshotted.
   While users might wish to snapshot their varstore, in some/many/most
   cases it is entirely uneccessary. Users are blocked from snapshotting
   their VM though due to this varstore.

These are solved by adding two parameters to the commands. The first is
a block device node name that identifies the image to store vmstate in,
and the second is a list of node names to exclude from snapshots.

In the block code I've only dealt with node names for block devices, as
IIUC, this is all that libvirt should need in the -blockdev world it now
lives in. IOW, I've made not attempt to cope with people wanting to use
these QMP commands in combination with -drive args.

I've done some minimal work in libvirt to start to make use of the new
commands to validate their functionality, but this isn't finished yet.

My ultimate goal is to make the GNOME Boxes maintainer happy again by
having internal snapshots work with OVMF:

  https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
f45c5f64048f16a6e

Daniel P. Berrang=C3=A9 (6):
  migration: improve error reporting of block driver state name
  migration: introduce savevm, loadvm, delvm QMP commands
  block: add ability to filter out blockdevs during snapshot
  block: allow specifying name of block device for vmstate storage
  migration: support excluding block devs in QMP snapshot commands
  migration: support picking vmstate disk in QMP snapshot commands

 block/monitor/block-hmp-cmds.c |  4 +-
 block/snapshot.c               | 68 +++++++++++++++++++------
 include/block/snapshot.h       | 21 +++++---
 include/migration/snapshot.h   | 10 +++-
 migration/savevm.c             | 71 +++++++++++++++++++-------
 monitor/hmp-cmds.c             | 20 ++------
 qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
 replay/replay-snapshot.c       |  4 +-
 softmmu/vl.c                   |  2 +-
 9 files changed, 228 insertions(+), 63 deletions(-)

--=20
2.26.2

[PATCH 1/6] migration: improve error reporting of block driver state name

[Daniel P. Berrangé]

With blockdev, a BlockDriverState may not have an device name,
so using a node name is required as an alternative.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 migration/savevm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index b979ea6e7f..72dbad95ed 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2648,7 +2648,7 @@ int save_snapshot(const char *name, Error **errp)
 
     if (!bdrv_all_can_snapshot(&bs)) {
         error_setg(errp, "Device '%s' is writable but does not support "
-                   "snapshots", bdrv_get_device_name(bs));
+                   "snapshots", bdrv_get_device_or_node_name(bs));
         return ret;
     }
 
@@ -2657,7 +2657,7 @@ int save_snapshot(const char *name, Error **errp)
         ret = bdrv_all_delete_snapshot(name, &bs1, errp);
         if (ret < 0) {
             error_prepend(errp, "Error while deleting snapshot on device "
-                          "'%s': ", bdrv_get_device_name(bs1));
+                          "'%s': ", bdrv_get_device_or_node_name(bs1));
             return ret;
         }
     }
@@ -2728,7 +2728,7 @@ int save_snapshot(const char *name, Error **errp)
     ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
     if (ret < 0) {
         error_setg(errp, "Error while creating snapshot on '%s'",
-                   bdrv_get_device_name(bs));
+                   bdrv_get_device_or_node_name(bs));
         goto the_end;
     }
 
@@ -2846,14 +2846,14 @@ int load_snapshot(const char *name, Error **errp)
     if (!bdrv_all_can_snapshot(&bs)) {
         error_setg(errp,
                    "Device '%s' is writable but does not support snapshots",
-                   bdrv_get_device_name(bs));
+                   bdrv_get_device_or_node_name(bs));
         return -ENOTSUP;
     }
     ret = bdrv_all_find_snapshot(name, &bs);
     if (ret < 0) {
         error_setg(errp,
                    "Device '%s' does not have the requested snapshot '%s'",
-                   bdrv_get_device_name(bs), name);
+                   bdrv_get_device_or_node_name(bs), name);
         return ret;
     }
 
@@ -2882,7 +2882,7 @@ int load_snapshot(const char *name, Error **errp)
     ret = bdrv_all_goto_snapshot(name, &bs, errp);
     if (ret < 0) {
         error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
-                      name, bdrv_get_device_name(bs));
+                      name, bdrv_get_device_or_node_name(bs));
         goto err_drain;
     }
 
-- 
2.26.2

[PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Daniel P. Berrangé]

savevm, loadvm and delvm are some of the few commands that have never
been converted to use QMP. The primary reason for this lack of
conversion is that they block execution of the thread for as long as
they run.

Despite this downside, however, libvirt and applications using libvirt
has used these commands for as long as QMP has existed, via the
"human-monitor-command" passthrough command. IOW, while it is clearly
desirable to be able to fix the blocking problem, this is not an
immediate obstacle to real world usage.

Meanwhile there is a need for other features which involve adding new
parameters to the commands. This is possible with HMP passthrough, but
it provides no reliable way for apps to introspect features, so using
QAPI modelling is highly desirable.

This patch thus introduces trival savevm, loadvm, delvm commands
to QMP that are functionally identical to the HMP counterpart, including
the blocking problem.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 migration/savevm.c  | 27 ++++++++++++++++
 monitor/hmp-cmds.c  | 18 ++---------
 qapi/migration.json | 76 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 15 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 72dbad95ed..53586a6406 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2943,3 +2943,30 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
 
     return !(vmsd && vmsd->unmigratable);
 }
+
+void qmp_savevm(const char *tag, Error **errp)
+{
+    save_snapshot(tag, errp);
+}
+
+void qmp_loadvm(const char *tag, Error **errp)
+{
+    int saved_vm_running  = runstate_is_running();
+
+    vm_stop(RUN_STATE_RESTORE_VM);
+
+    if (load_snapshot(tag, errp) == 0 && saved_vm_running) {
+        vm_start();
+    }
+}
+
+void qmp_delvm(const char *tag, Error **errp)
+{
+    BlockDriverState *bs;
+
+    if (bdrv_all_delete_snapshot(tag, &bs, errp) < 0) {
+        error_prepend(errp,
+                      "deleting snapshot on device '%s': ",
+                      bdrv_get_device_or_node_name(bs));
+    }
+}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2b0b58a336..26a5a1a701 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1089,15 +1089,9 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
 
 void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
-    int saved_vm_running  = runstate_is_running();
-    const char *name = qdict_get_str(qdict, "name");
     Error *err = NULL;
 
-    vm_stop(RUN_STATE_RESTORE_VM);
-
-    if (load_snapshot(name, &err) == 0 && saved_vm_running) {
-        vm_start();
-    }
+    qmp_loadvm(qdict_get_str(qdict, "name"), &err);
     hmp_handle_error(mon, err);
 }
 
@@ -1105,21 +1099,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
 
-    save_snapshot(qdict_get_try_str(qdict, "name"), &err);
+    qmp_savevm(qdict_get_try_str(qdict, "name"), &err);
     hmp_handle_error(mon, err);
 }
 
 void hmp_delvm(Monitor *mon, const QDict *qdict)
 {
-    BlockDriverState *bs;
     Error *err = NULL;
-    const char *name = qdict_get_str(qdict, "name");
 
-    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
-        error_prepend(&err,
-                      "deleting snapshot on device '%s': ",
-                      bdrv_get_device_name(bs));
-    }
+    qmp_delvm(qdict_get_str(qdict, "name"), &err);
     hmp_handle_error(mon, err);
 }
 
diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..849de38fb0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1621,3 +1621,79 @@
 ##
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
+
+##
+# @savevm:
+#
+# Save a VM snapshot
+#
+# @tag: name of the snapshot to create. If it already
+# exists it will be replaced.
+#
+# Note that execution of the VM will be paused during the time
+# it takes to save the snapshot
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "savevm",
+#      "data": {
+#         "tag": "my-snap"
+#      }
+#    }
+# <- { "return": { } }
+#
+# Since: 5.2
+##
+{ 'command': 'savevm',
+  'data': { 'tag': 'str' } }
+
+##
+# @loadvm:
+#
+# Load a VM snapshot
+#
+# @tag: name of the snapshot to load.
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "loadvm",
+#      "data": {
+#         "tag": "my-snap"
+#      }
+#    }
+# <- { "return": { } }
+#
+# Since: 5.2
+##
+{ 'command': 'loadvm',
+  'data': { 'tag': 'str' } }
+
+##
+# @delvm:
+#
+# Delete a VM snapshot
+#
+# @tag: name of the snapshot to delete.
+#
+# Note that execution of the VM will be paused during the time
+# it takes to delete the snapshot
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "delvm",
+#      "data": {
+#         "tag": "my-snap"
+#      }
+#    }
+# <- { "return": { } }
+#
+# Since: 5.2
+##
+{ 'command': 'delvm',
+  'data': { 'tag': 'str' } }
-- 
2.26.2

[PATCH 3/6] block: add ability to filter out blockdevs during snapshot

[Daniel P. Berrangé]

When executing snapshot logic, currently blockdevs are filtered out if
they are read-only or if there is no media present. In order to support
snapshotting when UEFI firmware is in use, we need the ability to filter
out the blockdev used for the firmware vars store. This can be achieved
by having a list of node names that should be excluded from
consideration for snapshots.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/monitor/block-hmp-cmds.c |  4 ++--
 block/snapshot.c               | 41 ++++++++++++++++++++++------------
 include/block/snapshot.h       | 19 +++++++++-------
 migration/savevm.c             | 18 +++++++--------
 4 files changed, 49 insertions(+), 33 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4c8c375172..0ee6e7a4be 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -899,7 +899,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     ImageEntry *image_entry, *next_ie;
     SnapshotEntry *snapshot_entry;
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(NULL);
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
@@ -951,7 +951,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     total = 0;
     for (i = 0; i < nb_sns; i++) {
         SnapshotEntry *next_sn;
-        if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
+        if (bdrv_all_find_snapshot(sn_tab[i].name, NULL, &bs1) == 0) {
             global_snapshots[total] = i;
             total++;
             QTAILQ_FOREACH(image_entry, &image_list, next) {
diff --git a/block/snapshot.c b/block/snapshot.c
index bd9fb01817..c8950b0b86 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -385,12 +385,22 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
     return ret;
 }
 
-static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
+static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs,
+                                           strList *exclude_bs)
 {
+    const char *node_name = bdrv_get_node_name(bs);
+
     if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
         return false;
     }
 
+    while (exclude_bs) {
+        if (g_str_equal(node_name, exclude_bs->value)) {
+            return false;
+        }
+        exclude_bs = exclude_bs->next;
+    }
+
     /* Include all nodes that are either in use by a BlockBackend, or that
      * aren't attached to any node, but owned by the monitor. */
     return bdrv_has_blk(bs) || QLIST_EMPTY(&bs->parents);
@@ -400,7 +410,7 @@ static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
  * These functions will properly handle dataplane (take aio_context_acquire
  * when appropriate for appropriate block drivers) */
 
-bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+bool bdrv_all_can_snapshot(strList *exclude_bs, BlockDriverState **first_bad_bs)
 {
     bool ok = true;
     BlockDriverState *bs;
@@ -410,7 +420,7 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
         AioContext *ctx = bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
-        if (bdrv_all_snapshots_includes_bs(bs)) {
+        if (bdrv_all_snapshots_includes_bs(bs, exclude_bs)) {
             ok = bdrv_can_snapshot(bs);
         }
         aio_context_release(ctx);
@@ -425,8 +435,8 @@ fail:
     return ok;
 }
 
-int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                             Error **errp)
+int bdrv_all_delete_snapshot(const char *name, strList *exclude_bs,
+                             BlockDriverState **first_bad_bs, Error **errp)
 {
     int ret = 0;
     BlockDriverState *bs;
@@ -437,7 +447,7 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
         AioContext *ctx = bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
-        if (bdrv_all_snapshots_includes_bs(bs) &&
+        if (bdrv_all_snapshots_includes_bs(bs, exclude_bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
             ret = bdrv_snapshot_delete(bs, snapshot->id_str,
@@ -456,8 +466,8 @@ fail:
 }
 
 
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                           Error **errp)
+int bdrv_all_goto_snapshot(const char *name, strList *exclude_bs,
+                           BlockDriverState **first_bad_bs, Error **errp)
 {
     int ret = 0;
     BlockDriverState *bs;
@@ -467,7 +477,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
         AioContext *ctx = bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
-        if (bdrv_all_snapshots_includes_bs(bs)) {
+        if (bdrv_all_snapshots_includes_bs(bs, exclude_bs)) {
             ret = bdrv_snapshot_goto(bs, name, errp);
         }
         aio_context_release(ctx);
@@ -482,7 +492,8 @@ fail:
     return ret;
 }
 
-int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+int bdrv_all_find_snapshot(const char *name, strList *exclude_bs,
+                           BlockDriverState **first_bad_bs)
 {
     QEMUSnapshotInfo sn;
     int err = 0;
@@ -493,7 +504,7 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
         AioContext *ctx = bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
-        if (bdrv_all_snapshots_includes_bs(bs)) {
+        if (bdrv_all_snapshots_includes_bs(bs, exclude_bs)) {
             err = bdrv_snapshot_find(bs, &sn, name);
         }
         aio_context_release(ctx);
@@ -511,6 +522,7 @@ fail:
 int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              BlockDriverState *vm_state_bs,
                              uint64_t vm_state_size,
+                             strList *exclude_bs,
                              BlockDriverState **first_bad_bs)
 {
     int err = 0;
@@ -524,7 +536,7 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
         if (bs == vm_state_bs) {
             sn->vm_state_size = vm_state_size;
             err = bdrv_snapshot_create(bs, sn);
-        } else if (bdrv_all_snapshots_includes_bs(bs)) {
+        } else if (bdrv_all_snapshots_includes_bs(bs, exclude_bs)) {
             sn->vm_state_size = 0;
             err = bdrv_snapshot_create(bs, sn);
         }
@@ -540,7 +552,7 @@ fail:
     return err;
 }
 
-BlockDriverState *bdrv_all_find_vmstate_bs(void)
+BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs)
 {
     BlockDriverState *bs;
     BdrvNextIterator it;
@@ -550,7 +562,8 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
         bool found;
 
         aio_context_acquire(ctx);
-        found = bdrv_all_snapshots_includes_bs(bs) && bdrv_can_snapshot(bs);
+        found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) &&
+            bdrv_can_snapshot(bs);
         aio_context_release(ctx);
 
         if (found) {
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 2bfcd57578..f20986ca37 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -25,7 +25,7 @@
 #ifndef SNAPSHOT_H
 #define SNAPSHOT_H
 
-
+#include "qapi/qapi-builtin-types.h"
 
 #define SNAPSHOT_OPT_BASE       "snapshot."
 #define SNAPSHOT_OPT_ID         "snapshot.id"
@@ -76,17 +76,20 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
  * These functions will properly handle dataplane (take aio_context_acquire
  * when appropriate for appropriate block drivers */
 
-bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
-int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
-                             Error **errp);
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                           Error **errp);
-int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
+bool bdrv_all_can_snapshot(strList *exclude_bs,
+                           BlockDriverState **first_bad_bs);
+int bdrv_all_delete_snapshot(const char *name, strList *exclude_bs,
+                             BlockDriverState **first_bsd_bs, Error **errp);
+int bdrv_all_goto_snapshot(const char *name, strList *exclude_bs,
+                           BlockDriverState **first_bad_bs, Error **errp);
+int bdrv_all_find_snapshot(const char *name, strList *exclude_bs,
+                           BlockDriverState **first_bad_bs);
 int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              BlockDriverState *vm_state_bs,
                              uint64_t vm_state_size,
+                             strList *exclude_bs,
                              BlockDriverState **first_bad_bs);
 
-BlockDriverState *bdrv_all_find_vmstate_bs(void);
+BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 53586a6406..4251aa0dde 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2646,7 +2646,7 @@ int save_snapshot(const char *name, Error **errp)
         return ret;
     }
 
-    if (!bdrv_all_can_snapshot(&bs)) {
+    if (!bdrv_all_can_snapshot(NULL, &bs)) {
         error_setg(errp, "Device '%s' is writable but does not support "
                    "snapshots", bdrv_get_device_or_node_name(bs));
         return ret;
@@ -2654,7 +2654,7 @@ int save_snapshot(const char *name, Error **errp)
 
     /* Delete old snapshots of the same name */
     if (name) {
-        ret = bdrv_all_delete_snapshot(name, &bs1, errp);
+        ret = bdrv_all_delete_snapshot(name, NULL, &bs1, errp);
         if (ret < 0) {
             error_prepend(errp, "Error while deleting snapshot on device "
                           "'%s': ", bdrv_get_device_or_node_name(bs1));
@@ -2662,7 +2662,7 @@ int save_snapshot(const char *name, Error **errp)
         }
     }
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(NULL);
     if (bs == NULL) {
         error_setg(errp, "No block device can accept snapshots");
         return ret;
@@ -2725,7 +2725,7 @@ int save_snapshot(const char *name, Error **errp)
     aio_context_release(aio_context);
     aio_context = NULL;
 
-    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
+    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, NULL, &bs);
     if (ret < 0) {
         error_setg(errp, "Error while creating snapshot on '%s'",
                    bdrv_get_device_or_node_name(bs));
@@ -2843,13 +2843,13 @@ int load_snapshot(const char *name, Error **errp)
         return -EINVAL;
     }
 
-    if (!bdrv_all_can_snapshot(&bs)) {
+    if (!bdrv_all_can_snapshot(NULL, &bs)) {
         error_setg(errp,
                    "Device '%s' is writable but does not support snapshots",
                    bdrv_get_device_or_node_name(bs));
         return -ENOTSUP;
     }
-    ret = bdrv_all_find_snapshot(name, &bs);
+    ret = bdrv_all_find_snapshot(name, NULL, &bs);
     if (ret < 0) {
         error_setg(errp,
                    "Device '%s' does not have the requested snapshot '%s'",
@@ -2857,7 +2857,7 @@ int load_snapshot(const char *name, Error **errp)
         return ret;
     }
 
-    bs_vm_state = bdrv_all_find_vmstate_bs();
+    bs_vm_state = bdrv_all_find_vmstate_bs(NULL);
     if (!bs_vm_state) {
         error_setg(errp, "No block device supports snapshots");
         return -ENOTSUP;
@@ -2879,7 +2879,7 @@ int load_snapshot(const char *name, Error **errp)
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all_begin();
 
-    ret = bdrv_all_goto_snapshot(name, &bs, errp);
+    ret = bdrv_all_goto_snapshot(name, NULL, &bs, errp);
     if (ret < 0) {
         error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
                       name, bdrv_get_device_or_node_name(bs));
@@ -2964,7 +2964,7 @@ void qmp_delvm(const char *tag, Error **errp)
 {
     BlockDriverState *bs;
 
-    if (bdrv_all_delete_snapshot(tag, &bs, errp) < 0) {
+    if (bdrv_all_delete_snapshot(tag, NULL, &bs, errp) < 0) {
         error_prepend(errp,
                       "deleting snapshot on device '%s': ",
                       bdrv_get_device_or_node_name(bs));
-- 
2.26.2

[PATCH 4/6] block: allow specifying name of block device for vmstate storage

[Daniel P. Berrangé]

Currently the vmstate will be stored in the first block device that
supports snapshots. Historically this would have usually been the
root device, but with UEFI it might be the variable store. There
needs to be a way to override the choice of block device to store
the state in.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/monitor/block-hmp-cmds.c |  2 +-
 block/snapshot.c               | 33 +++++++++++++++++++++++++++++----
 include/block/snapshot.h       |  4 +++-
 migration/savevm.c             |  6 ++----
 4 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 0ee6e7a4be..a698fac027 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -899,7 +899,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     ImageEntry *image_entry, *next_ie;
     SnapshotEntry *snapshot_entry;
 
-    bs = bdrv_all_find_vmstate_bs(NULL);
+    bs = bdrv_all_find_vmstate_bs(NULL, NULL, NULL);
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
diff --git a/block/snapshot.c b/block/snapshot.c
index c8950b0b86..4774a1890d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -552,7 +552,9 @@ fail:
     return err;
 }
 
-BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs)
+BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs,
+                                           strList *exclude_bs,
+                                           Error **errp)
 {
     BlockDriverState *bs;
     BdrvNextIterator it;
@@ -560,16 +562,39 @@ BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs)
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *ctx = bdrv_get_aio_context(bs);
         bool found;
+        Error *err = NULL;
 
         aio_context_acquire(ctx);
-        found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) &&
-            bdrv_can_snapshot(bs);
+        if (vmstate_bs == NULL) {
+            found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) &&
+                bdrv_can_snapshot(bs);
+        } else {
+            if (g_str_equal(vmstate_bs, bdrv_get_node_name(bs))) {
+                found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) &&
+                    bdrv_can_snapshot(bs);
+                if (!found) {
+                    error_setg(&err,
+                               "block device '%s' cannot accept snapshots",
+                               vmstate_bs);
+                }
+            } else {
+                found = false;
+            }
+        }
         aio_context_release(ctx);
 
-        if (found) {
+        if (found || err) {
             bdrv_next_cleanup(&it);
+            if (err) {
+                error_propagate(errp, err);
+                return NULL;
+            }
             break;
         }
     }
+
+    if (!bs) {
+        error_setg(errp, "No block device can accept snapshots");
+    }
     return bs;
 }
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index f20986ca37..ff627df5cb 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -90,6 +90,8 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              strList *exclude_bs,
                              BlockDriverState **first_bad_bs);
 
-BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs);
+BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs,
+                                           strList *exclude_bs,
+                                           Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4251aa0dde..b11c6a882d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2662,9 +2662,8 @@ int save_snapshot(const char *name, Error **errp)
         }
     }
 
-    bs = bdrv_all_find_vmstate_bs(NULL);
+    bs = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
     if (bs == NULL) {
-        error_setg(errp, "No block device can accept snapshots");
         return ret;
     }
     aio_context = bdrv_get_aio_context(bs);
@@ -2857,9 +2856,8 @@ int load_snapshot(const char *name, Error **errp)
         return ret;
     }
 
-    bs_vm_state = bdrv_all_find_vmstate_bs(NULL);
+    bs_vm_state = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
     if (!bs_vm_state) {
-        error_setg(errp, "No block device supports snapshots");
         return -ENOTSUP;
     }
     aio_context = bdrv_get_aio_context(bs_vm_state);
-- 
2.26.2

[PATCH 5/6] migration: support excluding block devs in QMP snapshot commands

[Daniel P. Berrangé]

This wires up support for a new "exclude" parameter to the QMP commands
for snapshots (savevm, loadvm, delvm). This parameter accepts a list of
block driver state node names.

One use case for this would be a VM using OVMF firmware where the
variables store is a raw disk image. Ideally the variable store would be
qcow2, allowing its contents to be included in the snapshot, but
typically today the variable store is raw. It is still useful to be able
to snapshot VMs using OVMF, even if the varstore is excluded, as the
main OS disk content is usually the stuff the user cares about.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/migration/snapshot.h |  6 ++++--
 migration/savevm.c           | 38 +++++++++++++++++++++---------------
 monitor/hmp-cmds.c           |  6 +++---
 qapi/migration.json          | 21 ++++++++++++++------
 replay/replay-snapshot.c     |  4 ++--
 softmmu/vl.c                 |  2 +-
 6 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index c85b6ec75b..dffb84dbe5 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -15,7 +15,9 @@
 #ifndef QEMU_MIGRATION_SNAPSHOT_H
 #define QEMU_MIGRATION_SNAPSHOT_H
 
-int save_snapshot(const char *name, Error **errp);
-int load_snapshot(const char *name, Error **errp);
+#include "qapi/qapi-builtin-types.h"
+
+int save_snapshot(const char *name, strList *exclude, Error **errp);
+int load_snapshot(const char *name, strList *exclude, Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index b11c6a882d..4b040676f7 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2624,7 +2624,7 @@ int qemu_load_device_state(QEMUFile *f)
     return 0;
 }
 
-int save_snapshot(const char *name, Error **errp)
+int save_snapshot(const char *name, strList *exclude, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2646,7 +2646,7 @@ int save_snapshot(const char *name, Error **errp)
         return ret;
     }
 
-    if (!bdrv_all_can_snapshot(NULL, &bs)) {
+    if (!bdrv_all_can_snapshot(exclude, &bs)) {
         error_setg(errp, "Device '%s' is writable but does not support "
                    "snapshots", bdrv_get_device_or_node_name(bs));
         return ret;
@@ -2654,7 +2654,7 @@ int save_snapshot(const char *name, Error **errp)
 
     /* Delete old snapshots of the same name */
     if (name) {
-        ret = bdrv_all_delete_snapshot(name, NULL, &bs1, errp);
+        ret = bdrv_all_delete_snapshot(name, exclude, &bs1, errp);
         if (ret < 0) {
             error_prepend(errp, "Error while deleting snapshot on device "
                           "'%s': ", bdrv_get_device_or_node_name(bs1));
@@ -2662,7 +2662,7 @@ int save_snapshot(const char *name, Error **errp)
         }
     }
 
-    bs = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
+    bs = bdrv_all_find_vmstate_bs(NULL, exclude, errp);
     if (bs == NULL) {
         return ret;
     }
@@ -2724,7 +2724,7 @@ int save_snapshot(const char *name, Error **errp)
     aio_context_release(aio_context);
     aio_context = NULL;
 
-    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, NULL, &bs);
+    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, exclude, &bs);
     if (ret < 0) {
         error_setg(errp, "Error while creating snapshot on '%s'",
                    bdrv_get_device_or_node_name(bs));
@@ -2827,7 +2827,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     migration_incoming_state_destroy();
 }
 
-int load_snapshot(const char *name, Error **errp)
+int load_snapshot(const char *name, strList *exclude, Error **errp)
 {
     BlockDriverState *bs, *bs_vm_state;
     QEMUSnapshotInfo sn;
@@ -2842,13 +2842,13 @@ int load_snapshot(const char *name, Error **errp)
         return -EINVAL;
     }
 
-    if (!bdrv_all_can_snapshot(NULL, &bs)) {
+    if (!bdrv_all_can_snapshot(exclude, &bs)) {
         error_setg(errp,
                    "Device '%s' is writable but does not support snapshots",
                    bdrv_get_device_or_node_name(bs));
         return -ENOTSUP;
     }
-    ret = bdrv_all_find_snapshot(name, NULL, &bs);
+    ret = bdrv_all_find_snapshot(name, exclude, &bs);
     if (ret < 0) {
         error_setg(errp,
                    "Device '%s' does not have the requested snapshot '%s'",
@@ -2856,7 +2856,7 @@ int load_snapshot(const char *name, Error **errp)
         return ret;
     }
 
-    bs_vm_state = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
+    bs_vm_state = bdrv_all_find_vmstate_bs(NULL, exclude, errp);
     if (!bs_vm_state) {
         return -ENOTSUP;
     }
@@ -2877,7 +2877,7 @@ int load_snapshot(const char *name, Error **errp)
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all_begin();
 
-    ret = bdrv_all_goto_snapshot(name, NULL, &bs, errp);
+    ret = bdrv_all_goto_snapshot(name, exclude, &bs, errp);
     if (ret < 0) {
         error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
                       name, bdrv_get_device_or_node_name(bs));
@@ -2942,27 +2942,33 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
     return !(vmsd && vmsd->unmigratable);
 }
 
-void qmp_savevm(const char *tag, Error **errp)
+void qmp_savevm(const char *tag,
+                bool has_exclude, strList *exclude,
+                Error **errp)
 {
-    save_snapshot(tag, errp);
+    save_snapshot(tag, exclude, errp);
 }
 
-void qmp_loadvm(const char *tag, Error **errp)
+void qmp_loadvm(const char *tag,
+                bool has_exclude, strList *exclude,
+                Error **errp)
 {
     int saved_vm_running  = runstate_is_running();
 
     vm_stop(RUN_STATE_RESTORE_VM);
 
-    if (load_snapshot(tag, errp) == 0 && saved_vm_running) {
+    if (load_snapshot(tag, exclude, errp) == 0 && saved_vm_running) {
         vm_start();
     }
 }
 
-void qmp_delvm(const char *tag, Error **errp)
+void qmp_delvm(const char *tag,
+               bool has_exclude, strList *exclude,
+               Error **errp)
 {
     BlockDriverState *bs;
 
-    if (bdrv_all_delete_snapshot(tag, NULL, &bs, errp) < 0) {
+    if (bdrv_all_delete_snapshot(tag, exclude, &bs, errp) < 0) {
         error_prepend(errp,
                       "deleting snapshot on device '%s': ",
                       bdrv_get_device_or_node_name(bs));
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 26a5a1a701..fcde649100 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1091,7 +1091,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
 
-    qmp_loadvm(qdict_get_str(qdict, "name"), &err);
+    qmp_loadvm(qdict_get_str(qdict, "name"), false, NULL, &err);
     hmp_handle_error(mon, err);
 }
 
@@ -1099,7 +1099,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
 
-    qmp_savevm(qdict_get_try_str(qdict, "name"), &err);
+    qmp_savevm(qdict_get_try_str(qdict, "name"), false, NULL, &err);
     hmp_handle_error(mon, err);
 }
 
@@ -1107,7 +1107,7 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
 
-    qmp_delvm(qdict_get_str(qdict, "name"), &err);
+    qmp_delvm(qdict_get_str(qdict, "name"), false, NULL, &err);
     hmp_handle_error(mon, err);
 }
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 849de38fb0..2388664077 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1629,6 +1629,7 @@
 #
 # @tag: name of the snapshot to create. If it already
 # exists it will be replaced.
+# @exclude: list of block device node names to exclude
 #
 # Note that execution of the VM will be paused during the time
 # it takes to save the snapshot
@@ -1639,7 +1640,8 @@
 #
 # -> { "execute": "savevm",
 #      "data": {
-#         "tag": "my-snap"
+#         "tag": "my-snap",
+#         "exclude": ["pflash0-vars"]
 #      }
 #    }
 # <- { "return": { } }
@@ -1647,7 +1649,8 @@
 # Since: 5.2
 ##
 { 'command': 'savevm',
-  'data': { 'tag': 'str' } }
+  'data': { 'tag': 'str',
+            '*exclude': ['str'] } }
 
 ##
 # @loadvm:
@@ -1655,6 +1658,7 @@
 # Load a VM snapshot
 #
 # @tag: name of the snapshot to load.
+# @exclude: list of block device node names to exclude
 #
 # Returns: nothing
 #
@@ -1662,7 +1666,8 @@
 #
 # -> { "execute": "loadvm",
 #      "data": {
-#         "tag": "my-snap"
+#         "tag": "my-snap",
+#         "exclude": ["pflash0-vars"]
 #      }
 #    }
 # <- { "return": { } }
@@ -1670,7 +1675,8 @@
 # Since: 5.2
 ##
 { 'command': 'loadvm',
-  'data': { 'tag': 'str' } }
+  'data': { 'tag': 'str',
+            '*exclude': ['str'] } }
 
 ##
 # @delvm:
@@ -1678,6 +1684,7 @@
 # Delete a VM snapshot
 #
 # @tag: name of the snapshot to delete.
+# @exclude: list of block device node names to exclude
 #
 # Note that execution of the VM will be paused during the time
 # it takes to delete the snapshot
@@ -1688,7 +1695,8 @@
 #
 # -> { "execute": "delvm",
 #      "data": {
-#         "tag": "my-snap"
+#         "tag": "my-snap",
+#         "exclude": ["pflash0-vars"]
 #      }
 #    }
 # <- { "return": { } }
@@ -1696,4 +1704,5 @@
 # Since: 5.2
 ##
 { 'command': 'delvm',
-  'data': { 'tag': 'str' } }
+  'data': { 'tag': 'str',
+            '*exclude': ['str'] } }
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index e26fa4c892..1351170c67 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -77,13 +77,13 @@ void replay_vmstate_init(void)
 
     if (replay_snapshot) {
         if (replay_mode == REPLAY_MODE_RECORD) {
-            if (save_snapshot(replay_snapshot, &err) != 0) {
+            if (save_snapshot(replay_snapshot, NULL, &err) != 0) {
                 error_report_err(err);
                 error_report("Could not create snapshot for icount record");
                 exit(1);
             }
         } else if (replay_mode == REPLAY_MODE_PLAY) {
-            if (load_snapshot(replay_snapshot, &err) != 0) {
+            if (load_snapshot(replay_snapshot, NULL, &err) != 0) {
                 error_report_err(err);
                 error_report("Could not load snapshot for icount replay");
                 exit(1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3e15ee2435..f7c8be8c6a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4452,7 +4452,7 @@ void qemu_init(int argc, char **argv, char **envp)
     register_global_state();
     if (loadvm) {
         Error *local_err = NULL;
-        if (load_snapshot(loadvm, &local_err) < 0) {
+        if (load_snapshot(loadvm, NULL, &local_err) < 0) {
             error_report_err(local_err);
             autostart = 0;
             exit(1);
-- 
2.26.2

[PATCH 6/6] migration: support picking vmstate disk in QMP snapshot commands

[Daniel P. Berrangé]

This wires up support for a new "vmstate" parameter to the QMP commands
for snapshots (savevm, loadvm). This parameter accepts block driver
state node name.

One use case for this would be a VM using OVMF firmware where the
variables store is the first snapshottable disk image. The vmstate
snapshot usually wants to be stored in the primary root disk of the
VM, not the firmeware varstore. Thus there needs to be a mechanism
to override the default choice of disk.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/migration/snapshot.h |  8 ++++++--
 migration/savevm.c           | 16 ++++++++++------
 monitor/hmp-cmds.c           |  6 ++++--
 qapi/migration.json          |  6 ++++++
 replay/replay-snapshot.c     |  4 ++--
 softmmu/vl.c                 |  2 +-
 6 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index dffb84dbe5..721147d3c1 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -17,7 +17,11 @@
 
 #include "qapi/qapi-builtin-types.h"
 
-int save_snapshot(const char *name, strList *exclude, Error **errp);
-int load_snapshot(const char *name, strList *exclude, Error **errp);
+int save_snapshot(const char *name,
+                  const char *vmstate, strList *exclude,
+                  Error **errp);
+int load_snapshot(const char *name,
+                  const char *vmstate, strList *exclude,
+                  Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4b040676f7..5fd593e475 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2624,7 +2624,8 @@ int qemu_load_device_state(QEMUFile *f)
     return 0;
 }
 
-int save_snapshot(const char *name, strList *exclude, Error **errp)
+int save_snapshot(const char *name, const char *vmstate,
+                  strList *exclude, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2662,7 +2663,7 @@ int save_snapshot(const char *name, strList *exclude, Error **errp)
         }
     }
 
-    bs = bdrv_all_find_vmstate_bs(NULL, exclude, errp);
+    bs = bdrv_all_find_vmstate_bs(vmstate, exclude, errp);
     if (bs == NULL) {
         return ret;
     }
@@ -2827,7 +2828,8 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     migration_incoming_state_destroy();
 }
 
-int load_snapshot(const char *name, strList *exclude, Error **errp)
+int load_snapshot(const char *name, const char *vmstate,
+                  strList *exclude, Error **errp)
 {
     BlockDriverState *bs, *bs_vm_state;
     QEMUSnapshotInfo sn;
@@ -2856,7 +2858,7 @@ int load_snapshot(const char *name, strList *exclude, Error **errp)
         return ret;
     }
 
-    bs_vm_state = bdrv_all_find_vmstate_bs(NULL, exclude, errp);
+    bs_vm_state = bdrv_all_find_vmstate_bs(vmstate, exclude, errp);
     if (!bs_vm_state) {
         return -ENOTSUP;
     }
@@ -2943,13 +2945,15 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
 }
 
 void qmp_savevm(const char *tag,
+                bool has_vmstate, const char *vmstate,
                 bool has_exclude, strList *exclude,
                 Error **errp)
 {
-    save_snapshot(tag, exclude, errp);
+    save_snapshot(tag, vmstate, exclude, errp);
 }
 
 void qmp_loadvm(const char *tag,
+                bool has_vmstate, const char *vmstate,
                 bool has_exclude, strList *exclude,
                 Error **errp)
 {
@@ -2957,7 +2961,7 @@ void qmp_loadvm(const char *tag,
 
     vm_stop(RUN_STATE_RESTORE_VM);
 
-    if (load_snapshot(tag, exclude, errp) == 0 && saved_vm_running) {
+    if (load_snapshot(tag, vmstate, exclude, errp) == 0 && saved_vm_running) {
         vm_start();
     }
 }
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index fcde649100..586676e179 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1091,7 +1091,8 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
 
-    qmp_loadvm(qdict_get_str(qdict, "name"), false, NULL, &err);
+    qmp_loadvm(qdict_get_str(qdict, "name"),
+               false, NULL, false, NULL, &err);
     hmp_handle_error(mon, err);
 }
 
@@ -1099,7 +1100,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
 
-    qmp_savevm(qdict_get_try_str(qdict, "name"), false, NULL, &err);
+    qmp_savevm(qdict_get_try_str(qdict, "name"),
+               false, NULL, false, NULL, &err);
     hmp_handle_error(mon, err);
 }
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 2388664077..91173f5b06 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1630,6 +1630,7 @@
 # @tag: name of the snapshot to create. If it already
 # exists it will be replaced.
 # @exclude: list of block device node names to exclude
+# @vmstate: block device node name to save vmstate to
 #
 # Note that execution of the VM will be paused during the time
 # it takes to save the snapshot
@@ -1641,6 +1642,7 @@
 # -> { "execute": "savevm",
 #      "data": {
 #         "tag": "my-snap",
+#         "vmstate": "disk0",
 #         "exclude": ["pflash0-vars"]
 #      }
 #    }
@@ -1650,6 +1652,7 @@
 ##
 { 'command': 'savevm',
   'data': { 'tag': 'str',
+            '*vmstate': 'str',
             '*exclude': ['str'] } }
 
 ##
@@ -1659,6 +1662,7 @@
 #
 # @tag: name of the snapshot to load.
 # @exclude: list of block device node names to exclude
+# @vmstate: block device node name to load vmstate from
 #
 # Returns: nothing
 #
@@ -1667,6 +1671,7 @@
 # -> { "execute": "loadvm",
 #      "data": {
 #         "tag": "my-snap",
+#         "vmstate": "disk0",
 #         "exclude": ["pflash0-vars"]
 #      }
 #    }
@@ -1676,6 +1681,7 @@
 ##
 { 'command': 'loadvm',
   'data': { 'tag': 'str',
+            '*vmstate': 'str',
             '*exclude': ['str'] } }
 
 ##
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 1351170c67..f0f45a4f24 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -77,13 +77,13 @@ void replay_vmstate_init(void)
 
     if (replay_snapshot) {
         if (replay_mode == REPLAY_MODE_RECORD) {
-            if (save_snapshot(replay_snapshot, NULL, &err) != 0) {
+            if (save_snapshot(replay_snapshot, NULL, NULL, &err) != 0) {
                 error_report_err(err);
                 error_report("Could not create snapshot for icount record");
                 exit(1);
             }
         } else if (replay_mode == REPLAY_MODE_PLAY) {
-            if (load_snapshot(replay_snapshot, NULL, &err) != 0) {
+            if (load_snapshot(replay_snapshot, NULL, NULL, &err) != 0) {
                 error_report_err(err);
                 error_report("Could not load snapshot for icount replay");
                 exit(1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f7c8be8c6a..fbaa326675 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4452,7 +4452,7 @@ void qemu_init(int argc, char **argv, char **envp)
     register_global_state();
     if (loadvm) {
         Error *local_err = NULL;
-        if (load_snapshot(loadvm, NULL, &local_err) < 0) {
+        if (load_snapshot(loadvm, NULL, NULL, &local_err) < 0) {
             error_report_err(local_err);
             autostart = 0;
             exit(1);
-- 
2.26.2

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Eric Blake]

On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> savevm, loadvm and delvm are some of the few commands that have never
> been converted to use QMP. The primary reason for this lack of
> conversion is that they block execution of the thread for as long as
> they run.
> 
> Despite this downside, however, libvirt and applications using libvirt
> has used these commands for as long as QMP has existed, via the
> "human-monitor-command" passthrough command. IOW, while it is clearly
> desirable to be able to fix the blocking problem, this is not an
> immediate obstacle to real world usage.
> 
> Meanwhile there is a need for other features which involve adding new
> parameters to the commands. This is possible with HMP passthrough, but
> it provides no reliable way for apps to introspect features, so using
> QAPI modelling is highly desirable.
> 
> This patch thus introduces trival savevm, loadvm, delvm commands

trivial

> to QMP that are functionally identical to the HMP counterpart, including
> the blocking problem.

Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves 
room to change them when we DO solve the blocking issue?  Or will the 
solution of the blocking issue introduce new QMP commands, at which 
point we can add QMP deprecation markers on these commands to eventually 
retire them?

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

> +++ b/qapi/migration.json
> @@ -1621,3 +1621,79 @@
>   ##
>   { 'event': 'UNPLUG_PRIMARY',
>     'data': { 'device-id': 'str' } }
> +
> +##
> +# @savevm:
> +#
> +# Save a VM snapshot
> +#
> +# @tag: name of the snapshot to create. If it already
> +# exists it will be replaced.
> +#
> +# Note that execution of the VM will be paused during the time
> +# it takes to save the snapshot
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "savevm",
> +#      "data": {
> +#         "tag": "my-snap"
> +#      }
> +#    }
> +# <- { "return": { } }
> +#
> +# Since: 5.2

I guess you are NOT trying to make 5.1 soft freeze next week?


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

Re: [PATCH 6/6] migration: support picking vmstate disk in QMP snapshot commands

[Eric Blake]

On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> This wires up support for a new "vmstate" parameter to the QMP commands
> for snapshots (savevm, loadvm). This parameter accepts block driver
> state node name.
> 
> One use case for this would be a VM using OVMF firmware where the
> variables store is the first snapshottable disk image. The vmstate
> snapshot usually wants to be stored in the primary root disk of the
> VM, not the firmeware varstore. Thus there needs to be a mechanism

firmware

> to override the default choice of disk.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

> +++ b/qapi/migration.json
> @@ -1630,6 +1630,7 @@
>   # @tag: name of the snapshot to create. If it already
>   # exists it will be replaced.
>   # @exclude: list of block device node names to exclude
> +# @vmstate: block device node name to save vmstate to
>   #
>   # Note that execution of the VM will be paused during the time
>   # it takes to save the snapshot
> @@ -1641,6 +1642,7 @@
>   # -> { "execute": "savevm",
>   #      "data": {
>   #         "tag": "my-snap",
> +#         "vmstate": "disk0",
>   #         "exclude": ["pflash0-vars"]
>   #      }
>   #    }
> @@ -1650,6 +1652,7 @@
>   ##
>   { 'command': 'savevm',
>     'data': { 'tag': 'str',
> +            '*vmstate': 'str',
>               '*exclude': ['str'] } }

During save, the list of block devices is obvious: everything that is 
not excluded.  But,

>   
>   ##
> @@ -1659,6 +1662,7 @@
>   #
>   # @tag: name of the snapshot to load.
>   # @exclude: list of block device node names to exclude
> +# @vmstate: block device node name to load vmstate from
>   #
>   # Returns: nothing
>   #
> @@ -1667,6 +1671,7 @@
>   # -> { "execute": "loadvm",
>   #      "data": {
>   #         "tag": "my-snap",
> +#         "vmstate": "disk0",
>   #         "exclude": ["pflash0-vars"]
>   #      }
>   #    }
> @@ -1676,6 +1681,7 @@
>   ##
>   { 'command': 'loadvm',
>     'data': { 'tag': 'str',
> +            '*vmstate': 'str',
>               '*exclude': ['str'] } }

...now that we support exclusion during saving, or even without 
exclusion but when the user has performed hotplug/unplug operations in 
the meantime from when the snapshot was created, isn't load better off 
listing all devices which SHOULD be restored, rather than excluding 
devices that should NOT be restored?  (After all, libvirt knows which 
disks existed at the time of the snapshot, which may be different than 
the set of disks that exist now even though we are throwing out the 
state now to go back to the state at the time of the snapshot)

Then there's the question of symmetry: if load needs an explicit list of 
blocks to load from (rather than the set of blocks that are currently 
associated with the machine), should save also take its list by positive 
inclusion rather than negative exclusion?

And why does delvm not need to specify which block is the vmstate? 
delvm is in the same boat as loadvm - the set of blocks involved at the 
time of the snapshot creation may be different than the set of blocks 
currently associated with the guest at the time you run load/delvm.

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

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Daniel P. Berrangé]

On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > savevm, loadvm and delvm are some of the few commands that have never
> > been converted to use QMP. The primary reason for this lack of
> > conversion is that they block execution of the thread for as long as
> > they run.
> > 
> > Despite this downside, however, libvirt and applications using libvirt
> > has used these commands for as long as QMP has existed, via the
> > "human-monitor-command" passthrough command. IOW, while it is clearly
> > desirable to be able to fix the blocking problem, this is not an
> > immediate obstacle to real world usage.
> > 
> > Meanwhile there is a need for other features which involve adding new
> > parameters to the commands. This is possible with HMP passthrough, but
> > it provides no reliable way for apps to introspect features, so using
> > QAPI modelling is highly desirable.
> > 
> > This patch thus introduces trival savevm, loadvm, delvm commands
> 
> trivial
> 
> > to QMP that are functionally identical to the HMP counterpart, including
> > the blocking problem.
> 
> Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> to change them when we DO solve the blocking issue?  Or will the solution of
> the blocking issue introduce new QMP commands, at which point we can add QMP
> deprecation markers on these commands to eventually retire them?

I was in two minds about this, so I'm open to arguments either way.

The primary goal is for libvirt to consume the APIs as soon as possible,
and generally libvirt doesn't want todo this is they are declared experimental
via a "x-" prefix. So that pushes me away from "x-".

If we don't have an "x-" prefix and want to make changes, we can add extra
parameters to trigger new behaviour in backwards compatible manner. Or we can
simply deprecate these commands, deleting them 2 releases later, while adding
completely new commands.

If we think the prposed design will definitely need incompatible changes in
a very short time frame though, that would push towards "x-".

So IMHO the right answer largely depends on whether there is a credible
strategy to implement the ideal non-blocking solution in a reasonable amount
of time. I can't justify spending much time on this myself, but I'm willing
to consider & try proposals for solving the blocking problem if they're not
too complex / invasive.

I just don't want to end up having a "x-savevm" command for another 10 years,
waiting for a perfect solution that never arrives because people always have
higher priority items, as apps are clearly able to accept the blocking problem
if the alternative is no snapshots at all.


> > +
> > +##
> > +# @savevm:
> > +#
> > +# Save a VM snapshot
> > +#
> > +# @tag: name of the snapshot to create. If it already
> > +# exists it will be replaced.
> > +#
> > +# Note that execution of the VM will be paused during the time
> > +# it takes to save the snapshot
> > +#
> > +# Returns: nothing
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "savevm",
> > +#      "data": {
> > +#         "tag": "my-snap"
> > +#      }
> > +#    }
> > +# <- { "return": { } }
> > +#
> > +# Since: 5.2
> 
> I guess you are NOT trying to make 5.1 soft freeze next week?

Correct. It is unrealistic to consider this for soft freeze.

I'd really love to have a solution in 5.2 though, even if it doesn't
solve all our problems. Something that can at least unblock apps that
want to use OVMF with internal snapshots today.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/6] migration: improve error reporting of block driver state name

[Eric Blake]

On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> With blockdev, a BlockDriverState may not have an device name,

s/an/a/

> so using a node name is required as an alternative.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   migration/savevm.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 

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

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

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200702175754.2211821-1-berrange@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

 (qemu) info snapshots
 No available block device supports snapshots
 (qemu) loadvm snap0
-Error: No block device supports snapshots
+Error: No block device can accept snapshots
 (qemu) quit
 
 
---
 Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
-Error: Device '' is writable but does not support snapshots
+Error: Device 'file' is writable but does not support snapshots
 (qemu) info snapshots
 No available block device supports snapshots
 (qemu) loadvm snap0
-Error: Device '' is writable but does not support snapshots
+Error: Device 'file' is writable but does not support snapshots
 (qemu) quit
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
---
Not run: 259
Failures: 267
Failed 1 of 119 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-aarch64: tests/qtest/qos-test
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=d415a115078246e4ab99c8a4d27787e7', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-vi4bosi3/src/docker-src.2020-07-02-14.36.29.6528:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=d415a115078246e4ab99c8a4d27787e7
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-vi4bosi3/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    16m29.279s
user    0m8.998s


The full log is available at
http://patchew.org/logs/20200702175754.2211821-1-berrange@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200702175754.2211821-1-berrange@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

 (qemu) info snapshots
 No available block device supports snapshots
 (qemu) loadvm snap0
-Error: No block device supports snapshots
+Error: No block device can accept snapshots
 (qemu) quit
 
 
---
 Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
-Error: Device '' is writable but does not support snapshots
+Error: Device 'file' is writable but does not support snapshots
 (qemu) info snapshots
 No available block device supports snapshots
 (qemu) loadvm snap0
-Error: Device '' is writable but does not support snapshots
+Error: Device 'file' is writable but does not support snapshots
 (qemu) quit
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
---
Not run: 259
Failures: 267
Failed 1 of 119 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-aarch64: tests/qtest/test-hmp
  TEST    check-qtest-aarch64: tests/qtest/qos-test
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=80ecabfdc3b44cdabd915b427d791afe', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-vlubojb7/src/docker-src.2020-07-02-14.52.56.31106:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=80ecabfdc3b44cdabd915b427d791afe
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-vlubojb7/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m49.668s
user    0m8.931s


The full log is available at
http://patchew.org/logs/20200702175754.2211821-1-berrange@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/6] migration: improve error reporting of block driver state name

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > With blockdev, a BlockDriverState may not have an device name,
> 
> s/an/a/
> 
> > so using a node name is required as an alternative.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   migration/savevm.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Why don't you send this one to trivial.

Dave

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 6/6] migration: support picking vmstate disk in QMP snapshot commands

[Daniel P. Berrangé]

On Thu, Jul 02, 2020 at 01:19:43PM -0500, Eric Blake wrote:
> On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > This wires up support for a new "vmstate" parameter to the QMP commands
> > for snapshots (savevm, loadvm). This parameter accepts block driver
> > state node name.
> > 
> > One use case for this would be a VM using OVMF firmware where the
> > variables store is the first snapshottable disk image. The vmstate
> > snapshot usually wants to be stored in the primary root disk of the
> > VM, not the firmeware varstore. Thus there needs to be a mechanism
> 
> firmware
> 
> > to override the default choice of disk.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> 
> > +++ b/qapi/migration.json
> > @@ -1630,6 +1630,7 @@
> >   # @tag: name of the snapshot to create. If it already
> >   # exists it will be replaced.
> >   # @exclude: list of block device node names to exclude
> > +# @vmstate: block device node name to save vmstate to
> >   #
> >   # Note that execution of the VM will be paused during the time
> >   # it takes to save the snapshot
> > @@ -1641,6 +1642,7 @@
> >   # -> { "execute": "savevm",
> >   #      "data": {
> >   #         "tag": "my-snap",
> > +#         "vmstate": "disk0",
> >   #         "exclude": ["pflash0-vars"]
> >   #      }
> >   #    }
> > @@ -1650,6 +1652,7 @@
> >   ##
> >   { 'command': 'savevm',
> >     'data': { 'tag': 'str',
> > +            '*vmstate': 'str',
> >               '*exclude': ['str'] } }
> 
> During save, the list of block devices is obvious: everything that is not
> excluded.  But,
> 
> >   ##
> > @@ -1659,6 +1662,7 @@
> >   #
> >   # @tag: name of the snapshot to load.
> >   # @exclude: list of block device node names to exclude
> > +# @vmstate: block device node name to load vmstate from
> >   #
> >   # Returns: nothing
> >   #
> > @@ -1667,6 +1671,7 @@
> >   # -> { "execute": "loadvm",
> >   #      "data": {
> >   #         "tag": "my-snap",
> > +#         "vmstate": "disk0",
> >   #         "exclude": ["pflash0-vars"]
> >   #      }
> >   #    }
> > @@ -1676,6 +1681,7 @@
> >   ##
> >   { 'command': 'loadvm',
> >     'data': { 'tag': 'str',
> > +            '*vmstate': 'str',
> >               '*exclude': ['str'] } }
> 
> ...now that we support exclusion during saving, or even without exclusion
> but when the user has performed hotplug/unplug operations in the meantime
> from when the snapshot was created, isn't load better off listing all
> devices which SHOULD be restored, rather than excluding devices that should
> NOT be restored?  (After all, libvirt knows which disks existed at the time
> of the snapshot, which may be different than the set of disks that exist now
> even though we are throwing out the state now to go back to the state at the
> time of the snapshot)

If the user has hotplugged / unplugged any devices, then I expect the
snapshot load to fail, because the vmstate will be referencing devices
that don't exist, or will be missing devices. Same way migration will
fail unless target QEMU has exact same device setup that was first
serialized into the vmstate

In theory I guess you could have kept device ABI the same, but switched
out disk backends, but I question the sanity of doing that while you have
saved snapshots, unless you're preserving those snapshots in the new
images in which case it will just work.

> Then there's the question of symmetry: if load needs an explicit list of
> blocks to load from (rather than the set of blocks that are currently
> associated with the machine), should save also take its list by positive
> inclusion rather than negative exclusion?

I choose exclusion because the normal case is that you want to snapshot
everything. You sometimes have a small number of exceptions, most notably
the OVMF varstore. IOW if you're currently relying on default behaviour
of snapshotting everything, it is much easier to just exclude one image
and than to switch to explicitly including everything. Essentially I can
just pass a static string associated with the varstore to be excluded,
instead of having to dynamically build up a list of everything.

I wouldn't mind supporting inclusion *and* exclusion, so users have the
choice of which approach to take.

> And why does delvm not need to specify which block is the vmstate? delvm is
> in the same boat as loadvm - the set of blocks involved at the time of the
> snapshot creation may be different than the set of blocks currently
> associated with the guest at the time you run load/delvm.

There's no code in delvm that needs to take any special action wrt
vmstate. It simply deletes snapshots from all the disks present.

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

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > savevm, loadvm and delvm are some of the few commands that have never
> > > been converted to use QMP. The primary reason for this lack of
> > > conversion is that they block execution of the thread for as long as
> > > they run.
> > > 
> > > Despite this downside, however, libvirt and applications using libvirt
> > > has used these commands for as long as QMP has existed, via the
> > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > desirable to be able to fix the blocking problem, this is not an
> > > immediate obstacle to real world usage.
> > > 
> > > Meanwhile there is a need for other features which involve adding new
> > > parameters to the commands. This is possible with HMP passthrough, but
> > > it provides no reliable way for apps to introspect features, so using
> > > QAPI modelling is highly desirable.
> > > 
> > > This patch thus introduces trival savevm, loadvm, delvm commands
> > 
> > trivial
> > 
> > > to QMP that are functionally identical to the HMP counterpart, including
> > > the blocking problem.
> > 
> > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > to change them when we DO solve the blocking issue?  Or will the solution of
> > the blocking issue introduce new QMP commands, at which point we can add QMP
> > deprecation markers on these commands to eventually retire them?
> 
> I was in two minds about this, so I'm open to arguments either way.
> 
> The primary goal is for libvirt to consume the APIs as soon as possible,
> and generally libvirt doesn't want todo this is they are declared experimental
> via a "x-" prefix. So that pushes me away from "x-".
> 
> If we don't have an "x-" prefix and want to make changes, we can add extra
> parameters to trigger new behaviour in backwards compatible manner. Or we can
> simply deprecate these commands, deleting them 2 releases later, while adding
> completely new commands.
> 
> If we think the prposed design will definitely need incompatible changes in
> a very short time frame though, that would push towards "x-".
> 
> So IMHO the right answer largely depends on whether there is a credible
> strategy to implement the ideal non-blocking solution in a reasonable amount
> of time. I can't justify spending much time on this myself, but I'm willing
> to consider & try proposals for solving the blocking problem if they're not
> too complex / invasive.

Remind me, what was the problem with just making a block: migration
channel, and then we can migrate to it?

Dave

> I just don't want to end up having a "x-savevm" command for another 10 years,
> waiting for a perfect solution that never arrives because people always have
> higher priority items, as apps are clearly able to accept the blocking problem
> if the alternative is no snapshots at all.
> 
> 
> > > +
> > > +##
> > > +# @savevm:
> > > +#
> > > +# Save a VM snapshot
> > > +#
> > > +# @tag: name of the snapshot to create. If it already
> > > +# exists it will be replaced.
> > > +#
> > > +# Note that execution of the VM will be paused during the time
> > > +# it takes to save the snapshot
> > > +#
> > > +# Returns: nothing
> > > +#
> > > +# Example:
> > > +#
> > > +# -> { "execute": "savevm",
> > > +#      "data": {
> > > +#         "tag": "my-snap"
> > > +#      }
> > > +#    }
> > > +# <- { "return": { } }
> > > +#
> > > +# Since: 5.2
> > 
> > I guess you are NOT trying to make 5.1 soft freeze next week?
> 
> Correct. It is unrealistic to consider this for soft freeze.
> 
> I'd really love to have a solution in 5.2 though, even if it doesn't
> solve all our problems. Something that can at least unblock apps that
> want to use OVMF with internal snapshots today.
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Daniel P. Berrangé]

On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > been converted to use QMP. The primary reason for this lack of
> > > > conversion is that they block execution of the thread for as long as
> > > > they run.
> > > > 
> > > > Despite this downside, however, libvirt and applications using libvirt
> > > > has used these commands for as long as QMP has existed, via the
> > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > desirable to be able to fix the blocking problem, this is not an
> > > > immediate obstacle to real world usage.
> > > > 
> > > > Meanwhile there is a need for other features which involve adding new
> > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > it provides no reliable way for apps to introspect features, so using
> > > > QAPI modelling is highly desirable.
> > > > 
> > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > 
> > > trivial
> > > 
> > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > the blocking problem.
> > > 
> > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > deprecation markers on these commands to eventually retire them?
> > 
> > I was in two minds about this, so I'm open to arguments either way.
> > 
> > The primary goal is for libvirt to consume the APIs as soon as possible,
> > and generally libvirt doesn't want todo this is they are declared experimental
> > via a "x-" prefix. So that pushes me away from "x-".
> > 
> > If we don't have an "x-" prefix and want to make changes, we can add extra
> > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > simply deprecate these commands, deleting them 2 releases later, while adding
> > completely new commands.
> > 
> > If we think the prposed design will definitely need incompatible changes in
> > a very short time frame though, that would push towards "x-".
> > 
> > So IMHO the right answer largely depends on whether there is a credible
> > strategy to implement the ideal non-blocking solution in a reasonable amount
> > of time. I can't justify spending much time on this myself, but I'm willing
> > to consider & try proposals for solving the blocking problem if they're not
> > too complex / invasive.
> 
> Remind me, what was the problem with just making a block: migration
> channel, and then we can migrate to it?

migration only does vmstate, not disks. The current blockdev commands
are all related to external snapshots, nothing for internal snapshots
AFAIK. So we still need commands to load/save internal snapshots of
the disk data in the qcow2 files.

So we could look at loadvm/savevm conceptually as a syntax sugar above
a migration transport that targets disk images, and blockdev QMP command
that can do internal snapshots. Neither of these exist though and feel
like a significantly larger amount of work than using existing functionality
that is currently working.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > been converted to use QMP. The primary reason for this lack of
> > > > > conversion is that they block execution of the thread for as long as
> > > > > they run.
> > > > > 
> > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > has used these commands for as long as QMP has existed, via the
> > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > immediate obstacle to real world usage.
> > > > > 
> > > > > Meanwhile there is a need for other features which involve adding new
> > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > it provides no reliable way for apps to introspect features, so using
> > > > > QAPI modelling is highly desirable.
> > > > > 
> > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > 
> > > > trivial
> > > > 
> > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > the blocking problem.
> > > > 
> > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > deprecation markers on these commands to eventually retire them?
> > > 
> > > I was in two minds about this, so I'm open to arguments either way.
> > > 
> > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > and generally libvirt doesn't want todo this is they are declared experimental
> > > via a "x-" prefix. So that pushes me away from "x-".
> > > 
> > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > completely new commands.
> > > 
> > > If we think the prposed design will definitely need incompatible changes in
> > > a very short time frame though, that would push towards "x-".
> > > 
> > > So IMHO the right answer largely depends on whether there is a credible
> > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > of time. I can't justify spending much time on this myself, but I'm willing
> > > to consider & try proposals for solving the blocking problem if they're not
> > > too complex / invasive.
> > 
> > Remind me, what was the problem with just making a block: migration
> > channel, and then we can migrate to it?
> 
> migration only does vmstate, not disks. The current blockdev commands
> are all related to external snapshots, nothing for internal snapshots
> AFAIK. So we still need commands to load/save internal snapshots of
> the disk data in the qcow2 files.
> 
> So we could look at loadvm/savevm conceptually as a syntax sugar above
> a migration transport that targets disk images, and blockdev QMP command
> that can do internal snapshots. Neither of these exist though and feel
> like a significantly larger amount of work than using existing functionality
> that is currently working.

I think that's what we should aim for; adding this wrapper isn't gaining
that much without moving a bit towards that; so I would stick with the
x- for now.

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Daniel P. Berrangé]

On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > > been converted to use QMP. The primary reason for this lack of
> > > > > > conversion is that they block execution of the thread for as long as
> > > > > > they run.
> > > > > > 
> > > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > > has used these commands for as long as QMP has existed, via the
> > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > > immediate obstacle to real world usage.
> > > > > > 
> > > > > > Meanwhile there is a need for other features which involve adding new
> > > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > > it provides no reliable way for apps to introspect features, so using
> > > > > > QAPI modelling is highly desirable.
> > > > > > 
> > > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > > 
> > > > > trivial
> > > > > 
> > > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > > the blocking problem.
> > > > > 
> > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > > deprecation markers on these commands to eventually retire them?
> > > > 
> > > > I was in two minds about this, so I'm open to arguments either way.
> > > > 
> > > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > > and generally libvirt doesn't want todo this is they are declared experimental
> > > > via a "x-" prefix. So that pushes me away from "x-".
> > > > 
> > > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > > completely new commands.
> > > > 
> > > > If we think the prposed design will definitely need incompatible changes in
> > > > a very short time frame though, that would push towards "x-".
> > > > 
> > > > So IMHO the right answer largely depends on whether there is a credible
> > > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > > of time. I can't justify spending much time on this myself, but I'm willing
> > > > to consider & try proposals for solving the blocking problem if they're not
> > > > too complex / invasive.
> > > 
> > > Remind me, what was the problem with just making a block: migration
> > > channel, and then we can migrate to it?
> > 
> > migration only does vmstate, not disks. The current blockdev commands
> > are all related to external snapshots, nothing for internal snapshots
> > AFAIK. So we still need commands to load/save internal snapshots of
> > the disk data in the qcow2 files.
> > 
> > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > a migration transport that targets disk images, and blockdev QMP command
> > that can do internal snapshots. Neither of these exist though and feel
> > like a significantly larger amount of work than using existing functionality
> > that is currently working.
> 
> I think that's what we should aim for; adding this wrapper isn't gaining
> that much without moving a bit towards that; so I would stick with the
> x- for now.

The question is how much work that approach will be and whether anyone can
realistically commit to doing that ? It looks like a much larger piece of
work in both QEMU and libvirt side to do that. I don't want to see us stuck
with a x-savevm for years because no one has resource to work on the perfect
solution. If we did get a perfect solution at a point in future, we can
still deprecate and then remove any "savevm" command we add to QMP.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > > > been converted to use QMP. The primary reason for this lack of
> > > > > > > conversion is that they block execution of the thread for as long as
> > > > > > > they run.
> > > > > > > 
> > > > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > > > has used these commands for as long as QMP has existed, via the
> > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > > > immediate obstacle to real world usage.
> > > > > > > 
> > > > > > > Meanwhile there is a need for other features which involve adding new
> > > > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > > > it provides no reliable way for apps to introspect features, so using
> > > > > > > QAPI modelling is highly desirable.
> > > > > > > 
> > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > > > 
> > > > > > trivial
> > > > > > 
> > > > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > > > the blocking problem.
> > > > > > 
> > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > > > deprecation markers on these commands to eventually retire them?
> > > > > 
> > > > > I was in two minds about this, so I'm open to arguments either way.
> > > > > 
> > > > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > > > and generally libvirt doesn't want todo this is they are declared experimental
> > > > > via a "x-" prefix. So that pushes me away from "x-".
> > > > > 
> > > > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > > > completely new commands.
> > > > > 
> > > > > If we think the prposed design will definitely need incompatible changes in
> > > > > a very short time frame though, that would push towards "x-".
> > > > > 
> > > > > So IMHO the right answer largely depends on whether there is a credible
> > > > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > > > of time. I can't justify spending much time on this myself, but I'm willing
> > > > > to consider & try proposals for solving the blocking problem if they're not
> > > > > too complex / invasive.
> > > > 
> > > > Remind me, what was the problem with just making a block: migration
> > > > channel, and then we can migrate to it?
> > > 
> > > migration only does vmstate, not disks. The current blockdev commands
> > > are all related to external snapshots, nothing for internal snapshots
> > > AFAIK. So we still need commands to load/save internal snapshots of
> > > the disk data in the qcow2 files.
> > > 
> > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > a migration transport that targets disk images, and blockdev QMP command
> > > that can do internal snapshots. Neither of these exist though and feel
> > > like a significantly larger amount of work than using existing functionality
> > > that is currently working.
> > 
> > I think that's what we should aim for; adding this wrapper isn't gaining
> > that much without moving a bit towards that; so I would stick with the
> > x- for now.
> 
> The question is how much work that approach will be and whether anyone can
> realistically commit to doing that ? It looks like a much larger piece of
> work in both QEMU and libvirt side to do that. I don't want to see us stuck
> with a x-savevm for years because no one has resource to work on the perfect
> solution. If we did get a perfect solution at a point in future, we can
> still deprecate and then remove any "savevm" command we add to QMP.

I'd at least like to understand that we've got a worklist for it though.
We've already got qemu_fopen_bdrv - what's actually wrong with that - is
that enough to do the migrate to a stream (given a tiny amount of
syntax) ?

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Peter Krempa]

On Fri, Jul 03, 2020 at 17:10:12 +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:

[...]

> > > Remind me, what was the problem with just making a block: migration
> > > channel, and then we can migrate to it?
> > 
> > migration only does vmstate, not disks. The current blockdev commands
> > are all related to external snapshots, nothing for internal snapshots
> > AFAIK. So we still need commands to load/save internal snapshots of
> > the disk data in the qcow2 files.
> > 
> > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > a migration transport that targets disk images, and blockdev QMP command
> > that can do internal snapshots. Neither of these exist though and feel
> > like a significantly larger amount of work than using existing functionality
> > that is currently working.
> 
> I think that's what we should aim for; adding this wrapper isn't gaining
> that much without moving a bit towards that; so I would stick with the
> x- for now.

Relying on the HMP variants is IMO even worse. Error handling is
terrible there. I'd vote even for a straight wrapper without any logic
at this point. IMO it's just necessary to document that it's an
intermediate solution which WILL be deprecated and removed as soon as a
suitable replacement is in place.

Not doing anything is the argument we hear for multiple years now and
savevm/delvm/loadvm are now the only 3 commands used via the HMP wrapper
in libvirt.

Since deprecation is now a thing I think we can add a disposable
inteface. In the end HMP will or will not need to remain anyways and the
deprecation there is IMO less clear.

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Dr. David Alan Gilbert]

* Peter Krempa (pkrempa@redhat.com) wrote:
> On Fri, Jul 03, 2020 at 17:10:12 +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> 
> [...]
> 
> > > > Remind me, what was the problem with just making a block: migration
> > > > channel, and then we can migrate to it?
> > > 
> > > migration only does vmstate, not disks. The current blockdev commands
> > > are all related to external snapshots, nothing for internal snapshots
> > > AFAIK. So we still need commands to load/save internal snapshots of
> > > the disk data in the qcow2 files.
> > > 
> > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > a migration transport that targets disk images, and blockdev QMP command
> > > that can do internal snapshots. Neither of these exist though and feel
> > > like a significantly larger amount of work than using existing functionality
> > > that is currently working.
> > 
> > I think that's what we should aim for; adding this wrapper isn't gaining
> > that much without moving a bit towards that; so I would stick with the
> > x- for now.
> 
> Relying on the HMP variants is IMO even worse. Error handling is
> terrible there. I'd vote even for a straight wrapper without any logic
> at this point. IMO it's just necessary to document that it's an
> intermediate solution which WILL be deprecated and removed as soon as a
> suitable replacement is in place.
> 
> Not doing anything is the argument we hear for multiple years now and
> savevm/delvm/loadvm are now the only 3 commands used via the HMP wrapper
> in libvirt.
> 
> Since deprecation is now a thing I think we can add a disposable
> inteface. In the end HMP will or will not need to remain anyways and the
> deprecation there is IMO less clear.

Only if we come up with a list of what we actually need to do to
properly fix it; I'm not suggesting we actually need to do the work, but
we should figure out what we need to do.

Dave

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

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Daniel P. Berrangé]

On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > > > > been converted to use QMP. The primary reason for this lack of
> > > > > > > > conversion is that they block execution of the thread for as long as
> > > > > > > > they run.
> > > > > > > > 
> > > > > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > > > > has used these commands for as long as QMP has existed, via the
> > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > > > > immediate obstacle to real world usage.
> > > > > > > > 
> > > > > > > > Meanwhile there is a need for other features which involve adding new
> > > > > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > > > > it provides no reliable way for apps to introspect features, so using
> > > > > > > > QAPI modelling is highly desirable.
> > > > > > > > 
> > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > > > > 
> > > > > > > trivial
> > > > > > > 
> > > > > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > > > > the blocking problem.
> > > > > > > 
> > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > > > > deprecation markers on these commands to eventually retire them?
> > > > > > 
> > > > > > I was in two minds about this, so I'm open to arguments either way.
> > > > > > 
> > > > > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > > > > and generally libvirt doesn't want todo this is they are declared experimental
> > > > > > via a "x-" prefix. So that pushes me away from "x-".
> > > > > > 
> > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > > > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > > > > completely new commands.
> > > > > > 
> > > > > > If we think the prposed design will definitely need incompatible changes in
> > > > > > a very short time frame though, that would push towards "x-".
> > > > > > 
> > > > > > So IMHO the right answer largely depends on whether there is a credible
> > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > > > > of time. I can't justify spending much time on this myself, but I'm willing
> > > > > > to consider & try proposals for solving the blocking problem if they're not
> > > > > > too complex / invasive.
> > > > > 
> > > > > Remind me, what was the problem with just making a block: migration
> > > > > channel, and then we can migrate to it?
> > > > 
> > > > migration only does vmstate, not disks. The current blockdev commands
> > > > are all related to external snapshots, nothing for internal snapshots
> > > > AFAIK. So we still need commands to load/save internal snapshots of
> > > > the disk data in the qcow2 files.
> > > > 
> > > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > > a migration transport that targets disk images, and blockdev QMP command
> > > > that can do internal snapshots. Neither of these exist though and feel
> > > > like a significantly larger amount of work than using existing functionality
> > > > that is currently working.
> > > 
> > > I think that's what we should aim for; adding this wrapper isn't gaining
> > > that much without moving a bit towards that; so I would stick with the
> > > x- for now.
> > 
> > The question is how much work that approach will be and whether anyone can
> > realistically commit to doing that ? It looks like a much larger piece of
> > work in both QEMU and libvirt side to do that. I don't want to see us stuck
> > with a x-savevm for years because no one has resource to work on the perfect
> > solution. If we did get a perfect solution at a point in future, we can
> > still deprecate and then remove any "savevm" command we add to QMP.
> 
> I'd at least like to understand that we've got a worklist for it though.
> We've already got qemu_fopen_bdrv - what's actually wrong with that - is
> that enough to do the migrate to a stream (given a tiny amount of
> syntax) ?

It is close. The migration code works with the QEMUFile layer, but in terms
of the monitor commands the current framework expects a QIOChannel based
QEMUFile. It would be possible to add new helpers to work with the bdrv
backed QEMUFile. The ideal would be to create a QIOChannel impl that is
backed by a block device though. At that point there would only be a single
QEMUFile impl based on QIOChannel. 

That would be another step closer to unlocking the ability to eliminate the
QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance,
that could be done in a QIOChannel layer too, as that's a concept useful
for other QIOChannel users too.

That's still only the vmstate part though, and a solution is needed for
the internal snapshot handling.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > > > > > been converted to use QMP. The primary reason for this lack of
> > > > > > > > > conversion is that they block execution of the thread for as long as
> > > > > > > > > they run.
> > > > > > > > > 
> > > > > > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > > > > > has used these commands for as long as QMP has existed, via the
> > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > > > > > immediate obstacle to real world usage.
> > > > > > > > > 
> > > > > > > > > Meanwhile there is a need for other features which involve adding new
> > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > > > > > it provides no reliable way for apps to introspect features, so using
> > > > > > > > > QAPI modelling is highly desirable.
> > > > > > > > > 
> > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > > > > > 
> > > > > > > > trivial
> > > > > > > > 
> > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > > > > > the blocking problem.
> > > > > > > > 
> > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > > > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > > > > > deprecation markers on these commands to eventually retire them?
> > > > > > > 
> > > > > > > I was in two minds about this, so I'm open to arguments either way.
> > > > > > > 
> > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > > > > > and generally libvirt doesn't want todo this is they are declared experimental
> > > > > > > via a "x-" prefix. So that pushes me away from "x-".
> > > > > > > 
> > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > > > > > completely new commands.
> > > > > > > 
> > > > > > > If we think the prposed design will definitely need incompatible changes in
> > > > > > > a very short time frame though, that would push towards "x-".
> > > > > > > 
> > > > > > > So IMHO the right answer largely depends on whether there is a credible
> > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > > > > > of time. I can't justify spending much time on this myself, but I'm willing
> > > > > > > to consider & try proposals for solving the blocking problem if they're not
> > > > > > > too complex / invasive.
> > > > > > 
> > > > > > Remind me, what was the problem with just making a block: migration
> > > > > > channel, and then we can migrate to it?
> > > > > 
> > > > > migration only does vmstate, not disks. The current blockdev commands
> > > > > are all related to external snapshots, nothing for internal snapshots
> > > > > AFAIK. So we still need commands to load/save internal snapshots of
> > > > > the disk data in the qcow2 files.
> > > > > 
> > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > > > a migration transport that targets disk images, and blockdev QMP command
> > > > > that can do internal snapshots. Neither of these exist though and feel
> > > > > like a significantly larger amount of work than using existing functionality
> > > > > that is currently working.
> > > > 
> > > > I think that's what we should aim for; adding this wrapper isn't gaining
> > > > that much without moving a bit towards that; so I would stick with the
> > > > x- for now.
> > > 
> > > The question is how much work that approach will be and whether anyone can
> > > realistically commit to doing that ? It looks like a much larger piece of
> > > work in both QEMU and libvirt side to do that. I don't want to see us stuck
> > > with a x-savevm for years because no one has resource to work on the perfect
> > > solution. If we did get a perfect solution at a point in future, we can
> > > still deprecate and then remove any "savevm" command we add to QMP.
> > 
> > I'd at least like to understand that we've got a worklist for it though.
> > We've already got qemu_fopen_bdrv - what's actually wrong with that - is
> > that enough to do the migrate to a stream (given a tiny amount of
> > syntax) ?
> 
> It is close. The migration code works with the QEMUFile layer, but in terms
> of the monitor commands the current framework expects a QIOChannel based
> QEMUFile. It would be possible to add new helpers to work with the bdrv
> backed QEMUFile. The ideal would be to create a QIOChannel impl that is
> backed by a block device though. At that point there would only be a single
> QEMUFile impl based on QIOChannel. 
> 
> That would be another step closer to unlocking the ability to eliminate the
> QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance,
> that could be done in a QIOChannel layer too, as that's a concept useful
> for other QIOChannel users too.

There's some separate patches on list to do buffering in bdrv for
vmsave because apparently the qemufile ones don't play well with it.

> That's still only the vmstate part though, and a solution is needed for
> the internal snapshot handling.

Which bit is the bit that makes it block?

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Daniel P. Berrangé]

On Fri, Jul 03, 2020 at 06:00:50PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > > > > > > been converted to use QMP. The primary reason for this lack of
> > > > > > > > > > conversion is that they block execution of the thread for as long as
> > > > > > > > > > they run.
> > > > > > > > > > 
> > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > > > > > > has used these commands for as long as QMP has existed, via the
> > > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > > > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > > > > > > immediate obstacle to real world usage.
> > > > > > > > > > 
> > > > > > > > > > Meanwhile there is a need for other features which involve adding new
> > > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > > > > > > it provides no reliable way for apps to introspect features, so using
> > > > > > > > > > QAPI modelling is highly desirable.
> > > > > > > > > > 
> > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > > > > > > 
> > > > > > > > > trivial
> > > > > > > > > 
> > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > > > > > > the blocking problem.
> > > > > > > > > 
> > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > > > > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > > > > > > deprecation markers on these commands to eventually retire them?
> > > > > > > > 
> > > > > > > > I was in two minds about this, so I'm open to arguments either way.
> > > > > > > > 
> > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > > > > > > and generally libvirt doesn't want todo this is they are declared experimental
> > > > > > > > via a "x-" prefix. So that pushes me away from "x-".
> > > > > > > > 
> > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > > > > > > completely new commands.
> > > > > > > > 
> > > > > > > > If we think the prposed design will definitely need incompatible changes in
> > > > > > > > a very short time frame though, that would push towards "x-".
> > > > > > > > 
> > > > > > > > So IMHO the right answer largely depends on whether there is a credible
> > > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > > > > > > of time. I can't justify spending much time on this myself, but I'm willing
> > > > > > > > to consider & try proposals for solving the blocking problem if they're not
> > > > > > > > too complex / invasive.
> > > > > > > 
> > > > > > > Remind me, what was the problem with just making a block: migration
> > > > > > > channel, and then we can migrate to it?
> > > > > > 
> > > > > > migration only does vmstate, not disks. The current blockdev commands
> > > > > > are all related to external snapshots, nothing for internal snapshots
> > > > > > AFAIK. So we still need commands to load/save internal snapshots of
> > > > > > the disk data in the qcow2 files.
> > > > > > 
> > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > > > > a migration transport that targets disk images, and blockdev QMP command
> > > > > > that can do internal snapshots. Neither of these exist though and feel
> > > > > > like a significantly larger amount of work than using existing functionality
> > > > > > that is currently working.
> > > > > 
> > > > > I think that's what we should aim for; adding this wrapper isn't gaining
> > > > > that much without moving a bit towards that; so I would stick with the
> > > > > x- for now.
> > > > 
> > > > The question is how much work that approach will be and whether anyone can
> > > > realistically commit to doing that ? It looks like a much larger piece of
> > > > work in both QEMU and libvirt side to do that. I don't want to see us stuck
> > > > with a x-savevm for years because no one has resource to work on the perfect
> > > > solution. If we did get a perfect solution at a point in future, we can
> > > > still deprecate and then remove any "savevm" command we add to QMP.
> > > 
> > > I'd at least like to understand that we've got a worklist for it though.
> > > We've already got qemu_fopen_bdrv - what's actually wrong with that - is
> > > that enough to do the migrate to a stream (given a tiny amount of
> > > syntax) ?
> > 
> > It is close. The migration code works with the QEMUFile layer, but in terms
> > of the monitor commands the current framework expects a QIOChannel based
> > QEMUFile. It would be possible to add new helpers to work with the bdrv
> > backed QEMUFile. The ideal would be to create a QIOChannel impl that is
> > backed by a block device though. At that point there would only be a single
> > QEMUFile impl based on QIOChannel. 
> > 
> > That would be another step closer to unlocking the ability to eliminate the
> > QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance,
> > that could be done in a QIOChannel layer too, as that's a concept useful
> > for other QIOChannel users too.
> 
> There's some separate patches on list to do buffering in bdrv for
> vmsave because apparently the qemufile ones don't play well with it.
> 
> > That's still only the vmstate part though, and a solution is needed for
> > the internal snapshot handling.
> 
> Which bit is the bit that makes it block?

The entire  save_snapshot / load_snapshot methods really. They doing I/O
operations throughout and this executes in context of the thread running
the monitor, so their execution time is proportional to I/O time.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Denis V. Lunev]

On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> When QMP was first introduced some 10+ years ago now, the snapshot
> related commands (savevm/loadvm/delvm) were not converted. This was
> primarily because their implementation causes blocking of the thread
> running the monitor commands. This was (and still is) considered
> undesirable behaviour both in HMP and QMP.
>
> In theory someone was supposed to fix this flaw at some point in the
> past 10 years and bring them into the QMP world. Sadly, thus far it
> hasn't happened as people always had more important things to work
> on. Enterprise apps were much more interested in external snapshots
> than internal snapshots as they have many more features.
>
> Meanwhile users still want to use internal snapshots as there is
> a certainly simplicity in having everything self-contained in one
> image, even though it has limitations. Thus the apps that end up
> executing the savevm/loadvm/delvm via the "human-monitor-command"
> QMP command.
>
>
> IOW, the problematic blocking behaviour that was one of the reasons
> for not having savevm/loadvm/delvm in QMP is experienced by applications
> regardless. By not portting the commands to QMP due to one design flaw,
> we've forced apps and users to suffer from other design flaws of HMP (
> bad error reporting, strong type checking of args, no introspection) for
> an additional 10 years. This feels rather sub-optimal :-(
>
> In practice users don't appear to care strongly about the fact that these
> commands block the VM while they run. I might have seen one bug report
> about it, but it certainly isn't something that comes up as a frequent
> topic except among us QEMU maintainers. Users do care about having
> access to the snapshot feature.
>
> Where I am seeing frequent complaints is wrt the use of OVMF combined
> with snapshots which has some serious pain points. This is getting worse
> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> across OS vendors and mgmt apps. Solving it requires new parameters to
> the commands, but doing this in HMP is super unappealing.
>
>
>
> After 10 years, I think it is time for us to be a little pragmatic about
> our handling of snapshots commands. My desire is that libvirt should never
> use "human-monitor-command" under any circumstances, because of the
> inherant flaws in HMP as a protocol for machine consumption. If there
> are flaws in QMP commands that's fine. If we fix them in future, we can
> deprecate the current QMP commands and remove them not too long after,
> without being locked in forever.
>
>
> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> not solve the blocking thread problem, but it does eliminate the error
> reporting, type checking and introspection problems inherant to HMP.
> We're winning on 3 out of the 4 long term problems.
>
> If someone can suggest a easy way to fix the thread blocking problem
> too, I'd be interested to hear it. If it involves a major refactoring
> then I think user are better served by unlocking what look like easy
> wins today.
>
> With a QMP variant, we reasonably deal with the problems related to OVMF:
>
>  - The logic to pick which disk to store the vmstate in is not
>    satsifactory.
>
>    The first block driver state cannot be assumed to be the root disk
>    image, it might be OVMF varstore and we don't want to store vmstate
>    in there.
>
>  - The logic to decide which disks must be snapshotted is hardwired
>    to all disks which are writable
>
>    Again with OVMF there might be a writable varstore, but this can be
>    raw rather than qcow2 format, and thus unable to be snapshotted.
>    While users might wish to snapshot their varstore, in some/many/most
>    cases it is entirely uneccessary. Users are blocked from snapshotting
>    their VM though due to this varstore.
>
> These are solved by adding two parameters to the commands. The first is
> a block device node name that identifies the image to store vmstate in,
> and the second is a list of node names to exclude from snapshots.
>
> In the block code I've only dealt with node names for block devices, as
> IIUC, this is all that libvirt should need in the -blockdev world it now
> lives in. IOW, I've made not attempt to cope with people wanting to use
> these QMP commands in combination with -drive args.
>
> I've done some minimal work in libvirt to start to make use of the new
> commands to validate their functionality, but this isn't finished yet.
>
> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> having internal snapshots work with OVMF:
>
>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> f45c5f64048f16a6e
>
> Daniel P. Berrang=C3=A9 (6):
>   migration: improve error reporting of block driver state name
>   migration: introduce savevm, loadvm, delvm QMP commands
>   block: add ability to filter out blockdevs during snapshot
>   block: allow specifying name of block device for vmstate storage
>   migration: support excluding block devs in QMP snapshot commands
>   migration: support picking vmstate disk in QMP snapshot commands
>
>  block/monitor/block-hmp-cmds.c |  4 +-
>  block/snapshot.c               | 68 +++++++++++++++++++------
>  include/block/snapshot.h       | 21 +++++---
>  include/migration/snapshot.h   | 10 +++-
>  migration/savevm.c             | 71 +++++++++++++++++++-------
>  monitor/hmp-cmds.c             | 20 ++------
>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
>  replay/replay-snapshot.c       |  4 +-
>  softmmu/vl.c                   |  2 +-
>  9 files changed, 228 insertions(+), 63 deletions(-)
>
> --=20
> 2.26.2
>
>
>
I have tried to work in this interface in 2016. That time
we have struggled with the idea that this QMP interface should
be ready to work asynchronously.

Write-protect userfaultfd was merged into vanilla Linux
thus it is time to async savevm interface, which will also
bring async loadvm and some rework for state storing.

Thus I think that with the introduction of the QMP interface
we should at least run save VM not from the main
thread but from the background with the event at the end.

Den

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> > When QMP was first introduced some 10+ years ago now, the snapshot
> > related commands (savevm/loadvm/delvm) were not converted. This was
> > primarily because their implementation causes blocking of the thread
> > running the monitor commands. This was (and still is) considered
> > undesirable behaviour both in HMP and QMP.
> >
> > In theory someone was supposed to fix this flaw at some point in the
> > past 10 years and bring them into the QMP world. Sadly, thus far it
> > hasn't happened as people always had more important things to work
> > on. Enterprise apps were much more interested in external snapshots
> > than internal snapshots as they have many more features.
> >
> > Meanwhile users still want to use internal snapshots as there is
> > a certainly simplicity in having everything self-contained in one
> > image, even though it has limitations. Thus the apps that end up
> > executing the savevm/loadvm/delvm via the "human-monitor-command"
> > QMP command.
> >
> >
> > IOW, the problematic blocking behaviour that was one of the reasons
> > for not having savevm/loadvm/delvm in QMP is experienced by applications
> > regardless. By not portting the commands to QMP due to one design flaw,
> > we've forced apps and users to suffer from other design flaws of HMP (
> > bad error reporting, strong type checking of args, no introspection) for
> > an additional 10 years. This feels rather sub-optimal :-(
> >
> > In practice users don't appear to care strongly about the fact that these
> > commands block the VM while they run. I might have seen one bug report
> > about it, but it certainly isn't something that comes up as a frequent
> > topic except among us QEMU maintainers. Users do care about having
> > access to the snapshot feature.
> >
> > Where I am seeing frequent complaints is wrt the use of OVMF combined
> > with snapshots which has some serious pain points. This is getting worse
> > as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> > across OS vendors and mgmt apps. Solving it requires new parameters to
> > the commands, but doing this in HMP is super unappealing.
> >
> >
> >
> > After 10 years, I think it is time for us to be a little pragmatic about
> > our handling of snapshots commands. My desire is that libvirt should never
> > use "human-monitor-command" under any circumstances, because of the
> > inherant flaws in HMP as a protocol for machine consumption. If there
> > are flaws in QMP commands that's fine. If we fix them in future, we can
> > deprecate the current QMP commands and remove them not too long after,
> > without being locked in forever.
> >
> >
> > Thus in this series I'm proposing a direct 1-1 mapping of the existing
> > HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> > not solve the blocking thread problem, but it does eliminate the error
> > reporting, type checking and introspection problems inherant to HMP.
> > We're winning on 3 out of the 4 long term problems.
> >
> > If someone can suggest a easy way to fix the thread blocking problem
> > too, I'd be interested to hear it. If it involves a major refactoring
> > then I think user are better served by unlocking what look like easy
> > wins today.
> >
> > With a QMP variant, we reasonably deal with the problems related to OVMF:
> >
> >  - The logic to pick which disk to store the vmstate in is not
> >    satsifactory.
> >
> >    The first block driver state cannot be assumed to be the root disk
> >    image, it might be OVMF varstore and we don't want to store vmstate
> >    in there.
> >
> >  - The logic to decide which disks must be snapshotted is hardwired
> >    to all disks which are writable
> >
> >    Again with OVMF there might be a writable varstore, but this can be
> >    raw rather than qcow2 format, and thus unable to be snapshotted.
> >    While users might wish to snapshot their varstore, in some/many/most
> >    cases it is entirely uneccessary. Users are blocked from snapshotting
> >    their VM though due to this varstore.
> >
> > These are solved by adding two parameters to the commands. The first is
> > a block device node name that identifies the image to store vmstate in,
> > and the second is a list of node names to exclude from snapshots.
> >
> > In the block code I've only dealt with node names for block devices, as
> > IIUC, this is all that libvirt should need in the -blockdev world it now
> > lives in. IOW, I've made not attempt to cope with people wanting to use
> > these QMP commands in combination with -drive args.
> >
> > I've done some minimal work in libvirt to start to make use of the new
> > commands to validate their functionality, but this isn't finished yet.
> >
> > My ultimate goal is to make the GNOME Boxes maintainer happy again by
> > having internal snapshots work with OVMF:
> >
> >   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> > f45c5f64048f16a6e
> >
> > Daniel P. Berrang=C3=A9 (6):
> >   migration: improve error reporting of block driver state name
> >   migration: introduce savevm, loadvm, delvm QMP commands
> >   block: add ability to filter out blockdevs during snapshot
> >   block: allow specifying name of block device for vmstate storage
> >   migration: support excluding block devs in QMP snapshot commands
> >   migration: support picking vmstate disk in QMP snapshot commands
> >
> >  block/monitor/block-hmp-cmds.c |  4 +-
> >  block/snapshot.c               | 68 +++++++++++++++++++------
> >  include/block/snapshot.h       | 21 +++++---
> >  include/migration/snapshot.h   | 10 +++-
> >  migration/savevm.c             | 71 +++++++++++++++++++-------
> >  monitor/hmp-cmds.c             | 20 ++------
> >  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> >  replay/replay-snapshot.c       |  4 +-
> >  softmmu/vl.c                   |  2 +-
> >  9 files changed, 228 insertions(+), 63 deletions(-)
> 
> I have tried to work in this interface in 2016. That time
> we have struggled with the idea that this QMP interface should
> be ready to work asynchronously.
> 
> Write-protect userfaultfd was merged into vanilla Linux
> thus it is time to async savevm interface, which will also
> bring async loadvm and some rework for state storing.
> 
> Thus I think that with the introduction of the QMP interface
> we should at least run save VM not from the main
> thread but from the background with the event at the end.

spawning a thread in which to invoke save_snapshot() and load_snapshot()
is easy enough.  I'm not at all clear on what we need in the way of
mutex locking though, to make those methods safe to run in a thread
that isn't the main event loop.

Even with savevm/loadvm being blocking, we could introduce a QMP event
straight away, and document that users shouldn't assume the operation
is complete until they see the event. That would let us make the commands
non-blocking later with same documented semantics.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Denis V. Lunev]

On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> savevm, loadvm and delvm are some of the few commands that have never
> been converted to use QMP. The primary reason for this lack of
> conversion is that they block execution of the thread for as long as
> they run.
>
> Despite this downside, however, libvirt and applications using libvirt
> has used these commands for as long as QMP has existed, via the
> "human-monitor-command" passthrough command. IOW, while it is clearly
> desirable to be able to fix the blocking problem, this is not an
> immediate obstacle to real world usage.
>
> Meanwhile there is a need for other features which involve adding new
> parameters to the commands. This is possible with HMP passthrough, but
> it provides no reliable way for apps to introspect features, so using
> QAPI modelling is highly desirable.
>
> This patch thus introduces trival savevm, loadvm, delvm commands
> to QMP that are functionally identical to the HMP counterpart, including
> the blocking problem.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  migration/savevm.c  | 27 ++++++++++++++++
>  monitor/hmp-cmds.c  | 18 ++---------
>  qapi/migration.json | 76 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 106 insertions(+), 15 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 72dbad95ed..53586a6406 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2943,3 +2943,30 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
>  
>      return !(vmsd && vmsd->unmigratable);
>  }
> +
> +void qmp_savevm(const char *tag, Error **errp)
> +{
> +    save_snapshot(tag, errp);
> +}
> +
> +void qmp_loadvm(const char *tag, Error **errp)
> +{
> +    int saved_vm_running  = runstate_is_running();
> +
> +    vm_stop(RUN_STATE_RESTORE_VM);
> +
> +    if (load_snapshot(tag, errp) == 0 && saved_vm_running) {
> +        vm_start();
> +    }
> +}
> +
> +void qmp_delvm(const char *tag, Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    if (bdrv_all_delete_snapshot(tag, &bs, errp) < 0) {
> +        error_prepend(errp,
> +                      "deleting snapshot on device '%s': ",
> +                      bdrv_get_device_or_node_name(bs));
> +    }
> +}
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 2b0b58a336..26a5a1a701 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1089,15 +1089,9 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
>  
>  void hmp_loadvm(Monitor *mon, const QDict *qdict)
>  {
> -    int saved_vm_running  = runstate_is_running();
> -    const char *name = qdict_get_str(qdict, "name");
>      Error *err = NULL;
>  
> -    vm_stop(RUN_STATE_RESTORE_VM);
> -
> -    if (load_snapshot(name, &err) == 0 && saved_vm_running) {
> -        vm_start();
> -    }
> +    qmp_loadvm(qdict_get_str(qdict, "name"), &err);
>      hmp_handle_error(mon, err);
>  }
>  
> @@ -1105,21 +1099,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>  
> -    save_snapshot(qdict_get_try_str(qdict, "name"), &err);
> +    qmp_savevm(qdict_get_try_str(qdict, "name"), &err);
>      hmp_handle_error(mon, err);
>  }
>  
>  void hmp_delvm(Monitor *mon, const QDict *qdict)
>  {
> -    BlockDriverState *bs;
>      Error *err = NULL;
> -    const char *name = qdict_get_str(qdict, "name");
>  
> -    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> -        error_prepend(&err,
> -                      "deleting snapshot on device '%s': ",
> -                      bdrv_get_device_name(bs));
> -    }
> +    qmp_delvm(qdict_get_str(qdict, "name"), &err);
>      hmp_handle_error(mon, err);
>  }
>  
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..849de38fb0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1621,3 +1621,79 @@
>  ##
>  { 'event': 'UNPLUG_PRIMARY',
>    'data': { 'device-id': 'str' } }
> +
> +##
> +# @savevm:
> +#
> +# Save a VM snapshot
> +#
> +# @tag: name of the snapshot to create. If it already
> +# exists it will be replaced.
> +#
> +# Note that execution of the VM will be paused during the time
> +# it takes to save the snapshot
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "savevm",
> +#      "data": {
> +#         "tag": "my-snap"
> +#      }
> +#    }
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'savevm',
> +  'data': { 'tag': 'str' } }
> +
> +##
> +# @loadvm:
> +#
> +# Load a VM snapshot
> +#
> +# @tag: name of the snapshot to load.
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "loadvm",
> +#      "data": {
> +#         "tag": "my-snap"
> +#      }
> +#    }
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'loadvm',
> +  'data': { 'tag': 'str' } }
> +
> +##
> +# @delvm:
> +#
> +# Delete a VM snapshot
> +#
> +# @tag: name of the snapshot to delete.
> +#
> +# Note that execution of the VM will be paused during the time
> +# it takes to delete the snapshot
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "delvm",
> +#      "data": {
> +#         "tag": "my-snap"
> +#      }
> +#    }
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'delvm',
> +  'data': { 'tag': 'str' } }
The interface proposed is not good from the interface point of view.
The problem is that savevm and loadvm are VERY lengthy operations
especially for VM with big amount of RAM. Thus they can run
for hours f.e. with 1 Tb RAM.

This "feature" was available in the old legacy HMP interface, but
this should be fixed in the new and clear one.

Den

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Jul 03, 2020 at 06:00:50PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote:
> > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > > > > > > > been converted to use QMP. The primary reason for this lack of
> > > > > > > > > > > conversion is that they block execution of the thread for as long as
> > > > > > > > > > > they run.
> > > > > > > > > > > 
> > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > > > > > > > has used these commands for as long as QMP has existed, via the
> > > > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > > > > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > > > > > > > immediate obstacle to real world usage.
> > > > > > > > > > > 
> > > > > > > > > > > Meanwhile there is a need for other features which involve adding new
> > > > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > > > > > > > it provides no reliable way for apps to introspect features, so using
> > > > > > > > > > > QAPI modelling is highly desirable.
> > > > > > > > > > > 
> > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > > > > > > > 
> > > > > > > > > > trivial
> > > > > > > > > > 
> > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > > > > > > > the blocking problem.
> > > > > > > > > > 
> > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > > > > > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > > > > > > > deprecation markers on these commands to eventually retire them?
> > > > > > > > > 
> > > > > > > > > I was in two minds about this, so I'm open to arguments either way.
> > > > > > > > > 
> > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > > > > > > > and generally libvirt doesn't want todo this is they are declared experimental
> > > > > > > > > via a "x-" prefix. So that pushes me away from "x-".
> > > > > > > > > 
> > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > > > > > > > completely new commands.
> > > > > > > > > 
> > > > > > > > > If we think the prposed design will definitely need incompatible changes in
> > > > > > > > > a very short time frame though, that would push towards "x-".
> > > > > > > > > 
> > > > > > > > > So IMHO the right answer largely depends on whether there is a credible
> > > > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > > > > > > > of time. I can't justify spending much time on this myself, but I'm willing
> > > > > > > > > to consider & try proposals for solving the blocking problem if they're not
> > > > > > > > > too complex / invasive.
> > > > > > > > 
> > > > > > > > Remind me, what was the problem with just making a block: migration
> > > > > > > > channel, and then we can migrate to it?
> > > > > > > 
> > > > > > > migration only does vmstate, not disks. The current blockdev commands
> > > > > > > are all related to external snapshots, nothing for internal snapshots
> > > > > > > AFAIK. So we still need commands to load/save internal snapshots of
> > > > > > > the disk data in the qcow2 files.
> > > > > > > 
> > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > > > > > a migration transport that targets disk images, and blockdev QMP command
> > > > > > > that can do internal snapshots. Neither of these exist though and feel
> > > > > > > like a significantly larger amount of work than using existing functionality
> > > > > > > that is currently working.
> > > > > > 
> > > > > > I think that's what we should aim for; adding this wrapper isn't gaining
> > > > > > that much without moving a bit towards that; so I would stick with the
> > > > > > x- for now.
> > > > > 
> > > > > The question is how much work that approach will be and whether anyone can
> > > > > realistically commit to doing that ? It looks like a much larger piece of
> > > > > work in both QEMU and libvirt side to do that. I don't want to see us stuck
> > > > > with a x-savevm for years because no one has resource to work on the perfect
> > > > > solution. If we did get a perfect solution at a point in future, we can
> > > > > still deprecate and then remove any "savevm" command we add to QMP.
> > > > 
> > > > I'd at least like to understand that we've got a worklist for it though.
> > > > We've already got qemu_fopen_bdrv - what's actually wrong with that - is
> > > > that enough to do the migrate to a stream (given a tiny amount of
> > > > syntax) ?
> > > 
> > > It is close. The migration code works with the QEMUFile layer, but in terms
> > > of the monitor commands the current framework expects a QIOChannel based
> > > QEMUFile. It would be possible to add new helpers to work with the bdrv
> > > backed QEMUFile. The ideal would be to create a QIOChannel impl that is
> > > backed by a block device though. At that point there would only be a single
> > > QEMUFile impl based on QIOChannel. 
> > > 
> > > That would be another step closer to unlocking the ability to eliminate the
> > > QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance,
> > > that could be done in a QIOChannel layer too, as that's a concept useful
> > > for other QIOChannel users too.
> > 
> > There's some separate patches on list to do buffering in bdrv for
> > vmsave because apparently the qemufile ones don't play well with it.
> > 
> > > That's still only the vmstate part though, and a solution is needed for
> > > the internal snapshot handling.
> > 
> > Which bit is the bit that makes it block?
> 
> The entire  save_snapshot / load_snapshot methods really. They doing I/O
> operations throughout and this executes in context of the thread running
> the monitor, so their execution time is proportional to I/O time.

Is there any reason for the migrationy bit to run synchronously then?

Could those snapshotting things be done with any of the block copy
processes that run asynchronously?

Dave


> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Denis V. Lunev]

On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
>> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
>>> When QMP was first introduced some 10+ years ago now, the snapshot
>>> related commands (savevm/loadvm/delvm) were not converted. This was
>>> primarily because their implementation causes blocking of the thread
>>> running the monitor commands. This was (and still is) considered
>>> undesirable behaviour both in HMP and QMP.
>>>
>>> In theory someone was supposed to fix this flaw at some point in the
>>> past 10 years and bring them into the QMP world. Sadly, thus far it
>>> hasn't happened as people always had more important things to work
>>> on. Enterprise apps were much more interested in external snapshots
>>> than internal snapshots as they have many more features.
>>>
>>> Meanwhile users still want to use internal snapshots as there is
>>> a certainly simplicity in having everything self-contained in one
>>> image, even though it has limitations. Thus the apps that end up
>>> executing the savevm/loadvm/delvm via the "human-monitor-command"
>>> QMP command.
>>>
>>>
>>> IOW, the problematic blocking behaviour that was one of the reasons
>>> for not having savevm/loadvm/delvm in QMP is experienced by applications
>>> regardless. By not portting the commands to QMP due to one design flaw,
>>> we've forced apps and users to suffer from other design flaws of HMP (
>>> bad error reporting, strong type checking of args, no introspection) for
>>> an additional 10 years. This feels rather sub-optimal :-(
>>>
>>> In practice users don't appear to care strongly about the fact that these
>>> commands block the VM while they run. I might have seen one bug report
>>> about it, but it certainly isn't something that comes up as a frequent
>>> topic except among us QEMU maintainers. Users do care about having
>>> access to the snapshot feature.
>>>
>>> Where I am seeing frequent complaints is wrt the use of OVMF combined
>>> with snapshots which has some serious pain points. This is getting worse
>>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
>>> across OS vendors and mgmt apps. Solving it requires new parameters to
>>> the commands, but doing this in HMP is super unappealing.
>>>
>>>
>>>
>>> After 10 years, I think it is time for us to be a little pragmatic about
>>> our handling of snapshots commands. My desire is that libvirt should never
>>> use "human-monitor-command" under any circumstances, because of the
>>> inherant flaws in HMP as a protocol for machine consumption. If there
>>> are flaws in QMP commands that's fine. If we fix them in future, we can
>>> deprecate the current QMP commands and remove them not too long after,
>>> without being locked in forever.
>>>
>>>
>>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
>>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
>>> not solve the blocking thread problem, but it does eliminate the error
>>> reporting, type checking and introspection problems inherant to HMP.
>>> We're winning on 3 out of the 4 long term problems.
>>>
>>> If someone can suggest a easy way to fix the thread blocking problem
>>> too, I'd be interested to hear it. If it involves a major refactoring
>>> then I think user are better served by unlocking what look like easy
>>> wins today.
>>>
>>> With a QMP variant, we reasonably deal with the problems related to OVMF:
>>>
>>>  - The logic to pick which disk to store the vmstate in is not
>>>    satsifactory.
>>>
>>>    The first block driver state cannot be assumed to be the root disk
>>>    image, it might be OVMF varstore and we don't want to store vmstate
>>>    in there.
>>>
>>>  - The logic to decide which disks must be snapshotted is hardwired
>>>    to all disks which are writable
>>>
>>>    Again with OVMF there might be a writable varstore, but this can be
>>>    raw rather than qcow2 format, and thus unable to be snapshotted.
>>>    While users might wish to snapshot their varstore, in some/many/most
>>>    cases it is entirely uneccessary. Users are blocked from snapshotting
>>>    their VM though due to this varstore.
>>>
>>> These are solved by adding two parameters to the commands. The first is
>>> a block device node name that identifies the image to store vmstate in,
>>> and the second is a list of node names to exclude from snapshots.
>>>
>>> In the block code I've only dealt with node names for block devices, as
>>> IIUC, this is all that libvirt should need in the -blockdev world it now
>>> lives in. IOW, I've made not attempt to cope with people wanting to use
>>> these QMP commands in combination with -drive args.
>>>
>>> I've done some minimal work in libvirt to start to make use of the new
>>> commands to validate their functionality, but this isn't finished yet.
>>>
>>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
>>> having internal snapshots work with OVMF:
>>>
>>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
>>> f45c5f64048f16a6e
>>>
>>> Daniel P. Berrang=C3=A9 (6):
>>>   migration: improve error reporting of block driver state name
>>>   migration: introduce savevm, loadvm, delvm QMP commands
>>>   block: add ability to filter out blockdevs during snapshot
>>>   block: allow specifying name of block device for vmstate storage
>>>   migration: support excluding block devs in QMP snapshot commands
>>>   migration: support picking vmstate disk in QMP snapshot commands
>>>
>>>  block/monitor/block-hmp-cmds.c |  4 +-
>>>  block/snapshot.c               | 68 +++++++++++++++++++------
>>>  include/block/snapshot.h       | 21 +++++---
>>>  include/migration/snapshot.h   | 10 +++-
>>>  migration/savevm.c             | 71 +++++++++++++++++++-------
>>>  monitor/hmp-cmds.c             | 20 ++------
>>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
>>>  replay/replay-snapshot.c       |  4 +-
>>>  softmmu/vl.c                   |  2 +-
>>>  9 files changed, 228 insertions(+), 63 deletions(-)
>> I have tried to work in this interface in 2016. That time
>> we have struggled with the idea that this QMP interface should
>> be ready to work asynchronously.
>>
>> Write-protect userfaultfd was merged into vanilla Linux
>> thus it is time to async savevm interface, which will also
>> bring async loadvm and some rework for state storing.
>>
>> Thus I think that with the introduction of the QMP interface
>> we should at least run save VM not from the main
>> thread but from the background with the event at the end.
> spawning a thread in which to invoke save_snapshot() and load_snapshot()
> is easy enough.  I'm not at all clear on what we need in the way of
> mutex locking though, to make those methods safe to run in a thread
> that isn't the main event loop.

I am unsure that this is so easy. We need to be protected from other
operations
coming through QMP interface. Right now parallel operations are not allowed.

> Even with savevm/loadvm being blocking, we could introduce a QMP event
> straight away, and document that users shouldn't assume the operation
> is complete until they see the event. That would let us make the commands
> non-blocking later with same documented semantics.
OK. Let us assume that you have added QMP savevm as proposed. It is
sync now. Sooner or later (I hope sooner) we will have to re-implement
this command with async version of the command, which will bring
again event etc and thus you will have to add compat layers to the
libvirt.

I think that it would be cleaner to start with the interface suitable for
further (coming) features and not copy obsolete implementation.
Yes, unfortunately, this is much more complex :(

Den

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

On Fri, Jul 03, 2020 at 08:29:08PM +0300, Denis V. Lunev wrote:
> On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> >>> When QMP was first introduced some 10+ years ago now, the snapshot
> >>> related commands (savevm/loadvm/delvm) were not converted. This was
> >>> primarily because their implementation causes blocking of the thread
> >>> running the monitor commands. This was (and still is) considered
> >>> undesirable behaviour both in HMP and QMP.
> >>>
> >>> In theory someone was supposed to fix this flaw at some point in the
> >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> >>> hasn't happened as people always had more important things to work
> >>> on. Enterprise apps were much more interested in external snapshots
> >>> than internal snapshots as they have many more features.
> >>>
> >>> Meanwhile users still want to use internal snapshots as there is
> >>> a certainly simplicity in having everything self-contained in one
> >>> image, even though it has limitations. Thus the apps that end up
> >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> >>> QMP command.
> >>>
> >>>
> >>> IOW, the problematic blocking behaviour that was one of the reasons
> >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> >>> regardless. By not portting the commands to QMP due to one design flaw,
> >>> we've forced apps and users to suffer from other design flaws of HMP (
> >>> bad error reporting, strong type checking of args, no introspection) for
> >>> an additional 10 years. This feels rather sub-optimal :-(
> >>>
> >>> In practice users don't appear to care strongly about the fact that these
> >>> commands block the VM while they run. I might have seen one bug report
> >>> about it, but it certainly isn't something that comes up as a frequent
> >>> topic except among us QEMU maintainers. Users do care about having
> >>> access to the snapshot feature.
> >>>
> >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> >>> with snapshots which has some serious pain points. This is getting worse
> >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> >>> the commands, but doing this in HMP is super unappealing.
> >>>
> >>>
> >>>
> >>> After 10 years, I think it is time for us to be a little pragmatic about
> >>> our handling of snapshots commands. My desire is that libvirt should never
> >>> use "human-monitor-command" under any circumstances, because of the
> >>> inherant flaws in HMP as a protocol for machine consumption. If there
> >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> >>> deprecate the current QMP commands and remove them not too long after,
> >>> without being locked in forever.
> >>>
> >>>
> >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> >>> not solve the blocking thread problem, but it does eliminate the error
> >>> reporting, type checking and introspection problems inherant to HMP.
> >>> We're winning on 3 out of the 4 long term problems.
> >>>
> >>> If someone can suggest a easy way to fix the thread blocking problem
> >>> too, I'd be interested to hear it. If it involves a major refactoring
> >>> then I think user are better served by unlocking what look like easy
> >>> wins today.
> >>>
> >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> >>>
> >>>  - The logic to pick which disk to store the vmstate in is not
> >>>    satsifactory.
> >>>
> >>>    The first block driver state cannot be assumed to be the root disk
> >>>    image, it might be OVMF varstore and we don't want to store vmstate
> >>>    in there.
> >>>
> >>>  - The logic to decide which disks must be snapshotted is hardwired
> >>>    to all disks which are writable
> >>>
> >>>    Again with OVMF there might be a writable varstore, but this can be
> >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> >>>    While users might wish to snapshot their varstore, in some/many/most
> >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> >>>    their VM though due to this varstore.
> >>>
> >>> These are solved by adding two parameters to the commands. The first is
> >>> a block device node name that identifies the image to store vmstate in,
> >>> and the second is a list of node names to exclude from snapshots.
> >>>
> >>> In the block code I've only dealt with node names for block devices, as
> >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> >>> these QMP commands in combination with -drive args.
> >>>
> >>> I've done some minimal work in libvirt to start to make use of the new
> >>> commands to validate their functionality, but this isn't finished yet.
> >>>
> >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> >>> having internal snapshots work with OVMF:
> >>>
> >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> >>> f45c5f64048f16a6e
> >>>
> >>> Daniel P. Berrang=C3=A9 (6):
> >>>   migration: improve error reporting of block driver state name
> >>>   migration: introduce savevm, loadvm, delvm QMP commands
> >>>   block: add ability to filter out blockdevs during snapshot
> >>>   block: allow specifying name of block device for vmstate storage
> >>>   migration: support excluding block devs in QMP snapshot commands
> >>>   migration: support picking vmstate disk in QMP snapshot commands
> >>>
> >>>  block/monitor/block-hmp-cmds.c |  4 +-
> >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> >>>  include/block/snapshot.h       | 21 +++++---
> >>>  include/migration/snapshot.h   | 10 +++-
> >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> >>>  monitor/hmp-cmds.c             | 20 ++------
> >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> >>>  replay/replay-snapshot.c       |  4 +-
> >>>  softmmu/vl.c                   |  2 +-
> >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> >> I have tried to work in this interface in 2016. That time
> >> we have struggled with the idea that this QMP interface should
> >> be ready to work asynchronously.
> >>
> >> Write-protect userfaultfd was merged into vanilla Linux
> >> thus it is time to async savevm interface, which will also
> >> bring async loadvm and some rework for state storing.
> >>
> >> Thus I think that with the introduction of the QMP interface
> >> we should at least run save VM not from the main
> >> thread but from the background with the event at the end.
> > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > is easy enough.  I'm not at all clear on what we need in the way of
> > mutex locking though, to make those methods safe to run in a thread
> > that isn't the main event loop.
> 
> I am unsure that this is so easy. We need to be protected from other
> operations
> coming through QMP interface. Right now parallel operations are not allowed.
> 
> > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > straight away, and document that users shouldn't assume the operation
> > is complete until they see the event. That would let us make the commands
> > non-blocking later with same documented semantics.
> OK. Let us assume that you have added QMP savevm as proposed. It is
> sync now. Sooner or later (I hope sooner) we will have to re-implement
> this command with async version of the command, which will bring
> again event etc and thus you will have to add compat layers to the
> libvirt.
> 
> I think that it would be cleaner to start with the interface suitable for
> further (coming) features and not copy obsolete implementation.
> Yes, unfortunately, this is much more complex :(

QMP-ifying the current design gives us a quick win by removing the
dependancy on HMP, for negligible effort. It further lets us unlock
a few extra useful features, again for negligible effort or maint
burden, since we already have pretty much all the code already for HMP.

Supporting internal snapshots is an explicit non-goal for Red Hat in terms
of the products we're using QEMU with, as they all use external snapshots.
Thus I'm very limited in time I can put into this feature myself. I am 
proposing this QMP-ification with my community hat on instead, as it looks
like a quick win to help out various apps today, but it limits the amount
of time I can justify spending on this.

If someone wants to work on a perfect design that's great, but I don't
want us to do nothing now, and continue to wait for a perfect solution
that has shown no sign of arriving in 10 years.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Kevin Wolf]

Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> >>> When QMP was first introduced some 10+ years ago now, the snapshot
> >>> related commands (savevm/loadvm/delvm) were not converted. This was
> >>> primarily because their implementation causes blocking of the thread
> >>> running the monitor commands. This was (and still is) considered
> >>> undesirable behaviour both in HMP and QMP.
> >>>
> >>> In theory someone was supposed to fix this flaw at some point in the
> >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> >>> hasn't happened as people always had more important things to work
> >>> on. Enterprise apps were much more interested in external snapshots
> >>> than internal snapshots as they have many more features.
> >>>
> >>> Meanwhile users still want to use internal snapshots as there is
> >>> a certainly simplicity in having everything self-contained in one
> >>> image, even though it has limitations. Thus the apps that end up
> >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> >>> QMP command.
> >>>
> >>>
> >>> IOW, the problematic blocking behaviour that was one of the reasons
> >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> >>> regardless. By not portting the commands to QMP due to one design flaw,
> >>> we've forced apps and users to suffer from other design flaws of HMP (
> >>> bad error reporting, strong type checking of args, no introspection) for
> >>> an additional 10 years. This feels rather sub-optimal :-(
> >>>
> >>> In practice users don't appear to care strongly about the fact that these
> >>> commands block the VM while they run. I might have seen one bug report
> >>> about it, but it certainly isn't something that comes up as a frequent
> >>> topic except among us QEMU maintainers. Users do care about having
> >>> access to the snapshot feature.
> >>>
> >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> >>> with snapshots which has some serious pain points. This is getting worse
> >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> >>> the commands, but doing this in HMP is super unappealing.
> >>>
> >>>
> >>>
> >>> After 10 years, I think it is time for us to be a little pragmatic about
> >>> our handling of snapshots commands. My desire is that libvirt should never
> >>> use "human-monitor-command" under any circumstances, because of the
> >>> inherant flaws in HMP as a protocol for machine consumption. If there
> >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> >>> deprecate the current QMP commands and remove them not too long after,
> >>> without being locked in forever.
> >>>
> >>>
> >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> >>> not solve the blocking thread problem, but it does eliminate the error
> >>> reporting, type checking and introspection problems inherant to HMP.
> >>> We're winning on 3 out of the 4 long term problems.
> >>>
> >>> If someone can suggest a easy way to fix the thread blocking problem
> >>> too, I'd be interested to hear it. If it involves a major refactoring
> >>> then I think user are better served by unlocking what look like easy
> >>> wins today.
> >>>
> >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> >>>
> >>>  - The logic to pick which disk to store the vmstate in is not
> >>>    satsifactory.
> >>>
> >>>    The first block driver state cannot be assumed to be the root disk
> >>>    image, it might be OVMF varstore and we don't want to store vmstate
> >>>    in there.
> >>>
> >>>  - The logic to decide which disks must be snapshotted is hardwired
> >>>    to all disks which are writable
> >>>
> >>>    Again with OVMF there might be a writable varstore, but this can be
> >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> >>>    While users might wish to snapshot their varstore, in some/many/most
> >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> >>>    their VM though due to this varstore.
> >>>
> >>> These are solved by adding two parameters to the commands. The first is
> >>> a block device node name that identifies the image to store vmstate in,
> >>> and the second is a list of node names to exclude from snapshots.
> >>>
> >>> In the block code I've only dealt with node names for block devices, as
> >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> >>> these QMP commands in combination with -drive args.
> >>>
> >>> I've done some minimal work in libvirt to start to make use of the new
> >>> commands to validate their functionality, but this isn't finished yet.
> >>>
> >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> >>> having internal snapshots work with OVMF:
> >>>
> >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> >>> f45c5f64048f16a6e
> >>>
> >>> Daniel P. Berrang=C3=A9 (6):
> >>>   migration: improve error reporting of block driver state name
> >>>   migration: introduce savevm, loadvm, delvm QMP commands
> >>>   block: add ability to filter out blockdevs during snapshot
> >>>   block: allow specifying name of block device for vmstate storage
> >>>   migration: support excluding block devs in QMP snapshot commands
> >>>   migration: support picking vmstate disk in QMP snapshot commands
> >>>
> >>>  block/monitor/block-hmp-cmds.c |  4 +-
> >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> >>>  include/block/snapshot.h       | 21 +++++---
> >>>  include/migration/snapshot.h   | 10 +++-
> >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> >>>  monitor/hmp-cmds.c             | 20 ++------
> >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> >>>  replay/replay-snapshot.c       |  4 +-
> >>>  softmmu/vl.c                   |  2 +-
> >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> >> I have tried to work in this interface in 2016. That time
> >> we have struggled with the idea that this QMP interface should
> >> be ready to work asynchronously.
> >>
> >> Write-protect userfaultfd was merged into vanilla Linux
> >> thus it is time to async savevm interface, which will also
> >> bring async loadvm and some rework for state storing.
> >>
> >> Thus I think that with the introduction of the QMP interface
> >> we should at least run save VM not from the main
> >> thread but from the background with the event at the end.
> > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > is easy enough.  I'm not at all clear on what we need in the way of
> > mutex locking though, to make those methods safe to run in a thread
> > that isn't the main event loop.
> 
> I am unsure that this is so easy. We need to be protected from other
> operations
> coming through QMP interface. Right now parallel operations are not allowed.
> 
> > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > straight away, and document that users shouldn't assume the operation
> > is complete until they see the event. That would let us make the commands
> > non-blocking later with same documented semantics.
> OK. Let us assume that you have added QMP savevm as proposed. It is
> sync now. Sooner or later (I hope sooner) we will have to re-implement
> this command with async version of the command, which will bring
> again event etc and thus you will have to add compat layers to the
> libvirt.
> 
> I think that it would be cleaner to start with the interface suitable for
> further (coming) features and not copy obsolete implementation.
> Yes, unfortunately, this is much more complex :(

Should we make this a job (may or may not be a block job) that just
happens to block the VM and return completion immediately with the
simple implementation we can have today? Then moving it later to a
truly async operation mode should become transparent to the QMP client.

Kevin

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
> Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> > On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> > >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> > >>> When QMP was first introduced some 10+ years ago now, the snapshot
> > >>> related commands (savevm/loadvm/delvm) were not converted. This was
> > >>> primarily because their implementation causes blocking of the thread
> > >>> running the monitor commands. This was (and still is) considered
> > >>> undesirable behaviour both in HMP and QMP.
> > >>>
> > >>> In theory someone was supposed to fix this flaw at some point in the
> > >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> > >>> hasn't happened as people always had more important things to work
> > >>> on. Enterprise apps were much more interested in external snapshots
> > >>> than internal snapshots as they have many more features.
> > >>>
> > >>> Meanwhile users still want to use internal snapshots as there is
> > >>> a certainly simplicity in having everything self-contained in one
> > >>> image, even though it has limitations. Thus the apps that end up
> > >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> > >>> QMP command.
> > >>>
> > >>>
> > >>> IOW, the problematic blocking behaviour that was one of the reasons
> > >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> > >>> regardless. By not portting the commands to QMP due to one design flaw,
> > >>> we've forced apps and users to suffer from other design flaws of HMP (
> > >>> bad error reporting, strong type checking of args, no introspection) for
> > >>> an additional 10 years. This feels rather sub-optimal :-(
> > >>>
> > >>> In practice users don't appear to care strongly about the fact that these
> > >>> commands block the VM while they run. I might have seen one bug report
> > >>> about it, but it certainly isn't something that comes up as a frequent
> > >>> topic except among us QEMU maintainers. Users do care about having
> > >>> access to the snapshot feature.
> > >>>
> > >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> > >>> with snapshots which has some serious pain points. This is getting worse
> > >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> > >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> > >>> the commands, but doing this in HMP is super unappealing.
> > >>>
> > >>>
> > >>>
> > >>> After 10 years, I think it is time for us to be a little pragmatic about
> > >>> our handling of snapshots commands. My desire is that libvirt should never
> > >>> use "human-monitor-command" under any circumstances, because of the
> > >>> inherant flaws in HMP as a protocol for machine consumption. If there
> > >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> > >>> deprecate the current QMP commands and remove them not too long after,
> > >>> without being locked in forever.
> > >>>
> > >>>
> > >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> > >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> > >>> not solve the blocking thread problem, but it does eliminate the error
> > >>> reporting, type checking and introspection problems inherant to HMP.
> > >>> We're winning on 3 out of the 4 long term problems.
> > >>>
> > >>> If someone can suggest a easy way to fix the thread blocking problem
> > >>> too, I'd be interested to hear it. If it involves a major refactoring
> > >>> then I think user are better served by unlocking what look like easy
> > >>> wins today.
> > >>>
> > >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> > >>>
> > >>>  - The logic to pick which disk to store the vmstate in is not
> > >>>    satsifactory.
> > >>>
> > >>>    The first block driver state cannot be assumed to be the root disk
> > >>>    image, it might be OVMF varstore and we don't want to store vmstate
> > >>>    in there.
> > >>>
> > >>>  - The logic to decide which disks must be snapshotted is hardwired
> > >>>    to all disks which are writable
> > >>>
> > >>>    Again with OVMF there might be a writable varstore, but this can be
> > >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> > >>>    While users might wish to snapshot their varstore, in some/many/most
> > >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> > >>>    their VM though due to this varstore.
> > >>>
> > >>> These are solved by adding two parameters to the commands. The first is
> > >>> a block device node name that identifies the image to store vmstate in,
> > >>> and the second is a list of node names to exclude from snapshots.
> > >>>
> > >>> In the block code I've only dealt with node names for block devices, as
> > >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> > >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> > >>> these QMP commands in combination with -drive args.
> > >>>
> > >>> I've done some minimal work in libvirt to start to make use of the new
> > >>> commands to validate their functionality, but this isn't finished yet.
> > >>>
> > >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> > >>> having internal snapshots work with OVMF:
> > >>>
> > >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> > >>> f45c5f64048f16a6e
> > >>>
> > >>> Daniel P. Berrang=C3=A9 (6):
> > >>>   migration: improve error reporting of block driver state name
> > >>>   migration: introduce savevm, loadvm, delvm QMP commands
> > >>>   block: add ability to filter out blockdevs during snapshot
> > >>>   block: allow specifying name of block device for vmstate storage
> > >>>   migration: support excluding block devs in QMP snapshot commands
> > >>>   migration: support picking vmstate disk in QMP snapshot commands
> > >>>
> > >>>  block/monitor/block-hmp-cmds.c |  4 +-
> > >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> > >>>  include/block/snapshot.h       | 21 +++++---
> > >>>  include/migration/snapshot.h   | 10 +++-
> > >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> > >>>  monitor/hmp-cmds.c             | 20 ++------
> > >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> > >>>  replay/replay-snapshot.c       |  4 +-
> > >>>  softmmu/vl.c                   |  2 +-
> > >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> > >> I have tried to work in this interface in 2016. That time
> > >> we have struggled with the idea that this QMP interface should
> > >> be ready to work asynchronously.
> > >>
> > >> Write-protect userfaultfd was merged into vanilla Linux
> > >> thus it is time to async savevm interface, which will also
> > >> bring async loadvm and some rework for state storing.
> > >>
> > >> Thus I think that with the introduction of the QMP interface
> > >> we should at least run save VM not from the main
> > >> thread but from the background with the event at the end.
> > > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > > is easy enough.  I'm not at all clear on what we need in the way of
> > > mutex locking though, to make those methods safe to run in a thread
> > > that isn't the main event loop.
> > 
> > I am unsure that this is so easy. We need to be protected from other
> > operations
> > coming through QMP interface. Right now parallel operations are not allowed.
> > 
> > > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > > straight away, and document that users shouldn't assume the operation
> > > is complete until they see the event. That would let us make the commands
> > > non-blocking later with same documented semantics.
> > OK. Let us assume that you have added QMP savevm as proposed. It is
> > sync now. Sooner or later (I hope sooner) we will have to re-implement
> > this command with async version of the command, which will bring
> > again event etc and thus you will have to add compat layers to the
> > libvirt.
> > 
> > I think that it would be cleaner to start with the interface suitable for
> > further (coming) features and not copy obsolete implementation.
> > Yes, unfortunately, this is much more complex :(
> 
> Should we make this a job (may or may not be a block job) that just
> happens to block the VM and return completion immediately with the
> simple implementation we can have today? Then moving it later to a
> truly async operation mode should become transparent to the QMP client.

What would making it a job / block job need from a QMP design POV ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Kevin Wolf]

Am 06.07.2020 um 17:29 hat Daniel P. Berrangé geschrieben:
> On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
> > Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> > > On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > > > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> > > >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> > > >>> When QMP was first introduced some 10+ years ago now, the snapshot
> > > >>> related commands (savevm/loadvm/delvm) were not converted. This was
> > > >>> primarily because their implementation causes blocking of the thread
> > > >>> running the monitor commands. This was (and still is) considered
> > > >>> undesirable behaviour both in HMP and QMP.
> > > >>>
> > > >>> In theory someone was supposed to fix this flaw at some point in the
> > > >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> > > >>> hasn't happened as people always had more important things to work
> > > >>> on. Enterprise apps were much more interested in external snapshots
> > > >>> than internal snapshots as they have many more features.
> > > >>>
> > > >>> Meanwhile users still want to use internal snapshots as there is
> > > >>> a certainly simplicity in having everything self-contained in one
> > > >>> image, even though it has limitations. Thus the apps that end up
> > > >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> > > >>> QMP command.
> > > >>>
> > > >>>
> > > >>> IOW, the problematic blocking behaviour that was one of the reasons
> > > >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> > > >>> regardless. By not portting the commands to QMP due to one design flaw,
> > > >>> we've forced apps and users to suffer from other design flaws of HMP (
> > > >>> bad error reporting, strong type checking of args, no introspection) for
> > > >>> an additional 10 years. This feels rather sub-optimal :-(
> > > >>>
> > > >>> In practice users don't appear to care strongly about the fact that these
> > > >>> commands block the VM while they run. I might have seen one bug report
> > > >>> about it, but it certainly isn't something that comes up as a frequent
> > > >>> topic except among us QEMU maintainers. Users do care about having
> > > >>> access to the snapshot feature.
> > > >>>
> > > >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> > > >>> with snapshots which has some serious pain points. This is getting worse
> > > >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> > > >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> > > >>> the commands, but doing this in HMP is super unappealing.
> > > >>>
> > > >>>
> > > >>>
> > > >>> After 10 years, I think it is time for us to be a little pragmatic about
> > > >>> our handling of snapshots commands. My desire is that libvirt should never
> > > >>> use "human-monitor-command" under any circumstances, because of the
> > > >>> inherant flaws in HMP as a protocol for machine consumption. If there
> > > >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> > > >>> deprecate the current QMP commands and remove them not too long after,
> > > >>> without being locked in forever.
> > > >>>
> > > >>>
> > > >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> > > >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> > > >>> not solve the blocking thread problem, but it does eliminate the error
> > > >>> reporting, type checking and introspection problems inherant to HMP.
> > > >>> We're winning on 3 out of the 4 long term problems.
> > > >>>
> > > >>> If someone can suggest a easy way to fix the thread blocking problem
> > > >>> too, I'd be interested to hear it. If it involves a major refactoring
> > > >>> then I think user are better served by unlocking what look like easy
> > > >>> wins today.
> > > >>>
> > > >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> > > >>>
> > > >>>  - The logic to pick which disk to store the vmstate in is not
> > > >>>    satsifactory.
> > > >>>
> > > >>>    The first block driver state cannot be assumed to be the root disk
> > > >>>    image, it might be OVMF varstore and we don't want to store vmstate
> > > >>>    in there.
> > > >>>
> > > >>>  - The logic to decide which disks must be snapshotted is hardwired
> > > >>>    to all disks which are writable
> > > >>>
> > > >>>    Again with OVMF there might be a writable varstore, but this can be
> > > >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> > > >>>    While users might wish to snapshot their varstore, in some/many/most
> > > >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> > > >>>    their VM though due to this varstore.
> > > >>>
> > > >>> These are solved by adding two parameters to the commands. The first is
> > > >>> a block device node name that identifies the image to store vmstate in,
> > > >>> and the second is a list of node names to exclude from snapshots.
> > > >>>
> > > >>> In the block code I've only dealt with node names for block devices, as
> > > >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> > > >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> > > >>> these QMP commands in combination with -drive args.
> > > >>>
> > > >>> I've done some minimal work in libvirt to start to make use of the new
> > > >>> commands to validate their functionality, but this isn't finished yet.
> > > >>>
> > > >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> > > >>> having internal snapshots work with OVMF:
> > > >>>
> > > >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> > > >>> f45c5f64048f16a6e
> > > >>>
> > > >>> Daniel P. Berrang=C3=A9 (6):
> > > >>>   migration: improve error reporting of block driver state name
> > > >>>   migration: introduce savevm, loadvm, delvm QMP commands
> > > >>>   block: add ability to filter out blockdevs during snapshot
> > > >>>   block: allow specifying name of block device for vmstate storage
> > > >>>   migration: support excluding block devs in QMP snapshot commands
> > > >>>   migration: support picking vmstate disk in QMP snapshot commands
> > > >>>
> > > >>>  block/monitor/block-hmp-cmds.c |  4 +-
> > > >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> > > >>>  include/block/snapshot.h       | 21 +++++---
> > > >>>  include/migration/snapshot.h   | 10 +++-
> > > >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> > > >>>  monitor/hmp-cmds.c             | 20 ++------
> > > >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> > > >>>  replay/replay-snapshot.c       |  4 +-
> > > >>>  softmmu/vl.c                   |  2 +-
> > > >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> > > >> I have tried to work in this interface in 2016. That time
> > > >> we have struggled with the idea that this QMP interface should
> > > >> be ready to work asynchronously.
> > > >>
> > > >> Write-protect userfaultfd was merged into vanilla Linux
> > > >> thus it is time to async savevm interface, which will also
> > > >> bring async loadvm and some rework for state storing.
> > > >>
> > > >> Thus I think that with the introduction of the QMP interface
> > > >> we should at least run save VM not from the main
> > > >> thread but from the background with the event at the end.
> > > > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > > > is easy enough.  I'm not at all clear on what we need in the way of
> > > > mutex locking though, to make those methods safe to run in a thread
> > > > that isn't the main event loop.
> > > 
> > > I am unsure that this is so easy. We need to be protected from other
> > > operations
> > > coming through QMP interface. Right now parallel operations are not allowed.
> > > 
> > > > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > > > straight away, and document that users shouldn't assume the operation
> > > > is complete until they see the event. That would let us make the commands
> > > > non-blocking later with same documented semantics.
> > > OK. Let us assume that you have added QMP savevm as proposed. It is
> > > sync now. Sooner or later (I hope sooner) we will have to re-implement
> > > this command with async version of the command, which will bring
> > > again event etc and thus you will have to add compat layers to the
> > > libvirt.
> > > 
> > > I think that it would be cleaner to start with the interface suitable for
> > > further (coming) features and not copy obsolete implementation.
> > > Yes, unfortunately, this is much more complex :(
> > 
> > Should we make this a job (may or may not be a block job) that just
> > happens to block the VM and return completion immediately with the
> > simple implementation we can have today? Then moving it later to a
> > truly async operation mode should become transparent to the QMP client.
> 
> What would making it a job / block job need from a QMP design POV ?

The actual QMP syntax for the command wouldn't look much different (I
think just a new option 'job-id'), but the difference would be that it's
not documented as performing the whole action, but just starting the
job. The expectation would then be that it can be managed with the
job-* commands and that it emits the job status events.

This may sound complicated, but most of it is actually covered by the
generic job infrastructure.

The simplest job that we have is blockdev-create, which is implemented
in block/create.c (99 lines including the license header). I think this
would be a good model for our new case.

Kevin

Re: [PATCH 5/6] migration: support excluding block devs in QMP snapshot commands

[Kevin Wolf]

Am 02.07.2020 um 19:57 hat Daniel P. Berrangé geschrieben:
> This wires up support for a new "exclude" parameter to the QMP commands
> for snapshots (savevm, loadvm, delvm). This parameter accepts a list of
> block driver state node names.
> 
> One use case for this would be a VM using OVMF firmware where the
> variables store is a raw disk image. Ideally the variable store would be
> qcow2, allowing its contents to be included in the snapshot, but
> typically today the variable store is raw. It is still useful to be able
> to snapshot VMs using OVMF, even if the varstore is excluded, as the
> main OS disk content is usually the stuff the user cares about.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Wouldn't it be better to take an optional list of nodes to _include_
that just defaults to our current set of nodes?

The problem with excluding is that we already don't snapshot many nodes,
and the criteria to choose the right ones weren't entirely obvious, so
we just went with something that seemed to make the most sense. But the
management application may actually want to snapshot more nodes than we
cover by default.

I feel things become clearer and less surprising if the client just
tells us explicitly which images are supposed to be snapshotted instead
of adding exceptions on top of a default selection that we're already
not really sure about.

Kevin

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

On Mon, Jul 06, 2020 at 05:50:11PM +0200, Kevin Wolf wrote:
> Am 06.07.2020 um 17:29 hat Daniel P. Berrangé geschrieben:
> > On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
> > > Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> > > > On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > > > > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> > > > >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> > > > >>> When QMP was first introduced some 10+ years ago now, the snapshot
> > > > >>> related commands (savevm/loadvm/delvm) were not converted. This was
> > > > >>> primarily because their implementation causes blocking of the thread
> > > > >>> running the monitor commands. This was (and still is) considered
> > > > >>> undesirable behaviour both in HMP and QMP.
> > > > >>>
> > > > >>> In theory someone was supposed to fix this flaw at some point in the
> > > > >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> > > > >>> hasn't happened as people always had more important things to work
> > > > >>> on. Enterprise apps were much more interested in external snapshots
> > > > >>> than internal snapshots as they have many more features.
> > > > >>>
> > > > >>> Meanwhile users still want to use internal snapshots as there is
> > > > >>> a certainly simplicity in having everything self-contained in one
> > > > >>> image, even though it has limitations. Thus the apps that end up
> > > > >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> > > > >>> QMP command.
> > > > >>>
> > > > >>>
> > > > >>> IOW, the problematic blocking behaviour that was one of the reasons
> > > > >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> > > > >>> regardless. By not portting the commands to QMP due to one design flaw,
> > > > >>> we've forced apps and users to suffer from other design flaws of HMP (
> > > > >>> bad error reporting, strong type checking of args, no introspection) for
> > > > >>> an additional 10 years. This feels rather sub-optimal :-(
> > > > >>>
> > > > >>> In practice users don't appear to care strongly about the fact that these
> > > > >>> commands block the VM while they run. I might have seen one bug report
> > > > >>> about it, but it certainly isn't something that comes up as a frequent
> > > > >>> topic except among us QEMU maintainers. Users do care about having
> > > > >>> access to the snapshot feature.
> > > > >>>
> > > > >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> > > > >>> with snapshots which has some serious pain points. This is getting worse
> > > > >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> > > > >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> > > > >>> the commands, but doing this in HMP is super unappealing.
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >>> After 10 years, I think it is time for us to be a little pragmatic about
> > > > >>> our handling of snapshots commands. My desire is that libvirt should never
> > > > >>> use "human-monitor-command" under any circumstances, because of the
> > > > >>> inherant flaws in HMP as a protocol for machine consumption. If there
> > > > >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> > > > >>> deprecate the current QMP commands and remove them not too long after,
> > > > >>> without being locked in forever.
> > > > >>>
> > > > >>>
> > > > >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> > > > >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> > > > >>> not solve the blocking thread problem, but it does eliminate the error
> > > > >>> reporting, type checking and introspection problems inherant to HMP.
> > > > >>> We're winning on 3 out of the 4 long term problems.
> > > > >>>
> > > > >>> If someone can suggest a easy way to fix the thread blocking problem
> > > > >>> too, I'd be interested to hear it. If it involves a major refactoring
> > > > >>> then I think user are better served by unlocking what look like easy
> > > > >>> wins today.
> > > > >>>
> > > > >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> > > > >>>
> > > > >>>  - The logic to pick which disk to store the vmstate in is not
> > > > >>>    satsifactory.
> > > > >>>
> > > > >>>    The first block driver state cannot be assumed to be the root disk
> > > > >>>    image, it might be OVMF varstore and we don't want to store vmstate
> > > > >>>    in there.
> > > > >>>
> > > > >>>  - The logic to decide which disks must be snapshotted is hardwired
> > > > >>>    to all disks which are writable
> > > > >>>
> > > > >>>    Again with OVMF there might be a writable varstore, but this can be
> > > > >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> > > > >>>    While users might wish to snapshot their varstore, in some/many/most
> > > > >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> > > > >>>    their VM though due to this varstore.
> > > > >>>
> > > > >>> These are solved by adding two parameters to the commands. The first is
> > > > >>> a block device node name that identifies the image to store vmstate in,
> > > > >>> and the second is a list of node names to exclude from snapshots.
> > > > >>>
> > > > >>> In the block code I've only dealt with node names for block devices, as
> > > > >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> > > > >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> > > > >>> these QMP commands in combination with -drive args.
> > > > >>>
> > > > >>> I've done some minimal work in libvirt to start to make use of the new
> > > > >>> commands to validate their functionality, but this isn't finished yet.
> > > > >>>
> > > > >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> > > > >>> having internal snapshots work with OVMF:
> > > > >>>
> > > > >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> > > > >>> f45c5f64048f16a6e
> > > > >>>
> > > > >>> Daniel P. Berrang=C3=A9 (6):
> > > > >>>   migration: improve error reporting of block driver state name
> > > > >>>   migration: introduce savevm, loadvm, delvm QMP commands
> > > > >>>   block: add ability to filter out blockdevs during snapshot
> > > > >>>   block: allow specifying name of block device for vmstate storage
> > > > >>>   migration: support excluding block devs in QMP snapshot commands
> > > > >>>   migration: support picking vmstate disk in QMP snapshot commands
> > > > >>>
> > > > >>>  block/monitor/block-hmp-cmds.c |  4 +-
> > > > >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> > > > >>>  include/block/snapshot.h       | 21 +++++---
> > > > >>>  include/migration/snapshot.h   | 10 +++-
> > > > >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> > > > >>>  monitor/hmp-cmds.c             | 20 ++------
> > > > >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> > > > >>>  replay/replay-snapshot.c       |  4 +-
> > > > >>>  softmmu/vl.c                   |  2 +-
> > > > >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> > > > >> I have tried to work in this interface in 2016. That time
> > > > >> we have struggled with the idea that this QMP interface should
> > > > >> be ready to work asynchronously.
> > > > >>
> > > > >> Write-protect userfaultfd was merged into vanilla Linux
> > > > >> thus it is time to async savevm interface, which will also
> > > > >> bring async loadvm and some rework for state storing.
> > > > >>
> > > > >> Thus I think that with the introduction of the QMP interface
> > > > >> we should at least run save VM not from the main
> > > > >> thread but from the background with the event at the end.
> > > > > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > > > > is easy enough.  I'm not at all clear on what we need in the way of
> > > > > mutex locking though, to make those methods safe to run in a thread
> > > > > that isn't the main event loop.
> > > > 
> > > > I am unsure that this is so easy. We need to be protected from other
> > > > operations
> > > > coming through QMP interface. Right now parallel operations are not allowed.
> > > > 
> > > > > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > > > > straight away, and document that users shouldn't assume the operation
> > > > > is complete until they see the event. That would let us make the commands
> > > > > non-blocking later with same documented semantics.
> > > > OK. Let us assume that you have added QMP savevm as proposed. It is
> > > > sync now. Sooner or later (I hope sooner) we will have to re-implement
> > > > this command with async version of the command, which will bring
> > > > again event etc and thus you will have to add compat layers to the
> > > > libvirt.
> > > > 
> > > > I think that it would be cleaner to start with the interface suitable for
> > > > further (coming) features and not copy obsolete implementation.
> > > > Yes, unfortunately, this is much more complex :(
> > > 
> > > Should we make this a job (may or may not be a block job) that just
> > > happens to block the VM and return completion immediately with the
> > > simple implementation we can have today? Then moving it later to a
> > > truly async operation mode should become transparent to the QMP client.
> > 
> > What would making it a job / block job need from a QMP design POV ?
> 
> The actual QMP syntax for the command wouldn't look much different (I
> think just a new option 'job-id'), but the difference would be that it's
> not documented as performing the whole action, but just starting the
> job. The expectation would then be that it can be managed with the
> job-* commands and that it emits the job status events.
> 
> This may sound complicated, but most of it is actually covered by the
> generic job infrastructure.
> 
> The simplest job that we have is blockdev-create, which is implemented
> in block/create.c (99 lines including the license header). I think this
> would be a good model for our new case.

The QMP design and internal API looks simple enough, but I'm wondering
what implications come with the job infra wrt locking/thread safety. In
particular I see the "job_start" command runs the impl in a coroutine.
I can't tell if that's going to cause any interactions wrto the current
loadvm/savevm impl and its assumptions about blocking execution while
running.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Denis V. Lunev]

On 7/6/20 5:28 PM, Daniel P. Berrangé wrote:
> On Fri, Jul 03, 2020 at 08:29:08PM +0300, Denis V. Lunev wrote:
>> On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
>>> On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
>>>> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
>>>>> When QMP was first introduced some 10+ years ago now, the snapshot
>>>>> related commands (savevm/loadvm/delvm) were not converted. This was
>>>>> primarily because their implementation causes blocking of the thread
>>>>> running the monitor commands. This was (and still is) considered
>>>>> undesirable behaviour both in HMP and QMP.
>>>>>
>>>>> In theory someone was supposed to fix this flaw at some point in the
>>>>> past 10 years and bring them into the QMP world. Sadly, thus far it
>>>>> hasn't happened as people always had more important things to work
>>>>> on. Enterprise apps were much more interested in external snapshots
>>>>> than internal snapshots as they have many more features.
>>>>>
>>>>> Meanwhile users still want to use internal snapshots as there is
>>>>> a certainly simplicity in having everything self-contained in one
>>>>> image, even though it has limitations. Thus the apps that end up
>>>>> executing the savevm/loadvm/delvm via the "human-monitor-command"
>>>>> QMP command.
>>>>>
>>>>>
>>>>> IOW, the problematic blocking behaviour that was one of the reasons
>>>>> for not having savevm/loadvm/delvm in QMP is experienced by applications
>>>>> regardless. By not portting the commands to QMP due to one design flaw,
>>>>> we've forced apps and users to suffer from other design flaws of HMP (
>>>>> bad error reporting, strong type checking of args, no introspection) for
>>>>> an additional 10 years. This feels rather sub-optimal :-(
>>>>>
>>>>> In practice users don't appear to care strongly about the fact that these
>>>>> commands block the VM while they run. I might have seen one bug report
>>>>> about it, but it certainly isn't something that comes up as a frequent
>>>>> topic except among us QEMU maintainers. Users do care about having
>>>>> access to the snapshot feature.
>>>>>
>>>>> Where I am seeing frequent complaints is wrt the use of OVMF combined
>>>>> with snapshots which has some serious pain points. This is getting worse
>>>>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
>>>>> across OS vendors and mgmt apps. Solving it requires new parameters to
>>>>> the commands, but doing this in HMP is super unappealing.
>>>>>
>>>>>
>>>>>
>>>>> After 10 years, I think it is time for us to be a little pragmatic about
>>>>> our handling of snapshots commands. My desire is that libvirt should never
>>>>> use "human-monitor-command" under any circumstances, because of the
>>>>> inherant flaws in HMP as a protocol for machine consumption. If there
>>>>> are flaws in QMP commands that's fine. If we fix them in future, we can
>>>>> deprecate the current QMP commands and remove them not too long after,
>>>>> without being locked in forever.
>>>>>
>>>>>
>>>>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
>>>>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
>>>>> not solve the blocking thread problem, but it does eliminate the error
>>>>> reporting, type checking and introspection problems inherant to HMP.
>>>>> We're winning on 3 out of the 4 long term problems.
>>>>>
>>>>> If someone can suggest a easy way to fix the thread blocking problem
>>>>> too, I'd be interested to hear it. If it involves a major refactoring
>>>>> then I think user are better served by unlocking what look like easy
>>>>> wins today.
>>>>>
>>>>> With a QMP variant, we reasonably deal with the problems related to OVMF:
>>>>>
>>>>>  - The logic to pick which disk to store the vmstate in is not
>>>>>    satsifactory.
>>>>>
>>>>>    The first block driver state cannot be assumed to be the root disk
>>>>>    image, it might be OVMF varstore and we don't want to store vmstate
>>>>>    in there.
>>>>>
>>>>>  - The logic to decide which disks must be snapshotted is hardwired
>>>>>    to all disks which are writable
>>>>>
>>>>>    Again with OVMF there might be a writable varstore, but this can be
>>>>>    raw rather than qcow2 format, and thus unable to be snapshotted.
>>>>>    While users might wish to snapshot their varstore, in some/many/most
>>>>>    cases it is entirely uneccessary. Users are blocked from snapshotting
>>>>>    their VM though due to this varstore.
>>>>>
>>>>> These are solved by adding two parameters to the commands. The first is
>>>>> a block device node name that identifies the image to store vmstate in,
>>>>> and the second is a list of node names to exclude from snapshots.
>>>>>
>>>>> In the block code I've only dealt with node names for block devices, as
>>>>> IIUC, this is all that libvirt should need in the -blockdev world it now
>>>>> lives in. IOW, I've made not attempt to cope with people wanting to use
>>>>> these QMP commands in combination with -drive args.
>>>>>
>>>>> I've done some minimal work in libvirt to start to make use of the new
>>>>> commands to validate their functionality, but this isn't finished yet.
>>>>>
>>>>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
>>>>> having internal snapshots work with OVMF:
>>>>>
>>>>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
>>>>> f45c5f64048f16a6e
>>>>>
>>>>> Daniel P. Berrang=C3=A9 (6):
>>>>>   migration: improve error reporting of block driver state name
>>>>>   migration: introduce savevm, loadvm, delvm QMP commands
>>>>>   block: add ability to filter out blockdevs during snapshot
>>>>>   block: allow specifying name of block device for vmstate storage
>>>>>   migration: support excluding block devs in QMP snapshot commands
>>>>>   migration: support picking vmstate disk in QMP snapshot commands
>>>>>
>>>>>  block/monitor/block-hmp-cmds.c |  4 +-
>>>>>  block/snapshot.c               | 68 +++++++++++++++++++------
>>>>>  include/block/snapshot.h       | 21 +++++---
>>>>>  include/migration/snapshot.h   | 10 +++-
>>>>>  migration/savevm.c             | 71 +++++++++++++++++++-------
>>>>>  monitor/hmp-cmds.c             | 20 ++------
>>>>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
>>>>>  replay/replay-snapshot.c       |  4 +-
>>>>>  softmmu/vl.c                   |  2 +-
>>>>>  9 files changed, 228 insertions(+), 63 deletions(-)
>>>> I have tried to work in this interface in 2016. That time
>>>> we have struggled with the idea that this QMP interface should
>>>> be ready to work asynchronously.
>>>>
>>>> Write-protect userfaultfd was merged into vanilla Linux
>>>> thus it is time to async savevm interface, which will also
>>>> bring async loadvm and some rework for state storing.
>>>>
>>>> Thus I think that with the introduction of the QMP interface
>>>> we should at least run save VM not from the main
>>>> thread but from the background with the event at the end.
>>> spawning a thread in which to invoke save_snapshot() and load_snapshot()
>>> is easy enough.  I'm not at all clear on what we need in the way of
>>> mutex locking though, to make those methods safe to run in a thread
>>> that isn't the main event loop.
>> I am unsure that this is so easy. We need to be protected from other
>> operations
>> coming through QMP interface. Right now parallel operations are not allowed.
>>
>>> Even with savevm/loadvm being blocking, we could introduce a QMP event
>>> straight away, and document that users shouldn't assume the operation
>>> is complete until they see the event. That would let us make the commands
>>> non-blocking later with same documented semantics.
>> OK. Let us assume that you have added QMP savevm as proposed. It is
>> sync now. Sooner or later (I hope sooner) we will have to re-implement
>> this command with async version of the command, which will bring
>> again event etc and thus you will have to add compat layers to the
>> libvirt.
>>
>> I think that it would be cleaner to start with the interface suitable for
>> further (coming) features and not copy obsolete implementation.
>> Yes, unfortunately, this is much more complex :(
> QMP-ifying the current design gives us a quick win by removing the
> dependancy on HMP, for negligible effort. It further lets us unlock
> a few extra useful features, again for negligible effort or maint
> burden, since we already have pretty much all the code already for HMP.
>
> Supporting internal snapshots is an explicit non-goal for Red Hat in terms
> of the products we're using QEMU with, as they all use external snapshots.
> Thus I'm very limited in time I can put into this feature myself. I am 
> proposing this QMP-ification with my community hat on instead, as it looks
> like a quick win to help out various apps today, but it limits the amount
> of time I can justify spending on this.
>
> If someone wants to work on a perfect design that's great, but I don't
> want us to do nothing now, and continue to wait for a perfect solution
> that has shown no sign of arriving in 10 years.
>
> Regards,
> Daniel
I understand this. My proposal is may be a bit controversal but affordable.
Let us make current synchronous implementation as-is, not changes
at all, but design API in async way. Thus the command

+{ 'command': 'savevm',
+  'data': { 'tag': 'str' } }

should be considered as "start the operations", while the end is
signaled via event.

In this case we will be able to separate API part and implementation
part. What do you think on this?

Den

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Denis V. Lunev]

On 7/6/20 7:03 PM, Daniel P. Berrangé wrote:
> On Mon, Jul 06, 2020 at 05:50:11PM +0200, Kevin Wolf wrote:
>> Am 06.07.2020 um 17:29 hat Daniel P. Berrangé geschrieben:
>>> On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
>>>> Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
>>>>> On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
>>>>>> On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
>>>>>>> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
>>>>>>>> When QMP was first introduced some 10+ years ago now, the snapshot
>>>>>>>> related commands (savevm/loadvm/delvm) were not converted. This was
>>>>>>>> primarily because their implementation causes blocking of the thread
>>>>>>>> running the monitor commands. This was (and still is) considered
>>>>>>>> undesirable behaviour both in HMP and QMP.
>>>>>>>>
>>>>>>>> In theory someone was supposed to fix this flaw at some point in the
>>>>>>>> past 10 years and bring them into the QMP world. Sadly, thus far it
>>>>>>>> hasn't happened as people always had more important things to work
>>>>>>>> on. Enterprise apps were much more interested in external snapshots
>>>>>>>> than internal snapshots as they have many more features.
>>>>>>>>
>>>>>>>> Meanwhile users still want to use internal snapshots as there is
>>>>>>>> a certainly simplicity in having everything self-contained in one
>>>>>>>> image, even though it has limitations. Thus the apps that end up
>>>>>>>> executing the savevm/loadvm/delvm via the "human-monitor-command"
>>>>>>>> QMP command.
>>>>>>>>
>>>>>>>>
>>>>>>>> IOW, the problematic blocking behaviour that was one of the reasons
>>>>>>>> for not having savevm/loadvm/delvm in QMP is experienced by applications
>>>>>>>> regardless. By not portting the commands to QMP due to one design flaw,
>>>>>>>> we've forced apps and users to suffer from other design flaws of HMP (
>>>>>>>> bad error reporting, strong type checking of args, no introspection) for
>>>>>>>> an additional 10 years. This feels rather sub-optimal :-(
>>>>>>>>
>>>>>>>> In practice users don't appear to care strongly about the fact that these
>>>>>>>> commands block the VM while they run. I might have seen one bug report
>>>>>>>> about it, but it certainly isn't something that comes up as a frequent
>>>>>>>> topic except among us QEMU maintainers. Users do care about having
>>>>>>>> access to the snapshot feature.
>>>>>>>>
>>>>>>>> Where I am seeing frequent complaints is wrt the use of OVMF combined
>>>>>>>> with snapshots which has some serious pain points. This is getting worse
>>>>>>>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
>>>>>>>> across OS vendors and mgmt apps. Solving it requires new parameters to
>>>>>>>> the commands, but doing this in HMP is super unappealing.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> After 10 years, I think it is time for us to be a little pragmatic about
>>>>>>>> our handling of snapshots commands. My desire is that libvirt should never
>>>>>>>> use "human-monitor-command" under any circumstances, because of the
>>>>>>>> inherant flaws in HMP as a protocol for machine consumption. If there
>>>>>>>> are flaws in QMP commands that's fine. If we fix them in future, we can
>>>>>>>> deprecate the current QMP commands and remove them not too long after,
>>>>>>>> without being locked in forever.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
>>>>>>>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
>>>>>>>> not solve the blocking thread problem, but it does eliminate the error
>>>>>>>> reporting, type checking and introspection problems inherant to HMP.
>>>>>>>> We're winning on 3 out of the 4 long term problems.
>>>>>>>>
>>>>>>>> If someone can suggest a easy way to fix the thread blocking problem
>>>>>>>> too, I'd be interested to hear it. If it involves a major refactoring
>>>>>>>> then I think user are better served by unlocking what look like easy
>>>>>>>> wins today.
>>>>>>>>
>>>>>>>> With a QMP variant, we reasonably deal with the problems related to OVMF:
>>>>>>>>
>>>>>>>>  - The logic to pick which disk to store the vmstate in is not
>>>>>>>>    satsifactory.
>>>>>>>>
>>>>>>>>    The first block driver state cannot be assumed to be the root disk
>>>>>>>>    image, it might be OVMF varstore and we don't want to store vmstate
>>>>>>>>    in there.
>>>>>>>>
>>>>>>>>  - The logic to decide which disks must be snapshotted is hardwired
>>>>>>>>    to all disks which are writable
>>>>>>>>
>>>>>>>>    Again with OVMF there might be a writable varstore, but this can be
>>>>>>>>    raw rather than qcow2 format, and thus unable to be snapshotted.
>>>>>>>>    While users might wish to snapshot their varstore, in some/many/most
>>>>>>>>    cases it is entirely uneccessary. Users are blocked from snapshotting
>>>>>>>>    their VM though due to this varstore.
>>>>>>>>
>>>>>>>> These are solved by adding two parameters to the commands. The first is
>>>>>>>> a block device node name that identifies the image to store vmstate in,
>>>>>>>> and the second is a list of node names to exclude from snapshots.
>>>>>>>>
>>>>>>>> In the block code I've only dealt with node names for block devices, as
>>>>>>>> IIUC, this is all that libvirt should need in the -blockdev world it now
>>>>>>>> lives in. IOW, I've made not attempt to cope with people wanting to use
>>>>>>>> these QMP commands in combination with -drive args.
>>>>>>>>
>>>>>>>> I've done some minimal work in libvirt to start to make use of the new
>>>>>>>> commands to validate their functionality, but this isn't finished yet.
>>>>>>>>
>>>>>>>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
>>>>>>>> having internal snapshots work with OVMF:
>>>>>>>>
>>>>>>>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
>>>>>>>> f45c5f64048f16a6e
>>>>>>>>
>>>>>>>> Daniel P. Berrang=C3=A9 (6):
>>>>>>>>   migration: improve error reporting of block driver state name
>>>>>>>>   migration: introduce savevm, loadvm, delvm QMP commands
>>>>>>>>   block: add ability to filter out blockdevs during snapshot
>>>>>>>>   block: allow specifying name of block device for vmstate storage
>>>>>>>>   migration: support excluding block devs in QMP snapshot commands
>>>>>>>>   migration: support picking vmstate disk in QMP snapshot commands
>>>>>>>>
>>>>>>>>  block/monitor/block-hmp-cmds.c |  4 +-
>>>>>>>>  block/snapshot.c               | 68 +++++++++++++++++++------
>>>>>>>>  include/block/snapshot.h       | 21 +++++---
>>>>>>>>  include/migration/snapshot.h   | 10 +++-
>>>>>>>>  migration/savevm.c             | 71 +++++++++++++++++++-------
>>>>>>>>  monitor/hmp-cmds.c             | 20 ++------
>>>>>>>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
>>>>>>>>  replay/replay-snapshot.c       |  4 +-
>>>>>>>>  softmmu/vl.c                   |  2 +-
>>>>>>>>  9 files changed, 228 insertions(+), 63 deletions(-)
>>>>>>> I have tried to work in this interface in 2016. That time
>>>>>>> we have struggled with the idea that this QMP interface should
>>>>>>> be ready to work asynchronously.
>>>>>>>
>>>>>>> Write-protect userfaultfd was merged into vanilla Linux
>>>>>>> thus it is time to async savevm interface, which will also
>>>>>>> bring async loadvm and some rework for state storing.
>>>>>>>
>>>>>>> Thus I think that with the introduction of the QMP interface
>>>>>>> we should at least run save VM not from the main
>>>>>>> thread but from the background with the event at the end.
>>>>>> spawning a thread in which to invoke save_snapshot() and load_snapshot()
>>>>>> is easy enough.  I'm not at all clear on what we need in the way of
>>>>>> mutex locking though, to make those methods safe to run in a thread
>>>>>> that isn't the main event loop.
>>>>> I am unsure that this is so easy. We need to be protected from other
>>>>> operations
>>>>> coming through QMP interface. Right now parallel operations are not allowed.
>>>>>
>>>>>> Even with savevm/loadvm being blocking, we could introduce a QMP event
>>>>>> straight away, and document that users shouldn't assume the operation
>>>>>> is complete until they see the event. That would let us make the commands
>>>>>> non-blocking later with same documented semantics.
>>>>> OK. Let us assume that you have added QMP savevm as proposed. It is
>>>>> sync now. Sooner or later (I hope sooner) we will have to re-implement
>>>>> this command with async version of the command, which will bring
>>>>> again event etc and thus you will have to add compat layers to the
>>>>> libvirt.
>>>>>
>>>>> I think that it would be cleaner to start with the interface suitable for
>>>>> further (coming) features and not copy obsolete implementation.
>>>>> Yes, unfortunately, this is much more complex :(
>>>> Should we make this a job (may or may not be a block job) that just
>>>> happens to block the VM and return completion immediately with the
>>>> simple implementation we can have today? Then moving it later to a
>>>> truly async operation mode should become transparent to the QMP client.
>>> What would making it a job / block job need from a QMP design POV ?
>> The actual QMP syntax for the command wouldn't look much different (I
>> think just a new option 'job-id'), but the difference would be that it's
>> not documented as performing the whole action, but just starting the
>> job. The expectation would then be that it can be managed with the
>> job-* commands and that it emits the job status events.
>>
>> This may sound complicated, but most of it is actually covered by the
>> generic job infrastructure.
>>
>> The simplest job that we have is blockdev-create, which is implemented
>> in block/create.c (99 lines including the license header). I think this
>> would be a good model for our new case.
This proposal looks perfect to me!

> The QMP design and internal API looks simple enough, but I'm wondering
> what implications come with the job infra wrt locking/thread safety. In
> particular I see the "job_start" command runs the impl in a coroutine.
> I can't tell if that's going to cause any interactions wrto the current
> loadvm/savevm impl and its assumptions about blocking execution while
> running.
>
> Regards,
> Daniel
Right now we don't care. This is API part. For the implentation part the
code remains as-is. In this case we just adopt libvirt to the new
approach while QEMU remains old. Converting QEMU to new iface
is indeed separate (much more complex) task.

Den

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

On Mon, Jul 06, 2020 at 07:10:16PM +0300, Denis V. Lunev wrote:
> On 7/6/20 7:03 PM, Daniel P. Berrangé wrote:
> > On Mon, Jul 06, 2020 at 05:50:11PM +0200, Kevin Wolf wrote:
> >> Am 06.07.2020 um 17:29 hat Daniel P. Berrangé geschrieben:
> >>> On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
> >>>> Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> >>>>> On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> >>>>>> On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> >>>>>>> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> >>>>>>>> When QMP was first introduced some 10+ years ago now, the snapshot
> >>>>>>>> related commands (savevm/loadvm/delvm) were not converted. This was
> >>>>>>>> primarily because their implementation causes blocking of the thread
> >>>>>>>> running the monitor commands. This was (and still is) considered
> >>>>>>>> undesirable behaviour both in HMP and QMP.
> >>>>>>>>
> >>>>>>>> In theory someone was supposed to fix this flaw at some point in the
> >>>>>>>> past 10 years and bring them into the QMP world. Sadly, thus far it
> >>>>>>>> hasn't happened as people always had more important things to work
> >>>>>>>> on. Enterprise apps were much more interested in external snapshots
> >>>>>>>> than internal snapshots as they have many more features.
> >>>>>>>>
> >>>>>>>> Meanwhile users still want to use internal snapshots as there is
> >>>>>>>> a certainly simplicity in having everything self-contained in one
> >>>>>>>> image, even though it has limitations. Thus the apps that end up
> >>>>>>>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> >>>>>>>> QMP command.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> IOW, the problematic blocking behaviour that was one of the reasons
> >>>>>>>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> >>>>>>>> regardless. By not portting the commands to QMP due to one design flaw,
> >>>>>>>> we've forced apps and users to suffer from other design flaws of HMP (
> >>>>>>>> bad error reporting, strong type checking of args, no introspection) for
> >>>>>>>> an additional 10 years. This feels rather sub-optimal :-(
> >>>>>>>>
> >>>>>>>> In practice users don't appear to care strongly about the fact that these
> >>>>>>>> commands block the VM while they run. I might have seen one bug report
> >>>>>>>> about it, but it certainly isn't something that comes up as a frequent
> >>>>>>>> topic except among us QEMU maintainers. Users do care about having
> >>>>>>>> access to the snapshot feature.
> >>>>>>>>
> >>>>>>>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> >>>>>>>> with snapshots which has some serious pain points. This is getting worse
> >>>>>>>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> >>>>>>>> across OS vendors and mgmt apps. Solving it requires new parameters to
> >>>>>>>> the commands, but doing this in HMP is super unappealing.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> After 10 years, I think it is time for us to be a little pragmatic about
> >>>>>>>> our handling of snapshots commands. My desire is that libvirt should never
> >>>>>>>> use "human-monitor-command" under any circumstances, because of the
> >>>>>>>> inherant flaws in HMP as a protocol for machine consumption. If there
> >>>>>>>> are flaws in QMP commands that's fine. If we fix them in future, we can
> >>>>>>>> deprecate the current QMP commands and remove them not too long after,
> >>>>>>>> without being locked in forever.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> >>>>>>>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> >>>>>>>> not solve the blocking thread problem, but it does eliminate the error
> >>>>>>>> reporting, type checking and introspection problems inherant to HMP.
> >>>>>>>> We're winning on 3 out of the 4 long term problems.
> >>>>>>>>
> >>>>>>>> If someone can suggest a easy way to fix the thread blocking problem
> >>>>>>>> too, I'd be interested to hear it. If it involves a major refactoring
> >>>>>>>> then I think user are better served by unlocking what look like easy
> >>>>>>>> wins today.
> >>>>>>>>
> >>>>>>>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> >>>>>>>>
> >>>>>>>>  - The logic to pick which disk to store the vmstate in is not
> >>>>>>>>    satsifactory.
> >>>>>>>>
> >>>>>>>>    The first block driver state cannot be assumed to be the root disk
> >>>>>>>>    image, it might be OVMF varstore and we don't want to store vmstate
> >>>>>>>>    in there.
> >>>>>>>>
> >>>>>>>>  - The logic to decide which disks must be snapshotted is hardwired
> >>>>>>>>    to all disks which are writable
> >>>>>>>>
> >>>>>>>>    Again with OVMF there might be a writable varstore, but this can be
> >>>>>>>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> >>>>>>>>    While users might wish to snapshot their varstore, in some/many/most
> >>>>>>>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> >>>>>>>>    their VM though due to this varstore.
> >>>>>>>>
> >>>>>>>> These are solved by adding two parameters to the commands. The first is
> >>>>>>>> a block device node name that identifies the image to store vmstate in,
> >>>>>>>> and the second is a list of node names to exclude from snapshots.
> >>>>>>>>
> >>>>>>>> In the block code I've only dealt with node names for block devices, as
> >>>>>>>> IIUC, this is all that libvirt should need in the -blockdev world it now
> >>>>>>>> lives in. IOW, I've made not attempt to cope with people wanting to use
> >>>>>>>> these QMP commands in combination with -drive args.
> >>>>>>>>
> >>>>>>>> I've done some minimal work in libvirt to start to make use of the new
> >>>>>>>> commands to validate their functionality, but this isn't finished yet.
> >>>>>>>>
> >>>>>>>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> >>>>>>>> having internal snapshots work with OVMF:
> >>>>>>>>
> >>>>>>>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> >>>>>>>> f45c5f64048f16a6e
> >>>>>>>>
> >>>>>>>> Daniel P. Berrang=C3=A9 (6):
> >>>>>>>>   migration: improve error reporting of block driver state name
> >>>>>>>>   migration: introduce savevm, loadvm, delvm QMP commands
> >>>>>>>>   block: add ability to filter out blockdevs during snapshot
> >>>>>>>>   block: allow specifying name of block device for vmstate storage
> >>>>>>>>   migration: support excluding block devs in QMP snapshot commands
> >>>>>>>>   migration: support picking vmstate disk in QMP snapshot commands
> >>>>>>>>
> >>>>>>>>  block/monitor/block-hmp-cmds.c |  4 +-
> >>>>>>>>  block/snapshot.c               | 68 +++++++++++++++++++------
> >>>>>>>>  include/block/snapshot.h       | 21 +++++---
> >>>>>>>>  include/migration/snapshot.h   | 10 +++-
> >>>>>>>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> >>>>>>>>  monitor/hmp-cmds.c             | 20 ++------
> >>>>>>>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> >>>>>>>>  replay/replay-snapshot.c       |  4 +-
> >>>>>>>>  softmmu/vl.c                   |  2 +-
> >>>>>>>>  9 files changed, 228 insertions(+), 63 deletions(-)
> >>>>>>> I have tried to work in this interface in 2016. That time
> >>>>>>> we have struggled with the idea that this QMP interface should
> >>>>>>> be ready to work asynchronously.
> >>>>>>>
> >>>>>>> Write-protect userfaultfd was merged into vanilla Linux
> >>>>>>> thus it is time to async savevm interface, which will also
> >>>>>>> bring async loadvm and some rework for state storing.
> >>>>>>>
> >>>>>>> Thus I think that with the introduction of the QMP interface
> >>>>>>> we should at least run save VM not from the main
> >>>>>>> thread but from the background with the event at the end.
> >>>>>> spawning a thread in which to invoke save_snapshot() and load_snapshot()
> >>>>>> is easy enough.  I'm not at all clear on what we need in the way of
> >>>>>> mutex locking though, to make those methods safe to run in a thread
> >>>>>> that isn't the main event loop.
> >>>>> I am unsure that this is so easy. We need to be protected from other
> >>>>> operations
> >>>>> coming through QMP interface. Right now parallel operations are not allowed.
> >>>>>
> >>>>>> Even with savevm/loadvm being blocking, we could introduce a QMP event
> >>>>>> straight away, and document that users shouldn't assume the operation
> >>>>>> is complete until they see the event. That would let us make the commands
> >>>>>> non-blocking later with same documented semantics.
> >>>>> OK. Let us assume that you have added QMP savevm as proposed. It is
> >>>>> sync now. Sooner or later (I hope sooner) we will have to re-implement
> >>>>> this command with async version of the command, which will bring
> >>>>> again event etc and thus you will have to add compat layers to the
> >>>>> libvirt.
> >>>>>
> >>>>> I think that it would be cleaner to start with the interface suitable for
> >>>>> further (coming) features and not copy obsolete implementation.
> >>>>> Yes, unfortunately, this is much more complex :(
> >>>> Should we make this a job (may or may not be a block job) that just
> >>>> happens to block the VM and return completion immediately with the
> >>>> simple implementation we can have today? Then moving it later to a
> >>>> truly async operation mode should become transparent to the QMP client.
> >>> What would making it a job / block job need from a QMP design POV ?
> >> The actual QMP syntax for the command wouldn't look much different (I
> >> think just a new option 'job-id'), but the difference would be that it's
> >> not documented as performing the whole action, but just starting the
> >> job. The expectation would then be that it can be managed with the
> >> job-* commands and that it emits the job status events.
> >>
> >> This may sound complicated, but most of it is actually covered by the
> >> generic job infrastructure.
> >>
> >> The simplest job that we have is blockdev-create, which is implemented
> >> in block/create.c (99 lines including the license header). I think this
> >> would be a good model for our new case.
> This proposal looks perfect to me!
> 
> > The QMP design and internal API looks simple enough, but I'm wondering
> > what implications come with the job infra wrt locking/thread safety. In
> > particular I see the "job_start" command runs the impl in a coroutine.
> > I can't tell if that's going to cause any interactions wrto the current
> > loadvm/savevm impl and its assumptions about blocking execution while
> > running.
>
> Right now we don't care. This is API part. For the implentation part the
> code remains as-is. In this case we just adopt libvirt to the new
> approach while QEMU remains old. Converting QEMU to new iface
> is indeed separate (much more complex) task.

If we're exposing a "job-id" in the QAPI schema though, applications are
going to expect the normal "job-XXX" commands to be functional.

We don't want to have a "job-id" that can't be used right now, and then
magically starts working later, because there'll be no way for apps to
introspect whether "job-id" works or not.

So if we expose job-id it needs to do something IMHO, otherwise we should
not expose job-id at all, and simply add it to the command later  once it
does actally do something useful.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Kevin Wolf]

Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben:
> On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> > > > > savevm, loadvm and delvm are some of the few commands that have never
> > > > > been converted to use QMP. The primary reason for this lack of
> > > > > conversion is that they block execution of the thread for as long as
> > > > > they run.
> > > > > 
> > > > > Despite this downside, however, libvirt and applications using libvirt
> > > > > has used these commands for as long as QMP has existed, via the
> > > > > "human-monitor-command" passthrough command. IOW, while it is clearly
> > > > > desirable to be able to fix the blocking problem, this is not an
> > > > > immediate obstacle to real world usage.
> > > > > 
> > > > > Meanwhile there is a need for other features which involve adding new
> > > > > parameters to the commands. This is possible with HMP passthrough, but
> > > > > it provides no reliable way for apps to introspect features, so using
> > > > > QAPI modelling is highly desirable.
> > > > > 
> > > > > This patch thus introduces trival savevm, loadvm, delvm commands
> > > > 
> > > > trivial
> > > > 
> > > > > to QMP that are functionally identical to the HMP counterpart, including
> > > > > the blocking problem.
> > > > 
> > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room
> > > > to change them when we DO solve the blocking issue?  Or will the solution of
> > > > the blocking issue introduce new QMP commands, at which point we can add QMP
> > > > deprecation markers on these commands to eventually retire them?
> > > 
> > > I was in two minds about this, so I'm open to arguments either way.
> > > 
> > > The primary goal is for libvirt to consume the APIs as soon as possible,
> > > and generally libvirt doesn't want todo this is they are declared experimental
> > > via a "x-" prefix. So that pushes me away from "x-".
> > > 
> > > If we don't have an "x-" prefix and want to make changes, we can add extra
> > > parameters to trigger new behaviour in backwards compatible manner. Or we can
> > > simply deprecate these commands, deleting them 2 releases later, while adding
> > > completely new commands.
> > > 
> > > If we think the prposed design will definitely need incompatible changes in
> > > a very short time frame though, that would push towards "x-".
> > > 
> > > So IMHO the right answer largely depends on whether there is a credible
> > > strategy to implement the ideal non-blocking solution in a reasonable amount
> > > of time. I can't justify spending much time on this myself, but I'm willing
> > > to consider & try proposals for solving the blocking problem if they're not
> > > too complex / invasive.
> > 
> > Remind me, what was the problem with just making a block: migration
> > channel, and then we can migrate to it?

For normal migration, the VM is still running and changing its state, so
you would have to wait until migration completes and then snapshot the
disk. This is possible, but it would store redundant data and the
snapshot would be at an arbitrary point after actually receiving the
snapshot command, which is probably not what users expect.

So what you really want is some kind of postcopy migration inside the
same process. Virtuozzo had a look at this some years ago and when the
discussion comes up again, they say they are still kind of interested,
though it's not their priority. I have never seen the actual code, but I
imagine it's not trivial (unless the migration code already supports
something like this today, but I'm not aware of another use case for it,
so probably not?)

> migration only does vmstate, not disks. The current blockdev commands
> are all related to external snapshots, nothing for internal snapshots
> AFAIK. So we still need commands to load/save internal snapshots of
> the disk data in the qcow2 files.
> 
> So we could look at loadvm/savevm conceptually as a syntax sugar above
> a migration transport that targets disk images, and blockdev QMP command
> that can do internal snapshots. Neither of these exist though and feel
> like a significantly larger amount of work than using existing functionality
> that is currently working.

There is blockdev-snapshot-internal-sync, which does a disk-only
snapshot of a single node. A snapshot of multiple nodes can be taken by
putting multiple blockdev-snapshot-internal-sync inside a 'transaction'
command.

If we want to build on top of this, we'd have to implement a
transactionable command for storing only the VM state to a specific
node. This would probably still be a long-running job.

So some of it does exist, but I'm torn on whether separating things into
different commands is actually a good idea or not.

Kevin

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Kevin Wolf]

Am 06.07.2020 um 18:03 hat Daniel P. Berrangé geschrieben:
> On Mon, Jul 06, 2020 at 05:50:11PM +0200, Kevin Wolf wrote:
> > Am 06.07.2020 um 17:29 hat Daniel P. Berrangé geschrieben:
> > > On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
> > > > Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> > > > > On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > > > > > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> > > > > >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> > > > > >>> When QMP was first introduced some 10+ years ago now, the snapshot
> > > > > >>> related commands (savevm/loadvm/delvm) were not converted. This was
> > > > > >>> primarily because their implementation causes blocking of the thread
> > > > > >>> running the monitor commands. This was (and still is) considered
> > > > > >>> undesirable behaviour both in HMP and QMP.
> > > > > >>>
> > > > > >>> In theory someone was supposed to fix this flaw at some point in the
> > > > > >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> > > > > >>> hasn't happened as people always had more important things to work
> > > > > >>> on. Enterprise apps were much more interested in external snapshots
> > > > > >>> than internal snapshots as they have many more features.
> > > > > >>>
> > > > > >>> Meanwhile users still want to use internal snapshots as there is
> > > > > >>> a certainly simplicity in having everything self-contained in one
> > > > > >>> image, even though it has limitations. Thus the apps that end up
> > > > > >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> > > > > >>> QMP command.
> > > > > >>>
> > > > > >>>
> > > > > >>> IOW, the problematic blocking behaviour that was one of the reasons
> > > > > >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> > > > > >>> regardless. By not portting the commands to QMP due to one design flaw,
> > > > > >>> we've forced apps and users to suffer from other design flaws of HMP (
> > > > > >>> bad error reporting, strong type checking of args, no introspection) for
> > > > > >>> an additional 10 years. This feels rather sub-optimal :-(
> > > > > >>>
> > > > > >>> In practice users don't appear to care strongly about the fact that these
> > > > > >>> commands block the VM while they run. I might have seen one bug report
> > > > > >>> about it, but it certainly isn't something that comes up as a frequent
> > > > > >>> topic except among us QEMU maintainers. Users do care about having
> > > > > >>> access to the snapshot feature.
> > > > > >>>
> > > > > >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> > > > > >>> with snapshots which has some serious pain points. This is getting worse
> > > > > >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> > > > > >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> > > > > >>> the commands, but doing this in HMP is super unappealing.
> > > > > >>>
> > > > > >>>
> > > > > >>>
> > > > > >>> After 10 years, I think it is time for us to be a little pragmatic about
> > > > > >>> our handling of snapshots commands. My desire is that libvirt should never
> > > > > >>> use "human-monitor-command" under any circumstances, because of the
> > > > > >>> inherant flaws in HMP as a protocol for machine consumption. If there
> > > > > >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> > > > > >>> deprecate the current QMP commands and remove them not too long after,
> > > > > >>> without being locked in forever.
> > > > > >>>
> > > > > >>>
> > > > > >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> > > > > >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> > > > > >>> not solve the blocking thread problem, but it does eliminate the error
> > > > > >>> reporting, type checking and introspection problems inherant to HMP.
> > > > > >>> We're winning on 3 out of the 4 long term problems.
> > > > > >>>
> > > > > >>> If someone can suggest a easy way to fix the thread blocking problem
> > > > > >>> too, I'd be interested to hear it. If it involves a major refactoring
> > > > > >>> then I think user are better served by unlocking what look like easy
> > > > > >>> wins today.
> > > > > >>>
> > > > > >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> > > > > >>>
> > > > > >>>  - The logic to pick which disk to store the vmstate in is not
> > > > > >>>    satsifactory.
> > > > > >>>
> > > > > >>>    The first block driver state cannot be assumed to be the root disk
> > > > > >>>    image, it might be OVMF varstore and we don't want to store vmstate
> > > > > >>>    in there.
> > > > > >>>
> > > > > >>>  - The logic to decide which disks must be snapshotted is hardwired
> > > > > >>>    to all disks which are writable
> > > > > >>>
> > > > > >>>    Again with OVMF there might be a writable varstore, but this can be
> > > > > >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> > > > > >>>    While users might wish to snapshot their varstore, in some/many/most
> > > > > >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> > > > > >>>    their VM though due to this varstore.
> > > > > >>>
> > > > > >>> These are solved by adding two parameters to the commands. The first is
> > > > > >>> a block device node name that identifies the image to store vmstate in,
> > > > > >>> and the second is a list of node names to exclude from snapshots.
> > > > > >>>
> > > > > >>> In the block code I've only dealt with node names for block devices, as
> > > > > >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> > > > > >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> > > > > >>> these QMP commands in combination with -drive args.
> > > > > >>>
> > > > > >>> I've done some minimal work in libvirt to start to make use of the new
> > > > > >>> commands to validate their functionality, but this isn't finished yet.
> > > > > >>>
> > > > > >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> > > > > >>> having internal snapshots work with OVMF:
> > > > > >>>
> > > > > >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> > > > > >>> f45c5f64048f16a6e
> > > > > >>>
> > > > > >>> Daniel P. Berrang=C3=A9 (6):
> > > > > >>>   migration: improve error reporting of block driver state name
> > > > > >>>   migration: introduce savevm, loadvm, delvm QMP commands
> > > > > >>>   block: add ability to filter out blockdevs during snapshot
> > > > > >>>   block: allow specifying name of block device for vmstate storage
> > > > > >>>   migration: support excluding block devs in QMP snapshot commands
> > > > > >>>   migration: support picking vmstate disk in QMP snapshot commands
> > > > > >>>
> > > > > >>>  block/monitor/block-hmp-cmds.c |  4 +-
> > > > > >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> > > > > >>>  include/block/snapshot.h       | 21 +++++---
> > > > > >>>  include/migration/snapshot.h   | 10 +++-
> > > > > >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> > > > > >>>  monitor/hmp-cmds.c             | 20 ++------
> > > > > >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> > > > > >>>  replay/replay-snapshot.c       |  4 +-
> > > > > >>>  softmmu/vl.c                   |  2 +-
> > > > > >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> > > > > >> I have tried to work in this interface in 2016. That time
> > > > > >> we have struggled with the idea that this QMP interface should
> > > > > >> be ready to work asynchronously.
> > > > > >>
> > > > > >> Write-protect userfaultfd was merged into vanilla Linux
> > > > > >> thus it is time to async savevm interface, which will also
> > > > > >> bring async loadvm and some rework for state storing.
> > > > > >>
> > > > > >> Thus I think that with the introduction of the QMP interface
> > > > > >> we should at least run save VM not from the main
> > > > > >> thread but from the background with the event at the end.
> > > > > > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > > > > > is easy enough.  I'm not at all clear on what we need in the way of
> > > > > > mutex locking though, to make those methods safe to run in a thread
> > > > > > that isn't the main event loop.
> > > > > 
> > > > > I am unsure that this is so easy. We need to be protected from other
> > > > > operations
> > > > > coming through QMP interface. Right now parallel operations are not allowed.
> > > > > 
> > > > > > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > > > > > straight away, and document that users shouldn't assume the operation
> > > > > > is complete until they see the event. That would let us make the commands
> > > > > > non-blocking later with same documented semantics.
> > > > > OK. Let us assume that you have added QMP savevm as proposed. It is
> > > > > sync now. Sooner or later (I hope sooner) we will have to re-implement
> > > > > this command with async version of the command, which will bring
> > > > > again event etc and thus you will have to add compat layers to the
> > > > > libvirt.
> > > > > 
> > > > > I think that it would be cleaner to start with the interface suitable for
> > > > > further (coming) features and not copy obsolete implementation.
> > > > > Yes, unfortunately, this is much more complex :(
> > > > 
> > > > Should we make this a job (may or may not be a block job) that just
> > > > happens to block the VM and return completion immediately with the
> > > > simple implementation we can have today? Then moving it later to a
> > > > truly async operation mode should become transparent to the QMP client.
> > > 
> > > What would making it a job / block job need from a QMP design POV ?
> > 
> > The actual QMP syntax for the command wouldn't look much different (I
> > think just a new option 'job-id'), but the difference would be that it's
> > not documented as performing the whole action, but just starting the
> > job. The expectation would then be that it can be managed with the
> > job-* commands and that it emits the job status events.
> > 
> > This may sound complicated, but most of it is actually covered by the
> > generic job infrastructure.
> > 
> > The simplest job that we have is blockdev-create, which is implemented
> > in block/create.c (99 lines including the license header). I think this
> > would be a good model for our new case.
> 
> The QMP design and internal API looks simple enough, but I'm wondering
> what implications come with the job infra wrt locking/thread safety. In
> particular I see the "job_start" command runs the impl in a coroutine.
> I can't tell if that's going to cause any interactions wrto the current
> loadvm/savevm impl and its assumptions about blocking execution while
> running.

Yes, the job infrastructure is build on coroutines and we'd need to
check that this is safe. But both loadvm and savevm call both vm_stop()
and bdrv_drain_all_begin/end(), so not much should be going on in
parallel.

If this doesn't easily work out, there is still a simple solution for
our sync implementation with an async interface: Just leave coroutine
context immediately again by scheduling a BH that does the actual work.

Kevin

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Peter Krempa]

On Mon, Jul 06, 2020 at 18:15:55 +0200, Kevin Wolf wrote:
> Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben:
> > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:

[...]

> > migration only does vmstate, not disks. The current blockdev commands
> > are all related to external snapshots, nothing for internal snapshots
> > AFAIK. So we still need commands to load/save internal snapshots of
> > the disk data in the qcow2 files.
> > 
> > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > a migration transport that targets disk images, and blockdev QMP command
> > that can do internal snapshots. Neither of these exist though and feel
> > like a significantly larger amount of work than using existing functionality
> > that is currently working.
> 
> There is blockdev-snapshot-internal-sync, which does a disk-only
> snapshot of a single node. A snapshot of multiple nodes can be taken by
> putting multiple blockdev-snapshot-internal-sync inside a 'transaction'
> command.

Libvirt never implemented support for disk-only internal snapshots as I
didn't think they are worth it. We also made a mistake by using the
VIR_DOMAIN_SNAPSHOT_DISK_ONLY to switch to an external snapshot, so
while the XML can modify the snapshot to be internal it's not very clear
nor user-friendly to force an internal disk only snapshot.

> If we want to build on top of this, we'd have to implement a
> transactionable command for storing only the VM state to a specific
> node. This would probably still be a long-running job.

IMO we really want this also for external snapshots. Driving the
migration as standard migration is really suboptimal especially when the
user wants minimal downtime. Transactioning a post-copy style copy-on
write migration would simplify this a lot. I agree though that this is
for a different conversation.

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

[Daniel P. Berrangé]

On Mon, Jul 06, 2020 at 06:21:56PM +0200, Kevin Wolf wrote:
> Am 06.07.2020 um 18:03 hat Daniel P. Berrangé geschrieben:
> > On Mon, Jul 06, 2020 at 05:50:11PM +0200, Kevin Wolf wrote:
> > > Am 06.07.2020 um 17:29 hat Daniel P. Berrangé geschrieben:
> > > > On Mon, Jul 06, 2020 at 05:27:01PM +0200, Kevin Wolf wrote:
> > > > > Am 03.07.2020 um 19:29 hat Denis V. Lunev geschrieben:
> > > > > > On 7/3/20 8:22 PM, Daniel P. Berrangé wrote:
> > > > > > > On Fri, Jul 03, 2020 at 08:15:44PM +0300, Denis V. Lunev wrote:
> > > > > > >> On 7/2/20 8:57 PM, Daniel P. Berrangé wrote:
> > > > > > >>> When QMP was first introduced some 10+ years ago now, the snapshot
> > > > > > >>> related commands (savevm/loadvm/delvm) were not converted. This was
> > > > > > >>> primarily because their implementation causes blocking of the thread
> > > > > > >>> running the monitor commands. This was (and still is) considered
> > > > > > >>> undesirable behaviour both in HMP and QMP.
> > > > > > >>>
> > > > > > >>> In theory someone was supposed to fix this flaw at some point in the
> > > > > > >>> past 10 years and bring them into the QMP world. Sadly, thus far it
> > > > > > >>> hasn't happened as people always had more important things to work
> > > > > > >>> on. Enterprise apps were much more interested in external snapshots
> > > > > > >>> than internal snapshots as they have many more features.
> > > > > > >>>
> > > > > > >>> Meanwhile users still want to use internal snapshots as there is
> > > > > > >>> a certainly simplicity in having everything self-contained in one
> > > > > > >>> image, even though it has limitations. Thus the apps that end up
> > > > > > >>> executing the savevm/loadvm/delvm via the "human-monitor-command"
> > > > > > >>> QMP command.
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> IOW, the problematic blocking behaviour that was one of the reasons
> > > > > > >>> for not having savevm/loadvm/delvm in QMP is experienced by applications
> > > > > > >>> regardless. By not portting the commands to QMP due to one design flaw,
> > > > > > >>> we've forced apps and users to suffer from other design flaws of HMP (
> > > > > > >>> bad error reporting, strong type checking of args, no introspection) for
> > > > > > >>> an additional 10 years. This feels rather sub-optimal :-(
> > > > > > >>>
> > > > > > >>> In practice users don't appear to care strongly about the fact that these
> > > > > > >>> commands block the VM while they run. I might have seen one bug report
> > > > > > >>> about it, but it certainly isn't something that comes up as a frequent
> > > > > > >>> topic except among us QEMU maintainers. Users do care about having
> > > > > > >>> access to the snapshot feature.
> > > > > > >>>
> > > > > > >>> Where I am seeing frequent complaints is wrt the use of OVMF combined
> > > > > > >>> with snapshots which has some serious pain points. This is getting worse
> > > > > > >>> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> > > > > > >>> across OS vendors and mgmt apps. Solving it requires new parameters to
> > > > > > >>> the commands, but doing this in HMP is super unappealing.
> > > > > > >>>
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> After 10 years, I think it is time for us to be a little pragmatic about
> > > > > > >>> our handling of snapshots commands. My desire is that libvirt should never
> > > > > > >>> use "human-monitor-command" under any circumstances, because of the
> > > > > > >>> inherant flaws in HMP as a protocol for machine consumption. If there
> > > > > > >>> are flaws in QMP commands that's fine. If we fix them in future, we can
> > > > > > >>> deprecate the current QMP commands and remove them not too long after,
> > > > > > >>> without being locked in forever.
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> Thus in this series I'm proposing a direct 1-1 mapping of the existing
> > > > > > >>> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> > > > > > >>> not solve the blocking thread problem, but it does eliminate the error
> > > > > > >>> reporting, type checking and introspection problems inherant to HMP.
> > > > > > >>> We're winning on 3 out of the 4 long term problems.
> > > > > > >>>
> > > > > > >>> If someone can suggest a easy way to fix the thread blocking problem
> > > > > > >>> too, I'd be interested to hear it. If it involves a major refactoring
> > > > > > >>> then I think user are better served by unlocking what look like easy
> > > > > > >>> wins today.
> > > > > > >>>
> > > > > > >>> With a QMP variant, we reasonably deal with the problems related to OVMF:
> > > > > > >>>
> > > > > > >>>  - The logic to pick which disk to store the vmstate in is not
> > > > > > >>>    satsifactory.
> > > > > > >>>
> > > > > > >>>    The first block driver state cannot be assumed to be the root disk
> > > > > > >>>    image, it might be OVMF varstore and we don't want to store vmstate
> > > > > > >>>    in there.
> > > > > > >>>
> > > > > > >>>  - The logic to decide which disks must be snapshotted is hardwired
> > > > > > >>>    to all disks which are writable
> > > > > > >>>
> > > > > > >>>    Again with OVMF there might be a writable varstore, but this can be
> > > > > > >>>    raw rather than qcow2 format, and thus unable to be snapshotted.
> > > > > > >>>    While users might wish to snapshot their varstore, in some/many/most
> > > > > > >>>    cases it is entirely uneccessary. Users are blocked from snapshotting
> > > > > > >>>    their VM though due to this varstore.
> > > > > > >>>
> > > > > > >>> These are solved by adding two parameters to the commands. The first is
> > > > > > >>> a block device node name that identifies the image to store vmstate in,
> > > > > > >>> and the second is a list of node names to exclude from snapshots.
> > > > > > >>>
> > > > > > >>> In the block code I've only dealt with node names for block devices, as
> > > > > > >>> IIUC, this is all that libvirt should need in the -blockdev world it now
> > > > > > >>> lives in. IOW, I've made not attempt to cope with people wanting to use
> > > > > > >>> these QMP commands in combination with -drive args.
> > > > > > >>>
> > > > > > >>> I've done some minimal work in libvirt to start to make use of the new
> > > > > > >>> commands to validate their functionality, but this isn't finished yet.
> > > > > > >>>
> > > > > > >>> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> > > > > > >>> having internal snapshots work with OVMF:
> > > > > > >>>
> > > > > > >>>   https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> > > > > > >>> f45c5f64048f16a6e
> > > > > > >>>
> > > > > > >>> Daniel P. Berrang=C3=A9 (6):
> > > > > > >>>   migration: improve error reporting of block driver state name
> > > > > > >>>   migration: introduce savevm, loadvm, delvm QMP commands
> > > > > > >>>   block: add ability to filter out blockdevs during snapshot
> > > > > > >>>   block: allow specifying name of block device for vmstate storage
> > > > > > >>>   migration: support excluding block devs in QMP snapshot commands
> > > > > > >>>   migration: support picking vmstate disk in QMP snapshot commands
> > > > > > >>>
> > > > > > >>>  block/monitor/block-hmp-cmds.c |  4 +-
> > > > > > >>>  block/snapshot.c               | 68 +++++++++++++++++++------
> > > > > > >>>  include/block/snapshot.h       | 21 +++++---
> > > > > > >>>  include/migration/snapshot.h   | 10 +++-
> > > > > > >>>  migration/savevm.c             | 71 +++++++++++++++++++-------
> > > > > > >>>  monitor/hmp-cmds.c             | 20 ++------
> > > > > > >>>  qapi/migration.json            | 91 ++++++++++++++++++++++++++++++++++
> > > > > > >>>  replay/replay-snapshot.c       |  4 +-
> > > > > > >>>  softmmu/vl.c                   |  2 +-
> > > > > > >>>  9 files changed, 228 insertions(+), 63 deletions(-)
> > > > > > >> I have tried to work in this interface in 2016. That time
> > > > > > >> we have struggled with the idea that this QMP interface should
> > > > > > >> be ready to work asynchronously.
> > > > > > >>
> > > > > > >> Write-protect userfaultfd was merged into vanilla Linux
> > > > > > >> thus it is time to async savevm interface, which will also
> > > > > > >> bring async loadvm and some rework for state storing.
> > > > > > >>
> > > > > > >> Thus I think that with the introduction of the QMP interface
> > > > > > >> we should at least run save VM not from the main
> > > > > > >> thread but from the background with the event at the end.
> > > > > > > spawning a thread in which to invoke save_snapshot() and load_snapshot()
> > > > > > > is easy enough.  I'm not at all clear on what we need in the way of
> > > > > > > mutex locking though, to make those methods safe to run in a thread
> > > > > > > that isn't the main event loop.
> > > > > > 
> > > > > > I am unsure that this is so easy. We need to be protected from other
> > > > > > operations
> > > > > > coming through QMP interface. Right now parallel operations are not allowed.
> > > > > > 
> > > > > > > Even with savevm/loadvm being blocking, we could introduce a QMP event
> > > > > > > straight away, and document that users shouldn't assume the operation
> > > > > > > is complete until they see the event. That would let us make the commands
> > > > > > > non-blocking later with same documented semantics.
> > > > > > OK. Let us assume that you have added QMP savevm as proposed. It is
> > > > > > sync now. Sooner or later (I hope sooner) we will have to re-implement
> > > > > > this command with async version of the command, which will bring
> > > > > > again event etc and thus you will have to add compat layers to the
> > > > > > libvirt.
> > > > > > 
> > > > > > I think that it would be cleaner to start with the interface suitable for
> > > > > > further (coming) features and not copy obsolete implementation.
> > > > > > Yes, unfortunately, this is much more complex :(
> > > > > 
> > > > > Should we make this a job (may or may not be a block job) that just
> > > > > happens to block the VM and return completion immediately with the
> > > > > simple implementation we can have today? Then moving it later to a
> > > > > truly async operation mode should become transparent to the QMP client.
> > > > 
> > > > What would making it a job / block job need from a QMP design POV ?
> > > 
> > > The actual QMP syntax for the command wouldn't look much different (I
> > > think just a new option 'job-id'), but the difference would be that it's
> > > not documented as performing the whole action, but just starting the
> > > job. The expectation would then be that it can be managed with the
> > > job-* commands and that it emits the job status events.
> > > 
> > > This may sound complicated, but most of it is actually covered by the
> > > generic job infrastructure.
> > > 
> > > The simplest job that we have is blockdev-create, which is implemented
> > > in block/create.c (99 lines including the license header). I think this
> > > would be a good model for our new case.
> > 
> > The QMP design and internal API looks simple enough, but I'm wondering
> > what implications come with the job infra wrt locking/thread safety. In
> > particular I see the "job_start" command runs the impl in a coroutine.
> > I can't tell if that's going to cause any interactions wrto the current
> > loadvm/savevm impl and its assumptions about blocking execution while
> > running.
> 
> Yes, the job infrastructure is build on coroutines and we'd need to
> check that this is safe. But both loadvm and savevm call both vm_stop()
> and bdrv_drain_all_begin/end(), so not much should be going on in
> parallel.
> 
> If this doesn't easily work out, there is still a simple solution for
> our sync implementation with an async interface: Just leave coroutine
> context immediately again by scheduling a BH that does the actual work.

Ok, I'm going to try out use of the job framework and post a v2.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] migration: support excluding block devs in QMP snapshot commands

[Daniel P. Berrangé]

On Mon, Jul 06, 2020 at 05:57:08PM +0200, Kevin Wolf wrote:
> Am 02.07.2020 um 19:57 hat Daniel P. Berrangé geschrieben:
> > This wires up support for a new "exclude" parameter to the QMP commands
> > for snapshots (savevm, loadvm, delvm). This parameter accepts a list of
> > block driver state node names.
> > 
> > One use case for this would be a VM using OVMF firmware where the
> > variables store is a raw disk image. Ideally the variable store would be
> > qcow2, allowing its contents to be included in the snapshot, but
> > typically today the variable store is raw. It is still useful to be able
> > to snapshot VMs using OVMF, even if the varstore is excluded, as the
> > main OS disk content is usually the stuff the user cares about.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Wouldn't it be better to take an optional list of nodes to _include_
> that just defaults to our current set of nodes?
> 
> The problem with excluding is that we already don't snapshot many nodes,
> and the criteria to choose the right ones weren't entirely obvious, so
> we just went with something that seemed to make the most sense. But the
> management application may actually want to snapshot more nodes than we
> cover by default.
> 
> I feel things become clearer and less surprising if the client just
> tells us explicitly which images are supposed to be snapshotted instead
> of adding exceptions on top of a default selection that we're already
> not really sure about.

I thought that QEMU just excluded nodes which are not capable of being
snapshotted. By using exclusions, the mgmt apps don't have to know
about what types of storage driver QEMU supports snapshots on, just let
QEMU decide. It also felt simpler to use exclusions as normal case would
be to snapshot everything.   Both inclusions/exclusions are easy to
implement in QEMU though.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] migration: support excluding block devs in QMP snapshot commands

[Kevin Wolf]

Am 07.07.2020 um 11:14 hat Daniel P. Berrangé geschrieben:
> On Mon, Jul 06, 2020 at 05:57:08PM +0200, Kevin Wolf wrote:
> > Am 02.07.2020 um 19:57 hat Daniel P. Berrangé geschrieben:
> > > This wires up support for a new "exclude" parameter to the QMP commands
> > > for snapshots (savevm, loadvm, delvm). This parameter accepts a list of
> > > block driver state node names.
> > > 
> > > One use case for this would be a VM using OVMF firmware where the
> > > variables store is a raw disk image. Ideally the variable store would be
> > > qcow2, allowing its contents to be included in the snapshot, but
> > > typically today the variable store is raw. It is still useful to be able
> > > to snapshot VMs using OVMF, even if the varstore is excluded, as the
> > > main OS disk content is usually the stuff the user cares about.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > Wouldn't it be better to take an optional list of nodes to _include_
> > that just defaults to our current set of nodes?
> > 
> > The problem with excluding is that we already don't snapshot many nodes,
> > and the criteria to choose the right ones weren't entirely obvious, so
> > we just went with something that seemed to make the most sense. But the
> > management application may actually want to snapshot more nodes than we
> > cover by default.
> > 
> > I feel things become clearer and less surprising if the client just
> > tells us explicitly which images are supposed to be snapshotted instead
> > of adding exceptions on top of a default selection that we're already
> > not really sure about.
> 
> I thought that QEMU just excluded nodes which are not capable of being
> snapshotted.

No, QEMU tries to figure out which nodes must be snapshotted to capture
the current state, and if any of these nodes doesn't support snapshots,
the snapshot operation fails.

> By using exclusions, the mgmt apps don't have to know
> about what types of storage driver QEMU supports snapshots on, just let
> QEMU decide. It also felt simpler to use exclusions as normal case would
> be to snapshot everything.   Both inclusions/exclusions are easy to
> implement in QEMU though.

The problem when going from device based operation to node based is that
you need to figure out which nodes actually contain something that the
user wants to snapshot. For example, you usually don't want to try
creating a snapshot on the protocol node when there is a format node on
top.

What do you do with nodes that aren't attached to the guest, but maybe
used as the backend of the NBD server? What if a node is both directly
attached to some user and there is another node on top of it? In these
non-trivial cases, the default is rather arbitrary because there really
isn't a single right answer.

What QEMU currently does is snapshotting every node that is opened
read-write and either attached to a BlockBackend (i.e. is used by a
device, NBD server, or I guess as the main node of a block job) or that
doesn't have any parents (i.e. the user added it explicitly, but didn't
use it yet).

Come to think of it, the read-write condition may lead to surprising
results with dynamic auto-read-only... Good that file-posix doesn't
support snapshots and nothing else implements dynamic auto-read-only
yet.

Kevin

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Kevin Wolf]

Am 07.07.2020 um 08:38 hat Peter Krempa geschrieben:
> On Mon, Jul 06, 2020 at 18:15:55 +0200, Kevin Wolf wrote:
> > Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben:
> > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote:
> > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote:
> 
> [...]
> 
> > > migration only does vmstate, not disks. The current blockdev commands
> > > are all related to external snapshots, nothing for internal snapshots
> > > AFAIK. So we still need commands to load/save internal snapshots of
> > > the disk data in the qcow2 files.
> > > 
> > > So we could look at loadvm/savevm conceptually as a syntax sugar above
> > > a migration transport that targets disk images, and blockdev QMP command
> > > that can do internal snapshots. Neither of these exist though and feel
> > > like a significantly larger amount of work than using existing functionality
> > > that is currently working.
> > 
> > There is blockdev-snapshot-internal-sync, which does a disk-only
> > snapshot of a single node. A snapshot of multiple nodes can be taken by
> > putting multiple blockdev-snapshot-internal-sync inside a 'transaction'
> > command.
> 
> Libvirt never implemented support for disk-only internal snapshots as I
> didn't think they are worth it. We also made a mistake by using the
> VIR_DOMAIN_SNAPSHOT_DISK_ONLY to switch to an external snapshot, so
> while the XML can modify the snapshot to be internal it's not very clear
> nor user-friendly to force an internal disk only snapshot.
> 
> > If we want to build on top of this, we'd have to implement a
> > transactionable command for storing only the VM state to a specific
> > node. This would probably still be a long-running job.
> 
> IMO we really want this also for external snapshots. Driving the
> migration as standard migration is really suboptimal especially when the
> user wants minimal downtime. Transactioning a post-copy style copy-on
> write migration would simplify this a lot. I agree though that this is
> for a different conversation.

This is an interesting point actually. And while the implementation of
the post-copy style live snapshotting is for a different conversation, I
think the implications it has on the API are relevant for us now.

But even if we have an all-in-one snapshot job instead of a transaction
to group all the individual operations together, I think you could still
represent that by just specifying an empty list of nodes to be
snapshotted. (I feel this is another argument for passing the nodes to
include rather than nodes to exclude from the disk snapshot.)

Kevin

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

[Peter Krempa]

On Tue, Jul 07, 2020 at 12:33:31 +0200, Kevin Wolf wrote:
> Am 07.07.2020 um 08:38 hat Peter Krempa geschrieben:
> > On Mon, Jul 06, 2020 at 18:15:55 +0200, Kevin Wolf wrote:
> > > Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben:

[...]

> > IMO we really want this also for external snapshots. Driving the
> > migration as standard migration is really suboptimal especially when the
> > user wants minimal downtime. Transactioning a post-copy style copy-on
> > write migration would simplify this a lot. I agree though that this is
> > for a different conversation.
> 
> This is an interesting point actually. And while the implementation of
> the post-copy style live snapshotting is for a different conversation, I
> think the implications it has on the API are relevant for us now.
> 
> But even if we have an all-in-one snapshot job instead of a transaction
> to group all the individual operations together, I think you could still
> represent that by just specifying an empty list of nodes to be
> snapshotted. (I feel this is another argument for passing the nodes to
> include rather than nodes to exclude from the disk snapshot.)

Definitely. From libvirt's POV it's IMO simpler and more future-proof to
enumerate everything rather than keep a database of what to skip.