5:43 p.m.
This series is a RFC for support of QEMU's mapped-ram migration
capability [1] for saving and restoring VMs. It implements the first
part of the design approach we discussed for supporting parallel
save/restore [2]. In summary, the approach is
1. Add mapped-ram migration capability
2. Steal an element from save header 'unused' for a 'features' variable
and bump save version to 3.
3. Add /etc/libvirt/qemu.conf knob for the save format version,
defaulting to latest v3
4. Use v3 (aka mapped-ram) by default
5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
6. include: Define constants for parallel save/restore
7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
8. qemu: Add support for parallel restore. Implies mapped-ram.
Reject if v2
9. tools: add parallel parameter to virsh save command
10. tools: add parallel parameter to virsh restore command
This series implements 1-5, with the BYPASS_CACHE support in patches 8
and 9 being quite hacky. They are included to discuss approaches to make
them less hacky. See the patches for details.
The QEMU mapped-ram capability currently does not support directio.
Fabino is working on that now [3]. This complicates merging support
in libvirt. I don't think it's reasonable to enable mapped-ram by
default when BYPASS_CACHE cannot be supported. Should we wait until
the mapped-ram directio support is merged in QEMU before supporting
mapped-ram in libvirt?
For the moment, compression is ignored in the new save version.
Currently, libvirt connects the output of QEMU's save stream to the
specified compression program via a pipe. This approach is incompatible
with mapped-ram since the fd provided to QEMU must be seekable. One
option is to reopen and compress the saved image after the actual save
operation has completed. This has the downside of requiring the iohelper
to handle BYPASS_CACHE, which would preclude us from removing it
sometime in the future. Other suggestions much welcomed.
Note the logical file size of mapped-ram saved images is slightly
larger than guest RAM size, so the files are often much larger than the
files produced by the existing, sequential format. However, actual blocks
written to disk is often lower with mapped-ram saved images. E.g. a saved
image from a 30G, freshly booted, idle guest results in the following
'Size' and 'Blocks' values reported by stat(1)
Size Blocks
sequential 998595770 1950392
mapped-ram 34368584225 1800456
With the same guest running a workload that dirties memory
Size Blocks
sequential 33173330615 64791672
mapped-ram 34368578210 64706944
Thanks for any comments on this RFC!
[1]
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/m...
[2]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/K...
[3] https://mail.gnu.org/archive/html/qemu-devel/2024-05/msg04432.html
Jim Fehlig (9):
qemu: Enable mapped-ram migration capability
qemu_fd: Add function to retrieve fdset ID
qemu: Add function to get migration params for save
qemu: Add a 'features' element to save image header and bump version
qemu: conf: Add setting for save image version
qemu: Add support for mapped-ram on save
qemu: Enable mapped-ram on restore
qemu: Support O_DIRECT with mapped-ram on save
qemu: Support O_DIRECT with mapped-ram on restore
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf.in | 6 +
src/qemu/qemu_conf.c | 8 ++
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_driver.c | 25 ++--
src/qemu/qemu_fd.c | 18 +++
src/qemu/qemu_fd.h | 3 +
src/qemu/qemu_migration.c | 99 ++++++++++++++-
src/qemu/qemu_migration.h | 11 +-
src/qemu/qemu_migration_params.c | 20 +++
src/qemu/qemu_migration_params.h | 4 +
src/qemu/qemu_monitor.c | 40 ++++++
src/qemu/qemu_monitor.h | 5 +
src/qemu/qemu_process.c | 63 +++++++---
src/qemu/qemu_process.h | 16 ++-
src/qemu/qemu_saveimage.c | 187 +++++++++++++++++++++++------
src/qemu/qemu_saveimage.h | 20 ++-
src/qemu/qemu_snapshot.c | 12 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 +
19 files changed, 455 insertions(+), 85 deletions(-)
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 1/9] qemu: Enable mapped-ram migration capability
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_migration_params.c | 1 +
src/qemu/qemu_migration_params.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 48f8657f71..201286e58c 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -105,6 +105,7 @@ VIR_ENUM_IMPL(qemuMigrationCapability,
"return-path",
"zero-copy-send",
"postcopy-preempt",
+ "mapped-ram",
);
diff --git a/src/qemu/qemu_migration_params.h b/src/qemu/qemu_migration_params.h
index 91bc6792cd..a14b189b48 100644
--- a/src/qemu/qemu_migration_params.h
+++ b/src/qemu/qemu_migration_params.h
@@ -41,6 +41,7 @@ typedef enum {
QEMU_MIGRATION_CAP_RETURN_PATH,
QEMU_MIGRATION_CAP_ZERO_COPY_SEND,
QEMU_MIGRATION_CAP_POSTCOPY_PREEMPT,
+ QEMU_MIGRATION_CAP_MAPPED_RAM,
QEMU_MIGRATION_CAP_LAST
} qemuMigrationCapability;
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 2/9] qemu_fd: Add function to retrieve fdset ID
Add new function qemuFDPassGetId(), to be used when adding support
for mapped-ram save format.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_fd.c | 18 ++++++++++++++++++
src/qemu/qemu_fd.h | 3 +++
2 files changed, 21 insertions(+)
diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c
index e847056573..3ae4a87a20 100644
--- a/src/qemu/qemu_fd.c
+++ b/src/qemu/qemu_fd.c
@@ -276,6 +276,24 @@ qemuFDPassGetPath(qemuFDPass *fdpass)
}
+/**
+ * qemuFDPassGetId:
+ * @fdpass: The fd passing helper struct
+ * @id: An out parameter for providing the fdset ID
+ *
+ * Returns 0 on success, -1 on error.
+ */
+int
+qemuFDPassGetId(qemuFDPass *fdpass, unsigned int *id)
+{
+ if (!fdpass)
+ return -1;
+
+ *id = fdpass->fdSetID;
+ return 0;
+}
+
+
struct _qemuFDPassDirect {
int fd;
char *name;
diff --git a/src/qemu/qemu_fd.h b/src/qemu/qemu_fd.h
index cd0ff2c690..dfcd5fb754 100644
--- a/src/qemu/qemu_fd.h
+++ b/src/qemu/qemu_fd.h
@@ -58,6 +58,9 @@ qemuFDPassTransferMonitorRollback(qemuFDPass *fdpass,
const char *
qemuFDPassGetPath(qemuFDPass *fdpass);
+int
+qemuFDPassGetId(qemuFDPass *fdpass, unsigned int *id);
+
typedef struct _qemuFDPassDirect qemuFDPassDirect;
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 3/9] qemu: Add function to get migration params for save
Introduce qemuMigrationParamsForMappedSave() to create a
qemuMigrationParams object initialized with appropriate migration
capabilities and parameters for a save operation using mapped-ram.
Note that mapped-ram also requires the multifd capability. For
now, the number of multifd channels is set to 1. Future work
to support parallel save/restore can set the number of channels
specified by the user.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_migration_params.c | 19 +++++++++++++++++++
src/qemu/qemu_migration_params.h | 3 +++
2 files changed, 22 insertions(+)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 201286e58c..96698bde9f 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -781,6 +781,25 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
}
+qemuMigrationParams *
+qemuMigrationParamsForMappedSave(void)
+{
+ g_autoptr(qemuMigrationParams) saveParams = NULL;
+
+ if (!(saveParams = qemuMigrationParamsNew()))
+ return NULL;
+
+ if (virBitmapSetBit(saveParams->caps, QEMU_MIGRATION_CAP_MAPPED_RAM) < 0)
+ return NULL;
+ if (virBitmapSetBit(saveParams->caps, QEMU_MIGRATION_CAP_MULTIFD) < 0)
+ return NULL;
+ saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].value.i = 1;
+ saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set = true;
+
+ return g_steal_pointer(&saveParams);
+}
+
+
int
qemuMigrationParamsDump(qemuMigrationParams *migParams,
virTypedParameterPtr *params,
diff --git a/src/qemu/qemu_migration_params.h b/src/qemu/qemu_migration_params.h
index a14b189b48..0f3ed07384 100644
--- a/src/qemu/qemu_migration_params.h
+++ b/src/qemu/qemu_migration_params.h
@@ -85,6 +85,9 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
unsigned int flags,
qemuMigrationParty party);
+qemuMigrationParams *
+qemuMigrationParamsForMappedSave(void);
+
int
qemuMigrationParamsDump(qemuMigrationParams *migParams,
virTypedParameterPtr *params,
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 4/9] qemu: Add a 'features' element to save image header and bump version
QEMU's new mapped-ram stream format [1] is incompatible with the
existing sequential format. In order to support the new format in
libvirt, a new 'features' element is added to the saved image header.
This element can be used now indicate the use of mapped-ram feature,
and provides a mechanism to support future save image features.
An older libvirt that is unaware of mapped-ram must not attempt to
open and restore a mapped-ram saved image. ATM, this can only be
done by bumping the save image version.
[1]
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/m...
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_saveimage.c | 7 +++++++
src/qemu/qemu_saveimage.h | 9 +++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 018ab5a222..50fec33f54 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -74,6 +74,7 @@ qemuSaveImageBswapHeader(virQEMUSaveHeader *hdr)
hdr->was_running = GUINT32_SWAP_LE_BE(hdr->was_running);
hdr->compressed = GUINT32_SWAP_LE_BE(hdr->compressed);
hdr->cookieOffset = GUINT32_SWAP_LE_BE(hdr->cookieOffset);
+ hdr->features = GUINT32_SWAP_LE_BE(hdr->features);
}
@@ -637,6 +638,12 @@ qemuSaveImageOpen(virQEMUDriver *driver,
return -1;
}
+ if (header->features && header->features !=
QEMU_SAVE_FEATURE_MAPPED_RAM) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("image contains unsupported features)"));
+ return -1;
+ }
+
if (header->cookieOffset)
xml_len = header->cookieOffset;
else
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index e541792153..9dd7de292d 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -28,10 +28,14 @@
*/
#define QEMU_SAVE_MAGIC "LibvirtQemudSave"
#define QEMU_SAVE_PARTIAL "LibvirtQemudPart"
-#define QEMU_SAVE_VERSION 2
+#define QEMU_SAVE_VERSION 3
G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL));
+typedef enum {
+ QEMU_SAVE_FEATURE_MAPPED_RAM = 1 << 0,
+} qemuSaveFeatures;
+
typedef struct _virQEMUSaveHeader virQEMUSaveHeader;
struct _virQEMUSaveHeader {
char magic[sizeof(QEMU_SAVE_MAGIC)-1];
@@ -40,7 +44,8 @@ struct _virQEMUSaveHeader {
uint32_t was_running;
uint32_t compressed;
uint32_t cookieOffset;
- uint32_t unused[14];
+ uint32_t features;
+ uint32_t unused[13];
};
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 5/9] qemu: conf: Add setting for save image version
Add a 'save_image_version' setting to qemu.conf to control the image
version when saving a VM with 'virsh save' or 'virsh managedsave'.
Default to the new version 3.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf.in | 6 ++++++
src/qemu/qemu_conf.c | 8 ++++++++
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_driver.c | 3 +--
src/qemu/qemu_saveimage.c | 19 ++++++++++++++-----
src/qemu/qemu_saveimage.h | 6 +++---
src/qemu/qemu_snapshot.c | 4 ++--
src/qemu/test_libvirtd_qemu.aug.in | 1 +
9 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 2b6526538f..acbfcd9fc3 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -92,6 +92,7 @@ module Libvirtd_qemu =
| str_array_entry "namespaces"
let save_entry = str_entry "save_image_format"
+ | int_entry "save_image_version"
| str_entry "dump_image_format"
| str_entry "snapshot_image_format"
| str_entry "auto_dump_path"
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index 6bc2140dcb..b5df8c1cc6 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -590,6 +590,11 @@
# at scheduled saving, and it is an error if the specified save_image_format
# is not valid, or the requested compression program can't be found.
#
+# save_image_version is applicable when using 'virsh save' or 'virsh
managed-save'.
+# Currently only versions 2 and 3 are supported, with version 3 being the default.
+# If saved images must be compatible with an older libvirt without this setting,
+# then set save_image_format_version to 2.
+#
# dump_image_format is used when you use 'virsh dump' at emergency
# crashdump, and if the specified dump_image_format is not valid, or
# the requested compression program can't be found, this falls
@@ -601,6 +606,7 @@
# or the requested compression program can't be found.
#
#save_image_format = "raw"
+#save_image_version = 3
#dump_image_format = "raw"
#snapshot_image_format = "raw"
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4050a82341..3ed17165b8 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -66,6 +66,10 @@ VIR_LOG_INIT("qemu.qemu_conf");
#define QEMU_MIGRATION_PORT_MIN 49152
#define QEMU_MIGRATION_PORT_MAX 49215
+/* Need to reconsile definition here and in qemu_saveimage.h */
+#define QEMU_SAVE_VERSION 3
+
+
VIR_ENUM_IMPL(virQEMUSchedCore,
QEMU_SCHED_CORE_LAST,
"none",
@@ -246,6 +250,8 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged,
cfg->migrationPortMin = QEMU_MIGRATION_PORT_MIN;
cfg->migrationPortMax = QEMU_MIGRATION_PORT_MAX;
+ cfg->saveImageVersion = QEMU_SAVE_VERSION;
+
/* For privileged driver, try and find hugetlbfs mounts automatically.
* Non-privileged driver requires admin to create a dir for the
* user, chown it, and then let user configure it manually. */
@@ -610,6 +616,8 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
{
if (virConfGetValueString(conf, "save_image_format",
&cfg->saveImageFormat) < 0)
return -1;
+ if (virConfGetValueUInt(conf, "save_image_version",
&cfg->saveImageVersion) < 0)
+ return -1;
if (virConfGetValueString(conf, "dump_image_format",
&cfg->dumpImageFormat) < 0)
return -1;
if (virConfGetValueString(conf, "snapshot_image_format",
&cfg->snapshotImageFormat) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 36049b4bfa..30f5c2151e 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -193,6 +193,7 @@ struct _virQEMUDriverConfig {
bool securityRequireConfined;
char *saveImageFormat;
+ unsigned int saveImageVersion;
char *dumpImageFormat;
char *snapshotImageFormat;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e2698c7924..f9761242d2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2696,8 +2696,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver,
if (!(cookie = qemuDomainSaveCookieNew(vm)))
goto endjob;
- if (!(data = virQEMUSaveDataNew(xml, cookie, was_running, compressed,
- driver->xmlopt)))
+ if (!(data = virQEMUSaveDataNew(driver, xml, cookie, was_running, compressed)))
goto endjob;
xml = NULL;
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 50fec33f54..30085dc7bc 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -95,25 +95,34 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree);
* This function steals @domXML on success.
*/
virQEMUSaveData *
-virQEMUSaveDataNew(char *domXML,
+virQEMUSaveDataNew(virQEMUDriver *driver,
+ char *domXML,
qemuDomainSaveCookie *cookieObj,
bool running,
- int compressed,
- virDomainXMLOption *xmlopt)
+ int compressed)
{
virQEMUSaveData *data = NULL;
virQEMUSaveHeader *header;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
data = g_new0(virQEMUSaveData, 1);
if (cookieObj &&
!(data->cookie = virSaveCookieFormat((virObject *) cookieObj,
- virDomainXMLOptionGetSaveCookie(xmlopt))))
+
virDomainXMLOptionGetSaveCookie(driver->xmlopt))))
goto error;
header = &data->header;
memcpy(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic));
- header->version = QEMU_SAVE_VERSION;
+
+ if (cfg->saveImageVersion < 2 || cfg->saveImageVersion >
QEMU_SAVE_VERSION) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid save image version %1$u"),
+ cfg->saveImageVersion);
+ goto error;
+ }
+ header->version = cfg->saveImageVersion;
+
header->was_running = running ? 1 : 0;
header->compressed = compressed;
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 9dd7de292d..63ad5508ed 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -123,11 +123,11 @@ virQEMUSaveDataWrite(virQEMUSaveData *data,
const char *path);
virQEMUSaveData *
-virQEMUSaveDataNew(char *domXML,
+virQEMUSaveDataNew(virQEMUDriver *driver,
+ char *domXML,
qemuDomainSaveCookie *cookieObj,
bool running,
- int compressed,
- virDomainXMLOption *xmlopt);
+ int compressed);
void
virQEMUSaveDataFree(virQEMUSaveData *data);
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index f5260c4a22..1d75208814 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1390,9 +1390,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
!(snapdef->cookie = (virObject *) qemuDomainSaveCookieNew(vm)))
goto cleanup;
- if (!(data = virQEMUSaveDataNew(xml,
+ if (!(data = virQEMUSaveDataNew(driver, xml,
(qemuDomainSaveCookie *) snapdef->cookie,
- resume, compressed, driver->xmlopt)))
+ resume, compressed)))
goto cleanup;
xml = NULL;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index b97e6de11e..8bcb332020 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -70,6 +70,7 @@ module Test_libvirtd_qemu =
{ "8" = "/dev/userfaultfd" }
}
{ "save_image_format" = "raw" }
+{ "save_image_version" = "3" }
{ "dump_image_format" = "raw" }
{ "snapshot_image_format" = "raw" }
{ "auto_dump_path" = "/var/lib/libvirt/qemu/dump" }
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 6/9] qemu: Add support for mapped-ram on save
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_migration.c | 79 ++++++++++++++++++++++++++++
src/qemu/qemu_migration.h | 7 +++
src/qemu/qemu_monitor.c | 32 ++++++++++++
src/qemu/qemu_monitor.h | 4 ++
src/qemu/qemu_saveimage.c | 105 ++++++++++++++++++++++++++++++--------
src/qemu/qemu_saveimage.h | 1 +
src/qemu/qemu_snapshot.c | 2 +-
8 files changed, 208 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f9761242d2..34f37210d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2696,7 +2696,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver,
if (!(cookie = qemuDomainSaveCookieNew(vm)))
goto endjob;
- if (!(data = virQEMUSaveDataNew(driver, xml, cookie, was_running, compressed)))
+ if (!(data = virQEMUSaveDataNew(driver, vm, xml, cookie, was_running, compressed)))
goto endjob;
xml = NULL;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1faab5dd23..3110ef2621 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -7072,6 +7072,85 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm,
}
+int
+qemuMigrationSrcToMappedFile(virQEMUDriver *driver, virDomainObj *vm,
+ int fd,
+ virDomainAsyncJob asyncJob)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(qemuMigrationParams) saveParams = NULL;
+ unsigned long saveMigBandwidth = priv->migMaxBandwidth;
+ int rc;
+ int ret = -1;
+ virErrorPtr orig_err = NULL;
+
+ if (qemuMigrationSetDBusVMState(driver, vm) < 0)
+ return -1;
+
+ if (!(saveParams = qemuMigrationParamsForMappedSave()))
+ return -1;
+
+ /* Increase migration bandwidth to unlimited since target is a file.
+ * Failure to change migration speed is not fatal. */
+ if (qemuMigrationParamsSetULL(saveParams,
+ QEMU_MIGRATION_PARAM_MAX_BANDWIDTH,
+ QEMU_DOMAIN_MIG_BANDWIDTH_MAX * 1024 * 1024) < 0)
+ return -1;
+
+ if (qemuMigrationParamsApply(vm, asyncJob, saveParams, 0) < 0)
+ return -1;
+
+ priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ /* nothing to tear down */
+ return -1;
+ }
+
+ if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
+ goto cleanup;
+
+ rc = qemuMonitorMigrateToFdSet(vm, 0, fd);
+ qemuDomainObjExitMonitor(vm);
+ if (rc < 0)
+ goto cleanup;
+
+ rc = qemuMigrationSrcWaitForCompletion(vm, asyncJob, NULL, 0);
+
+ if (rc < 0) {
+ if (rc == -2) {
+ virErrorPreserveLast(&orig_err);
+ if (virDomainObjIsActive(vm))
+ qemuMigrationSrcCancel(vm, asyncJob, true);
+ }
+ goto cleanup;
+ }
+
+ qemuDomainEventEmitJobCompleted(driver, vm);
+ ret = 0;
+
+ cleanup:
+ if (ret < 0 && !orig_err)
+ virErrorPreserveLast(&orig_err);
+
+ /* Restore max migration bandwidth */
+ if (virDomainObjIsActive(vm)) {
+ if (qemuMigrationParamsSetULL(saveParams,
+ QEMU_MIGRATION_PARAM_MAX_BANDWIDTH,
+ saveMigBandwidth * 1024 * 1024) == 0)
+ ignore_value(qemuMigrationParamsApply(vm, asyncJob,
+ saveParams, 0));
+ priv->migMaxBandwidth = saveMigBandwidth;
+ }
+
+ virErrorRestore(&orig_err);
+
+ return ret;
+}
+
+
/**
* This function is supposed to be used only to while reconnecting to a domain
* with an active migration job.
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index ed62fd4a91..f845a0198b 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -241,6 +241,13 @@ qemuMigrationSrcToFile(virQEMUDriver *driver,
virDomainAsyncJob asyncJob)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
+int
+qemuMigrationSrcToMappedFile(virQEMUDriver *driver,
+ virDomainObj *vm,
+ int fd,
+ virDomainAsyncJob asyncJob)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
+
int
qemuMigrationSrcCancelUnattended(virDomainObj *vm,
virDomainJobObj *oldJob);
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 34e2ccab97..4c92bd740a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2237,6 +2237,38 @@ qemuMonitorMigrateToFd(qemuMonitor *mon,
}
+int
+qemuMonitorMigrateToFdSet(virDomainObj *vm,
+ unsigned int flags,
+ int fd)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ qemuMonitor *mon = priv->mon;
+ off_t offset;
+ g_autoptr(qemuFDPass) fdPassMigrate = NULL;
+ unsigned int setId;
+ g_autofree char *uri = NULL;
+
+ VIR_DEBUG("fd=%d flags=0x%x", fd, flags);
+
+ QEMU_CHECK_MONITOR(mon);
+
+ if ((offset = lseek(fd, 0, SEEK_CUR)) == -1)
+ return -1;
+
+ fdPassMigrate = qemuFDPassNew("migrate", priv);
+ qemuFDPassAddFD(fdPassMigrate, &fd, "-fd");
+ qemuFDPassTransferMonitor(fdPassMigrate, mon);
+
+ if (qemuFDPassGetId(fdPassMigrate, &setId) < 0)
+ return -1;
+
+ uri = g_strdup_printf("file:/dev/fdset/%u,offset=%#lx", setId, offset);
+
+ return qemuMonitorJSONMigrate(mon, flags, uri);
+}
+
+
int
qemuMonitorMigrateToHost(qemuMonitor *mon,
unsigned int flags,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 6e81945201..c477def138 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -843,6 +843,10 @@ int qemuMonitorMigrateToFd(qemuMonitor *mon,
unsigned int flags,
int fd);
+int qemuMonitorMigrateToFdSet(virDomainObj *vm,
+ unsigned int flags,
+ int fd);
+
int qemuMonitorMigrateToHost(qemuMonitor *mon,
unsigned int flags,
const char *protocol,
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 30085dc7bc..8f28770086 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -96,6 +96,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree);
*/
virQEMUSaveData *
virQEMUSaveDataNew(virQEMUDriver *driver,
+ virDomainObj *vm,
char *domXML,
qemuDomainSaveCookie *cookieObj,
bool running,
@@ -122,6 +123,10 @@ virQEMUSaveDataNew(virQEMUDriver *driver,
goto error;
}
header->version = cfg->saveImageVersion;
+ /* Enable mapped-ram feature if available and save version >= 3 */
+ if (header->version >= QEMU_SAVE_VERSION &&
+ qemuMigrationCapsGet(vm, QEMU_MIGRATION_CAP_MAPPED_RAM))
+ header->features |= QEMU_SAVE_FEATURE_MAPPED_RAM;
header->was_running = running ? 1 : 0;
header->compressed = compressed;
@@ -369,6 +374,79 @@ qemuSaveImageDecompressionStop(virCommand *cmd,
}
+static int
+qemuSaveImageCreateSequential(virQEMUDriver *driver,
+ virDomainObj *vm,
+ const char *path,
+ int fd,
+ virQEMUSaveData *data,
+ virCommand *compressor,
+ unsigned int flags,
+ virDomainAsyncJob asyncJob)
+{
+ int ret = -1;
+ virFileWrapperFd *wrapperFd = NULL;
+ unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
+
+ if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE))
+ wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
+
+ if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
+ goto cleanup;
+
+ if (virQEMUSaveDataWrite(data, fd, path) < 0)
+ goto cleanup;
+
+ /* Perform the migration */
+ if (qemuMigrationSrcToFile(driver, vm, fd, compressor, asyncJob) < 0)
+ goto cleanup;
+
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno, _("unable to close %1$s"), path);
+ goto cleanup;
+ }
+
+ if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ VIR_FORCE_CLOSE(fd);
+ if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
+ ret = -1;
+ virFileWrapperFdFree(wrapperFd);
+
+ return ret;
+}
+
+
+static int
+qemuSaveImageCreateMapped(virQEMUDriver *driver,
+ virDomainObj *vm,
+ const char *path,
+ int fd,
+ virQEMUSaveData *data,
+ unsigned int flags,
+ virDomainAsyncJob asyncJob)
+{
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
+ /* mapped-ram does not support directIO */
+ if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("bypass cache unsupported by this system"));
+ return -1;
+ }
+
+ if (virQEMUSaveDataWrite(data, fd, path) < 0)
+ return -1;
+
+ /* Perform the migration */
+ return qemuMigrationSrcToMappedFile(driver, vm, fd, asyncJob);
+}
+
+
/* Helper function to execute a migration to file with a correct save header
* the caller needs to make sure that the processors are stopped and do all other
* actions besides saving memory */
@@ -386,12 +464,9 @@ qemuSaveImageCreate(virQEMUDriver *driver,
int ret = -1;
int fd = -1;
int directFlag = 0;
- virFileWrapperFd *wrapperFd = NULL;
- unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
/* Obtain the file handle. */
if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
- wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
directFlag = virFileDirectFdFlag();
if (directFlag < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
@@ -409,14 +484,12 @@ qemuSaveImageCreate(virQEMUDriver *driver,
if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
goto cleanup;
- if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
- goto cleanup;
+ if (data->header.features & QEMU_SAVE_FEATURE_MAPPED_RAM)
+ ret = qemuSaveImageCreateMapped(driver, vm, path, fd, data, flags, asyncJob);
+ else
+ ret = qemuSaveImageCreateSequential(driver, vm, path, fd, data, compressor,
flags, asyncJob);
- if (virQEMUSaveDataWrite(data, fd, path) < 0)
- goto cleanup;
-
- /* Perform the migration */
- if (qemuMigrationSrcToFile(driver, vm, fd, compressor, asyncJob) < 0)
+ if (ret < 0)
goto cleanup;
/* Touch up file header to mark image complete. */
@@ -425,14 +498,6 @@ qemuSaveImageCreate(virQEMUDriver *driver,
* up to seek backwards on wrapperFd. The reopened fd will
* trigger a single page of file system cache pollution, but
* that's acceptable. */
- if (VIR_CLOSE(fd) < 0) {
- virReportSystemError(errno, _("unable to close %1$s"), path);
- goto cleanup;
- }
-
- if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
- goto cleanup;
-
if ((fd = qemuDomainOpenFile(cfg, vm->def, path, O_WRONLY, NULL)) < 0 ||
virQEMUSaveDataFinish(data, &fd, path) < 0)
goto cleanup;
@@ -441,10 +506,6 @@ qemuSaveImageCreate(virQEMUDriver *driver,
cleanup:
VIR_FORCE_CLOSE(fd);
- if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
- ret = -1;
- virFileWrapperFdFree(wrapperFd);
-
if (ret < 0 && needUnlink)
unlink(path);
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 63ad5508ed..81d93bf33c 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -124,6 +124,7 @@ virQEMUSaveDataWrite(virQEMUSaveData *data,
virQEMUSaveData *
virQEMUSaveDataNew(virQEMUDriver *driver,
+ virDomainObj *vm,
char *domXML,
qemuDomainSaveCookie *cookieObj,
bool running,
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 1d75208814..1e9e0e31d7 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1390,7 +1390,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
!(snapdef->cookie = (virObject *) qemuDomainSaveCookieNew(vm)))
goto cleanup;
- if (!(data = virQEMUSaveDataNew(driver, xml,
+ if (!(data = virQEMUSaveDataNew(driver, vm, xml,
(qemuDomainSaveCookie *) snapdef->cookie,
resume, compressed)))
goto cleanup;
--
2.44.0
7:33 a.m.
New subject: [PATCH RFC 6/9] qemu: Add support for mapped-ram on save
On Thu, Jun 13, 2024 at 04:43:20PM -0600, Jim Fehlig via Devel wrote:
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_migration.c | 79 ++++++++++++++++++++++++++++
src/qemu/qemu_migration.h | 7 +++
src/qemu/qemu_monitor.c | 32 ++++++++++++
src/qemu/qemu_monitor.h | 4 ++
src/qemu/qemu_saveimage.c | 105 ++++++++++++++++++++++++++++++--------
src/qemu/qemu_saveimage.h | 1 +
src/qemu/qemu_snapshot.c | 2 +-
8 files changed, 208 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f9761242d2..34f37210d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2696,7 +2696,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver,
if (!(cookie = qemuDomainSaveCookieNew(vm)))
goto endjob;
- if (!(data = virQEMUSaveDataNew(driver, xml, cookie, was_running, compressed)))
+ if (!(data = virQEMUSaveDataNew(driver, vm, xml, cookie, was_running, compressed)))
goto endjob;
xml = NULL;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1faab5dd23..3110ef2621 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -7072,6 +7072,85 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm,
}
+int
+qemuMigrationSrcToMappedFile(virQEMUDriver *driver, virDomainObj *vm,
+ int fd,
+ virDomainAsyncJob asyncJob)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(qemuMigrationParams) saveParams = NULL;
+ unsigned long saveMigBandwidth = priv->migMaxBandwidth;
+ int rc;
+ int ret = -1;
+ virErrorPtr orig_err = NULL;
+
+ if (qemuMigrationSetDBusVMState(driver, vm) < 0)
+ return -1;
+
+ if (!(saveParams = qemuMigrationParamsForMappedSave()))
+ return -1;
+
+ /* Increase migration bandwidth to unlimited since target is a file.
+ * Failure to change migration speed is not fatal. */
+ if (qemuMigrationParamsSetULL(saveParams,
+ QEMU_MIGRATION_PARAM_MAX_BANDWIDTH,
+ QEMU_DOMAIN_MIG_BANDWIDTH_MAX * 1024 * 1024) < 0)
+ return -1;
+
+ if (qemuMigrationParamsApply(vm, asyncJob, saveParams, 0) < 0)
+ return -1;
+
+ priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ /* nothing to tear down */
+ return -1;
+ }
+
+ if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
+ goto cleanup;
+
+ rc = qemuMonitorMigrateToFdSet(vm, 0, fd);
+ qemuDomainObjExitMonitor(vm);
+ if (rc < 0)
+ goto cleanup;
+
+ rc = qemuMigrationSrcWaitForCompletion(vm, asyncJob, NULL, 0);
+
+ if (rc < 0) {
+ if (rc == -2) {
+ virErrorPreserveLast(&orig_err);
+ if (virDomainObjIsActive(vm))
+ qemuMigrationSrcCancel(vm, asyncJob, true);
+ }
+ goto cleanup;
+ }
+
+ qemuDomainEventEmitJobCompleted(driver, vm);
+ ret = 0;
+
+ cleanup:
+ if (ret < 0 && !orig_err)
+ virErrorPreserveLast(&orig_err);
+
+ /* Restore max migration bandwidth */
+ if (virDomainObjIsActive(vm)) {
+ if (qemuMigrationParamsSetULL(saveParams,
+ QEMU_MIGRATION_PARAM_MAX_BANDWIDTH,
+ saveMigBandwidth * 1024 * 1024) == 0)
+ ignore_value(qemuMigrationParamsApply(vm, asyncJob,
+ saveParams, 0));
+ priv->migMaxBandwidth = saveMigBandwidth;
+ }
+
+ virErrorRestore(&orig_err);
+
+ return ret;
+}
+
+
/**
* This function is supposed to be used only to while reconnecting to a domain
* with an active migration job.
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index ed62fd4a91..f845a0198b 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -241,6 +241,13 @@ qemuMigrationSrcToFile(virQEMUDriver *driver,
virDomainAsyncJob asyncJob)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
+int
+qemuMigrationSrcToMappedFile(virQEMUDriver *driver,
+ virDomainObj *vm,
+ int fd,
+ virDomainAsyncJob asyncJob)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
+
int
qemuMigrationSrcCancelUnattended(virDomainObj *vm,
virDomainJobObj *oldJob);
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 34e2ccab97..4c92bd740a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2237,6 +2237,38 @@ qemuMonitorMigrateToFd(qemuMonitor *mon,
}
+int
+qemuMonitorMigrateToFdSet(virDomainObj *vm,
+ unsigned int flags,
+ int fd)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ qemuMonitor *mon = priv->mon;
+ off_t offset;
+ g_autoptr(qemuFDPass) fdPassMigrate = NULL;
+ unsigned int setId;
+ g_autofree char *uri = NULL;
+
+ VIR_DEBUG("fd=%d flags=0x%x", fd, flags);
+
+ QEMU_CHECK_MONITOR(mon);
+
+ if ((offset = lseek(fd, 0, SEEK_CUR)) == -1)
+ return -1;
virReportSystemError() should probably be here or this could be
checked/gathered before entering the monitor
+
+ fdPassMigrate = qemuFDPassNew("migrate", priv);
+ qemuFDPassAddFD(fdPassMigrate, &fd, "-fd");
+ qemuFDPassTransferMonitor(fdPassMigrate, mon);
This function can fail (and set an error)
+
+ if (qemuFDPassGetId(fdPassMigrate, &setId) < 0)
+ return -1;
No error set here, but I see no way for it failing in this case.
+
+ uri = g_strdup_printf("file:/dev/fdset/%u,offset=%#lx", setId, offset);
+
+ return qemuMonitorJSONMigrate(mon, flags, uri);
+}
+
+
int
qemuMonitorMigrateToHost(qemuMonitor *mon,
unsigned int flags,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 6e81945201..c477def138 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -843,6 +843,10 @@ int qemuMonitorMigrateToFd(qemuMonitor *mon,
unsigned int flags,
int fd);
+int qemuMonitorMigrateToFdSet(virDomainObj *vm,
+ unsigned int flags,
+ int fd);
+
int qemuMonitorMigrateToHost(qemuMonitor *mon,
unsigned int flags,
const char *protocol,
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 30085dc7bc..8f28770086 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -96,6 +96,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree);
*/
virQEMUSaveData *
virQEMUSaveDataNew(virQEMUDriver *driver,
+ virDomainObj *vm,
char *domXML,
qemuDomainSaveCookie *cookieObj,
bool running,
@@ -122,6 +123,10 @@ virQEMUSaveDataNew(virQEMUDriver *driver,
goto error;
}
header->version = cfg->saveImageVersion;
+ /* Enable mapped-ram feature if available and save version >= 3 */
+ if (header->version >= QEMU_SAVE_VERSION &&
This should really be "3" and not the save version in case we increase
it, but to be readable without magic numbers and also more error proof,
there should be a mapping of feature->minVersion maybe?
+ qemuMigrationCapsGet(vm, QEMU_MIGRATION_CAP_MAPPED_RAM))
+ header->features |= QEMU_SAVE_FEATURE_MAPPED_RAM;
header->was_running = running ? 1 : 0;
header->compressed = compressed;
Also in case we add support for compressed mapped-ram save images this
might screw up later since you record the compression, but don't execute
it (as mentioned in the cover letter).
5:43 p.m.
New subject: [PATCH RFC 7/9] qemu: Enable mapped-ram on restore
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_migration.c | 19 +++++++++++----
src/qemu/qemu_migration.h | 3 ++-
src/qemu/qemu_process.c | 49 ++++++++++++++++++++++++++++-----------
src/qemu/qemu_process.h | 13 +++++++----
src/qemu/qemu_saveimage.c | 26 ++++++++++++++-------
5 files changed, 76 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 3110ef2621..b1d27e07e1 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2281,13 +2281,23 @@ qemuMigrationDstGetURI(const char *migrateFrom,
int
qemuMigrationDstRun(virDomainObj *vm,
const char *uri,
- virDomainAsyncJob asyncJob)
+ virDomainAsyncJob asyncJob,
+ unsigned int flags)
{
qemuDomainObjPrivate *priv = vm->privateData;
int rv;
VIR_DEBUG("Setting up incoming migration with URI %s", uri);
+ if (flags & VIR_QEMU_PROCESS_START_MAPPED_RAM) {
+ g_autoptr(qemuMigrationParams) migParams = NULL;
+
+ if (!(migParams = qemuMigrationParamsForMappedSave()))
+ return -1;
+
+ qemuMigrationParamsApply(vm, asyncJob, migParams, 0);
+ }
+
if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
return -1;
@@ -2945,7 +2955,6 @@ qemuMigrationDstPrepare(virDomainObj *vm,
unsigned short port,
int fd)
{
- qemuDomainObjPrivate *priv = vm->privateData;
g_autofree char *migrateFrom = NULL;
if (tunnel) {
@@ -2999,8 +3008,8 @@ qemuMigrationDstPrepare(virDomainObj *vm,
migrateFrom = g_strdup_printf(incFormat, protocol, listenAddress, port);
}
- return qemuProcessIncomingDefNew(priv->qemuCaps, listenAddress,
- migrateFrom, fd, NULL);
+ return qemuProcessIncomingDefNew(vm, listenAddress,
+ migrateFrom, fd, NULL, 0);
}
@@ -3238,7 +3247,7 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver,
}
if (qemuMigrationDstRun(vm, incoming->uri,
- VIR_ASYNC_JOB_MIGRATION_IN) < 0)
+ VIR_ASYNC_JOB_MIGRATION_IN, 0) < 0)
goto error;
if (qemuProcessFinishStartup(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN,
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index f845a0198b..864f3280e5 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -286,7 +286,8 @@ qemuMigrationDstGetURI(const char *migrateFrom,
int
qemuMigrationDstRun(virDomainObj *vm,
const char *uri,
- virDomainAsyncJob asyncJob);
+ virDomainAsyncJob asyncJob,
+ unsigned int flags);
void
qemuMigrationSrcPostcopyFailed(virDomainObj *vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7ef7040a85..f700390a8c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4684,28 +4684,43 @@ qemuProcessIncomingDefFree(qemuProcessIncomingDef *inc)
* qemuProcessIncomingDefFree will NOT close it.
*/
qemuProcessIncomingDef *
-qemuProcessIncomingDefNew(virQEMUCaps *qemuCaps,
+qemuProcessIncomingDefNew(virDomainObj *vm,
const char *listenAddress,
const char *migrateFrom,
int fd,
- const char *path)
+ const char *path,
+ unsigned int flags)
{
+ qemuDomainObjPrivate *priv = vm->privateData;
qemuProcessIncomingDef *inc = NULL;
- if (qemuMigrationDstCheckProtocol(qemuCaps, migrateFrom) < 0)
+ if (qemuMigrationDstCheckProtocol(priv->qemuCaps, migrateFrom) < 0)
return NULL;
inc = g_new0(qemuProcessIncomingDef, 1);
inc->address = g_strdup(listenAddress);
-
- inc->uri = qemuMigrationDstGetURI(migrateFrom, fd);
- if (!inc->uri)
- goto error;
-
inc->fd = fd;
inc->path = path;
+ if (flags & VIR_QEMU_PROCESS_START_MAPPED_RAM) {
+ unsigned int fdsetId;
+ off_t offset;
+
+ if ((offset = lseek(fd, 0, SEEK_CUR)) == -1)
+ offset = 0;
+
+ inc->fdPassMigrate = qemuFDPassNew("migrate", priv);
+ qemuFDPassAddFD(inc->fdPassMigrate, &fd, "-fd");
+ qemuFDPassGetId(inc->fdPassMigrate, &fdsetId);
+ inc->uri = g_strdup_printf("file:/dev/fdset/%u,offset=%#lx",
fdsetId, offset);
+ } else {
+ inc->uri = qemuMigrationDstGetURI(migrateFrom, fd);
+ }
+
+ if (!inc->uri)
+ goto error;
+
return inc;
error:
@@ -7667,7 +7682,8 @@ qemuProcessLaunch(virConnectPtr conn,
VIR_QEMU_PROCESS_START_AUTODESTROY |
VIR_QEMU_PROCESS_START_NEW |
VIR_QEMU_PROCESS_START_GEN_VMID |
- VIR_QEMU_PROCESS_START_RESET_NVRAM, -1);
+ VIR_QEMU_PROCESS_START_RESET_NVRAM |
+ VIR_QEMU_PROCESS_START_MAPPED_RAM, -1);
cfg = virQEMUDriverGetConfig(driver);
@@ -7717,8 +7733,12 @@ qemuProcessLaunch(virConnectPtr conn,
&nnicindexes, &nicindexes)))
goto cleanup;
- if (incoming && incoming->fd != -1)
+ if (incoming && incoming->fd != -1) {
virCommandPassFD(cmd, incoming->fd, 0);
+ if (incoming->fdPassMigrate != NULL) {
+ qemuFDPassTransferCommand(incoming->fdPassMigrate, cmd);
+ }
+ }
/* now that we know it is about to start call the hook if present */
if (qemuProcessStartHook(driver, vm,
@@ -8153,7 +8173,8 @@ qemuProcessStart(virConnectPtr conn,
VIR_QEMU_PROCESS_START_PAUSED |
VIR_QEMU_PROCESS_START_AUTODESTROY |
VIR_QEMU_PROCESS_START_GEN_VMID |
- VIR_QEMU_PROCESS_START_RESET_NVRAM, cleanup);
+ VIR_QEMU_PROCESS_START_RESET_NVRAM |
+ VIR_QEMU_PROCESS_START_MAPPED_RAM, cleanup);
if (!migrateFrom && !snapshot)
flags |= VIR_QEMU_PROCESS_START_NEW;
@@ -8163,8 +8184,8 @@ qemuProcessStart(virConnectPtr conn,
goto cleanup;
if (migrateFrom) {
- incoming = qemuProcessIncomingDefNew(priv->qemuCaps, NULL, migrateFrom,
- migrateFd, migratePath);
+ incoming = qemuProcessIncomingDefNew(vm, NULL, migrateFrom,
+ migrateFd, migratePath, flags);
if (!incoming)
goto stop;
}
@@ -8191,7 +8212,7 @@ qemuProcessStart(virConnectPtr conn,
relabel = true;
if (incoming) {
- if (qemuMigrationDstRun(vm, incoming->uri, asyncJob) < 0)
+ if (qemuMigrationDstRun(vm, incoming->uri, asyncJob, flags) < 0)
goto stop;
} else {
/* Refresh state of devices from QEMU. During migration this happens
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index c1ea949215..a5212ee56e 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -54,14 +54,16 @@ struct _qemuProcessIncomingDef {
char *address; /* address where QEMU is supposed to listen */
char *uri; /* used when calling migrate-incoming QMP command */
int fd; /* for fd:N URI */
+ qemuFDPass *fdPassMigrate; /* for file:/dev/fdset/n,offset=x URI */
const char *path; /* path associated with fd */
};
-qemuProcessIncomingDef *qemuProcessIncomingDefNew(virQEMUCaps *qemuCaps,
- const char *listenAddress,
- const char *migrateFrom,
- int fd,
- const char *path);
+qemuProcessIncomingDef *qemuProcessIncomingDefNew(virDomainObj *vm,
+ const char *listenAddress,
+ const char *migrateFrom,
+ int fd,
+ const char *path,
+ unsigned int flags);
void qemuProcessIncomingDefFree(qemuProcessIncomingDef *inc);
int qemuProcessBeginJob(virDomainObj *vm,
@@ -77,6 +79,7 @@ typedef enum {
VIR_QEMU_PROCESS_START_NEW = 1 << 4, /* internal, new VM is starting
*/
VIR_QEMU_PROCESS_START_GEN_VMID = 1 << 5, /* Generate a new VMID */
VIR_QEMU_PROCESS_START_RESET_NVRAM = 1 << 6, /* Re-initialize NVRAM from
template */
+ VIR_QEMU_PROCESS_START_MAPPED_RAM = 1 << 7, /* Re-initialize NVRAM from
template */
} qemuProcessStartFlags;
int qemuProcessStart(virConnectPtr conn,
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 8f28770086..1545c00472 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -628,6 +628,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
int oflags = open_write ? O_RDWR : O_RDONLY;
size_t xml_len;
size_t cookie_len;
+ bool use_mapped_ram = false;
if (bypass_cache) {
int directFlag = virFileDirectFdFlag();
@@ -642,11 +643,6 @@ qemuSaveImageOpen(virQEMUDriver *driver,
if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
return -1;
- if (bypass_cache &&
- !(*wrapperFd = virFileWrapperFdNew(&fd, path,
- VIR_FILE_WRAPPER_BYPASS_CACHE)))
- return -1;
-
data = g_new0(virQEMUSaveData, 1);
header = &data->header;
@@ -708,10 +704,14 @@ qemuSaveImageOpen(virQEMUDriver *driver,
return -1;
}
- if (header->features && header->features !=
QEMU_SAVE_FEATURE_MAPPED_RAM) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("image contains unsupported features)"));
- return -1;
+ if (header->features) {
+ if (header->features == QEMU_SAVE_FEATURE_MAPPED_RAM) {
+ use_mapped_ram = true;
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("image contains unsupported features)"));
+ return -1;
+ }
}
if (header->cookieOffset)
@@ -739,6 +739,11 @@ qemuSaveImageOpen(virQEMUDriver *driver,
}
}
+ if (bypass_cache && !use_mapped_ram &&
+ !(*wrapperFd = virFileWrapperFdNew(&fd, path,
+ VIR_FILE_WRAPPER_BYPASS_CACHE)))
+ return -1;
+
/* Create a domain from this XML */
if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps,
VIR_DOMAIN_DEF_PARSE_INACTIVE |
@@ -777,6 +782,9 @@ qemuSaveImageStartVM(virConnectPtr conn,
if (reset_nvram)
start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM;
+ if (header->features & QEMU_SAVE_FEATURE_MAPPED_RAM)
+ start_flags |= VIR_QEMU_PROCESS_START_MAPPED_RAM;
+
if (qemuProcessStartWithMemoryState(conn, driver, vm, fd, path, NULL, data,
asyncJob, start_flags, "restored",
&started) < 0) {
--
2.44.0
7:34 a.m.
New subject: [PATCH RFC 7/9] qemu: Enable mapped-ram on restore
On Thu, Jun 13, 2024 at 04:43:21PM -0600, Jim Fehlig via Devel wrote:
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_migration.c | 19 +++++++++++----
src/qemu/qemu_migration.h | 3 ++-
src/qemu/qemu_process.c | 49 ++++++++++++++++++++++++++++-----------
src/qemu/qemu_process.h | 13 +++++++----
src/qemu/qemu_saveimage.c | 26 ++++++++++++++-------
5 files changed, 76 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7ef7040a85..f700390a8c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7717,8 +7733,12 @@ qemuProcessLaunch(virConnectPtr conn,
&nnicindexes, &nicindexes)))
goto cleanup;
- if (incoming && incoming->fd != -1)
+ if (incoming && incoming->fd != -1) {
virCommandPassFD(cmd, incoming->fd, 0);
+ if (incoming->fdPassMigrate != NULL) {
+ qemuFDPassTransferCommand(incoming->fdPassMigrate, cmd);
No need for this last condition, qemuFDPassTransferCommand is no-op when
the first parameter is NULL.
+ }
+ }
/* now that we know it is about to start call the hook if present */
if (qemuProcessStartHook(driver, vm,
@@ -8153,7 +8173,8 @@ qemuProcessStart(virConnectPtr conn,
VIR_QEMU_PROCESS_START_PAUSED |
VIR_QEMU_PROCESS_START_AUTODESTROY |
VIR_QEMU_PROCESS_START_GEN_VMID |
- VIR_QEMU_PROCESS_START_RESET_NVRAM, cleanup);
+ VIR_QEMU_PROCESS_START_RESET_NVRAM |
+ VIR_QEMU_PROCESS_START_MAPPED_RAM, cleanup);
if (!migrateFrom && !snapshot)
flags |= VIR_QEMU_PROCESS_START_NEW;
@@ -8163,8 +8184,8 @@ qemuProcessStart(virConnectPtr conn,
goto cleanup;
if (migrateFrom) {
- incoming = qemuProcessIncomingDefNew(priv->qemuCaps, NULL, migrateFrom,
- migrateFd, migratePath);
+ incoming = qemuProcessIncomingDefNew(vm, NULL, migrateFrom,
+ migrateFd, migratePath, flags);
if (!incoming)
goto stop;
}
@@ -8191,7 +8212,7 @@ qemuProcessStart(virConnectPtr conn,
relabel = true;
if (incoming) {
- if (qemuMigrationDstRun(vm, incoming->uri, asyncJob) < 0)
+ if (qemuMigrationDstRun(vm, incoming->uri, asyncJob, flags) < 0)
goto stop;
} else {
/* Refresh state of devices from QEMU. During migration this happens
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index c1ea949215..a5212ee56e 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -54,14 +54,16 @@ struct _qemuProcessIncomingDef {
char *address; /* address where QEMU is supposed to listen */
char *uri; /* used when calling migrate-incoming QMP command */
int fd; /* for fd:N URI */
+ qemuFDPass *fdPassMigrate; /* for file:/dev/fdset/n,offset=x URI */
const char *path; /* path associated with fd */
};
-qemuProcessIncomingDef *qemuProcessIncomingDefNew(virQEMUCaps *qemuCaps,
- const char *listenAddress,
- const char *migrateFrom,
- int fd,
- const char *path);
+qemuProcessIncomingDef *qemuProcessIncomingDefNew(virDomainObj *vm,
+ const char *listenAddress,
+ const char *migrateFrom,
+ int fd,
+ const char *path,
+ unsigned int flags);
void qemuProcessIncomingDefFree(qemuProcessIncomingDef *inc);
int qemuProcessBeginJob(virDomainObj *vm,
@@ -77,6 +79,7 @@ typedef enum {
VIR_QEMU_PROCESS_START_NEW = 1 << 4, /* internal, new VM is starting
*/
VIR_QEMU_PROCESS_START_GEN_VMID = 1 << 5, /* Generate a new VMID */
VIR_QEMU_PROCESS_START_RESET_NVRAM = 1 << 6, /* Re-initialize NVRAM from
template */
+ VIR_QEMU_PROCESS_START_MAPPED_RAM = 1 << 7, /* Re-initialize NVRAM from
template */
copy paste error in comment ;)
} qemuProcessStartFlags;
int qemuProcessStart(virConnectPtr conn,
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 8f28770086..1545c00472 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -628,6 +628,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
int oflags = open_write ? O_RDWR : O_RDONLY;
size_t xml_len;
size_t cookie_len;
+ bool use_mapped_ram = false;
if (bypass_cache) {
int directFlag = virFileDirectFdFlag();
@@ -642,11 +643,6 @@ qemuSaveImageOpen(virQEMUDriver *driver,
if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
return -1;
- if (bypass_cache &&
- !(*wrapperFd = virFileWrapperFdNew(&fd, path,
- VIR_FILE_WRAPPER_BYPASS_CACHE)))
- return -1;
-
data = g_new0(virQEMUSaveData, 1);
header = &data->header;
@@ -708,10 +704,14 @@ qemuSaveImageOpen(virQEMUDriver *driver,
return -1;
}
- if (header->features && header->features !=
QEMU_SAVE_FEATURE_MAPPED_RAM) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("image contains unsupported features)"));
- return -1;
+ if (header->features) {
+ if (header->features == QEMU_SAVE_FEATURE_MAPPED_RAM) {
+ use_mapped_ram = true;
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("image contains unsupported features)"));
+ return -1;
+ }
}
if (header->cookieOffset)
@@ -739,6 +739,11 @@ qemuSaveImageOpen(virQEMUDriver *driver,
}
}
+ if (bypass_cache && !use_mapped_ram &&
+ !(*wrapperFd = virFileWrapperFdNew(&fd, path,
+ VIR_FILE_WRAPPER_BYPASS_CACHE)))
+ return -1;
+
Shouldn't this be:
if (bypass_cache) {
if (use_mapped_ram)
error out
virFileWarpperFdNew
}
??
But only until the next patches which add the possibility of
bypass_cache + mapped_ram. Or maybe if QEMU does not support mapped_ram
with direct-io this should error out.
/* Create a domain from this XML */
if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps,
VIR_DOMAIN_DEF_PARSE_INACTIVE |
@@ -777,6 +782,9 @@ qemuSaveImageStartVM(virConnectPtr conn,
if (reset_nvram)
start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM;
+ if (header->features & QEMU_SAVE_FEATURE_MAPPED_RAM)
+ start_flags |= VIR_QEMU_PROCESS_START_MAPPED_RAM;
+
if (qemuProcessStartWithMemoryState(conn, driver, vm, fd, path, NULL, data,
asyncJob, start_flags, "restored",
&started) < 0) {
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 8/9] qemu: Support O_DIRECT with mapped-ram on save
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
I'm not happy with this and the subsequent patch, which pass another FD
around for QEMU to use for reading/writing unaligned state when
BYPASS_CACHE has been specified. One idea is to pass the qemuFdPass object
around the various functions, but qemu_fd.h already includes qemu_monitor.h.
E.g. passing a qemuFdPass to qemuMonitorMigrateToMappedFile requires
including qemu_fd.h in qemu_monitor.h, so a cyclical dependency. I could
move qemuFDPass*TransferMonitor* functions to qemu_monitor.[ch] to avoid
the include in qemu_fd.h, but would likely require moving the _qemuFDPass
struct in qemu_fd.c to qemu_fd.h.
Another idea is to move qemuProcessIncomingDefNew in qemu_process.c to
qemuMigrationConnectionDefNew() in qemu_migration.c. Also move
qemuProcessIncomingDef object in qemu_process.h to qemuMigrationConnectDef
(or something like that) in qemu_migration.h. The qemuMigrationConnectDef
can encapsulate all the connection info related to the migration. E.g.
traditional fd, path associated with fd, uri, listen address, qemuFDPass
object containing fds for mapped_ram, etc. This can be used by both save
and restore paths.
Other suggestions kindly welcomed :-).
src/qemu/qemu_migration.c | 3 ++-
src/qemu/qemu_migration.h | 1 +
src/qemu/qemu_monitor.c | 14 +++++++++++---
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_saveimage.c | 25 +++++++++++++++++--------
5 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b1d27e07e1..a0435a0572 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -7084,6 +7084,7 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm,
int
qemuMigrationSrcToMappedFile(virQEMUDriver *driver, virDomainObj *vm,
int fd,
+ int nondirectFD,
virDomainAsyncJob asyncJob)
{
qemuDomainObjPrivate *priv = vm->privateData;
@@ -7121,7 +7122,7 @@ qemuMigrationSrcToMappedFile(virQEMUDriver *driver, virDomainObj
*vm,
if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
goto cleanup;
- rc = qemuMonitorMigrateToFdSet(vm, 0, fd);
+ rc = qemuMonitorMigrateToFdSet(vm, 0, fd, nondirectFD);
qemuDomainObjExitMonitor(vm);
if (rc < 0)
goto cleanup;
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 864f3280e5..939205ef13 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -245,6 +245,7 @@ int
qemuMigrationSrcToMappedFile(virQEMUDriver *driver,
virDomainObj *vm,
int fd,
+ int nondirectFD,
virDomainAsyncJob asyncJob)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4c92bd740a..c0936bee6b 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2240,7 +2240,8 @@ qemuMonitorMigrateToFd(qemuMonitor *mon,
int
qemuMonitorMigrateToFdSet(virDomainObj *vm,
unsigned int flags,
- int fd)
+ int fd,
+ int nondirectFD)
{
qemuDomainObjPrivate *priv = vm->privateData;
qemuMonitor *mon = priv->mon;
@@ -2249,15 +2250,22 @@ qemuMonitorMigrateToFdSet(virDomainObj *vm,
unsigned int setId;
g_autofree char *uri = NULL;
- VIR_DEBUG("fd=%d flags=0x%x", fd, flags);
+ VIR_DEBUG("fd=%d nondirectFD=%d flags=0x%x", fd, nondirectFD, flags);
QEMU_CHECK_MONITOR(mon);
- if ((offset = lseek(fd, 0, SEEK_CUR)) == -1)
+ if (nondirectFD != -1)
+ offset = lseek(nondirectFD, 0, SEEK_CUR);
+ else
+ offset = lseek(fd, 0, SEEK_CUR);
+
+ if (offset == -1)
return -1;
fdPassMigrate = qemuFDPassNew("migrate", priv);
qemuFDPassAddFD(fdPassMigrate, &fd, "-fd");
+ if (nondirectFD != -1)
+ qemuFDPassAddFD(fdPassMigrate, &nondirectFD, "-nondirect-fd");
qemuFDPassTransferMonitor(fdPassMigrate, mon);
if (qemuFDPassGetId(fdPassMigrate, &setId) < 0)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index c477def138..0fbb2dadc1 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -845,7 +845,8 @@ int qemuMonitorMigrateToFd(qemuMonitor *mon,
int qemuMonitorMigrateToFdSet(virDomainObj *vm,
unsigned int flags,
- int fd);
+ int fd,
+ int nondirectFD);
int qemuMonitorMigrateToHost(qemuMonitor *mon,
unsigned int flags,
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 1545c00472..2b0281895a 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -431,19 +431,28 @@ qemuSaveImageCreateMapped(virQEMUDriver *driver,
virDomainAsyncJob asyncJob)
{
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+ VIR_AUTOCLOSE nondirectFD = -1;
- /* mapped-ram does not support directIO */
+ /* If BYPASS_CACHE has been specified, fd has been opened with O_DIRECT.
+ * In this case, QEMU requires a second FD without O_DIRECT for writing
+ * unaligned state. We'll use this FD as well to write the header.
+ * Relative to VM RAM size, this data is a drop in the bucket and fine
+ * to write without O_DIRECT.
+ */
if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("bypass cache unsupported by this system"));
- return -1;
+ if ((nondirectFD = qemuDomainOpenFile(cfg, vm->def, path, O_WRONLY, NULL))
< 0)
+ return -1;
+ if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
nondirectFD) < 0)
+ return -1;
+ if (virQEMUSaveDataWrite(data, nondirectFD, path) < 0)
+ return -1;
+ } else {
+ if (virQEMUSaveDataWrite(data, fd, path) < 0)
+ return -1;
}
- if (virQEMUSaveDataWrite(data, fd, path) < 0)
- return -1;
-
/* Perform the migration */
- return qemuMigrationSrcToMappedFile(driver, vm, fd, asyncJob);
+ return qemuMigrationSrcToMappedFile(driver, vm, fd, nondirectFD, asyncJob);
}
--
2.44.0
5:43 p.m.
New subject: [PATCH RFC 9/9] qemu: Support O_DIRECT with mapped-ram on restore
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_driver.c | 22 +++++++++++++---------
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_process.c | 20 ++++++++++++++++----
src/qemu/qemu_process.h | 3 +++
src/qemu/qemu_saveimage.c | 29 +++++++++++++++++++++++++----
src/qemu/qemu_saveimage.h | 4 +++-
src/qemu/qemu_snapshot.c | 8 ++++----
7 files changed, 65 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 34f37210d9..53674458bf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1624,7 +1624,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
}
if (qemuProcessStart(conn, driver, vm, NULL, VIR_ASYNC_JOB_START,
- NULL, -1, NULL, NULL,
+ NULL, -1, -1, NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
start_flags) < 0) {
virDomainAuditStart(vm, "booted", false);
@@ -5742,6 +5742,7 @@ qemuDomainRestoreInternal(virConnectPtr conn,
g_autofree char *xmlout = NULL;
const char *newxml = dxml;
int fd = -1;
+ int nondirectFd = -1;
int ret = -1;
virQEMUSaveData *data = NULL;
virFileWrapperFd *wrapperFd = NULL;
@@ -5758,7 +5759,7 @@ qemuDomainRestoreInternal(virConnectPtr conn,
fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
(flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0,
- &wrapperFd, false, false);
+ &wrapperFd, &nondirectFd, false, false);
if (fd < 0)
goto cleanup;
@@ -5812,13 +5813,14 @@ qemuDomainRestoreInternal(virConnectPtr conn,
if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_RESTORE, flags) < 0)
goto cleanup;
- ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path,
+ ret = qemuSaveImageStartVM(conn, driver, vm, &fd, nondirectFd, data, path,
false, reset_nvram, VIR_ASYNC_JOB_START);
qemuProcessEndJob(vm);
cleanup:
VIR_FORCE_CLOSE(fd);
+ VIR_FORCE_CLOSE(nondirectFd);
if (virFileWrapperFdClose(wrapperFd) < 0)
ret = -1;
virFileWrapperFdFree(wrapperFd);
@@ -5893,7 +5895,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path,
virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL);
fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
- false, NULL, false, false);
+ false, NULL, NULL, false, false);
if (fd < 0)
goto cleanup;
@@ -5930,7 +5932,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
state = 0;
fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
- false, NULL, true, false);
+ false, NULL, NULL, true, false);
if (fd < 0)
goto cleanup;
@@ -6011,7 +6013,7 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int
flags)
}
if ((fd = qemuSaveImageOpen(driver, priv->qemuCaps, path, &def, &data,
- false, NULL, false, false)) < 0)
+ false, NULL, NULL, false, false)) < 0)
goto cleanup;
ret = qemuDomainDefFormatXML(driver, priv->qemuCaps, def, flags);
@@ -6069,13 +6071,14 @@ qemuDomainObjRestore(virConnectPtr conn,
g_autoptr(virDomainDef) def = NULL;
qemuDomainObjPrivate *priv = vm->privateData;
int fd = -1;
+ int nondirectFd = -1;
int ret = -1;
g_autofree char *xmlout = NULL;
virQEMUSaveData *data = NULL;
virFileWrapperFd *wrapperFd = NULL;
fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
- bypass_cache, &wrapperFd, false, true);
+ bypass_cache, &wrapperFd, &nondirectFd, false, true);
if (fd < 0) {
if (fd == -3)
ret = 1;
@@ -6120,12 +6123,13 @@ qemuDomainObjRestore(virConnectPtr conn,
virDomainObjAssignDef(vm, &def, true, NULL);
- ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path,
+ ret = qemuSaveImageStartVM(conn, driver, vm, &fd, nondirectFd, data, path,
start_paused, reset_nvram, asyncJob);
cleanup:
virQEMUSaveDataFree(data);
VIR_FORCE_CLOSE(fd);
+ VIR_FORCE_CLOSE(nondirectFd);
if (virFileWrapperFdClose(wrapperFd) < 0)
ret = -1;
virFileWrapperFdFree(wrapperFd);
@@ -6321,7 +6325,7 @@ qemuDomainObjStart(virConnectPtr conn,
}
ret = qemuProcessStart(conn, driver, vm, NULL, asyncJob,
- NULL, -1, NULL, NULL,
+ NULL, -1, -1, NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags);
virDomainAuditStart(vm, "booted", ret >= 0);
if (ret >= 0) {
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a0435a0572..0b1e03394e 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3009,7 +3009,7 @@ qemuMigrationDstPrepare(virDomainObj *vm,
}
return qemuProcessIncomingDefNew(vm, listenAddress,
- migrateFrom, fd, NULL, 0);
+ migrateFrom, fd, -1, NULL, 0);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f700390a8c..f5b2f9ea2b 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4688,6 +4688,7 @@ qemuProcessIncomingDefNew(virDomainObj *vm,
const char *listenAddress,
const char *migrateFrom,
int fd,
+ int nondirectFd,
const char *path,
unsigned int flags)
{
@@ -4707,11 +4708,18 @@ qemuProcessIncomingDefNew(virDomainObj *vm,
unsigned int fdsetId;
off_t offset;
- if ((offset = lseek(fd, 0, SEEK_CUR)) == -1)
- offset = 0;
+ if (nondirectFd == -1)
+ offset = lseek(fd, 0, SEEK_CUR);
+ else
+ offset = lseek(nondirectFd, 0, SEEK_CUR);
+
+ if (offset < 0)
+ goto error;
inc->fdPassMigrate = qemuFDPassNew("migrate", priv);
qemuFDPassAddFD(inc->fdPassMigrate, &fd, "-fd");
+ if (nondirectFd != -1)
+ qemuFDPassAddFD(inc->fdPassMigrate, &nondirectFd,
"-nondirect-fd");
qemuFDPassGetId(inc->fdPassMigrate, &fdsetId);
inc->uri = g_strdup_printf("file:/dev/fdset/%u,offset=%#lx",
fdsetId, offset);
} else {
@@ -8148,6 +8156,7 @@ qemuProcessStart(virConnectPtr conn,
virDomainAsyncJob asyncJob,
const char *migrateFrom,
int migrateFd,
+ int migrateNondirectFd,
const char *migratePath,
virDomainMomentObj *snapshot,
virNetDevVPortProfileOp vmop,
@@ -8185,7 +8194,8 @@ qemuProcessStart(virConnectPtr conn,
if (migrateFrom) {
incoming = qemuProcessIncomingDefNew(vm, NULL, migrateFrom,
- migrateFd, migratePath, flags);
+ migrateFd, migrateNondirectFd,
+ migratePath, flags);
if (!incoming)
goto stop;
}
@@ -8264,6 +8274,7 @@ qemuProcessStart(virConnectPtr conn,
* @driver: qemu driver object
* @vm: domain object
* @fd: FD pointer of memory state file
+ * @nondirectFd: FD for memory state file, opened without O_DIRECT
* @path: path to memory state file
* @snapshot: internal snapshot to load when starting QEMU process or NULL
* @data: data from memory state file or NULL
@@ -8290,6 +8301,7 @@ qemuProcessStartWithMemoryState(virConnectPtr conn,
virQEMUDriver *driver,
virDomainObj *vm,
int *fd,
+ int nondirectFd,
const char *path,
virDomainMomentObj *snapshot,
virQEMUSaveData *data,
@@ -8329,7 +8341,7 @@ qemuProcessStartWithMemoryState(virConnectPtr conn,
priv->disableSlirp = true;
if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL,
- asyncJob, migrateFrom, *fd, path, snapshot,
+ asyncJob, migrateFrom, *fd, nondirectFd, path, snapshot,
VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
start_flags) == 0)
*started = true;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index a5212ee56e..6e37dac2ca 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -62,6 +62,7 @@ qemuProcessIncomingDef *qemuProcessIncomingDefNew(virDomainObj *vm,
const char *listenAddress,
const char *migrateFrom,
int fd,
+ int nondirectFd,
const char *path,
unsigned int flags);
void qemuProcessIncomingDefFree(qemuProcessIncomingDef *inc);
@@ -89,6 +90,7 @@ int qemuProcessStart(virConnectPtr conn,
virDomainAsyncJob asyncJob,
const char *migrateFrom,
int stdin_fd,
+ int nondirectFd,
const char *stdin_path,
virDomainMomentObj *snapshot,
virNetDevVPortProfileOp vmop,
@@ -98,6 +100,7 @@ int qemuProcessStartWithMemoryState(virConnectPtr conn,
virQEMUDriver *driver,
virDomainObj *vm,
int *fd,
+ int nondirectFd,
const char *path,
virDomainMomentObj *snapshot,
virQEMUSaveData *data,
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 2b0281895a..8e634862a6 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -610,6 +610,7 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
* @ret_data: returns structure filled with data from the image header
* @bypass_cache: bypass cache when opening the file
* @wrapperFd: returns the file wrapper structure
+ * @nondirectFd: returns file descriptor without O_DIRECT set
* @open_write: open the file for writing (for updates)
* @unlink_corrupt: remove the image file if it is corrupted
*
@@ -625,11 +626,14 @@ qemuSaveImageOpen(virQEMUDriver *driver,
virQEMUSaveData **ret_data,
bool bypass_cache,
virFileWrapperFd **wrapperFd,
+ int *nondirectFd,
bool open_write,
bool unlink_corrupt)
{
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
VIR_AUTOCLOSE fd = -1;
+ VIR_AUTOCLOSE nondirect_fd = -1;
+ int read_fd;
int ret = -1;
g_autoptr(virQEMUSaveData) data = NULL;
virQEMUSaveHeader *header;
@@ -646,6 +650,13 @@ qemuSaveImageOpen(virQEMUDriver *driver,
_("bypass cache unsupported by this system"));
return -1;
}
+
+ /* Also open the file without O_DIRECT for reading header and for
+ * qemu to use when reading unaligned state
+ */
+ if ((nondirect_fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
+ return -1;
+
oflags |= directFlag;
}
@@ -655,7 +666,12 @@ qemuSaveImageOpen(virQEMUDriver *driver,
data = g_new0(virQEMUSaveData, 1);
header = &data->header;
- if (saferead(fd, header, sizeof(*header)) != sizeof(*header)) {
+ if (nondirect_fd != -1)
+ read_fd = nondirect_fd;
+ else
+ read_fd = fd;
+
+ if (saferead(read_fd, header, sizeof(*header)) != sizeof(*header)) {
if (unlink_corrupt) {
if (unlink(path) < 0) {
virReportSystemError(errno,
@@ -732,7 +748,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
data->xml = g_new0(char, xml_len);
- if (saferead(fd, data->xml, xml_len) != xml_len) {
+ if (saferead(read_fd, data->xml, xml_len) != xml_len) {
virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("failed to read domain XML"));
return -1;
@@ -741,7 +757,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
if (cookie_len > 0) {
data->cookie = g_new0(char, cookie_len);
- if (saferead(fd, data->cookie, cookie_len) != cookie_len) {
+ if (saferead(read_fd, data->cookie, cookie_len) != cookie_len) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("failed to read cookie"));
return -1;
@@ -761,6 +777,10 @@ qemuSaveImageOpen(virQEMUDriver *driver,
*ret_def = g_steal_pointer(&def);
*ret_data = g_steal_pointer(&data);
+ if (nondirectFd) {
+ *nondirectFd = nondirect_fd;
+ nondirect_fd = -1;
+ }
ret = fd;
fd = -1;
@@ -774,6 +794,7 @@ qemuSaveImageStartVM(virConnectPtr conn,
virQEMUDriver *driver,
virDomainObj *vm,
int *fd,
+ int nondirectFd,
virQEMUSaveData *data,
const char *path,
bool start_paused,
@@ -794,7 +815,7 @@ qemuSaveImageStartVM(virConnectPtr conn,
if (header->features & QEMU_SAVE_FEATURE_MAPPED_RAM)
start_flags |= VIR_QEMU_PROCESS_START_MAPPED_RAM;
- if (qemuProcessStartWithMemoryState(conn, driver, vm, fd, path, NULL, data,
+ if (qemuProcessStartWithMemoryState(conn, driver, vm, fd, nondirectFd, path, NULL,
data,
asyncJob, start_flags, "restored",
&started) < 0) {
goto cleanup;
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 81d93bf33c..0a8b35158a 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -67,12 +67,13 @@ qemuSaveImageStartVM(virConnectPtr conn,
virQEMUDriver *driver,
virDomainObj *vm,
int *fd,
+ int nondirectFd,
virQEMUSaveData *data,
const char *path,
bool start_paused,
bool reset_nvram,
virDomainAsyncJob asyncJob)
- ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6);
+ ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
int
qemuSaveImageOpen(virQEMUDriver *driver,
@@ -82,6 +83,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
virQEMUSaveData **ret_data,
bool bypass_cache,
virFileWrapperFd **wrapperFd,
+ int *nondirectFd,
bool open_write,
bool unlink_corrupt)
ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 1e9e0e31d7..f137a79a8d 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2115,7 +2115,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
memdata->path = snapdef->memorysnapshotfile;
memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path,
&savedef, &memdata->data,
- false, NULL,
+ false, NULL, NULL,
false, false);
if (memdata->fd < 0)
@@ -2359,7 +2359,7 @@ qemuSnapshotRevertActive(virDomainObj *vm,
virDomainObjAssignDef(vm, config, true, NULL);
if (qemuProcessStartWithMemoryState(snapshot->domain->conn, driver, vm,
- &memdata.fd, memdata.path, loadSnap,
+ &memdata.fd, -1, memdata.path, loadSnap,
memdata.data, VIR_ASYNC_JOB_SNAPSHOT,
start_flags, "from-snapshot",
&started) < 0) {
@@ -2513,7 +2513,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm,
start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL,
- VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, NULL,
+ VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, -1, NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
start_flags);
virDomainAuditStart(vm, "from-snapshot", rc >= 0);
@@ -2991,7 +2991,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
if (!virDomainObjIsActive(vm)) {
if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT,
- NULL, -1, NULL, NULL,
+ NULL, -1, -1, NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
VIR_QEMU_PROCESS_START_PAUSED) < 0) {
return -1;
--
2.44.0
7:32 a.m.
On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote:
This series is a RFC for support of QEMU's mapped-ram migration
capability [1] for saving and restoring VMs. It implements the first
part of the design approach we discussed for supporting parallel
save/restore [2]. In summary, the approach is
1. Add mapped-ram migration capability
2. Steal an element from save header 'unused' for a 'features' variable
and bump save version to 3.
3. Add /etc/libvirt/qemu.conf knob for the save format version,
defaulting to latest v3
4. Use v3 (aka mapped-ram) by default
5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
6. include: Define constants for parallel save/restore
7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
8. qemu: Add support for parallel restore. Implies mapped-ram.
Reject if v2
9. tools: add parallel parameter to virsh save command
10. tools: add parallel parameter to virsh restore command
This series implements 1-5, with the BYPASS_CACHE support in patches 8
and 9 being quite hacky. They are included to discuss approaches to make
them less hacky. See the patches for details.
They might seem tiny bit hacky, but it's not that big of a deal I think.
You could eliminate two conditions by making the first FD always
non-direct (as in either there is no BYPASS_CACHE or it's already
wrapped by the I/O helper), but it would complicate other things in the
code and would get even more hairy IMHO.
The QEMU mapped-ram capability currently does not support directio.
Fabino is working on that now [3]. This complicates merging support
in libvirt. I don't think it's reasonable to enable mapped-ram by
default when BYPASS_CACHE cannot be supported. Should we wait until
the mapped-ram directio support is merged in QEMU before supporting
mapped-ram in libvirt?
By the time I looked at this series the direct-io work has already went
in, but there is still the need for the second descriptor to do some
unaligned I/O.
From the QEMU patches I'm not sure whether you also need to set the
direct-io migration capability/flag when migrating to an fdset. Maybe
that's needed for migration into a file directly.
For the moment, compression is ignored in the new save version.
Currently, libvirt connects the output of QEMU's save stream to the
specified compression program via a pipe. This approach is incompatible
with mapped-ram since the fd provided to QEMU must be seekable. One
option is to reopen and compress the saved image after the actual save
operation has completed. This has the downside of requiring the iohelper
to handle BYPASS_CACHE, which would preclude us from removing it
sometime in the future. Other suggestions much welcomed.
I was wondering whether it would make sense to use user-space block I/O,
but we'd have to use some compression on a block-by-block basis and
since you need to be able to compress each write separately, that means
you might just save few bytes here and there. And on top of that you'd
have to compress each individual block and that block needs to be
allocated as a whole, so no space would be saved at all. So that does
not make sense unless there is some new format.
And compression after the save is finished is in my opinion kind of
pointless. You don't save time and you only save disk space _after_ the
compression step is done. Not to mention you'd have to uncompress it
again before starting QEMU from it. I'd be fine with making users
choose between compression and mapped-ram, at least for now. They can
compress the resulting file on their own.
Note the logical file size of mapped-ram saved images is slightly
larger than guest RAM size, so the files are often much larger than the
files produced by the existing, sequential format. However, actual blocks
written to disk is often lower with mapped-ram saved images. E.g. a saved
image from a 30G, freshly booted, idle guest results in the following
'Size' and 'Blocks' values reported by stat(1)
Size Blocks
sequential 998595770 1950392
mapped-ram 34368584225 1800456
With the same guest running a workload that dirties memory
Size Blocks
sequential 33173330615 64791672
mapped-ram 34368578210 64706944
That's fine and even better. It saves space, the only thing that
everyone needs to keep in mind is to treat it as a sparse file.
The other option for the space saving would be to consolidate the
streamed changes in the resulting file, but for little to no gain.
The mapped-ram is better.
Thanks for any comments on this RFC!
[1]
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/m...
[2]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/K...
[3] https://mail.gnu.org/archive/html/qemu-devel/2024-05/msg04432.html
Some more teeny tiny comments in two patches will follow.
Martin
Jim Fehlig (9):
qemu: Enable mapped-ram migration capability
qemu_fd: Add function to retrieve fdset ID
qemu: Add function to get migration params for save
qemu: Add a 'features' element to save image header and bump version
qemu: conf: Add setting for save image version
qemu: Add support for mapped-ram on save
qemu: Enable mapped-ram on restore
qemu: Support O_DIRECT with mapped-ram on save
qemu: Support O_DIRECT with mapped-ram on restore
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf.in | 6 +
src/qemu/qemu_conf.c | 8 ++
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_driver.c | 25 ++--
src/qemu/qemu_fd.c | 18 +++
src/qemu/qemu_fd.h | 3 +
src/qemu/qemu_migration.c | 99 ++++++++++++++-
src/qemu/qemu_migration.h | 11 +-
src/qemu/qemu_migration_params.c | 20 +++
src/qemu/qemu_migration_params.h | 4 +
src/qemu/qemu_monitor.c | 40 ++++++
src/qemu/qemu_monitor.h | 5 +
src/qemu/qemu_process.c | 63 +++++++---
src/qemu/qemu_process.h | 16 ++-
src/qemu/qemu_saveimage.c | 187 +++++++++++++++++++++++------
src/qemu/qemu_saveimage.h | 20 ++-
src/qemu/qemu_snapshot.c | 12 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 +
19 files changed, 455 insertions(+), 85 deletions(-)
--
2.44.0
10:49 a.m.
On Wed, Aug 07, 2024 at 02:32:57PM +0200, Martin Kletzander wrote:
On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel
wrote:
> This series is a RFC for support of QEMU's mapped-ram migration
> capability [1] for saving and restoring VMs. It implements the first
> part of the design approach we discussed for supporting parallel
> save/restore [2]. In summary, the approach is
>
> 1. Add mapped-ram migration capability
> 2. Steal an element from save header 'unused' for a 'features'
variable
> and bump save version to 3.
> 3. Add /etc/libvirt/qemu.conf knob for the save format version,
> defaulting to latest v3
> 4. Use v3 (aka mapped-ram) by default
> 5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
> 6. include: Define constants for parallel save/restore
> 7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
> 8. qemu: Add support for parallel restore. Implies mapped-ram.
> Reject if v2
> 9. tools: add parallel parameter to virsh save command
> 10. tools: add parallel parameter to virsh restore command
>
> This series implements 1-5, with the BYPASS_CACHE support in patches 8
> and 9 being quite hacky. They are included to discuss approaches to make
> them less hacky. See the patches for details.
>
They might seem tiny bit hacky, but it's not that big of a deal I think.
You could eliminate two conditions by making the first FD always
non-direct (as in either there is no BYPASS_CACHE or it's already
wrapped by the I/O helper), but it would complicate other things in the
code and would get even more hairy IMHO.
> The QEMU mapped-ram capability currently does not support directio.
> Fabino is working on that now [3]. This complicates merging support
> in libvirt. I don't think it's reasonable to enable mapped-ram by
> default when BYPASS_CACHE cannot be supported. Should we wait until
> the mapped-ram directio support is merged in QEMU before supporting
> mapped-ram in libvirt?
>
By the time I looked at this series the direct-io work has already went
in, but there is still the need for the second descriptor to do some
unaligned I/O.
From the QEMU patches I'm not sure whether you also need to set the
direct-io migration capability/flag when migrating to an fdset. Maybe
that's needed for migration into a file directly.
> For the moment, compression is ignored in the new save version.
> Currently, libvirt connects the output of QEMU's save stream to the
> specified compression program via a pipe. This approach is incompatible
> with mapped-ram since the fd provided to QEMU must be seekable. One
> option is to reopen and compress the saved image after the actual save
> operation has completed. This has the downside of requiring the iohelper
> to handle BYPASS_CACHE, which would preclude us from removing it
> sometime in the future. Other suggestions much welcomed.
>
I was wondering whether it would make sense to use user-space block I/O,
but we'd have to use some compression on a block-by-block basis and
since you need to be able to compress each write separately, that means
you might just save few bytes here and there. And on top of that you'd
have to compress each individual block and that block needs to be
allocated as a whole, so no space would be saved at all. So that does
not make sense unless there is some new format.
And compression after the save is finished is in my opinion kind of
pointless. You don't save time and you only save disk space _after_ the
compression step is done. Not to mention you'd have to uncompress it
again before starting QEMU from it. I'd be fine with making users
choose between compression and mapped-ram, at least for now. They can
compress the resulting file on their own.
That argument for compressing on their own applies to the existing
code too. The reason we want it in libvirt is that we make it
'just work' without requiring apps to have a login shell to the
hypervisor to run commands out of band.
So basically it depends whether disk space is more important than
overall wallclock time. It might still be worthwhile if the use of
multifd with mapped-ram massively reduces the overall save duration
and we also had a parallelized compression tool we were spawning.
eg xz can be told to use all CPU thrfads.
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 :|
1:14 p.m.
On 8/7/24 09:49, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 02:32:57PM +0200, Martin Kletzander wrote:
> On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote:
>> This series is a RFC for support of QEMU's mapped-ram migration
>> capability [1] for saving and restoring VMs. It implements the first
>> part of the design approach we discussed for supporting parallel
>> save/restore [2]. In summary, the approach is
>>
>> 1. Add mapped-ram migration capability
>> 2. Steal an element from save header 'unused' for a 'features'
variable
>> and bump save version to 3.
>> 3. Add /etc/libvirt/qemu.conf knob for the save format version,
>> defaulting to latest v3
>> 4. Use v3 (aka mapped-ram) by default
>> 5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
>> 6. include: Define constants for parallel save/restore
>> 7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
>> 8. qemu: Add support for parallel restore. Implies mapped-ram.
>> Reject if v2
>> 9. tools: add parallel parameter to virsh save command
>> 10. tools: add parallel parameter to virsh restore command
>>
>> This series implements 1-5, with the BYPASS_CACHE support in patches 8
>> and 9 being quite hacky. They are included to discuss approaches to make
>> them less hacky. See the patches for details.
>>
>
> They might seem tiny bit hacky, but it's not that big of a deal I think.
>
> You could eliminate two conditions by making the first FD always
> non-direct (as in either there is no BYPASS_CACHE or it's already
> wrapped by the I/O helper), but it would complicate other things in the
> code and would get even more hairy IMHO.
>
>> The QEMU mapped-ram capability currently does not support directio.
>> Fabino is working on that now [3]. This complicates merging support
>> in libvirt. I don't think it's reasonable to enable mapped-ram by
>> default when BYPASS_CACHE cannot be supported. Should we wait until
>> the mapped-ram directio support is merged in QEMU before supporting
>> mapped-ram in libvirt?
>>
>
> By the time I looked at this series the direct-io work has already went
> in, but there is still the need for the second descriptor to do some
> unaligned I/O.
>
> From the QEMU patches I'm not sure whether you also need to set the
> direct-io migration capability/flag when migrating to an fdset. Maybe
> that's needed for migration into a file directly.
>
>> For the moment, compression is ignored in the new save version.
>> Currently, libvirt connects the output of QEMU's save stream to the
>> specified compression program via a pipe. This approach is incompatible
>> with mapped-ram since the fd provided to QEMU must be seekable. One
>> option is to reopen and compress the saved image after the actual save
>> operation has completed. This has the downside of requiring the iohelper
>> to handle BYPASS_CACHE, which would preclude us from removing it
>> sometime in the future. Other suggestions much welcomed.
>>
>
> I was wondering whether it would make sense to use user-space block I/O,
> but we'd have to use some compression on a block-by-block basis and
> since you need to be able to compress each write separately, that means
> you might just save few bytes here and there. And on top of that you'd
> have to compress each individual block and that block needs to be
> allocated as a whole, so no space would be saved at all. So that does
> not make sense unless there is some new format.
>
> And compression after the save is finished is in my opinion kind of
> pointless. You don't save time and you only save disk space _after_ the
> compression step is done. Not to mention you'd have to uncompress it
> again before starting QEMU from it. I'd be fine with making users
> choose between compression and mapped-ram, at least for now. They can
> compress the resulting file on their own.
That argument for compressing on their own applies to the existing
code too. The reason we want it in libvirt is that we make it
'just work' without requiring apps to have a login shell to the
hypervisor to run commands out of band.
So basically it depends whether disk space is more important than
overall wallclock time. It might still be worthwhile if the use of
multifd with mapped-ram massively reduces the overall save duration
and we also had a parallelized compression tool we were spawning.
eg xz can be told to use all CPU thrfads.
mapped-ram+direct-io+multifd is quite an improvement over the current
save/restore mechanism. The following table shows some save/restore stats from a
guest with 32G RAM, 30G dirty, 1 vcpu in tight loop dirtying memory.
| save | restore |
| time | time | Size | Blocks
-----------------------+---------+---------+--------------+---------
legacy | 25.800s | 14.332s | 33154309983 | 64754512
-----------------------+---------+---------+--------------+---------
mapped-ram | 18.742s | 15.027s | 34368559228 | 64617160
-----------------------+---------+---------+--------------+---------
legacy + direct IO | 13.115s | 18.050s | 33154310496 | 64754520
-----------------------+---------+---------+--------------+---------
mapped-ram + direct IO | 13.623s | 15.959s | 34368557392 | 64662040
-----------------------+-------- +---------+--------------+---------
mapped-ram + direct IO | | | |
+ multifd-channels=8 | 6.994s | 6.470s | 34368554980 | 64665776
--------------------------------------------------------------------
In all cases, the save and restore operations are to/from a block device
comprised of two NVMe disks in RAID0 configuration with xfs (~8600MiB/s). The
values in the 'save time' and 'restore time' columns were scraped from the
'real' time reported by time(1). The 'Size' and 'Blocks' columns
were provided
by the corresponding outputs of stat(1).
Regards,
Jim
12:20 p.m.
Hi Martin,
On 8/7/24 06:32, Martin Kletzander wrote:
On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel
wrote:
> This series is a RFC for support of QEMU's mapped-ram migration
> capability [1] for saving and restoring VMs. It implements the first
> part of the design approach we discussed for supporting parallel
> save/restore [2]. In summary, the approach is
>
> 1. Add mapped-ram migration capability
> 2. Steal an element from save header 'unused' for a 'features'
variable
> and bump save version to 3.
> 3. Add /etc/libvirt/qemu.conf knob for the save format version,
> defaulting to latest v3
> 4. Use v3 (aka mapped-ram) by default
> 5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
> 6. include: Define constants for parallel save/restore
> 7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
> 8. qemu: Add support for parallel restore. Implies mapped-ram.
> Reject if v2
> 9. tools: add parallel parameter to virsh save command
> 10. tools: add parallel parameter to virsh restore command
>
> This series implements 1-5, with the BYPASS_CACHE support in patches 8
> and 9 being quite hacky. They are included to discuss approaches to make
> them less hacky. See the patches for details.
>
They might seem tiny bit hacky, but it's not that big of a deal I think.
In the meantime I've been working on a V1 that's a bit less hacky :-).
You could eliminate two conditions by making the first FD always
non-direct (as in either there is no BYPASS_CACHE or it's already
wrapped by the I/O helper), but it would complicate other things in the
code and would get even more hairy IMHO.
In V1, the first FD is always buffered. The directio one, if needed, is opened a
bit later in the save process.
> The QEMU mapped-ram capability currently does not support directio.
> Fabino is working on that now [3]. This complicates merging support
> in libvirt. I don't think it's reasonable to enable mapped-ram by
> default when BYPASS_CACHE cannot be supported. Should we wait until
> the mapped-ram directio support is merged in QEMU before supporting
> mapped-ram in libvirt?
>
By the time I looked at this series the direct-io work has already went
in, but there is still the need for the second descriptor to do some
unaligned I/O.
Correct. However, ATM I don't know how to detect if qemu supports the direct-io
migration parameter.
From the QEMU patches I'm not sure whether you also need to set the
direct-io migration capability/flag when migrating to an fdset. Maybe
that's needed for migration into a file directly.
The direct-io migration parameter must be set to 'true' and QEMU provided a
second FD with O_DIRECT set.
> For the moment, compression is ignored in the new save version.
> Currently, libvirt connects the output of QEMU's save stream to the
> specified compression program via a pipe. This approach is incompatible
> with mapped-ram since the fd provided to QEMU must be seekable. One
> option is to reopen and compress the saved image after the actual save
> operation has completed. This has the downside of requiring the iohelper
> to handle BYPASS_CACHE, which would preclude us from removing it
> sometime in the future. Other suggestions much welcomed.
>
I was wondering whether it would make sense to use user-space block I/O,
but we'd have to use some compression on a block-by-block basis and
since you need to be able to compress each write separately, that means
you might just save few bytes here and there. And on top of that you'd
have to compress each individual block and that block needs to be
allocated as a whole, so no space would be saved at all. So that does
not make sense unless there is some new format.
And compression after the save is finished is in my opinion kind of
pointless. You don't save time and you only save disk space _after_ the
compression step is done. Not to mention you'd have to uncompress it
again before starting QEMU from it. I'd be fine with making users
choose between compression and mapped-ram, at least for now. They can
compress the resulting file on their own.
I'd prefer to make compression incompatible with mapped-ram too. Sounds like
Daniel is agreeable to that as well.
Regards,
Jim
> Note the logical file size of mapped-ram saved images is slightly
> larger than guest RAM size, so the files are often much larger than the
> files produced by the existing, sequential format. However, actual blocks
> written to disk is often lower with mapped-ram saved images. E.g. a saved
> image from a 30G, freshly booted, idle guest results in the following
> 'Size' and 'Blocks' values reported by stat(1)
>
> Size Blocks
> sequential 998595770 1950392
> mapped-ram 34368584225 1800456
>
> With the same guest running a workload that dirties memory
>
> Size Blocks
> sequential 33173330615 64791672
> mapped-ram 34368578210 64706944
>
That's fine and even better. It saves space, the only thing that
everyone needs to keep in mind is to treat it as a sparse file.
The other option for the space saving would be to consolidate the
streamed changes in the resulting file, but for little to no gain.
The mapped-ram is better.
> Thanks for any comments on this RFC!
>
> [1]
>
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/m...
> [2]
>
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/K...
> [3] https://mail.gnu.org/archive/html/qemu-devel/2024-05/msg04432.html
Some more teeny tiny comments in two patches will follow.
Martin
>
> Jim Fehlig (9):
> qemu: Enable mapped-ram migration capability
> qemu_fd: Add function to retrieve fdset ID
> qemu: Add function to get migration params for save
> qemu: Add a 'features' element to save image header and bump version
> qemu: conf: Add setting for save image version
> qemu: Add support for mapped-ram on save
> qemu: Enable mapped-ram on restore
> qemu: Support O_DIRECT with mapped-ram on save
> qemu: Support O_DIRECT with mapped-ram on restore
>
> src/qemu/libvirtd_qemu.aug | 1 +
> src/qemu/qemu.conf.in | 6 +
> src/qemu/qemu_conf.c | 8 ++
> src/qemu/qemu_conf.h | 1 +
> src/qemu/qemu_driver.c | 25 ++--
> src/qemu/qemu_fd.c | 18 +++
> src/qemu/qemu_fd.h | 3 +
> src/qemu/qemu_migration.c | 99 ++++++++++++++-
> src/qemu/qemu_migration.h | 11 +-
> src/qemu/qemu_migration_params.c | 20 +++
> src/qemu/qemu_migration_params.h | 4 +
> src/qemu/qemu_monitor.c | 40 ++++++
> src/qemu/qemu_monitor.h | 5 +
> src/qemu/qemu_process.c | 63 +++++++---
> src/qemu/qemu_process.h | 16 ++-
> src/qemu/qemu_saveimage.c | 187 +++++++++++++++++++++++------
> src/qemu/qemu_saveimage.h | 20 ++-
> src/qemu/qemu_snapshot.c | 12 +-
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 19 files changed, 455 insertions(+), 85 deletions(-)
>
> --
> 2.44.0
>
10:45 a.m.
On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote:
This series is a RFC for support of QEMU's mapped-ram migration
capability [1] for saving and restoring VMs. It implements the first
part of the design approach we discussed for supporting parallel
save/restore [2]. In summary, the approach is
1. Add mapped-ram migration capability
2. Steal an element from save header 'unused' for a 'features' variable
and bump save version to 3.
3. Add /etc/libvirt/qemu.conf knob for the save format version,
defaulting to latest v3
4. Use v3 (aka mapped-ram) by default
5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
6. include: Define constants for parallel save/restore
7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
8. qemu: Add support for parallel restore. Implies mapped-ram.
Reject if v2
9. tools: add parallel parameter to virsh save command
10. tools: add parallel parameter to virsh restore command
This series implements 1-5, with the BYPASS_CACHE support in patches 8
and 9 being quite hacky. They are included to discuss approaches to make
them less hacky. See the patches for details.
I'm a little on the edge about the user interface for the choice
of formats - whether the version number knob in qemu.conf is the
ideal approach or not.
The QEMU mapped-ram capability currently does not support directio.
Fabino is working on that now [3]. This complicates merging support
in libvirt. I don't think it's reasonable to enable mapped-ram by
default when BYPASS_CACHE cannot be supported. Should we wait until
the mapped-ram directio support is merged in QEMU before supporting
mapped-ram in libvirt?
For the moment, compression is ignored in the new save version.
Currently, libvirt connects the output of QEMU's save stream to the
specified compression program via a pipe. This approach is incompatible
with mapped-ram since the fd provided to QEMU must be seekable. One
option is to reopen and compress the saved image after the actual save
operation has completed. This has the downside of requiring the iohelper
to handle BYPASS_CACHE, which would preclude us from removing it
sometime in the future. Other suggestions much welcomed.
Going back to the original motivation for mapped-ram. The first key
factor was that it will make it more viable to use multi-fd for
parallelized saving and restoring, as it lets threads write concurrently
without needing synchronization. The predictable worst case file size
when the VM is live & dirtying memory, was an added benefit.
The the streaming format, we can compress as we stream, so adding
compression burns more CPU, but this is parallelized with the QEMU
saving, so the wallclock time doesn't increase as badly.
With mapped-ram, if we want to compress, we need to wait for the
image to be fully written, as we can be re-writing regions if the
guest is live. IOW, compression cannot be parallelized with the
file writing. So compression will add significant wallclock time,
as well as having the transient extra disk usage penalty.
We can optimize to avoid the extra disk penalty by using discard
on the intermediate file every time we read a chunk. eg this
loop:
while () {
read 1 MB from save file
discard 1MB from save file
write 1 MB to compressor pipe
}
We can't avoid the wallclock penalty from compression, and as you
say, we still need the iohelper chained after the compressor to
get O_DIRECT.
Note the logical file size of mapped-ram saved images is slightly
larger than guest RAM size, so the files are often much larger than the
files produced by the existing, sequential format. However, actual blocks
written to disk is often lower with mapped-ram saved images. E.g. a saved
image from a 30G, freshly booted, idle guest results in the following
'Size' and 'Blocks' values reported by stat(1)
Size Blocks
sequential 998595770 1950392
mapped-ram 34368584225 1800456
With the same guest running a workload that dirties memory
Size Blocks
sequential 33173330615 64791672
mapped-ram 34368578210 64706944
I'm a little concerned this difference between sparse and non-sparse
formats could trip up existing applications that are using libvirt.
eg, openstack uses the save/restore to file facilities and IIUC can
upload the saved file to its image storage (glance). Experiance tells
us that applications will often not handle sparseness correctly if
they have never been expecting it.
Overall I'm wondering if we need to give a direct choice to mgmt
apps.
We added a the save/restore variants that accept virTypedParameters,
so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts
'stream' and 'mapped' as options. This choice would then influence
whether we save in v2 or v3 format. On restore we don't need a
parameter as we just probe the on disk format.
As a documentation task we can then save that compression is
incompatible with 'mapped'.
Annoyingly we already have a 'save_image_formt' in qemu.conf though
taking 'raw', 'zstd', 'lzop', etc to choose the compression
type.
So we have a terminology clash.
We probably should have exposed this compression choice in the API
too, via a VIR_DOMAIN_SAVE_PARAM_COMPRESSION typed parameter, taking
values 'default', 'raw', 'zstd', 'lzop', etc, where
'default' (or
omitted) means honour the qemu.conf setting and all others override
qemu.conf
Might suggest we name the new API parameter VR_DOMAIN_SAVE_PARAM_LAYOUT
for stream vs mapped choice ?
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 :|
1:04 p.m.
On 8/7/24 09:45, Daniel P. Berrangé wrote:
On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel
wrote:
> This series is a RFC for support of QEMU's mapped-ram migration
> capability [1] for saving and restoring VMs. It implements the first
> part of the design approach we discussed for supporting parallel
> save/restore [2]. In summary, the approach is
>
> 1. Add mapped-ram migration capability
> 2. Steal an element from save header 'unused' for a 'features'
variable
> and bump save version to 3.
> 3. Add /etc/libvirt/qemu.conf knob for the save format version,
> defaulting to latest v3
> 4. Use v3 (aka mapped-ram) by default
> 5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
> 6. include: Define constants for parallel save/restore
> 7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
> 8. qemu: Add support for parallel restore. Implies mapped-ram.
> Reject if v2
> 9. tools: add parallel parameter to virsh save command
> 10. tools: add parallel parameter to virsh restore command
>
> This series implements 1-5, with the BYPASS_CACHE support in patches 8
> and 9 being quite hacky. They are included to discuss approaches to make
> them less hacky. See the patches for details.
I'm a little on the edge about the user interface for the choice
of formats - whether the version number knob in qemu.conf is the
ideal approach or not.
Yeah, I'm not fond of the idea either after seeing it in code and experimenting
with the result.
> The QEMU mapped-ram capability currently does not support directio.
> Fabino is working on that now [3]. This complicates merging support
> in libvirt. I don't think it's reasonable to enable mapped-ram by
> default when BYPASS_CACHE cannot be supported. Should we wait until
> the mapped-ram directio support is merged in QEMU before supporting
> mapped-ram in libvirt?
>
> For the moment, compression is ignored in the new save version.
> Currently, libvirt connects the output of QEMU's save stream to the
> specified compression program via a pipe. This approach is incompatible
> with mapped-ram since the fd provided to QEMU must be seekable. One
> option is to reopen and compress the saved image after the actual save
> operation has completed. This has the downside of requiring the iohelper
> to handle BYPASS_CACHE, which would preclude us from removing it
> sometime in the future. Other suggestions much welcomed.
Going back to the original motivation for mapped-ram. The first key
factor was that it will make it more viable to use multi-fd for
parallelized saving and restoring, as it lets threads write concurrently
without needing synchronization. The predictable worst case file size
when the VM is live & dirtying memory, was an added benefit.
Correct. The main motivation for mapped-ram is parallelizing save/restore. To
that end, we could base the whole feature on whether parallel is requested. If
so, we save with mapped-ram (failing if not supported by underlying qemu) and
don't support compression. If parallel not requested, use existing save/restore
implementation.
The the streaming format, we can compress as we stream, so adding
compression burns more CPU, but this is parallelized with the QEMU
saving, so the wallclock time doesn't increase as badly.
With mapped-ram, if we want to compress, we need to wait for the
image to be fully written, as we can be re-writing regions if the
guest is live. IOW, compression cannot be parallelized with the
file writing. So compression will add significant wallclock time,
as well as having the transient extra disk usage penalty.
We can optimize to avoid the extra disk penalty by using discard
on the intermediate file every time we read a chunk. eg this
loop:
while () {
read 1 MB from save file
discard 1MB from save file
write 1 MB to compressor pipe
}
We can't avoid the wallclock penalty from compression, and as you
say, we still need the iohelper chained after the compressor to
get O_DIRECT.
> Note the logical file size of mapped-ram saved images is slightly
> larger than guest RAM size, so the files are often much larger than the
> files produced by the existing, sequential format. However, actual blocks
> written to disk is often lower with mapped-ram saved images. E.g. a saved
> image from a 30G, freshly booted, idle guest results in the following
> 'Size' and 'Blocks' values reported by stat(1)
>
> Size Blocks
> sequential 998595770 1950392
> mapped-ram 34368584225 1800456
>
> With the same guest running a workload that dirties memory
>
> Size Blocks
> sequential 33173330615 64791672
> mapped-ram 34368578210 64706944
I'm a little concerned this difference between sparse and non-sparse
formats could trip up existing applications that are using libvirt.
eg, openstack uses the save/restore to file facilities and IIUC can
upload the saved file to its image storage (glance). Experiance tells
us that applications will often not handle sparseness correctly if
they have never been expecting it.
We can avoid these cases if mapped-ram is only used in conjunction with
parallel. To make use of the parallel feature, users would need to adjust their
tooling, if needed. Any opinions on that?
Overall I'm wondering if we need to give a direct choice to mgmt
apps.
We added a the save/restore variants that accept virTypedParameters,
so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts
'stream' and 'mapped' as options. This choice would then influence
whether we save in v2 or v3 format. On restore we don't need a
parameter as we just probe the on disk format.
I worry these options will confuse users. IMO we'd need a basic description of
the stream formats, along with caveats of file size, lack of compression
support, etc. It seems like quite a bit for the average user to absorb.
As a documentation task we can then save that compression is
incompatible with 'mapped'.
Annoyingly we already have a 'save_image_formt' in qemu.conf though
taking 'raw', 'zstd', 'lzop', etc to choose the compression
type.
So we have a terminology clash.
We probably should have exposed this compression choice in the API
too, via a VIR_DOMAIN_SAVE_PARAM_COMPRESSION typed parameter, taking
values 'default', 'raw', 'zstd', 'lzop', etc, where
'default' (or
omitted) means honour the qemu.conf setting and all others override
qemu.conf
Agree with that suggestion. And VIR_DOMAIN_SAVE_PARAM_COMPRESSION would be easy
to describe and clear to average user :-).
Might suggest we name the new API parameter VR_DOMAIN_SAVE_PARAM_LAYOUT
for stream vs mapped choice ?
These stream formats seem like an implementation detail we should attempt to
hide from the user.
Regards,
Jim
1:39 p.m.
On Wed, Aug 07, 2024 at 12:04:18PM -0600, Jim Fehlig wrote:
On 8/7/24 09:45, Daniel P. Berrangé wrote:
> On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote:
> > The QEMU mapped-ram capability currently does not support directio.
> > Fabino is working on that now [3]. This complicates merging support
> > in libvirt. I don't think it's reasonable to enable mapped-ram by
> > default when BYPASS_CACHE cannot be supported. Should we wait until
> > the mapped-ram directio support is merged in QEMU before supporting
> > mapped-ram in libvirt?
> >
> > For the moment, compression is ignored in the new save version.
> > Currently, libvirt connects the output of QEMU's save stream to the
> > specified compression program via a pipe. This approach is incompatible
> > with mapped-ram since the fd provided to QEMU must be seekable. One
> > option is to reopen and compress the saved image after the actual save
> > operation has completed. This has the downside of requiring the iohelper
> > to handle BYPASS_CACHE, which would preclude us from removing it
> > sometime in the future. Other suggestions much welcomed.
>
> Going back to the original motivation for mapped-ram. The first key
> factor was that it will make it more viable to use multi-fd for
> parallelized saving and restoring, as it lets threads write concurrently
> without needing synchronization. The predictable worst case file size
> when the VM is live & dirtying memory, was an added benefit.
Correct. The main motivation for mapped-ram is parallelizing save/restore.
To that end, we could base the whole feature on whether parallel is
requested. If so, we save with mapped-ram (failing if not supported by
underlying qemu) and don't support compression. If parallel not requested,
use existing save/restore implementation.
We had discussed that as a strategy previously and thought it
was a good idea.
Your other reply to me though makes me re-consider. You're
showing that /without/ O_DIRECT and multifd, then mapped-ram
is already a big win over the legacy method. The scale of that
improvement is somewhat surprising to me and if reliably so,
that is compelling to want all the time.
> Overall I'm wondering if we need to give a direct choice to
mgmt
> apps.
>
> We added a the save/restore variants that accept virTypedParameters,
> so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts
> 'stream' and 'mapped' as options. This choice would then influence
> whether we save in v2 or v3 format. On restore we don't need a
> parameter as we just probe the on disk format.
I worry these options will confuse users. IMO we'd need a basic description
of the stream formats, along with caveats of file size, lack of compression
support, etc. It seems like quite a bit for the average user to absorb.
>
> As a documentation task we can then save that compression is
> incompatible with 'mapped'.
>
> Annoyingly we already have a 'save_image_formt' in qemu.conf though
> taking 'raw', 'zstd', 'lzop', etc to choose the compression
type.
> So we have a terminology clash.
>
> We probably should have exposed this compression choice in the API
> too, via a VIR_DOMAIN_SAVE_PARAM_COMPRESSION typed parameter, taking
> values 'default', 'raw', 'zstd', 'lzop', etc, where
'default' (or
> omitted) means honour the qemu.conf setting and all others override
> qemu.conf
Agree with that suggestion. And VIR_DOMAIN_SAVE_PARAM_COMPRESSION would be
easy to describe and clear to average user :-).
>
> Might suggest we name the new API parameter VR_DOMAIN_SAVE_PARAM_LAYOUT
> for stream vs mapped choice ?
These stream formats seem like an implementation detail we should attempt to
hide from the user.
Yeah it is rather an impl detail, but perhaps it is an impl detail we
cannot avoid exposing users to. After all they need to be aware of
use of sparseness in order to handle the files without ballooning up
their size.
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 :|
6:46 p.m.
On 8/7/24 12:39, Daniel P. Berrangé wrote:
On Wed, Aug 07, 2024 at 12:04:18PM -0600, Jim Fehlig wrote:
> On 8/7/24 09:45, Daniel P. Berrangé wrote:
>> On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote:
>>> The QEMU mapped-ram capability currently does not support directio.
>>> Fabino is working on that now [3]. This complicates merging support
>>> in libvirt. I don't think it's reasonable to enable mapped-ram by
>>> default when BYPASS_CACHE cannot be supported. Should we wait until
>>> the mapped-ram directio support is merged in QEMU before supporting
>>> mapped-ram in libvirt?
>>>
>>> For the moment, compression is ignored in the new save version.
>>> Currently, libvirt connects the output of QEMU's save stream to the
>>> specified compression program via a pipe. This approach is incompatible
>>> with mapped-ram since the fd provided to QEMU must be seekable. One
>>> option is to reopen and compress the saved image after the actual save
>>> operation has completed. This has the downside of requiring the iohelper
>>> to handle BYPASS_CACHE, which would preclude us from removing it
>>> sometime in the future. Other suggestions much welcomed.
>>
>> Going back to the original motivation for mapped-ram. The first key
>> factor was that it will make it more viable to use multi-fd for
>> parallelized saving and restoring, as it lets threads write concurrently
>> without needing synchronization. The predictable worst case file size
>> when the VM is live & dirtying memory, was an added benefit.
>
> Correct. The main motivation for mapped-ram is parallelizing save/restore.
> To that end, we could base the whole feature on whether parallel is
> requested. If so, we save with mapped-ram (failing if not supported by
> underlying qemu) and don't support compression. If parallel not requested,
> use existing save/restore implementation.
We had discussed that as a strategy previously and thought it
was a good idea.
Your other reply to me though makes me re-consider. You're
showing that /without/ O_DIRECT and multifd, then mapped-ram
is already a big win over the legacy method. The scale of that
improvement is somewhat surprising to me and if reliably so,
that is compelling to want all the time.
How do we have it all of the time without issues of spareness you describe?
Seems it will always be opt-in, unless using the new parallel option.
>> Overall I'm wondering if we need to give a direct choice to mgmt
>> apps.
>>
>> We added a the save/restore variants that accept virTypedParameters,
>> so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts
>> 'stream' and 'mapped' as options. This choice would then
influence
>> whether we save in v2 or v3 format. On restore we don't need a
>> parameter as we just probe the on disk format.
>
> I worry these options will confuse users. IMO we'd need a basic description
> of the stream formats, along with caveats of file size, lack of compression
> support, etc. It seems like quite a bit for the average user to absorb.
>
>>
>> As a documentation task we can then save that compression is
>> incompatible with 'mapped'.
>>
>> Annoyingly we already have a 'save_image_formt' in qemu.conf though
>> taking 'raw', 'zstd', 'lzop', etc to choose the
compression type.
>> So we have a terminology clash.
>>
>> We probably should have exposed this compression choice in the API
>> too, via a VIR_DOMAIN_SAVE_PARAM_COMPRESSION typed parameter, taking
>> values 'default', 'raw', 'zstd', 'lzop', etc,
where 'default' (or
>> omitted) means honour the qemu.conf setting and all others override
>> qemu.conf
>
> Agree with that suggestion. And VIR_DOMAIN_SAVE_PARAM_COMPRESSION would be
> easy to describe and clear to average user :-).
>
>>
>> Might suggest we name the new API parameter VR_DOMAIN_SAVE_PARAM_LAYOUT
>> for stream vs mapped choice ?
>
> These stream formats seem like an implementation detail we should attempt to
> hide from the user.
Yeah it is rather an impl detail, but perhaps it is an impl detail we
cannot avoid exposing users to. After all they need to be aware of
use of sparseness in order to handle the files without ballooning up
their size.
As for a name for the new parameter, I can't think of anything better than your
suggestion of VIR_DOMAIN_SAVE_PARAM_LAYOUT. I much prefer _FORMAT, but yeah,
that ship has sailed.
Before working on the API parameter, I'll summarize my understanding of the
options for supporting mapped-ram:
1. No user control. If qemu supports mapped-ram, use it
2. Control at driver level
3. Control at the API level
We don't want 1 due to incompatibilities with compression, sparseness, etc.
Experience with the compression feature has shown that 2 is inflexible. Control
at the API level is preferred for maximum user flexibility.
Regards,
Jim
6:50 p.m.
On 8/8/24 17:46, Jim Fehlig wrote:
On 8/7/24 12:39, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:04:18PM -0600, Jim Fehlig wrote:
>> On 8/7/24 09:45, Daniel P. Berrangé wrote:
>>> On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote:
>>>> The QEMU mapped-ram capability currently does not support directio.
>>>> Fabino is working on that now [3]. This complicates merging support
>>>> in libvirt. I don't think it's reasonable to enable mapped-ram
by
>>>> default when BYPASS_CACHE cannot be supported. Should we wait until
>>>> the mapped-ram directio support is merged in QEMU before supporting
>>>> mapped-ram in libvirt?
>>>>
>>>> For the moment, compression is ignored in the new save version.
>>>> Currently, libvirt connects the output of QEMU's save stream to the
>>>> specified compression program via a pipe. This approach is incompatible
>>>> with mapped-ram since the fd provided to QEMU must be seekable. One
>>>> option is to reopen and compress the saved image after the actual save
>>>> operation has completed. This has the downside of requiring the iohelper
>>>> to handle BYPASS_CACHE, which would preclude us from removing it
>>>> sometime in the future. Other suggestions much welcomed.
>>>
>>> Going back to the original motivation for mapped-ram. The first key
>>> factor was that it will make it more viable to use multi-fd for
>>> parallelized saving and restoring, as it lets threads write concurrently
>>> without needing synchronization. The predictable worst case file size
>>> when the VM is live & dirtying memory, was an added benefit.
>>
>> Correct. The main motivation for mapped-ram is parallelizing save/restore.
>> To that end, we could base the whole feature on whether parallel is
>> requested. If so, we save with mapped-ram (failing if not supported by
>> underlying qemu) and don't support compression. If parallel not requested,
>> use existing save/restore implementation.
>
> We had discussed that as a strategy previously and thought it
> was a good idea.
>
> Your other reply to me though makes me re-consider. You're
> showing that /without/ O_DIRECT and multifd, then mapped-ram
> is already a big win over the legacy method. The scale of that
> improvement is somewhat surprising to me and if reliably so,
> that is compelling to want all the time.
How do we have it all of the time without issues of spareness you describe?
Seems it will always be opt-in, unless using the new parallel option.
>
>>> Overall I'm wondering if we need to give a direct choice to mgmt
>>> apps.
>>>
>>> We added a the save/restore variants that accept virTypedParameters,
>>> so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts
>>> 'stream' and 'mapped' as options. This choice would then
influence
>>> whether we save in v2 or v3 format. On restore we don't need a
>>> parameter as we just probe the on disk format.
>>
>> I worry these options will confuse users. IMO we'd need a basic description
>> of the stream formats, along with caveats of file size, lack of compression
>> support, etc. It seems like quite a bit for the average user to absorb.
>>
>>>
>>> As a documentation task we can then save that compression is
>>> incompatible with 'mapped'.
>>>
>>> Annoyingly we already have a 'save_image_formt' in qemu.conf though
>>> taking 'raw', 'zstd', 'lzop', etc to choose the
compression type.
>>> So we have a terminology clash.
>>>
>>> We probably should have exposed this compression choice in the API
>>> too, via a VIR_DOMAIN_SAVE_PARAM_COMPRESSION typed parameter, taking
>>> values 'default', 'raw', 'zstd', 'lzop', etc,
where 'default' (or
>>> omitted) means honour the qemu.conf setting and all others override
>>> qemu.conf
>>
>> Agree with that suggestion. And VIR_DOMAIN_SAVE_PARAM_COMPRESSION would be
>> easy to describe and clear to average user :-).
>>
>>>
>>> Might suggest we name the new API parameter VR_DOMAIN_SAVE_PARAM_LAYOUT
>>> for stream vs mapped choice ?
>>
>> These stream formats seem like an implementation detail we should attempt to
>> hide from the user.
>
> Yeah it is rather an impl detail, but perhaps it is an impl detail we
> cannot avoid exposing users to. After all they need to be aware of
> use of sparseness in order to handle the files without ballooning up
> their size.
As for a name for the new parameter, I can't think of anything better than your
suggestion of VIR_DOMAIN_SAVE_PARAM_LAYOUT. I much prefer _FORMAT, but yeah,
that ship has sailed.
Before working on the API parameter, I'll summarize my understanding of the
options for supporting mapped-ram:
1. No user control. If qemu supports mapped-ram, use it
2. Control at driver level
Forgot to mention: I sent the implementation for this option since it was ready
anyhow
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MN...
Regards,
Jim
6:16 p.m.
On 8/7/24 09:45, Daniel P. Berrangé wrote:
On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel
wrote:
> This series is a RFC for support of QEMU's mapped-ram migration
> capability [1] for saving and restoring VMs. It implements the first
> part of the design approach we discussed for supporting parallel
> save/restore [2]. In summary, the approach is
>
> 1. Add mapped-ram migration capability
> 2. Steal an element from save header 'unused' for a 'features'
variable
> and bump save version to 3.
> 3. Add /etc/libvirt/qemu.conf knob for the save format version,
> defaulting to latest v3
> 4. Use v3 (aka mapped-ram) by default
> 5. Use mapped-ram with BYPASS_CACHE for v3, old approach for v2
> 6. include: Define constants for parallel save/restore
> 7. qemu: Add support for parallel save. Implies mapped-ram, reject if v2
> 8. qemu: Add support for parallel restore. Implies mapped-ram.
> Reject if v2
> 9. tools: add parallel parameter to virsh save command
> 10. tools: add parallel parameter to virsh restore command
>
> This series implements 1-5, with the BYPASS_CACHE support in patches 8
> and 9 being quite hacky. They are included to discuss approaches to make
> them less hacky. See the patches for details.
I'm a little on the edge about the user interface for the choice
of formats - whether the version number knob in qemu.conf is the
ideal approach or not.
> The QEMU mapped-ram capability currently does not support directio.
> Fabino is working on that now [3]. This complicates merging support
> in libvirt. I don't think it's reasonable to enable mapped-ram by
> default when BYPASS_CACHE cannot be supported. Should we wait until
> the mapped-ram directio support is merged in QEMU before supporting
> mapped-ram in libvirt?
>
> For the moment, compression is ignored in the new save version.
> Currently, libvirt connects the output of QEMU's save stream to the
> specified compression program via a pipe. This approach is incompatible
> with mapped-ram since the fd provided to QEMU must be seekable. One
> option is to reopen and compress the saved image after the actual save
> operation has completed. This has the downside of requiring the iohelper
> to handle BYPASS_CACHE, which would preclude us from removing it
> sometime in the future. Other suggestions much welcomed.
Going back to the original motivation for mapped-ram. The first key
factor was that it will make it more viable to use multi-fd for
parallelized saving and restoring, as it lets threads write concurrently
without needing synchronization. The predictable worst case file size
when the VM is live & dirtying memory, was an added benefit.
The the streaming format, we can compress as we stream, so adding
compression burns more CPU, but this is parallelized with the QEMU
saving, so the wallclock time doesn't increase as badly.
With mapped-ram, if we want to compress, we need to wait for the
image to be fully written, as we can be re-writing regions if the
guest is live. IOW, compression cannot be parallelized with the
file writing. So compression will add significant wallclock time,
as well as having the transient extra disk usage penalty.
We can optimize to avoid the extra disk penalty by using discard
on the intermediate file every time we read a chunk. eg this
loop:
while () {
read 1 MB from save file
discard 1MB from save file
write 1 MB to compressor pipe
}
We can't avoid the wallclock penalty from compression, and as you
say, we still need the iohelper chained after the compressor to
get O_DIRECT.
> Note the logical file size of mapped-ram saved images is slightly
> larger than guest RAM size, so the files are often much larger than the
> files produced by the existing, sequential format. However, actual blocks
> written to disk is often lower with mapped-ram saved images. E.g. a saved
> image from a 30G, freshly booted, idle guest results in the following
> 'Size' and 'Blocks' values reported by stat(1)
>
> Size Blocks
> sequential 998595770 1950392
> mapped-ram 34368584225 1800456
>
> With the same guest running a workload that dirties memory
>
> Size Blocks
> sequential 33173330615 64791672
> mapped-ram 34368578210 64706944
I'm a little concerned this difference between sparse and non-sparse
formats could trip up existing applications that are using libvirt.
eg, openstack uses the save/restore to file facilities and IIUC can
upload the saved file to its image storage (glance). Experiance tells
us that applications will often not handle sparseness correctly if
they have never been expecting it.
Overall I'm wondering if we need to give a direct choice to mgmt
apps.
We added a the save/restore variants that accept virTypedParameters,
so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts
'stream' and 'mapped' as options. This choice would then influence
whether we save in v2 or v3 format. On restore we don't need a
parameter as we just probe the on disk format.
As a documentation task we can then save that compression is
incompatible with 'mapped'.
Annoyingly we already have a 'save_image_formt' in qemu.conf though
taking 'raw', 'zstd', 'lzop', etc to choose the compression
type.
So we have a terminology clash.
Thinking about this more, and your previous idea about abusing the header
'compressed' field [1], I'm beginning to warm to the idea of adding a new item
to the list currently accepted by save_image_format. I'm also warming to
Fabiano's suggestion to call the new item 'sparse' :-).
AFAICT, from a user perspective, save_image_format does not imply compression.
The implication primarily stems from variable and function naming in the code
:-). The current documentation of save_image_format may slightly imply
compression, but we can easily improve that. The setting already accepts 'raw'
for the existing (non-compressed) stream format. Any opinions on adding 'sparse'
as another supported save_image_format?
Regards,
Jim
[1]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/P...
12:23 p.m.
On 8/12/24 17:16, Jim Fehlig wrote:
On 8/7/24 09:45, Daniel P. Berrangé wrote:
> Annoyingly we already have a 'save_image_formt' in qemu.conf though
> taking 'raw', 'zstd', 'lzop', etc to choose the compression
type.
> So we have a terminology clash.
Thinking about this more, and your previous idea about abusing the header
'compressed' field [1], I'm beginning to warm to the idea of adding a new
item
to the list currently accepted by save_image_format. I'm also warming to
Fabiano's suggestion to call the new item 'sparse' :-).
AFAICT, from a user perspective, save_image_format does not imply compression.
The implication primarily stems from variable and function naming in the code
:-). The current documentation of save_image_format may slightly imply
compression, but we can easily improve that. The setting already accepts 'raw'
for the existing (non-compressed) stream format.
E.g. something like this small series
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/BX...
Regards,
Jim
97
days inactive
161
days old
22 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Jim Fehlig -
Martin Kletzander