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