From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuiMx-0001CC-4v for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:52:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuiMt-0004hw-Ud for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:51:59 -0500 Received: from relay.parallels.com ([195.214.232.42]:49842) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuiMt-0004hm-II for qemu-devel@nongnu.org; Fri, 06 Nov 2015 09:51:55 -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: <563CBE7E.5030207@openvz.org> Date: Fri, 6 Nov 2015 17:51:42 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; 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: Fam Zheng , qemu-devel , Stefan Hajnoczi On 11/06/2015 05:44 PM, Stefan Hajnoczi wrote: > On Fri, Nov 6, 2015 at 2: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 stack trace is fine since hw/scsi/virtio-scsi.c:virtio_scsi_hotplug(): > aio_context_acquire(s->ctx); > blk_set_aio_context(sd->conf.blk, s->ctx); > aio_context_release(s->ctx); > > What bug does this patch fix? I still don't see the problem. Program received signal SIGABRT, Aborted. 0x00007ffff3e8c267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 55 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt #0 0x00007ffff3e8c267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 #1 0x00007ffff3e8deca in __GI_abort () at abort.c:89 #2 0x00007ffff3e8503d in __assert_fail_base ( fmt=0x7ffff3fe7028 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x555555aab968 "aio_context_is_owner(ctx)", file=file@entry=0x555555aab94a "aio-posix.c", line=line@entry=244, function=function@entry=0x555555aab9b0 <__PRETTY_FUNCTION__.21740> "aio_poll") at assert.c:92 #3 0x00007ffff3e850f2 in __GI___assert_fail ( assertion=0x555555aab968 "aio_context_is_owner(ctx)", file=0x555555aab94a "aio-posix.c", line=244, function=0x555555aab9b0 <__PRETTY_FUNCTION__.21740> "aio_poll") at assert.c:101 #4 0x0000555555966a53 in aio_poll (ctx=0x5555563fc470, blocking=false) at aio-posix.c:244 #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 (gdb) frame 8 #8 0x00005555556a4c52 in virtio_scsi_hotplug (hotplug_dev=0x5555579f22b0, dev=0x555557bc8470, errp=0x7fffffffd9e0) at /home/den/src/qemu/hw/scsi/virtio-scsi.c:773 warning: Source file is more recent than executable. 773 blk_set_aio_context(sd->conf.blk, s->ctx); (gdb) list 768 if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { 769 return; 770 } 771 blk_op_block_all(sd->conf.blk, s->blocker); 772 aio_context_acquire(s->ctx); 773 blk_set_aio_context(sd->conf.blk, s->ctx); 774 aio_context_release(s->ctx); 775 } 776 777 if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { (gdb) p s->ctx $1 = (AioContext *) 0x55555640c100 (gdb) p blk_get_aio_context(sd->conf.blk) [Thread 0x7fffecc10700 (LWP 480) exited] $2 = (AioContext *) 0x5555563fc470 (gdb)