On 4/28/22 6:27 PM, Daniel P. Berrangé wrote:
> 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
>
Yes of course, I'll work on this,
if that's ok in the next-next spin, so I can first iron out the initial patches that are more ripe for upstreaming.
Thanks,
Claudio
