On Mon, Nov 18, 2013 at 03:35:20PM +0100, Andreas Färber wrote:
> Am 18.11.2013 13:29, schrieb Amos Kong:
> > On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas Färber wrote:
> >> From: Stefan Hajnoczi <stefa...@redhat.com>
> >>
> >> qdev_device_add() leaks the created device upon failure. I suspect this
> >> problem crept in because qdev_free() unparents the device but does not
> >> drop a reference - confusing name.
> >>
> >> Cc: qemu-sta...@nongnu.org
> >> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> >> Signed-off-by: Andreas Färber <afaer...@suse.de>
> >
> > Hi Stefan,
> >
> > This commit caused a regression bug:
> >
> > hotplug more than 32 disks to vm, qemu crash
> >
> > ---
> >
> > [amos@amosk qemu]$ cat radd.sh
> > for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e
> > 1f;do
> > for j in `seq 1 7` 0;do
> > /bin/cp /images/none.qcow2 /tmp/resize$i$j.qcow2
> >
> > echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none
> > echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none | nc -U
> > /tmp/m
> >
> > echo device_add
> > virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on
> > echo device_add
> > virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on | nc
> > -U /tmp/m
> > done
> > done
>
> Hi, thanks for catching this.
>
> Is this only with virtio-blk-pci or with any PCI card or only when
> drives are involved?
I can reproduce by hotplugging virtio-net-pci NIC
for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
for j in `seq 1 7` 0;do
echo "netdev_add tap,id=dev$i$j" | nc -U /tmp/m
echo "device_add
virtio-net-pci,netdev=dev$i$j,id=nic$i$j,addr=0x$i.$j,multifunction=on" | nc -U
/tmp/m
done
done
> Either way it would be really great if you could
> add such tests to Stefan's qtest using QMP, so that it can easily be run
> by everyone.
I will do it.
> The stacktrace below is not really telling to me. I wonder if we forget
> to clean up some MemoryRegion in the device that still has a back
> reference or whether the Memory API still references MemoryRegions that
> have been destroyed by the device or forgets the reference devices it
> still needs... Paolo?
>
> I had reviewed the call paths and believe the patch to be 100% good, so
> the fault will very likely be elsewhere.
>
> Regards,
> Andreas
>
> >
> > ----
> >
> > #0 0x00005555558b7f95 in flatview_ref (view=0x0) at
> > /home/devel/qemu/memory.c:300
> > #1 0x00005555558b9689 in address_space_get_flatview (as=0x55555645d660) at
> > /home/devel/qemu/memory.c:656
> > #2 0x00005555558ba416 in address_space_update_topology (as=0x55555645d660)
> > at /home/devel/qemu/memory.c:760
> > #3 0x00005555558ba5cf in memory_region_transaction_commit () at
> > /home/devel/qemu/memory.c:799
> > #4 0x00005555558bcfcc in memory_region_set_enabled (mr=0x55555647af08,
> > enabled=false) at /home/devel/qemu/memory.c:1503
> > #5 0x000055555571a0af in do_pci_register_device (pci_dev=0x55555647ac10,
> > bus=0x5555564132b0, name=0x555556261100 "virtio-blk-pci", devfn=26) at
> > hw/pci/pci.c:846
> > #6 0x000055555571c6cc in pci_qdev_init (qdev=0x55555647ac10) at
> > hw/pci/pci.c:1751
> > #7 0x0000555555694d70 in device_realize (dev=0x55555647ac10,
> > err=0x7fffffffc8e8) at hw/core/qdev.c:178
> > #8 0x00005555556966fc in device_set_realized (obj=0x55555647ac10,
> > value=true, err=0x7fffffffca60) at hw/core/qdev.c:699
> > #9 0x00005555557e7b57 in property_set_bool (obj=0x55555647ac10,
> > v=0x55555679a830, opaque=0x555556461b10, name=0x5555559922ae "realized",
> > errp=0x7fffffffca60)
> > at qom/object.c:1315
> > #10 0x00005555557e665b in object_property_set (obj=0x55555647ac10,
> > v=0x55555679a830, name=0x5555559922ae "realized", errp=0x7fffffffca60) at
> > qom/object.c:803
> > #11 0x00005555557e816e in object_property_set_qobject (obj=0x55555647ac10,
> > value=0x555556678880, name=0x5555559922ae "realized", errp=0x7fffffffca60)
> > at qom/qom-qobject.c:24
> > #12 0x00005555557e6950 in object_property_set_bool (obj=0x55555647ac10,
> > value=true, name=0x5555559922ae "realized", errp=0x7fffffffca60) at
> > qom/object.c:866
> > #13 0x0000555555694ca7 in qdev_init (dev=0x55555647ac10) at
> > hw/core/qdev.c:163
> > #14 0x00005555557c60ee in qdev_device_add (opts=0x555556525370) at
> > qdev-monitor.c:543
> > #15 0x00005555557c6730 in do_device_add (mon=0x5555562fb760,
> > qdict=0x55555645d440, ret_data=0x7fffffffcb80) at qdev-monitor.c:656
> > #16 0x00005555558c8892 in handle_user_command (mon=0x5555562fb760,
> > cmdline=0x5555563f0f60 "device_add
> > virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on")
> > at /home/devel/qemu/monitor.c:4137
> > #17 0x00005555558ca10f in monitor_command_cb (mon=0x5555562fb760,
> > cmdline=0x5555563f0f60 "device_add
> > virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on",
> > opaque=0x0) at /home/devel/qemu/monitor.c:4757
> > #18 0x00005555557e9491 in readline_handle_byte (rs=0x5555563f0f60, ch=10)
> > at readline.c:373
> > #19 0x00005555558ca045 in monitor_read (opaque=0x5555562fb760,
> > buf=0x7fffffffccf0 "\n\315\377\377\377\177", size=1) at
> > /home/devel/qemu/monitor.c:4743
> > #20 0x00005555557c6cc8 in qemu_chr_be_write (s=0x555556269040,
> > buf=0x7fffffffccf0 "\n\315\377\377\377\177", len=1) at qemu-char.c:165
> > #21 0x00005555557cb026 in tcp_chr_read (chan=0x55555645fe40, cond=G_IO_IN,
> > opaque=0x555556269040) at qemu-char.c:2487
> > #22 0x00007ffff76ede06 in g_main_context_dispatch () from
> > /lib64/libglib-2.0.so.0
> > #23 0x000055555578ef33 in glib_pollfds_poll () at main-loop.c:189
> > #24 0x000055555578f028 in os_host_main_loop_wait (timeout=77312299) at
> > main-loop.c:234
> > #25 0x000055555578f100 in main_loop_wait (nonblocking=0) at main-loop.c:483
> > #26 0x000055555582e234 in main_loop () at vl.c:2014
> > #27 0x0000555555835697 in main (argc=14, argv=0x7fffffffe298,
> > envp=0x7fffffffe310) at vl.c:4362
> >
> >> ---
> >> qdev-monitor.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >> index b1ce26a..531b258 100644
> >> --- a/qdev-monitor.c
> >> +++ b/qdev-monitor.c
> >> @@ -518,6 +518,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >> }
> >> if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
> >> qdev_free(qdev);
> >> + object_unref(OBJECT(qdev));
> >> return NULL;
> >> }
> >> if (qdev->id) {
> >> @@ -531,6 +532,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >> g_free(name);
> >> }
> >> if (qdev_init(qdev) < 0) {
> >> + object_unref(OBJECT(qdev));
> >> qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> >> return NULL;
> >> }
> >> --
> >> 1.8.1.4
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Amos.
