Re: [libvirt] [PATCH v2 3/4] qemu: Use predictable file names for memory-backend-file

On 10/24/2017 07:41 AM, 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 generate predictable paths. In this case: > > $memoryBackingDir/libvirt/qemu/$shortName/$alias > > where $shortName is result of virDomainObjGetShortName(). s/Obj/Def > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 9 +- > src/qemu/qemu_conf.c | 55 ++++++++- > src/qemu/qemu_conf.h | 9 +- > src/qemu/qemu_driver.c | 17 +++ > src/qemu/qemu_hotplug.c | 2 +- > src/qemu/qemu_process.c | 137 +++++++++++++++------ > src/qemu/qemu_process.h | 8 +- > .../qemuxml2argv-cpu-numa-memshared.args | 6 +- > .../qemuxml2argv-fd-memory-numa-topology.args | 3 +- > .../qemuxml2argv-fd-memory-numa-topology2.args | 6 +- > .../qemuxml2argv-fd-memory-numa-topology3.args | 9 +- > .../qemuxml2argv-hugepages-memaccess2.args | 9 +- > 12 files changed, 207 insertions(+), 63 deletions(-) > There's parts of this that would seemingly be able to be separated out so that it's easier to review... > 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; > + This is just a simple trivial move and not related... > *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..22c023255 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def, > } > > > +int > +qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg, > + char **path) > +{ > + return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir); Not of a fan of redundancy, but I'm also not sure what better way there is... For privileged domains it's "/var/lib/libvirt/qemu/ram/libvirt/qemu/" and unpriv'd it's $XDG_CONFIG_HOME+"/qemu/ram/libvirt/qemu". The former repeats "libvirt/qemu" while the latter only repeats "qemu". I would think we could drop the "qemu" at the very least, but it doesn't matter that much to me.

Random thought - do memory file paths even work for non priv'd domains?

> +} > + > + > +int > +qemuGetMemoryBackingDomainPath(const virDomainDef *def, > + virQEMUDriverConfigPtr cfg, > + char **path) > +{ > + char *shortName = NULL; > + char *base = NULL; > + int ret = -1; > + > + if (!(shortName = virDomainDefGetShortName(def)) || > + qemuGetMemoryBackingBasePath(cfg, &base) < 0 || > + virAsprintf(path, "%s/%s", base, shortName) < 0) > + goto cleanup; > + As I noted in review of patch 1... This continues the logic of creating paths that start with "-", which to me is, well, not right. Maybe others aren't offended by it. Is there anyway we can prepend something here to "shortName" - I'd say "ram", but that's redundant with the path, but that to me is at least better than a directory name starting with "-".

> + 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 +1793,27 @@ 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 (!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; > + > + ret = 0; > + cleanup: > + VIR_FREE(domainPath); > + return ret; > } > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index 9d6866816..a553e30e2 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -364,6 +364,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 135c20749..b22fb7518 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,21 @@ qemuStateInitialize(bool privileged, > VIR_FREE(hugepagePath); > } > > + 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; > + > if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew())) > goto error; > > @@ -945,6 +961,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..4fe973c59 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3324,60 +3324,117 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def, > } > > > +static bool > +qemuProcessNeedMemoryBackingPath(virDomainDefPtr def, > + virDomainMemoryDefPtr mem) > +{ > + size_t i; > + size_t numaNodes; > + > + 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; > +} > + > + Starting here... > +static int > +qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver, > + virDomainDefPtr def, > + const char *path, > + bool build) Could just be qemuProcessBuildDestroyPath as it's one of many possible calls within (at least) the hugepage loop. > +{ > + 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 (virFileDeleteTree(path) < 0 && > + errno != ENOENT) > + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)", This error message would need to change to remove "hugepage" > + path, errno); > + } > + > + return 0; > +} > + > + ... this seems separable too, modulo the change to use virFileDeleteTree which then would be a follow-up patch to be accepted on its own merits... The other option being a check to add some sort of isDir type logic or flag is input considering that hugepages wouldn't need to delete the path, right? Or should they? As Dan points out in v1 - we do need to keep the $dir/libvirt/qemu though for other domains and we wouldn't want those subdirectories to get deleted from/for Hugepages before we do so for memory backing, right? I think the way you have this we'd delete those before getting to them in the logic, but my eyes are really tired right now. > int > -qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver, > - virDomainObjPtr vm, > - virDomainMemoryDefPtr mem, > - bool build) > +qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainMemoryDefPtr mem, > + bool build) You could seemingly make the API name change separately too, IDC, but some do generally speaking of course... > { > 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, 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 +5607,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 +6311,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, > goto endjob; > } > > - qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false); > + qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false); > > vm->def->id = -1; > > @@ -7112,7 +7169,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/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 > @@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \ > -M pc \ > -m 214 \ > -smp 16,sockets=2,cores=4,threads=2 \ > --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 this is just where it really obvious what's happening - although it took me a bit to figure it all out, especially in my post KVM Forum travel haze.