[libvirt] [PATCHv4 0/3] Add support for file memorybacking

[Michal Privoznik]

On 01/30/2017 09:07 AM, Safka, JaroslavX wrote:
> Hi, 
> first , thanks much for review.
> Comments inside
> 
>> -----Original Message-----
>> From: Michal Privoznik [mailto:mprivozn at redhat.com]
>> Sent: Saturday, January 28, 2017 3:03 PM
>> To: Safka, JaroslavX <jaroslavx.safka at intel.com>; libvir-list at redhat.com
>> Cc: Mooney, Sean K <sean.k.mooney at intel.com>; Ptacek, MichalX
>> <michalx.ptacek at intel.com>; Daniel P. Berrange <berrange at redhat.com>
>> Subject: Re: [libvirt] [PATCHv4 0/3] Add support for file memorybacking
>>
>> On 13.12.2016 13:12, Jaroslav Safka wrote:
>>> Hi,
>>> we would like to introduce 3 new elements source,access and allocation in
>> memoryBacking element.
>>> For now it was made for numa topology.
>>>
>>> <memoryBacking>
>>>      <source type="file|anonymous"/>
>>>      <access mode="shared|private"/>
>>>      <allocation mode="immediate|ondemand"/> </memoryBacking>
>>>
>>> If allocation is immediate then -mem-prealloc should be added to the qemu
>> commanline.
>>> If source is file then
>>> -object memory-backend-file,id=mem,size=1024M,mem-path=*lib dir path*
>>> -numa node,memdev=mem Will be added to the qemu commandline
>>>
>>> If access is shared then the "share=on" parameter will be added to the
>> memory-backend-file e.g.
>>> -object
>>> memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,s
>>> hare=on
>>>
>>> The access mode can be overriden by specifying token memAccess in numa
>> cell.
>>>
>>> The test cpu-numa-memshared was removed, because behaviour was
>> changed
>>> and is not needed anymore
>>
>> I beg to disagree. What if you don't have any <memoryBacking/>?
> [Jarek] disagree with removing the test or with xml change?

XML change is okay. But I don't think that behaviour is changed (rather
than extended). I mean, prior to these patches the following
configuration errors out:

<cpu>
  <numa>
    <cell id='0' cpus='0-3' memory='4194304' unit='KiB'
          memAccess='shared'/>
  </numa>
</cpu>

With your patches, the error is no longer observed. Wait, is this
expected? If so, than you need to remove that check in
qemuBuildMemoryBackendStr in 3/3 which I told you not to do and also:

diff --git i/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c
index f09c5460f..feb50f7a2 100644
--- i/tests/qemuxml2argvtest.c
+++ w/tests/qemuxml2argvtest.c
@@ -1516,7 +1516,7 @@ mymain(void)
     DO_TEST_PARSE_ERROR("cpu-numa3", NONE);
     DO_TEST_FAILURE("cpu-numa-disjoint", NONE);
     DO_TEST("cpu-numa-disjoint", QEMU_CAPS_NUMA);
-    DO_TEST_FAILURE("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_RAM);
+    DO_TEST("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_RAM);
     DO_TEST_FAILURE("cpu-numa-memshared", NONE);
     DO_TEST("cpu-host-model", NONE);
     DO_TEST("cpu-host-model-vendor", NONE);

> 
> 
>>
>> I like these patches, but I'm not certainly sure about:
>> a) domain XML (in the past we used to require an ACK on schema change from
>> one of the Dans)
>> b) the location for qemu to create its mmaped files (patch 3/3).
>> cfg->libDir looks very suspicious.
>>
> [Jarek] is there any link or keywords for find to these previous discussions?
> I'm not able to find it :(

What discussion do you have in mind? The a) rule was not a written kind
of rule.

> 
> 
>> I have the patches applied locally with all the changes I've pointed out applied.
>> If we have answers to both of the questions, I can push these.

This is still true. So no need to rework the patches on your side (you
can view this as my attempt to say sorry for such late review).

Michal