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

[Stefan Hajnoczi]

On Fri, Nov 6, 2015 at 2:19 PM, Denis V. Lunev <address@hidden> wrote:
> On 11/06/2015 05:09 PM, Denis V. Lunev wrote:
>>
>> 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
>
> it seems that virt_block and virt-scsi take locks in
> a wrong order.. Something like this should be
> applied.
>
> There is something similar in virt-block code.

/* Context: QEMU global mutex held */
void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
{

>     blk_set_aio_context(s->conf->conf.blk, s->ctx);

The requirement is that the blk_set_aio_context() caller holds the
current AioContext for blk.  In this case the function is documented
as holding the global mutex and we're moving the BDS from the main
loop AioContext to an IOThread AioContext.  We currently hold the
global mutex so it's safe to call blk_set_aio_context().

This seems ok to me.