From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56829) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuhs9-00028B-NJ for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:20:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zuhs5-000330-Q4 for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:20:09 -0500 Received: from relay.parallels.com ([195.214.232.42]:45463) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuhs5-000326-Ib for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:20:05 -0500 References: <1446658044-22042-1-git-send-email-den@openvz.org> <1446658044-22042-2-git-send-email-den@openvz.org> <20151106133545.GK12285@stefanha-x1.localdomain> <563CB48D.1010301@openvz.org> From: "Denis V. Lunev" Message-ID: <563CB709.4060907@openvz.org> Date: Fri, 6 Nov 2015 17:19:53 +0300 MIME-Version: 1.0 In-Reply-To: <563CB48D.1010301@openvz.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] block: add missed aio_context_acquire around bdrv_set_aio_context List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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 to continue, or q 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 , > func=0x5555557724c3 , 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