Changeset
dom.xml                 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
mem.xml                 |   6 +++
src/qemu/qemu_hotplug.c |   7 +++
3 files changed, 123 insertions(+)
create mode 100644 dom.xml
create mode 100644 mem.xml
Git apply log
Switched to a new branch 'cover.1520962521.git.pkrempa@redhat.com'
Applying: REPRODUCER -- DO NOT PUSH
Applying: qemu: hotplug: Clean up memory backing files after failed memory hotplug
To https://github.com/patchew-project/libvirt
 * [new tag]         patchew/cover.1520962521.git.pkrempa@redhat.com -> patchew/cover.1520962521.git.pkrempa@redhat.com
Test failed: syntax-check

loading

[libvirt] [PATCH 0/2] qemu: Remove memory backing files on failed memory hotplug
Posted by Peter Krempa, 14 weeks ago
see 2/2

Peter Krempa (2):
  REPRODUCER -- DO NOT PUSH
  qemu: hotplug: Clean up memory backing files after failed memory
    hotplug

 dom.xml                 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
 mem.xml                 |   6 +++
 src/qemu/qemu_hotplug.c |   7 +++
 3 files changed, 123 insertions(+)
 create mode 100644 dom.xml
 create mode 100644 mem.xml

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] REPRODUCER -- DO NOT PUSH
Posted by Peter Krempa, 14 weeks ago
https://bugzilla.redhat.com/show_bug.cgi?id=1553085

This breaks hotplug so that we can see whether the file was left
dangling.

Reproduce by:

