Re: [Qemu-devel] [PATCH 1/3] block: add missed aio_context_acquire around bdrv_set_aio_context

["Denis V. Lunev" <den@openvz.org>]

On 11/06/2015 04:35 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:27:22PM +0300, Denis V. Lunev wrote:
>> It is required for bdrv_drain.
> What bug does this patch fix?
>
> Existing blk_set_aio_context() callers acquire the AioContext or are
> sure it's already acquired by their caller, so I don't see where the bug
> is.
>
> No function in block/block-backend.c acquires AioContext internally.
> The same reasoning I mentioned in another email thread about consistent
> locking strategy applies here.  Either all blk_*() functions should take
> the AioContext internally (which I disagree with) or none of them should
> take it.

#5  0x00005555559b31bf in bdrv_drain (bs=0x5555564274a0) at block/io.c:258
#6  0x0000555555963321 in bdrv_set_aio_context (bs=0x5555564274a0,
     new_context=0x55555640c100) at block.c:3764
#7  0x00005555559a8b67 in blk_set_aio_context (blk=0x555556425d20,
     new_context=0x55555640c100) at block/block-backend.c:1068
#8  0x00005555556a4c52 in virtio_scsi_hotplug (hotplug_dev=0x5555579f22b0,
     dev=0x555557bc8470, errp=0x7fffffffd9e0)
     at /home/den/src/qemu/hw/scsi/virtio-scsi.c:773
#9  0x00005555557eb3e2 in hotplug_handler_plug 
(plug_handler=0x5555579f22b0,
     plugged_dev=0x555557bc8470, errp=0x7fffffffd9e0) at 
hw/core/hotplug.c:22
#10 0x00005555557e7794 in device_set_realized (obj=0x555557bc8470, 
value=true,
     errp=0x7fffffffdb90) at hw/core/qdev.c:1066
#11 0x000055555595580b in property_set_bool (obj=0x555557bc8470,
     v=0x555557b51bc0, opaque=0x555557bc8740, name=0x555555a43841 
"realized",
     errp=0x7fffffffdb90) at qom/object.c:1708
#12 0x0000555555953e2a in object_property_set (obj=0x555557bc8470,
     v=0x555557b51bc0, name=0x555555a43841 "realized", errp=0x7fffffffdb90)
     at qom/object.c:965
#13 0x00005555559566c7 in object_property_set_qobject (obj=0x555557bc8470,
     value=0x555557bc8370, name=0x555555a43841 "realized", 
errp=0x7fffffffdb90)
---Type <return> to continue, or q <return> to quit---
     at qom/qom-qobject.c:24
#14 0x00005555559540cd in object_property_set_bool (obj=0x555557bc8470,
     value=true, name=0x555555a43841 "realized", errp=0x7fffffffdb90)
     at qom/object.c:1034
#15 0x0000555555760e47 in qdev_device_add (opts=0x5555563f90f0,
     errp=0x7fffffffdc18) at qdev-monitor.c:589
#16 0x0000555555772501 in device_init_func (opaque=0x0, 
opts=0x5555563f90f0,
     errp=0x0) at vl.c:2305
#17 0x0000555555a198cb in qemu_opts_foreach (
     list=0x555555e96a60 <qemu_device_opts>,
     func=0x5555557724c3 <device_init_func>, opaque=0x0, errp=0x0)
     at util/qemu-option.c:1114
#18 0x0000555555777b20 in main (argc=83, argv=0x7fffffffe058,
     envp=0x7fffffffe2f8) at vl.c:4523

You want the lock to be taken by the function. It would be
quite natural to add assert(aio_context_is_owner(ctx)) in that
case.

This assert will fail if the lock is not taken even if the thread
is not started yet. With assert that lock is taken qemu
will crash here and in bdrv_close called through
bdrv_delete. In that case caller can re-acquire the lock.
We can take the lock in the main thread immediately when
iothread was stopped.

Den