From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuhhv-0003QL-IP for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:09:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zuhhq-00089a-Kd for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:09:35 -0500 Received: from relay.parallels.com ([195.214.232.42]:44374) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuhhq-000894-Cc for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:09:30 -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> From: "Denis V. Lunev" Message-ID: <563CB48D.1010301@openvz.org> Date: Fri, 6 Nov 2015 17:09:17 +0300 MIME-Version: 1.0 In-Reply-To: <20151106133545.GK12285@stefanha-x1.localdomain> 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 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