virsh create dom.xml
virsh attach-device memplug mem.xml
ls /var/lib/libvirt/qemu/ram/libvirt/qemu/*-memplug

After the failed hotplug, qemu should leave behind the file called
'dimm0' in the above path.

The original bugreport was caused by failing in the 'device-add' phase.
---
 dom.xml                 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
 mem.xml                 |   6 +++
 src/qemu/qemu_hotplug.c |   3 ++
 3 files changed, 119 insertions(+)
 create mode 100644 dom.xml
 create mode 100644 mem.xml

diff --git a/dom.xml b/dom.xml
new file mode 100644
index 0000000000..242615569e
--- /dev/null
+++ b/dom.xml
@@ -0,0 +1,110 @@
+<domain type='kvm'>
+  <name>memplug</name>
+  <uuid>41277c77-9bb0-416c-aacc-4dcac4e03cea</uuid>
+  <maxMemory slots='16' unit='KiB'>152428800</maxMemory>
+  <memory unit='KiB'>1024000</memory>
+  <currentMemory unit='KiB'>1024000</currentMemory>
+  <memoryBacking>
+    <source type='file'/>
+    <access mode='shared'/>
+    <allocation mode='immediate'/>
+  </memoryBacking>
+  <vcpu placement='static' current='2'>8</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-i440fx-2.9'>hvm</type>
+    <bootmenu enable='yes'/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <vmport state='off'/>
+  </features>
+  <cpu>
+    <numa>
+      <cell id='0' cpus='0,2,4,6' memory='512000' unit='KiB'/>
+      <cell id='1' cpus='1,3,5,7' memory='512000' unit='KiB'/>
+    </numa>
+  </cpu>
+  <clock offset='utc'>
+    <timer name='rtc' tickpolicy='catchup'/>
+    <timer name='pit' tickpolicy='delay'/>
+    <timer name='hpet' present='no'/>
+  </clock>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <pm>
+    <suspend-to-mem enabled='no'/>
+    <suspend-to-disk enabled='no'/>
+  </pm>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0' model='ich9-ehci1'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x7'/>
+    </controller>
+    <controller type='usb' index='0' model='ich9-uhci1'>
+      <master startport='0'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' multifunction='on'/>
+    </controller>
+    <controller type='usb' index='0' model='ich9-uhci2'>
+      <master startport='2'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x1'/>
+    </controller>
+    <controller type='usb' index='0' model='ich9-uhci3'>
+      <master startport='4'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='virtio-serial' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
+    </controller>
+    <controller type='ide' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+    </controller>
+    <controller type='scsi' index='0' model='lsilogic'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
+    </controller>
+    <serial type='pty'>
+      <target type='isa-serial' port='0'>
+        <model name='isa-serial'/>
+      </target>
+    </serial>
+    <console type='pty'>
+      <target type='serial' port='0'/>
+    </console>
+    <channel type='unix'>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='1'/>
+    </channel>
+    <channel type='spicevmc'>
+      <target type='virtio' name='com.redhat.spice.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='2'/>
+    </channel>
+    <input type='tablet' bus='usb'>
+      <address type='usb' bus='0' port='1'/>
+    </input>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <graphics type='spice' autoport='yes'>
+      <listen type='address'/>
+      <image compression='off'/>
+    </graphics>
+    <sound model='ich6'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+    </sound>
+    <video>
+      <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' primary='yes'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </video>
+    <redirdev bus='usb' type='spicevmc'>
+      <address type='usb' bus='0' port='2'/>
+    </redirdev>
+    <redirdev bus='usb' type='spicevmc'>
+      <address type='usb' bus='0' port='3'/>
+    </redirdev>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
+    </memballoon>
+  </devices>
+</domain>
+
diff --git a/mem.xml b/mem.xml
new file mode 100644
index 0000000000..d536786b8b
--- /dev/null
+++ b/mem.xml
@@ -0,0 +1,6 @@
+<memory model='dimm' access='private'>
+    <target>
+      <size unit='MiB'>128</size>
+      <node>0</node>
+    </target>
+  </memory>
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e0a5300f08..cff40625f8 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2162,6 +2162,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
         goto exit_monitor;
     objAdded = true;

+    /* !!! REPRODUCER !!! */
+    goto exit_monitor;
+
     if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
         goto exit_monitor;

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] REPRODUCER -- DO NOT PUSH
Posted by Michal Privoznik, 14 weeks ago
On 03/13/2018 06:36 PM, Peter Krempa wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1553085
> 
> This breaks hotplug so that we can see whether the file was left
> dangling.
> 
> Reproduce by:
> 
> virsh create dom.xml
> virsh attach-device memplug mem.xml
> ls /var/lib/libvirt/qemu/ram/libvirt/qemu/*-memplug
> 
> After the failed hotplug, qemu should leave behind the file called
> 'dimm0' in the above path.
> 
> The original bugreport was caused by failing in the 'device-add' phase.
> ---
>  dom.xml                 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
>  mem.xml                 |   6 +++
>  src/qemu/qemu_hotplug.c |   3 ++
>  3 files changed, 119 insertions(+)
>  create mode 100644 dom.xml
>  create mode 100644 mem.xml
> 
> diff --git a/dom.xml b/dom.xml
> new file mode 100644
> index 0000000000..242615569e
> --- /dev/null
> +++ b/dom.xml
> @@ -0,0 +1,110 @@
> +<domain type='kvm'>
> +  <name>memplug</name>
> +  <uuid>41277c77-9bb0-416c-aacc-4dcac4e03cea</uuid>
> +  <maxMemory slots='16' unit='KiB'>152428800</maxMemory>
> +  <memory unit='KiB'>1024000</memory>
> +  <currentMemory unit='KiB'>1024000</currentMemory>
> +  <memoryBacking>
> +    <source type='file'/>
> +    <access mode='shared'/>
> +    <allocation mode='immediate'/>
> +  </memoryBacking>
> +  <vcpu placement='static' current='2'>8</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc-i440fx-2.9'>hvm</type>
> +    <bootmenu enable='yes'/>
> +  </os>
> +  <features>
> +    <acpi/>
> +    <apic/>
> +    <vmport state='off'/>
> +  </features>
> +  <cpu>
> +    <numa>
> +      <cell id='0' cpus='0,2,4,6' memory='512000' unit='KiB'/>
> +      <cell id='1' cpus='1,3,5,7' memory='512000' unit='KiB'/>
> +    </numa>
> +  </cpu>
> +  <clock offset='utc'>
> +    <timer name='rtc' tickpolicy='catchup'/>
> +    <timer name='pit' tickpolicy='delay'/>
> +    <timer name='hpet' present='no'/>
> +  </clock>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>restart</on_crash>
> +  <pm>
> +    <suspend-to-mem enabled='no'/>
> +    <suspend-to-disk enabled='no'/>
> +  </pm>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-x86_64</emulator>
> +    <controller type='usb' index='0' model='ich9-ehci1'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x7'/>
> +    </controller>
> +    <controller type='usb' index='0' model='ich9-uhci1'>
> +      <master startport='0'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' multifunction='on'/>
> +    </controller>
> +    <controller type='usb' index='0' model='ich9-uhci2'>
> +      <master startport='2'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x1'/>
> +    </controller>
> +    <controller type='usb' index='0' model='ich9-uhci3'>
> +      <master startport='4'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x2'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <controller type='virtio-serial' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
> +    </controller>
> +    <controller type='ide' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> +    </controller>
> +    <controller type='scsi' index='0' model='lsilogic'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
> +    </controller>
> +    <serial type='pty'>
> +      <target type='isa-serial' port='0'>
> +        <model name='isa-serial'/>
> +      </target>
> +    </serial>
> +    <console type='pty'>
> +      <target type='serial' port='0'/>
> +    </console>
> +    <channel type='unix'>
> +      <target type='virtio' name='org.qemu.guest_agent.0'/>
> +      <address type='virtio-serial' controller='0' bus='0' port='1'/>
> +    </channel>
> +    <channel type='spicevmc'>
> +      <target type='virtio' name='com.redhat.spice.0'/>
> +      <address type='virtio-serial' controller='0' bus='0' port='2'/>
> +    </channel>
> +    <input type='tablet' bus='usb'>
> +      <address type='usb' bus='0' port='1'/>
> +    </input>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <graphics type='spice' autoport='yes'>
> +      <listen type='address'/>
> +      <image compression='off'/>
> +    </graphics>
> +    <sound model='ich6'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> +    </sound>
> +    <video>
> +      <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' primary='yes'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> +    </video>
> +    <redirdev bus='usb' type='spicevmc'>
> +      <address type='usb' bus='0' port='2'/>
> +    </redirdev>
> +    <redirdev bus='usb' type='spicevmc'>
> +      <address type='usb' bus='0' port='3'/>
> +    </redirdev>
> +    <memballoon model='virtio'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
> +    </memballoon>
> +  </devices>
> +</domain>
> +

Empty line at EOF :-P

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] REPRODUCER -- DO NOT PUSH
Posted by Ján Tomko, 14 weeks ago
On Tue, Mar 13, 2018 at 06:36:32PM +0100, Peter Krempa wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1553085
>
>This breaks hotplug so that we can see whether the file was left
>dangling.
>

NACK, this breaks hotplug.

>Reproduce by:
>
>virsh create dom.xml
>virsh attach-device memplug mem.xml
>ls /var/lib/libvirt/qemu/ram/libvirt/qemu/*-memplug
>
>After the failed hotplug, qemu should leave behind the file called
>'dimm0' in the above path.
>
>The original bugreport was caused by failing in the 'device-add' phase.
>---
> dom.xml                 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
> mem.xml                 |   6 +++
> src/qemu/qemu_hotplug.c |   3 ++
> 3 files changed, 119 insertions(+)
> create mode 100644 dom.xml
> create mode 100644 mem.xml
>

Also, 'make syntax-check' does not pass:
dom.xml
maint.mk: empty line(s) or no newline at EOF
make: *** [maint.mk:930: sc_prohibit_empty_lines_at_EOF] Error 1
make: *** Waiting for unfinished jobs....

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: hotplug: Clean up memory backing files after failed memory hotplug
Posted by Peter Krempa, 14 weeks ago
Libvirt provides full path to the backing file since commit
fec8f9c49afb479f6. This made qemu create the backend object but did not
delete it. This was fixed for unplug case in 4d83a6722f but not in case
of failure to hotplug the frontend. We'd leave the files behind which
would make memory unusable in case of hugepages.

https://bugzilla.redhat.com/show_bug.cgi?id=1553085

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_hotplug.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index cff40625f8..70325a1246 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2219,6 +2219,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
     if (!mem)
         goto audit;

+    /* we need to remove the memory backing file so that it does not hog memory */
+    if (objAdded)
+        ignore_value(qemuProcessDestroyMemoryBackingPath(driver, vm, mem));
+
  removedef:
     if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
         mem = virDomainMemoryRemove(vm->def, id);
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: hotplug: Clean up memory backing files after failed memory hotplug
Posted by Ján Tomko, 14 weeks ago
On Tue, Mar 13, 2018 at 06:36:33PM +0100, Peter Krempa wrote:
>Libvirt provides full path to the backing file since commit
>fec8f9c49afb479f6. This made qemu create the backend object but did not
>delete it. This was fixed for unplug case in 4d83a6722f but not in case
>of failure to hotplug the frontend. We'd leave the files behind which
>would make memory unusable in case of hugepages.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=1553085
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_hotplug.c | 4 ++++
> 1 file changed, 4 insertions(+)
>

ACK

>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index cff40625f8..70325a1246 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -2219,6 +2219,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>     if (!mem)
>         goto audit;
>
>+    /* we need to remove the memory backing file so that it does not hog memory */

This comment does not really help with understanding what's happening.
I suggest deleting it since readers will have to read the code anyway.

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: hotplug: Clean up memory backing files after failed memory hotplug
Posted by Michal Privoznik, 14 weeks ago
On 03/13/2018 06:36 PM, Peter Krempa wrote:
> Libvirt provides full path to the backing file since commit
> fec8f9c49afb479f6. This made qemu create the backend object but did not
> delete it. This was fixed for unplug case in 4d83a6722f but not in case
> of failure to hotplug the frontend. We'd leave the files behind which
> would make memory unusable in case of hugepages.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1553085
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list