[libvirt PATCH 0/6] cleanup and fixes of the mapped-ram feature
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag tools: use virDomainRestoreParams only when necessary tools: use virDomainSaveParams only when necessary virsh: add --image-format option to the save command docs/manpages/virsh.rst | 24 +++++---- include/libvirt/libvirt-domain.h | 1 - src/qemu/qemu_driver.c | 6 +-- src/qemu/qemu_migration_params.c | 34 ++++++------ tools/virsh-domain.c | 93 ++++++++++++++++---------------- 5 files changed, 80 insertions(+), 78 deletions(-) -- 2.48.1
From: Pavel Hrdina <phrdina@redhat.com> There is no need to have --parallel and --parallel-channels especially when --parallel on its own is the same as not used at all. In both cases libvirt will default to single channel. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/manpages/virsh.rst | 8 ++++---- tools/virsh-domain.c | 17 ++++++----------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 8143366826..fede984e11 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4125,7 +4125,7 @@ restore :: restore state-file [--bypass-cache] [--xml file] - [{--running | --paused}] [--reset-nvram] [--parallel] [--parallel-channels] + [{--running | --paused}] [--reset-nvram] [--parallel-channels] Restores a domain from a ``virsh save`` state file. See *save* for more info. @@ -4147,9 +4147,9 @@ domain should be started in. If *--reset-nvram* is specified, any existing NVRAM file will be deleted and re-initialized from its pristine template. -*--parallel* option will cause the save data to be loaded using the number -of parallel IO channels specified with *--parallel-channels*. Parallel -channels will help speed up large restore operations. +*--parallel-channels* option can specify number of parallel IO channels +to be used when loading memory from file. Parallel save may significantly +reduce the time required to save large memory domains. ``Note``: To avoid corrupting file system contents within the domain, you should not reuse the saved state file for a second ``restore`` unless you diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1bee969824..bb49860604 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5666,10 +5666,6 @@ static const vshCmdOptDef opts_restore[] = { .type = VSH_OT_BOOL, .help = N_("avoid file system cache when restoring") }, - {.name = "parallel", - .type = VSH_OT_BOOL, - .help = N_("enable parallel restore") - }, {.name = "parallel-channels", .type = VSH_OT_INT, .help = N_("number of IO channels to use for parallel restore") @@ -5706,13 +5702,11 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) virTypedParameterPtr params = NULL; int nparams = 0; int maxparams = 0; - int nchannels = 1; + int nchannels = 0; int rc; if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; - if (vshCommandOptBool(cmd, "parallel")) - flags |= VIR_DOMAIN_SAVE_PARALLEL; if (vshCommandOptBool(cmd, "running")) flags |= VIR_DOMAIN_SAVE_RUNNING; if (vshCommandOptBool(cmd, "paused")) @@ -5738,13 +5732,14 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) VIR_DOMAIN_SAVE_PARAM_DXML, xml) < 0) return false; - if (flags & VIR_DOMAIN_SAVE_PARALLEL) { - if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) - return false; - + if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) + return false; + if (rc == 1) { if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) return false; + + flags |= VIR_DOMAIN_SAVE_PARALLEL; } if (flags || xml) { -- 2.48.1
From: Pavel Hrdina <phrdina@redhat.com> There is no need to have --parallel and --parallel-channels especially when --parallel on its own is the same as not used at all. In both cases libvirt will default to single channel. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/manpages/virsh.rst | 11 +++++------ tools/virsh-domain.c | 20 +++++++------------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fede984e11..65ec03cf20 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4179,7 +4179,7 @@ save :: save domain state-file [--bypass-cache] [--xml file] - [--parallel] [--parallel-channels channels] + [--parallel-channels channels] [{--running | --paused}] [--verbose] Saves a running domain (RAM, but not disk state) to a state file so that @@ -4213,11 +4213,10 @@ based on the state the domain was in when the save was done; passing either the *--running* or *--paused* flag will allow overriding which state the ``restore`` should use. -*--parallel* option will cause the save data to be written to file -over multiple parallel IO channels. The number of channels can be -specified using *--parallel-channels*. Using parallel IO channels -requires the use of ``sparse`` image save format. Parallel save may -significantly reduce the time required to save large memory domains. +*--parallel-channels* option can specify number of parallel IO channels +to be used when saving memory to file. Using parallel IO channels requires +the use of ``sparse`` image save format. Parallel save may significantly +reduce the time required to save large memory domains. Domain saved state files assume that disk images will be unchanged between the creation and restore point. For a more complete system diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bb49860604..7adf6c16fa 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4530,13 +4530,9 @@ static const vshCmdOptDef opts_save[] = { .type = VSH_OT_BOOL, .help = N_("avoid file system cache when saving") }, - {.name = "parallel", - .type = VSH_OT_BOOL, - .help = N_("enable parallel save") - }, {.name = "parallel-channels", .type = VSH_OT_INT, - .help = N_("number of extra IO channels to use for parallel save") + .help = N_("number of IO channels to use for parallel save") }, {.name = "xml", .type = VSH_OT_STRING, @@ -4571,8 +4567,7 @@ doSave(void *opaque) virTypedParameterPtr params = NULL; int nparams = 0; int maxparams = 0; - int nchannels = 1; - int rv = -1; + int nchannels = 0; unsigned int flags = 0; const char *xmlfile = NULL; g_autofree char *xml = NULL; @@ -4592,8 +4587,6 @@ doSave(void *opaque) flags |= VIR_DOMAIN_SAVE_RUNNING; if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SAVE_PAUSED; - if (vshCommandOptBool(cmd, "parallel")) - flags |= VIR_DOMAIN_SAVE_PARALLEL; if (vshCommandOptString(ctl, cmd, "file", &to) < 0) goto out; @@ -4602,13 +4595,14 @@ doSave(void *opaque) VIR_DOMAIN_SAVE_PARAM_FILE, to) < 0) goto out; - if (flags & VIR_DOMAIN_SAVE_PARALLEL) { - if ((rv = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) - goto out; - + if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) + goto out; + if (rc == 1) { if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) goto out; + + flags |= VIR_DOMAIN_SAVE_PARALLEL; } if (vshCommandOptString(ctl, cmd, "xml", &xmlfile) < 0) -- 2.48.1
From: Pavel Hrdina <phrdina@redhat.com> There is no need to use extra flag in addition to the new "parallel.channels" param. Using the flag without param would result in using uninitialized variable. Fixing it would result in error that parallel channels cannot be less then 1 or setting 1 as default. Using the param without the flag is ignored. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- include/libvirt/libvirt-domain.h | 1 - src/qemu/qemu_driver.c | 6 ++---- src/qemu/qemu_migration_params.c | 34 +++++++++++++++----------------- tools/virsh-domain.c | 22 ++++++++------------- 4 files changed, 26 insertions(+), 37 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 233bfd927a..6e11baa3d3 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1655,7 +1655,6 @@ typedef enum { VIR_DOMAIN_SAVE_RUNNING = 1 << 1, /* Favor running over paused (Since: 0.9.5) */ VIR_DOMAIN_SAVE_PAUSED = 1 << 2, /* Favor paused over running (Since: 0.9.5) */ VIR_DOMAIN_SAVE_RESET_NVRAM = 1 << 3, /* Re-initialize NVRAM from template (Since: 8.1.0) */ - VIR_DOMAIN_SAVE_PARALLEL = 1 << 4, /* Save and restore using parallel channels (Since: 10.6.0) */ } virDomainSaveRestoreFlags; int virDomainSave (virDomainPtr domain, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 21df94961b..3cf21380ed 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2822,8 +2822,7 @@ qemuDomainSaveParams(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | - VIR_DOMAIN_SAVE_PAUSED | - VIR_DOMAIN_SAVE_PARALLEL, -1); + VIR_DOMAIN_SAVE_PAUSED, -1); if (virTypedParamsValidate(params, nparams, VIR_DOMAIN_SAVE_PARAM_FILE, @@ -5762,8 +5761,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED | - VIR_DOMAIN_SAVE_RESET_NVRAM | - VIR_DOMAIN_SAVE_PARALLEL, -1); + VIR_DOMAIN_SAVE_RESET_NVRAM, -1); if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index b696b0d13e..17d08f4aa5 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -803,11 +803,20 @@ qemuMigrationParamsForSave(virTypedParameterPtr params, unsigned int flags) { g_autoptr(qemuMigrationParams) saveParams = NULL; + int nchannels = 0; + int rv; - if (flags & VIR_DOMAIN_SAVE_PARALLEL && !sparse) { + if ((rv = virTypedParamsGetInt(params, nparams, + VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, + &nchannels)) < 0) + return NULL; + + if (rv == 1 && !sparse) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Parallel save is only supported with the 'sparse' save image format")); return NULL; + } else if (rv == 0) { + nchannels = 1; } if (!(saveParams = qemuMigrationParamsNew())) @@ -819,24 +828,13 @@ qemuMigrationParamsForSave(virTypedParameterPtr params, if (virBitmapSetBit(saveParams->caps, QEMU_MIGRATION_CAP_MULTIFD) < 0) return NULL; - if (flags & VIR_DOMAIN_SAVE_PARALLEL) { - int nchannels; - - if (params && virTypedParamsGetInt(params, nparams, - VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, - &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; + 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; saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set = true; if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) { diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7adf6c16fa..56ddf4d701 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4597,13 +4597,10 @@ doSave(void *opaque) if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) goto out; - if (rc == 1) { - if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, - VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) - goto out; - - flags |= VIR_DOMAIN_SAVE_PARALLEL; - } + if (rc == 1 && + virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) + goto out; if (vshCommandOptString(ctl, cmd, "xml", &xmlfile) < 0) goto out; @@ -5728,13 +5725,10 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) return false; - if (rc == 1) { - if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, - VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) - return false; - - flags |= VIR_DOMAIN_SAVE_PARALLEL; - } + if (rc == 1 && + virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) + return false; if (flags || xml) { rc = virDomainRestoreParams(priv->conn, params, nparams, flags); -- 2.48.1
From: Pavel Hrdina <phrdina@redhat.com> We should use the newest API only when user sets parallel-channels. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh-domain.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 56ddf4d701..c21cf847c8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5707,10 +5707,6 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(ctl, cmd, "file", &from) < 0) return false; - if (from && - virTypedParamsAddString(¶ms, &nparams, &maxparams, - VIR_DOMAIN_SAVE_PARAM_FILE, from) < 0) - return false; if (vshCommandOptString(ctl, cmd, "xml", &xmlfile) < 0) return false; @@ -5718,10 +5714,6 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (xmlfile && virFileReadAll(xmlfile, VSH_MAX_XML_FILE, &xml) < 0) return false; - if (xml && - virTypedParamsAddString(¶ms, &nparams, &maxparams, - VIR_DOMAIN_SAVE_PARAM_DXML, xml) < 0) - return false; if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) return false; @@ -5730,8 +5722,20 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) return false; - if (flags || xml) { + if (nparams > 0) { + if (from && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_FILE, from) < 0) + return false; + + if (xml && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_DXML, xml) < 0) + return false; + rc = virDomainRestoreParams(priv->conn, params, nparams, flags); + } else if (flags || xml) { + rc = virDomainRestoreFlags(priv->conn, from, xml, flags); } else { rc = virDomainRestore(priv->conn, from); } -- 2.48.1
From: Pavel Hrdina <phrdina@redhat.com> We should use the newest API only when user sets parallel-channels. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh-domain.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c21cf847c8..98f0e60ed4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4590,10 +4590,6 @@ doSave(void *opaque) if (vshCommandOptString(ctl, cmd, "file", &to) < 0) goto out; - if (to && - virTypedParamsAddString(¶ms, &nparams, &maxparams, - VIR_DOMAIN_SAVE_PARAM_FILE, to) < 0) - goto out; if ((rc = vshCommandOptInt(ctl, cmd, "parallel-channels", &nchannels)) < 0) goto out; @@ -4613,13 +4609,21 @@ doSave(void *opaque) vshReportError(ctl); goto out; } - if (xml && - virTypedParamsAddString(¶ms, &nparams, &maxparams, - VIR_DOMAIN_SAVE_PARAM_DXML, xml) < 0) - goto out; - if (flags || xml) { + if (nparams > 0) { + if (to && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_FILE, to) < 0) + goto out; + + if (xml && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_DXML, xml) < 0) + goto out; + rc = virDomainSaveParams(dom, params, nparams, flags); + } else if (flags || xml) { + rc = virDomainSaveFlags(dom, to, xml, flags); } else { rc = virDomainSave(dom, to); } -- 2.48.1
From: Pavel Hrdina <phrdina@redhat.com> Option --parallel-channels would require changing configuration file to be used so introduce this option as well to make it convenient for users. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/manpages/virsh.rst | 5 +++++ tools/virsh-domain.c | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 65ec03cf20..35ae6af547 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4179,6 +4179,7 @@ save :: save domain state-file [--bypass-cache] [--xml file] + [--image-format format] [--parallel-channels channels] [{--running | --paused}] [--verbose] @@ -4213,6 +4214,10 @@ based on the state the domain was in when the save was done; passing either the *--running* or *--paused* flag will allow overriding which state the ``restore`` should use. +*--image-format* option can change the default image format used to +save data into file. For more details consult the qemu.conf configuration +file. + *--parallel-channels* option can specify number of parallel IO channels to be used when saving memory to file. Using parallel IO channels requires the use of ``sparse`` image save format. Parallel save may significantly diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 98f0e60ed4..ceff6789d8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4534,6 +4534,10 @@ static const vshCmdOptDef opts_save[] = { .type = VSH_OT_INT, .help = N_("number of IO channels to use for parallel save") }, + {.name = "image-format", + .type = VSH_OT_STRING, + .help = N_("format of the save image file") + }, {.name = "xml", .type = VSH_OT_STRING, .unwanted_positional = true, @@ -4570,6 +4574,7 @@ doSave(void *opaque) int nchannels = 0; unsigned int flags = 0; const char *xmlfile = NULL; + const char *format = NULL; g_autofree char *xml = NULL; int rc; #ifndef WIN32 @@ -4598,6 +4603,13 @@ doSave(void *opaque) VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS, nchannels) < 0) goto out; + if (vshCommandOptString(ctl, cmd, "image-format", &format) < 0) + goto out; + if (format && + virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT, format) < 0) + goto out; + if (vshCommandOptString(ctl, cmd, "xml", &xmlfile) < 0) goto out; -- 2.48.1
On 3/20/25 17:07, Pavel Hrdina via Devel wrote:
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag
Heh, I'm having one of those "why did I not realize that" moments :-).
tools: use virDomainRestoreParams only when necessary tools: use virDomainSaveParams only when necessary
Fair enough.
virsh: add --image-format option to the save command
Thanks! Although I intended to do this, I forgot to add it to my todo list and would have likely forgot.
docs/manpages/virsh.rst | 24 +++++---- include/libvirt/libvirt-domain.h | 1 - src/qemu/qemu_driver.c | 6 +-- src/qemu/qemu_migration_params.c | 34 ++++++------ tools/virsh-domain.c | 93 ++++++++++++++++---------------- 5 files changed, 80 insertions(+), 78 deletions(-)
For series: Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote:
On 3/20/25 17:07, Pavel Hrdina via Devel wrote:
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag
Heh, I'm having one of those "why did I not realize that" moments :-).
IIRC, original we were using --parallel to decide whether to enable the new format, so --parallel with channels == 1 /was/ different from not setting --parallel at all. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Mar 21, 2025 at 08:29:30AM +0000, Daniel P. Berrangé via Devel wrote:
On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote:
On 3/20/25 17:07, Pavel Hrdina via Devel wrote:
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag
Heh, I'm having one of those "why did I not realize that" moments :-).
IIRC, original we were using --parallel to decide whether to enable the new format, so --parallel with channels == 1 /was/ different from not setting --parallel at all.
Well, restore should figure out the format and save still has --image-format without which parallel save will fail (unless you have it set as default in a config). So I think this series is fine, at least it fixes the unit tests. Reviewed-by: Martin Kletzander <mkletzan@redhat.com> But there's more to it than meets the eye. There is also a check that the number of parallel channels is not lower than 1, for saving. Restoring happily takes parallel.channels=0 and (at least for non-sparse images) fails weirdly with: error: Failed to restore domain from test.img error: Failed to open file 'test.img': No such file or directory and the daemon reports: 2025-03-21 10:00:08.490+0000: 4076349: error : virFileIsSharedFSType:3598 : Invalid relative path 'test.img': Invalid argument 2025-03-21 10:00:08.490+0000: 4076349: error : virQEMUFileOpenAs:10448 : Failed to open file 'test.img': No such file or directory even though the file exists. And it works without --parallel-channels, this only happens with --parallel-channels 0. Last, but not least, our CI is broken with the patches here. And that is because now one cannot do save-image-edit, with both the new and the old format with: error: failed to write header to domain save file '/home/nert/test.img': Bad file descriptor And that's about what I've found out. I'll spend some time on this, trying to fix it up, but if anyone has a fix ready, then even better. What I'd like to know is whether we can
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/21/25 04:21, Martin Kletzander wrote:
On Fri, Mar 21, 2025 at 08:29:30AM +0000, Daniel P. Berrangé via Devel wrote:
On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote:
On 3/20/25 17:07, Pavel Hrdina via Devel wrote:
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag
Heh, I'm having one of those "why did I not realize that" moments :-).
IIRC, original we were using --parallel to decide whether to enable the new format, so --parallel with channels == 1 /was/ different from not setting --parallel at all.
Well, restore should figure out the format and save still has --image-format without which parallel save will fail (unless you have it set as default in a config). So I think this series is fine, at least it fixes the unit tests.
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
But there's more to it than meets the eye.
There is also a check that the number of parallel channels is not lower than 1, for saving. Restoring happily takes parallel.channels=0 and (at least for non-sparse images) fails weirdly with:
error: Failed to restore domain from test.img error: Failed to open file 'test.img': No such file or directory
and the daemon reports:
2025-03-21 10:00:08.490+0000: 4076349: error : virFileIsSharedFSType:3598 : Invalid relative path 'test.img': Invalid argument 2025-03-21 10:00:08.490+0000: 4076349: error : virQEMUFileOpenAs:10448 : Failed to open file 'test.img': No such file or directory
even though the file exists. And it works without --parallel-channels, this only happens with --parallel-channels 0.
Opps, I didn't test that. I did verify save with '--parallel-channels 0' is handled properly.
Last, but not least, our CI is broken with the patches here. And that is because now one cannot do save-image-edit, with both the new and the old format with:
error: failed to write header to domain save file '/home/nert/test.img': Bad file descriptor
I didn't check save-image-edit :-/.
And that's about what I've found out. I'll spend some time on this, trying to fix it up, but if anyone has a fix ready, then even better.
I've committed to a task that involves others today, so wont be able to look at these issues until later.
What I'd like to know is whether we can
Did you have more to say? :-) Regards, Jim
On 3/21/25 04:21, Martin Kletzander wrote:
On Fri, Mar 21, 2025 at 08:29:30AM +0000, Daniel P. Berrangé via Devel wrote:
On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote:
On 3/20/25 17:07, Pavel Hrdina via Devel wrote:
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag
Heh, I'm having one of those "why did I not realize that" moments :-).
IIRC, original we were using --parallel to decide whether to enable the new format, so --parallel with channels == 1 /was/ different from not setting --parallel at all.
Well, restore should figure out the format and save still has --image-format without which parallel save will fail (unless you have it set as default in a config). So I think this series is fine, at least it fixes the unit tests.
Agreed. The new format is enabled with --image-format parameter or by setting it as default in qemu.conf. And trying to use --parallel-channels without also specifying sparse fails as expected # virsh save --parallel-channels 4 test-vm /data/test-vm.sav error: Failed to save domain 'test-vm' to /data/test-vm.sav error: invalid argument: Parallel save is only supported with the 'sparse' save image format
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
But there's more to it than meets the eye.
There is also a check that the number of parallel channels is not lower than 1, for saving. Restoring happily takes parallel.channels=0 and (at least for non-sparse images) fails weirdly with:
error: Failed to restore domain from test.img error: Failed to open file 'test.img': No such file or directory
and the daemon reports:
2025-03-21 10:00:08.490+0000: 4076349: error : virFileIsSharedFSType:3598 : Invalid relative path 'test.img': Invalid argument 2025-03-21 10:00:08.490+0000: 4076349: error : virQEMUFileOpenAs:10448 : Failed to open file 'test.img': No such file or directory
even though the file exists. And it works without --parallel-channels, this only happens with --parallel-channels 0.
Hmm, I don't see this issue # virsh restore --parallel-channels 0 /data/test-vm.sav.sparse error: Failed to restore domain from /data/test-vm.sav.sparse error: invalid argument: number of parallel save channels cannot be less than 1 Could it be that you are using a relative path? Daniel and I just discussed this in patch1 of the V4 mapped-ram series. I'd provide a link, but https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/ is currently down for me. I dropped that patch since we prefer absolute paths.
Last, but not least, our CI is broken with the patches here. And that is because now one cannot do save-image-edit, with both the new and the old format with:
error: failed to write header to domain save file '/home/nert/test.img': Bad file descriptor
I do see this issue and should have time to investigate it tomorrow. Regards, Jim
participants (4)
-
Daniel P. Berrangé -
Jim Fehlig -
Martin Kletzander -
Pavel Hrdina