12:10 p.m.
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:
>
Correct.
$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.
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(a)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