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(-)
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
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
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
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 :|
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
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 :|
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
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
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 :|
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
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 :|
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
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 :|
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
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 :|
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
© 2016 - 2024 Red Hat, Inc.