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

[Claudio Fontana]

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 at 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.
> 
> 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