Re: [PATCH v1 3/5] qemu.conf: Introduce memory_predictable_file_names

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux