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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
