On 10/10/24 07:43, Daniel P. Berrangé wrote:
On Thu, Aug 08, 2024 at 05:38:11PM -0600, Jim Fehlig via Devel wrote:
Add support for parallel save and restore by mapping libvirt's "parallel-connections" parameter to QEMU's "multifd-channels" migration parameter.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 31 ++++++++++++++++++++----------- src/qemu/qemu_migration_params.c | 25 +++++++++++++++++++++++-- src/qemu/qemu_migration_params.h | 5 ++++- 3 files changed, 47 insertions(+), 14 deletions(-)
Unless I'm missing something, nothing in this patch is validating the the image is about to be written in v3 format with MAPPED_RAM set. We must reject VIR_DOMAIN_SAVE_PARALLEL if we're not using MAPPED_RAM. Likewise when restoring we must reject PARALLEL if the loaded image doesnt have MAPPED_RAM feature set.
Yeah, good catch. The existing logic essentially ignores PARALLEL if mapped-ram is not supported. [snip]
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 8f6003005c..8c656af163 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -788,7 +788,10 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
qemuMigrationParams * -qemuMigrationParamsForSave(bool mappedRam, unsigned int flags) +qemuMigrationParamsForSave(virTypedParameterPtr params, + int nparams, + bool mappedRam, + unsigned int flags) { g_autoptr(qemuMigrationParams) saveParams = NULL;
@@ -800,7 +803,25 @@ qemuMigrationParamsForSave(bool mappedRam, unsigned int flags) return NULL; if (virBitmapSetBit(saveParams->caps, QEMU_MIGRATION_CAP_MULTIFD) < 0) return NULL; - saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].value.i = 1; + + if (flags & VIR_DOMAIN_SAVE_PARALLEL) { + int nchannels; + + if (params && virTypedParamsGetInt(params, nparams, + VIR_DOMAIN_SAVE_PARAM_PARALLEL_CONNECTIONS, + &nchannels) < 0) + return NULL; + + if (nchannels < 1) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("number of parallel save channels cannot be less than 1")); + return NULL; + } + + saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].value.i = nchannels; + } else { + saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].value.i = 1; + }
It's not shown in the diff, but this block is nestled within a 'if (mappedRam)' block. I've squashed the blow hunk into this patch in my local branch. Regards, Jim diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 8c656af163..6fa09c272e 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -795,6 +795,12 @@ qemuMigrationParamsForSave(virTypedParameterPtr params, { g_autoptr(qemuMigrationParams) saveParams = NULL; + if (flags & VIR_DOMAIN_SAVE_PARALLEL && !mapped_ram) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Parallel save is not available with this QEMU binary")); + return NULL; + } + if (!(saveParams = qemuMigrationParamsNew())) return NULL;