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