In some cases management application needs to allocate memory for
qemu upfront and then just let qemu use that. Since we don't want
to expose path for memory-backend-file anywhere in the domain
XML, we can have a configuration knob that when enabled generated
predictable paths. In this case:
$memoryBackingDir/libvirt/qemu/$shortName/$alias
where $shortName is result of virDomainObjGetShortName().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 5 ++
src/qemu/qemu_command.c | 9 +--
src/qemu/qemu_conf.c | 76 +++++++++++++++++++-
src/qemu/qemu_conf.h | 10 ++-
src/qemu/qemu_driver.c | 19 +++++
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_process.c | 141 ++++++++++++++++++++++++++-----------
src/qemu/qemu_process.h | 8 +--
src/qemu/test_libvirtd_qemu.aug.in | 1 +
10 files changed, 220 insertions(+), 52 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index c19bf3a43..77d466b14 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -114,6 +114,7 @@ module Libvirtd_qemu =
let gluster_debug_level_entry = int_entry "gluster_debug_level"
let memory_entry = str_entry "memory_backing_dir"
+ | bool_entry "memory_predictable_file_names"
let vxhs_entry = bool_entry "vxhs_tls"
| str_entry "vxhs_tls_x509_cert_dir"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 2e8370a5a..dc3098148 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -769,3 +769,8 @@
# This directory is used for memoryBacking source if configured as file.
# NOTE: big files will be stored here
#memory_backing_dir = "/var/lib/libvirt/qemu/ram"
+
+# Use predictable file names. If this is enabled, Libvirt constructs
+# full paths for memory-backing-file objects. This is experimental
+# feature and generated paths can change across releases. Don't use it.
+#memory_predictable_file_names = 1
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82d2fb2a3..d6d2654cd 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
} else {
/* We can have both pagesize and mem source. If that's the case,
* prefer hugepages as those are more specific. */
- if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
+ if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, &memPath) < 0)
goto cleanup;
}
@@ -3542,12 +3542,13 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
cell);
+ if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
+ goto cleanup;
+
*backendStr = NULL;
mem.size = memsize;
mem.targetNode = cell;
-
- if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
- goto cleanup;
+ mem.info.alias = alias;
if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, priv->qemuCaps,
def, &mem, priv->autoNodeset, false)) < 0)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 23dcc25e1..34f411137 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -912,6 +912,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0)
goto cleanup;
+ if (virConfGetValueBool(conf, "memory_predictable_file_names",
+ &cfg->memoryPredictableFileNames) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
@@ -1750,9 +1754,53 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
}
+int
+qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+ if (!cfg->memoryPredictableFileNames) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("predictable file names are disabled"));
+ return -1;
+ }
+
+ return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir);
+}
+
+
+int
+qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+ char *shortName = NULL;
+ char *base = NULL;
+ int ret = -1;
+
+ if (!cfg->memoryPredictableFileNames) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("predictable file names are disabled"));
+ return -1;
+ }
+
+ if (!(shortName = virDomainDefGetShortName(def)) ||
+ qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
+ virAsprintf(path, "%s/%s", base, shortName) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(base);
+ VIR_FREE(shortName);
+ return ret;
+}
+
+
/**
* qemuGetMemoryBackingPath:
+ * @def: domain definition
* @cfg: the driver config
+ * @alias: memory object alias
* @memPath: constructed path
*
* Constructs path to memory backing dir and stores it at @memPath.
@@ -1761,8 +1809,32 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
* -1 otherwise (with error reported).
*/
int
-qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
char **memPath)
{
- return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+ char *domainPath = NULL;
+ int ret = -1;
+
+ if (cfg->memoryPredictableFileNames) {
+ if (!alias) {
+ /* This should never happen (TM) */
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("memory device alias is not assigned"));
+ goto cleanup;
+ }
+
+ if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
+ virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
+ goto cleanup;
+ } else {
+ if (VIR_STRDUP(*memPath, cfg->memoryBackingDir) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(domainPath);
+ return ret;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 9d6866816..5b72788c9 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -203,6 +203,7 @@ struct _virQEMUDriverConfig {
unsigned int glusterDebugLevel;
char *memoryBackingDir;
+ bool memoryPredictableFileNames;
bool vxhsTLS;
char *vxhsTLSx509certdir;
@@ -364,6 +365,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
unsigned long long pagesize,
char **memPath);
-int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+int qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
char **memPath);
#endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dacfca9a9..7bdefd7e8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -630,6 +630,7 @@ qemuStateInitialize(bool privileged,
uid_t run_uid = -1;
gid_t run_gid = -1;
char *hugepagePath = NULL;
+ char *memoryBackingPath = NULL;
size_t i;
if (VIR_ALLOC(qemu_driver) < 0)
@@ -888,6 +889,23 @@ qemuStateInitialize(bool privileged,
VIR_FREE(hugepagePath);
}
+ if (cfg->memoryPredictableFileNames) {
+ if (qemuGetMemoryBackingBasePath(cfg, &memoryBackingPath) < 0)
+ goto error;
+
+ if (virFileMakePath(memoryBackingPath) < 0) {
+ virReportSystemError(errno,
+ _("unable to create memory backing path %s"),
+ memoryBackingPath);
+ goto error;
+ }
+ if (privileged &&
+ virFileUpdatePerm(memoryBackingPath,
+ 0, S_IXGRP | S_IXOTH) < 0)
+ goto error;
+ VIR_FREE(memoryBackingPath);
+ }
+
if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
goto error;
@@ -945,6 +963,7 @@ qemuStateInitialize(bool privileged,
virObjectUnref(conn);
VIR_FREE(driverConf);
VIR_FREE(hugepagePath);
+ VIR_FREE(memoryBackingPath);
qemuStateCleanup();
return -1;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 91f7f9ed6..5701c033b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2077,7 +2077,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
priv->qemuCaps, vm->def, mem, NULL, true) < 0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0)
goto cleanup;
if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fdc868912..5dea02718 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3324,60 +3324,121 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def,
}
+static bool
+qemuProcessNeedMemoryBackingPath(virDomainDefPtr def,
+ virQEMUDriverConfigPtr cfg,
+ virDomainMemoryDefPtr mem)
+{
+ size_t i;
+ size_t numaNodes;
+
+ if (!cfg->memoryPredictableFileNames)
+ return false;
+
+ if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE ||
+ def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
+ return true;
+
+ numaNodes = virDomainNumaGetNodeCount(def->numa);
+ for (i = 0; i < numaNodes; i++) {
+ if (virDomainNumaGetNodeMemoryAccessMode(def->numa, i)
+ != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
+ return true;
+ }
+
+ if (mem &&
+ mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ (mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT ||
+ (mem->targetNode >= 0 &&
+ virDomainNumaGetNodeMemoryAccessMode(def->numa, mem->targetNode)
+ != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)))
+ return true;
+
+ return false;
+}
+
+
+static int
+qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ const char *path,
+ bool build)
+{
+ if (build) {
+ if (virFileExists(path))
+ return 0;
+
+ if (virFileMakePathWithMode(path, 0700) < 0) {
+ virReportSystemError(errno,
+ _("Unable to create %s"),
+ path);
+ return -1;
+ }
+
+ if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+ def, path) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to label %s"), path);
+ return -1;
+ }
+ } else {
+ if (rmdir(path) < 0 &&
+ errno != ENOENT)
+ VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
+ path, errno);
+ }
+
+ return 0;
+}
+
+
int
-qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build)
+qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
- char *hugepagePath = NULL;
+ char *path = NULL;
size_t i;
- bool shouldBuild = false;
+ bool shouldBuildHP = false;
+ bool shouldBuildMB = false;
int ret = -1;
- if (build)
- shouldBuild = qemuProcessNeedHugepagesPath(vm->def, mem);
+ if (build) {
+ shouldBuildHP = qemuProcessNeedHugepagesPath(vm->def, mem);
+ shouldBuildMB = qemuProcessNeedMemoryBackingPath(vm->def, cfg, mem);
+ }
- if (!build || shouldBuild) {
+ if (!build || shouldBuildHP) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
- VIR_FREE(hugepagePath);
- hugepagePath = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]);
+ path = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]);
- if (!hugepagePath)
+ if (!path)
goto cleanup;
- if (build) {
- if (virFileExists(hugepagePath)) {
- ret = 0;
- goto cleanup;
- }
-
- if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
- virReportSystemError(errno,
- _("Unable to create %s"),
- hugepagePath);
- goto cleanup;
- }
+ if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+ path, build) < 0)
+ goto cleanup;
- if (qemuSecurityDomainSetPathLabel(driver->securityManager,
- vm->def, hugepagePath) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Unable to set huge path in security driver"));
- goto cleanup;
- }
- } else {
- if (rmdir(hugepagePath) < 0 &&
- errno != ENOENT)
- VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
- hugepagePath, errno);
- }
+ VIR_FREE(path);
}
}
+ if (!build || shouldBuildMB) {
+ if (qemuGetMemoryBackingDomainPath(vm->def, cfg, &path) < 0)
+ goto cleanup;
+
+ if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+ path, build) < 0)
+ goto cleanup;
+
+ VIR_FREE(path);
+ }
+
ret = 0;
cleanup:
- VIR_FREE(hugepagePath);
+ VIR_FREE(path);
virObjectUnref(cfg);
return ret;
}
@@ -5550,7 +5611,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
NULL) < 0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0)
goto cleanup;
/* Ensure no historical cgroup for this VM is lying around bogus
@@ -6254,7 +6315,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
goto endjob;
}
- qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
+ qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
vm->def->id = -1;
@@ -7112,7 +7173,7 @@ qemuProcessReconnect(void *opaque)
goto cleanup;
}
- if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, obj, NULL, true) < 0)
goto error;
if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 814b86d8a..cd9a72031 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -38,10 +38,10 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
virDomainPausedReason reason,
qemuDomainAsyncJob asyncJob);
-int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build);
+int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build);
void qemuProcessAutostartAll(virQEMUDriverPtr driver);
void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 688e5b9fd..2f0a1277d 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -100,3 +100,4 @@ module Test_libvirtd_qemu =
{ "1" = "mount" }
}
{ "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
+{ "memory_predictable_file_names" = "1" }
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 23, 2017 at 05:43:16PM +0200, Michal Privoznik wrote:
> In some cases management application needs to allocate memory for
> qemu upfront and then just let qemu use that. Since we don't want
> to expose path for memory-backend-file anywhere in the domain
> XML, we can have a configuration knob that when enabled generated
> predictable paths. In this case:
>
> $memoryBackingDir/libvirt/qemu/$shortName/$alias
>
> where $shortName is result of virDomainObjGetShortName().
Looking at the current test cases, it seems that for huge pages
we currently create a directory
$hugepagesMountPoint/libvirt/qemu/$shortName
and for non-hugepages we just use
$memoryBackingDir (== /var/lib/libvirt/ram)
In both cases, we then just let QEMU create a random tempfile
name within these directories.
So, IIUC, your proposal here is just making the non-hugepages
case work the same way the hugepages case works, except that
it is adding an extra level of $alias in the directory path.
I don't think this extra level of nesting is really desirable,
or needed. We should just give QEMU a filename, so that it
doesn't create a random tempfile. This is achieved by simply
doing a
mkdir($memoryBackingDir/libvirt/qemu/$shortName)
but then giving a
mem-path=$memoryBackingDir/libvirt/qemu/$shortName/$alias
QEMU will try to open that, and will get ENOENT, at which
point it to O_CREAT that file with the exact name we've
given, instead of using a tempfile name.
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/qemu/libvirtd_qemu.aug | 1 +
> src/qemu/qemu.conf | 5 ++
> src/qemu/qemu_command.c | 9 +--
> src/qemu/qemu_conf.c | 76 +++++++++++++++++++-
> src/qemu/qemu_conf.h | 10 ++-
> src/qemu/qemu_driver.c | 19 +++++
> src/qemu/qemu_hotplug.c | 2 +-
> src/qemu/qemu_process.c | 141 ++++++++++++++++++++++++++-----------
> src/qemu/qemu_process.h | 8 +--
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 10 files changed, 220 insertions(+), 52 deletions(-)
I'd expect to see current tests updated wrt the new naming scheme
for paths
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index c19bf3a43..77d466b14 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -114,6 +114,7 @@ module Libvirtd_qemu =
> let gluster_debug_level_entry = int_entry "gluster_debug_level"
>
> let memory_entry = str_entry "memory_backing_dir"
> + | bool_entry "memory_predictable_file_names"
>
> let vxhs_entry = bool_entry "vxhs_tls"
> | str_entry "vxhs_tls_x509_cert_dir"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 2e8370a5a..dc3098148 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -769,3 +769,8 @@
> # This directory is used for memoryBacking source if configured as file.
> # NOTE: big files will be stored here
> #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
> +
> +# Use predictable file names. If this is enabled, Libvirt constructs
> +# full paths for memory-backing-file objects. This is experimental
> +# feature and generated paths can change across releases. Don't use it.
> +#memory_predictable_file_names = 1
I don't think we should expose this in the config file at all - it means
we get 2 codepaths, one of which will never be tested by 99% of the
deployments.
I think we should just make normal mem paths uses the same naming
scheme as hugepage mempaths unconditionally. There's no benefit to
keeping the old naming scheme around.
At the same time though, we don't need to promise anything wrt the new
naming scheme. If someone wants to rely on it they can, but we're not
going to promise forever compat.
> +static int
> +qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + const char *path,
> + bool build)
> +{
> + if (build) {
> + if (virFileExists(path))
> + return 0;
> +
> + if (virFileMakePathWithMode(path, 0700) < 0) {
> + virReportSystemError(errno,
> + _("Unable to create %s"),
> + path);
> + return -1;
> + }
NB, this creates potentially many levels in the dir hiearchy
$memoryBackingDir/libvirt
$memoryBackingDir/libvirt/qemu/
$memoryBackingDir/libvirt/qemu/$shortName
$memoryBackingDir/libvirt/qemu/$shortName/$alias
> +
> + if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> + def, path) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to label %s"), path);
> + return -1;
> + }
> + } else {
> + if (rmdir(path) < 0 &&
> + errno != ENOENT)
> + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
> + path, errno);
This only deletes
$memoryBackingDir/libvirt/qemu/$shortName/$alias
so we're leaving around
$memoryBackingDir/libvirt/qemu/$shortName
forever. So we need to delete both levels - leaving
$memoryBackingDir/libvirt/qemu is ok for other guests
though.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/23/2017 06:47 PM, Daniel P. Berrange wrote:
> On Mon, Oct 23, 2017 at 05:43:16PM +0200, Michal Privoznik wrote:
>> In some cases management application needs to allocate memory for
>> qemu upfront and then just let qemu use that. Since we don't want
>> to expose path for memory-backend-file anywhere in the domain
>> XML, we can have a configuration knob that when enabled generated
>> predictable paths. In this case:
>>
>> $memoryBackingDir/libvirt/qemu/$shortName/$alias
>>
>> where $shortName is result of virDomainObjGetShortName().
>
> Looking at the current test cases, it seems that for huge pages
> we currently create a directory
>
> $hugepagesMountPoint/libvirt/qemu/$shortName
>
> and for non-hugepages we just use
>
> $memoryBackingDir (== /var/lib/libvirt/ram)
>
> In both cases, we then just let QEMU create a random tempfile
> name within these directories.
>
Correct.
> So, IIUC, your proposal here is just making the non-hugepages
> case work the same way the hugepages case works, except that
> it is adding an extra level of $alias in the directory path.
Almost. $alias is actual file, not dir.
>
> I don't think this extra level of nesting is really desirable,
> or needed. We should just give QEMU a filename, so that it
> doesn't create a random tempfile. This is achieved by simply
> doing a
>
> mkdir($memoryBackingDir/libvirt/qemu/$shortName)
>
> but then giving a
>
> mem-path=$memoryBackingDir/libvirt/qemu/$shortName/$alias
Yes. That's exactly what I'm doing. A snippet from next patch where we
can see this in action:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
index 5700c3413..352819429 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
@@ -13 +13,1 @@ QEMU_AUDIO_DRV=none \
--object
memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object
memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
And even though you're not disputing '/libvirt/qemu/' part in the path,
I'll just point out for future reference that memoryBackingDir can be
just any path in the system. It's just that the default is
/var/lib/libvirt/qemu which makes the generated path ugly (and indeed
renders the part redundant).
>
> QEMU will try to open that, and will get ENOENT, at which
> point it to O_CREAT that file with the exact name we've
> given, instead of using a tempfile name.
>
>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/libvirtd_qemu.aug | 1 +
>> src/qemu/qemu.conf | 5 ++
>> src/qemu/qemu_command.c | 9 +--
>> src/qemu/qemu_conf.c | 76 +++++++++++++++++++-
>> src/qemu/qemu_conf.h | 10 ++-
>> src/qemu/qemu_driver.c | 19 +++++
>> src/qemu/qemu_hotplug.c | 2 +-
>> src/qemu/qemu_process.c | 141 ++++++++++++++++++++++++++-----------
>> src/qemu/qemu_process.h | 8 +--
>> src/qemu/test_libvirtd_qemu.aug.in | 1 +
>> 10 files changed, 220 insertions(+), 52 deletions(-)
>
> I'd expect to see current tests updated wrt the new naming scheme
> for paths
See next patch.
>
>> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
>> index c19bf3a43..77d466b14 100644
>> --- a/src/qemu/libvirtd_qemu.aug
>> +++ b/src/qemu/libvirtd_qemu.aug
>> @@ -114,6 +114,7 @@ module Libvirtd_qemu =
>> let gluster_debug_level_entry = int_entry "gluster_debug_level"
>>
>> let memory_entry = str_entry "memory_backing_dir"
>> + | bool_entry "memory_predictable_file_names"
>>
>> let vxhs_entry = bool_entry "vxhs_tls"
>> | str_entry "vxhs_tls_x509_cert_dir"
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index 2e8370a5a..dc3098148 100644
>> --- a/src/qemu/qemu.conf
>> +++ b/src/qemu/qemu.conf
>> @@ -769,3 +769,8 @@
>> # This directory is used for memoryBacking source if configured as file.
>> # NOTE: big files will be stored here
>> #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
>> +
>> +# Use predictable file names. If this is enabled, Libvirt constructs
>> +# full paths for memory-backing-file objects. This is experimental
>> +# feature and generated paths can change across releases. Don't use it.
>> +#memory_predictable_file_names = 1
>
> I don't think we should expose this in the config file at all - it means
> we get 2 codepaths, one of which will never be tested by 99% of the
> deployments.
>
> I think we should just make normal mem paths uses the same naming
> scheme as hugepage mempaths unconditionally. There's no benefit to
> keeping the old naming scheme around.
>
> At the same time though, we don't need to promise anything wrt the new
> naming scheme. If someone wants to rely on it they can, but we're not
> going to promise forever compat.
Even better! Cool.
>
>
>> +static int
>> +qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
>> + virDomainDefPtr def,
>> + const char *path,
>> + bool build)
>> +{
>> + if (build) {
>> + if (virFileExists(path))
>> + return 0;
>> +
>> + if (virFileMakePathWithMode(path, 0700) < 0) {
>> + virReportSystemError(errno,
>> + _("Unable to create %s"),
>> + path);
>> + return -1;
>> + }
>
> NB, this creates potentially many levels in the dir hiearchy
>
> $memoryBackingDir/libvirt
> $memoryBackingDir/libvirt/qemu/
> $memoryBackingDir/libvirt/qemu/$shortName
Only these ^^ are dirs.
> $memoryBackingDir/libvirt/qemu/$shortName/$alias
This is actual file.
>
>> +
>> + if (qemuSecurityDomainSetPathLabel(driver->securityManager,
>> + def, path) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Unable to label %s"), path);
>> + return -1;
>> + }
>> + } else {
>> + if (rmdir(path) < 0 &&
>> + errno != ENOENT)
>> + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
>> + path, errno);
>
> This only deletes
>
> $memoryBackingDir/libvirt/qemu/$shortName/$alias
>
> so we're leaving around
>
> $memoryBackingDir/libvirt/qemu/$shortName
Is that so? I'm fairly certain it removes
$memoryBackingDir/libvirt/qemu/$shortName
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 23, 2017 at 07:10:04PM +0200, Michal Privoznik wrote:
> On 10/23/2017 06:47 PM, Daniel P. Berrange wrote:
> > On Mon, Oct 23, 2017 at 05:43:16PM +0200, Michal Privoznik wrote:
> >> In some cases management application needs to allocate memory for
> >> qemu upfront and then just let qemu use that. Since we don't want
> >> to expose path for memory-backend-file anywhere in the domain
> >> XML, we can have a configuration knob that when enabled generated
> >> predictable paths. In this case:
> >>
> >> $memoryBackingDir/libvirt/qemu/$shortName/$alias
> >>
> >> where $shortName is result of virDomainObjGetShortName().
> >
> > Looking at the current test cases, it seems that for huge pages
> > we currently create a directory
> >
> > $hugepagesMountPoint/libvirt/qemu/$shortName
> >
> > and for non-hugepages we just use
> >
> > $memoryBackingDir (== /var/lib/libvirt/ram)
> >
> > In both cases, we then just let QEMU create a random tempfile
> > name within these directories.
> >
>
> Correct.
>
> > So, IIUC, your proposal here is just making the non-hugepages
> > case work the same way the hugepages case works, except that
> > it is adding an extra level of $alias in the directory path.
>
> Almost. $alias is actual file, not dir.
Ah, ok, I couldn't see that from the diff :-)
> > I don't think this extra level of nesting is really desirable,
> > or needed. We should just give QEMU a filename, so that it
> > doesn't create a random tempfile. This is achieved by simply
> > doing a
> >
> > mkdir($memoryBackingDir/libvirt/qemu/$shortName)
> >
> > but then giving a
> >
> > mem-path=$memoryBackingDir/libvirt/qemu/$shortName/$alias
>
> Yes. That's exactly what I'm doing. A snippet from next patch where we
> can see this in action:
>
>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> index 5700c3413..352819429 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> @@ -13 +13,1 @@ QEMU_AUDIO_DRV=none \
> --object
> memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
> +-object
> memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
>
>
> And even though you're not disputing '/libvirt/qemu/' part in the path,
> I'll just point out for future reference that memoryBackingDir can be
> just any path in the system. It's just that the default is
> /var/lib/libvirt/qemu which makes the generated path ugly (and indeed
> renders the part redundant).
Yeah, that makes sense, even if it feels wierd. Better to be consistent
than to try to special case this. That said, it would be reasonable to
consider /var/lib/libvirt/ram as default mem path, given we include
the driver name.
> > QEMU will try to open that, and will get ENOENT, at which
> > point it to O_CREAT that file with the exact name we've
> > given, instead of using a tempfile name.
> >
> >
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >> src/qemu/libvirtd_qemu.aug | 1 +
> >> src/qemu/qemu.conf | 5 ++
> >> src/qemu/qemu_command.c | 9 +--
> >> src/qemu/qemu_conf.c | 76 +++++++++++++++++++-
> >> src/qemu/qemu_conf.h | 10 ++-
> >> src/qemu/qemu_driver.c | 19 +++++
> >> src/qemu/qemu_hotplug.c | 2 +-
> >> src/qemu/qemu_process.c | 141 ++++++++++++++++++++++++++-----------
> >> src/qemu/qemu_process.h | 8 +--
> >> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> >> 10 files changed, 220 insertions(+), 52 deletions(-)
> >
> > I'd expect to see current tests updated wrt the new naming scheme
> > for paths
>
> See next patch.
Ok, when you get rid of the config option, I guess you'll need to merge
that into this patch to avoid temporary regression in tests.
> > NB, this creates potentially many levels in the dir hiearchy
> >
> > $memoryBackingDir/libvirt
> > $memoryBackingDir/libvirt/qemu/
> > $memoryBackingDir/libvirt/qemu/$shortName
>
> Only these ^^ are dirs.
>
> > $memoryBackingDir/libvirt/qemu/$shortName/$alias
>
> This is actual file.
Ok
> >> + if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> >> + def, path) < 0) {
> >> + virReportError(VIR_ERR_INTERNAL_ERROR,
> >> + _("Unable to label %s"), path);
> >> + return -1;
> >> + }
> >> + } else {
> >> + if (rmdir(path) < 0 &&
> >> + errno != ENOENT)
> >> + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
> >> + path, errno);
> >
> > This only deletes
> >
> > $memoryBackingDir/libvirt/qemu/$shortName/$alias
> >
> > so we're leaving around
> >
> > $memoryBackingDir/libvirt/qemu/$shortName
>
> Is that so? I'm fairly certain it removes
> $memoryBackingDir/libvirt/qemu/$shortName
If you say so, I'll believe you, since I can't really follow the diff
too well.
One other thought though - when QEMU uses a named file, rather than an
autogenerated tempfile, I don't see where QEMU calls unlink() on the file.
Normally for tempfiles it calls unlink() immediately, but for named files
it doesn't, and I don't see where it does it during shutdown. Even if it
did though, I think libvirt needs to explicitly unlink() each named file
to be safe in case QEMU crashes. Maybe your patchseries already does this,
but if not....
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.