9:28 a.m.
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(a)suse.de>
> ---
> 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.
Thanks,
Claudio