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

[Denis V. Lunev]

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.

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

    /* Kick right away to begin processing requests already in vring */
    event_notifier_set(virtio_queue_get_host_notifier(vq));

    /* Get this show started by hooking up our callbacks */
    aio_context_acquire(s->ctx);
    aio_set_event_notifier(s->ctx, &s->host_notifier, true,
                           handle_notify);
    aio_context_release(s->ctx);
    return;

Den