On Thu, Apr 28, 2022 at 04:28:22PM +0200, Claudio Fontana wrote: > On 4/28/22 3:11 PM, Daniel P. Berrangé wrote: > > On Wed, Apr 27, 2022 at 11:13:39PM +0200, Claudio Fontana wrote: > >> use zstd which is the only really interesting one. > >> > >> Signed-off-by: Claudio Fontana <[email protected]> > >> --- > >> src/qemu/qemu_capabilities.c | 2 + > >> src/qemu/qemu_capabilities.h | 1 + > >> src/qemu/qemu_migration.c | 6 +++ > >> src/qemu/qemu_migration_params.c | 49 +++++++++---------- > >> src/qemu/qemu_migration_params.h | 6 +++ > >> src/qemu/qemu_saveimage.c | 6 +++ > >> .../caps_5.0.0.aarch64.xml | 1 + > >> .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + > >> .../caps_5.0.0.riscv64.xml | 1 + > >> .../caps_5.0.0.x86_64.xml | 1 + > >> .../qemucapabilitiesdata/caps_5.1.0.sparc.xml | 1 + > >> .../caps_5.1.0.x86_64.xml | 1 + > >> .../caps_5.2.0.aarch64.xml | 1 + > >> .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + > >> .../caps_5.2.0.riscv64.xml | 1 + > >> .../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + > >> .../caps_5.2.0.x86_64.xml | 1 + > >> .../caps_6.0.0.aarch64.xml | 1 + > >> .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + > >> .../caps_6.0.0.x86_64.xml | 1 + > >> .../caps_6.1.0.x86_64.xml | 1 + > >> .../caps_6.2.0.aarch64.xml | 1 + > >> .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml | 1 + > >> .../caps_6.2.0.x86_64.xml | 1 + > >> .../caps_7.0.0.aarch64.xml | 1 + > >> .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml | 1 + > >> .../caps_7.0.0.x86_64.xml | 1 + > >> 27 files changed, 66 insertions(+), 25 deletions(-) > > > > > >> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > >> index 7bab913fe5..f9c86de67e 100644 > >> --- a/src/qemu/qemu_migration.c > >> +++ b/src/qemu/qemu_migration.c > >> @@ -5950,6 +5950,12 @@ qemuMigrationSrcToFileAux(virQEMUDriver *driver, > >> virDomainObj *vm, > >> > >> QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS, > >> nchannels) < 0) > >> return -1; > >> + if (virQEMUCapsGet(priv->qemuCaps, > >> QEMU_CAPS_MIGRATION_PARAM_MULTIFD_COMPRESSION)) { > >> + if (qemuMigrationParamsSetString(migParams, > >> + > >> QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION, > >> + "zstd") < 0) > >> + return -1; > >> + } > > > > snip > > > >> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c > >> index 2bc81035ae..a1e9e1c3e8 100644 > >> --- a/src/qemu/qemu_saveimage.c > >> +++ b/src/qemu/qemu_saveimage.c > >> @@ -607,6 +607,12 @@ int qemuSaveImageLoadMultiFd(virConnectPtr conn, > >> virDomainObj *vm, int oflags, > >> > >> QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS, > >> nchannels) < 0) > >> goto cleanup; > >> + if (virQEMUCapsGet(priv->qemuCaps, > >> QEMU_CAPS_MIGRATION_PARAM_MULTIFD_COMPRESSION)) { > >> + if (qemuMigrationParamsSetString(migParams, > >> + > >> QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION, > >> + "zstd") < 0) > >> + goto cleanup; > >> + } > >> if (qemuMigrationParamsApply(driver, vm, asyncJob, migParams) < 0) > >> goto cleanup; > >> > > > > This is going to break if an image is savd with a QEMU that does not > > support zstd, and is later restored with a QEMU that has enabled > > zstd. > > > > The current save/restore code supports compression, settable globally > > via /etc/libvirt/qemu.conf - raw, gzip, bzip2, xz, lzop. > > > > With your new APIs for save/restore with TypedParameters, we could > > make the compression an API driven choice, which would be nice. > > > > Also, we should add zstd to the choices with existing non-parallel > > migrate. > > > > When we add parallel migrate, we should wire up whichever options > > make sense, but we need to record the use of the compression in the > > save image header. > > > > IOW, when loading we only want to use zstd if the original image > > header shows use of zstd *and* QEMU supports it. > > > > So I'd suggest we want 3 patches. One for adding zstd as a choice, > > one of making compression tunable fvia the APIs, and finally one > > for parallel migrate. We don';t have to support all algorithms > > with parallel migrate - we can do just a subset that make sense > > or are possible. > > > > With regards, > > Daniel > > > > of course agree with the overall comments, > > just wanted to add that the only practical thing that is useful for > multifd-compression > (which is a completely separate parameter from the normal QEMU migration > compression parameter), seems to be "zstd", > > likely using the default zstd multifd compression level (1), but likely Dave > will correct me.
For now.... We've seen enough new compression algorithms that I'm not going to bet on zstd being the last and/or best forever :-) With 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 :|
