8:07 a.m.
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
8:07 a.m.
New subject: [libvirt PATCH 1/6] tools: remove --parallel from virsh restore command
From: Pavel Hrdina <phrdina(a)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(a)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
8:07 a.m.
New subject: [libvirt PATCH 2/6] tools: remote --parallel from virsh save command
From: Pavel Hrdina <phrdina(a)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(a)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
8:07 a.m.
New subject: [libvirt PATCH 3/6] qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag
From: Pavel Hrdina <phrdina(a)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(a)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
8:07 a.m.
New subject: [libvirt PATCH 4/6] tools: use virDomainRestoreParams only when necessary
From: Pavel Hrdina <phrdina(a)redhat.com>
We should use the newest API only when user sets parallel-channels.
Signed-off-by: Pavel Hrdina <phrdina(a)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
8:07 a.m.
New subject: [libvirt PATCH 5/6] tools: use virDomainSaveParams only when necessary
From: Pavel Hrdina <phrdina(a)redhat.com>
We should use the newest API only when user sets parallel-channels.
Signed-off-by: Pavel Hrdina <phrdina(a)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
8:07 a.m.
New subject: [libvirt PATCH 6/6] virsh: add --image-format option to the save command
From: Pavel Hrdina <phrdina(a)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(a)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
12:36 p.m.
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(a)suse.com>
Regards,
Jim
5:29 p.m.
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 :|
7:21 p.m.
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(a)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 :|
11:44 p.m.
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(a)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
11:20 a.m.
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(a)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
9
days inactive
13
days old
11 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Jim Fehlig -
Martin Kletzander -
Pavel Hrdina