Re: [libvirt RFCv4 20/20] qemu: add migration parameter multifd-compression

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&gt; >> --- >> 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.