| Message ID | 20200702175754.2211821-6-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: bring savevm/loadvm/delvm over to QMP | expand |
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
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
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
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);
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(-)