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 <cfontana@xxxxxxx>
> >> ---
> >> 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 :|
