From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 48288C77B7F for ; Wed, 3 May 2023 11:41:52 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1puAr2-0007tL-M3; Wed, 03 May 2023 07:41:04 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1puAr0-0007rp-E6 for qemu-devel@nongnu.org; Wed, 03 May 2023 07:41:02 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1puAqy-00037o-4y for qemu-devel@nongnu.org; Wed, 03 May 2023 07:41:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683114058; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=dRwOSZS087B+j9NDbbckQmSe0NEnNmqFyABRgslLrL0=; b=BDOUwj80VvLKG2y/6Pu6MEr4gj8NPzdB3n8cFFLU214EGPqpiAyeSB6wTXx/lh6y2aGKgY +qYO4vxyza/Zu+h7pbRDHRlq4KLq9lxKlFF51/zDw78zFJCKpCaTj8lg3O1p2kktM+kkhW +FQ2A3r9ixJSkVu1Zvu99ocO/EtFx1E= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-638-0-m7RWIiMhaNdaGkO17WiQ-1; Wed, 03 May 2023 07:40:55 -0400 X-MC-Unique: 0-m7RWIiMhaNdaGkO17WiQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6BEB7A0F386; Wed, 3 May 2023 11:40:54 +0000 (UTC) Received: from redhat.com (dhcp-192-205.str.redhat.com [10.33.192.205]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 132F1405DBBC; Wed, 3 May 2023 11:40:49 +0000 (UTC) Date: Wed, 3 May 2023 13:40:49 +0200 From: Kevin Wolf To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, Daniel =?iso-8859-1?Q?P=2E_Berrang=E9?= , Juan Quintela , Julia Suvorova , xen-devel@lists.xenproject.org, eesposit@redhat.com, Richard Henderson , Fam Zheng , "Michael S. Tsirkin" , Coiby Xu , David Woodhouse , Marcel Apfelbaum , Peter Lieven , Paul Durrant , "Richard W.M. Jones" , qemu-block@nongnu.org, Stefano Garzarella , Anthony Perard , Stefan Weil , Xie Yongji , Paolo Bonzini , Aarushi Mehta , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= , Eduardo Habkost , Stefano Stabellini , Hanna Reitz , Ronnie Sahlberg , Zhengui Li , Daniil Tatianin Subject: Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug Message-ID: References: <20230425172716.1033562-1-stefanha@redhat.com> <20230425172716.1033562-5-stefanha@redhat.com> <20230501150934.GA14869@fedora> <20230502200243.GD535070@fedora> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Sh4VljrAVjcetdM0" Content-Disposition: inline In-Reply-To: <20230502200243.GD535070@fedora> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 Received-SPF: pass client-ip=170.10.133.124; envelope-from=kwolf@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -22 X-Spam_score: -2.3 X-Spam_bar: -- X-Spam_report: (-2.3 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.161, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --Sh4VljrAVjcetdM0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 02.05.2023 um 22:02 hat Stefan Hajnoczi geschrieben: > On Tue, May 02, 2023 at 03:19:52PM +0200, Kevin Wolf wrote: > > Am 01.05.2023 um 17:09 hat Stefan Hajnoczi geschrieben: > > > On Fri, Apr 28, 2023 at 04:22:55PM +0200, Kevin Wolf wrote: > > > > Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben: > > > > > This patch is part of an effort to remove the aio_disable_externa= l() > > > > > API because it does not fit in a multi-queue block layer world wh= ere > > > > > many AioContexts may be submitting requests to the same disk. > > > > >=20 > > > > > The SCSI emulation code is already in good shape to stop using > > > > > aio_disable_external(). It was only used by commit 9c5aad84da1c > > > > > ("virtio-scsi: fixed virtio_scsi_ctx_check failed when detaching = scsi > > > > > disk") to ensure that virtio_scsi_hotunplug() works while the gue= st > > > > > driver is submitting I/O. > > > > >=20 > > > > > Ensure virtio_scsi_hotunplug() is safe as follows: > > > > >=20 > > > > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() -> > > > > > device_set_realized() calls qatomic_set(&dev->realized, false)= so > > > > > that future scsi_device_get() calls return NULL because they e= xclude > > > > > SCSIDevices with realized=3Dfalse. > > > > >=20 > > > > > That means virtio-scsi will reject new I/O requests to this > > > > > SCSIDevice with VIRTIO_SCSI_S_BAD_TARGET even while > > > > > virtio_scsi_hotunplug() is still executing. We are protected a= gainst > > > > > new requests! > > > > >=20 > > > > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize= () so > > > > > that in-flight requests are cancelled synchronously. This ensu= res > > > > > that no in-flight requests remain once qdev_simple_device_unpl= ug_cb() > > > > > returns. > > > > >=20 > > > > > Thanks to these two conditions we don't need aio_disable_external= () > > > > > anymore. > > > > >=20 > > > > > Cc: Zhengui Li > > > > > Reviewed-by: Paolo Bonzini > > > > > Reviewed-by: Daniil Tatianin > > > > > Signed-off-by: Stefan Hajnoczi > > > >=20 > > > > qemu-iotests 040 starts failing for me after this patch, with what = looks > > > > like a use-after-free error of some kind. > > > >=20 > > > > (gdb) bt > > > > #0 0x000055b6e3e1f31c in job_type (job=3D0xe3e3e3e3e3e3e3e3) at ..= /job.c:238 > > > > #1 0x000055b6e3e1cee5 in is_block_job (job=3D0xe3e3e3e3e3e3e3e3) a= t ../blockjob.c:41 > > > > #2 0x000055b6e3e1ce7d in block_job_next_locked (bjob=3D0x55b6e72b7= 570) at ../blockjob.c:54 > > > > #3 0x000055b6e3df6370 in blockdev_mark_auto_del (blk=3D0x55b6e74af= 0a0) at ../blockdev.c:157 > > > > #4 0x000055b6e393e23b in scsi_qdev_unrealize (qdev=3D0x55b6e7c04d4= 0) at ../hw/scsi/scsi-bus.c:303 > > > > #5 0x000055b6e3db0d0e in device_set_realized (obj=3D0x55b6e7c04d40= , value=3Dfalse, errp=3D0x55b6e497c918 ) at ../hw/core/qdev.c:= 599 > > > > #6 0x000055b6e3dba36e in property_set_bool (obj=3D0x55b6e7c04d40, = v=3D0x55b6e7d7f290, name=3D0x55b6e41bd6d8 "realized", opaque=3D0x55b6e7246d= 20, errp=3D0x55b6e497c918 ) > > > > at ../qom/object.c:2285 > > > > #7 0x000055b6e3db7e65 in object_property_set (obj=3D0x55b6e7c04d40= , name=3D0x55b6e41bd6d8 "realized", v=3D0x55b6e7d7f290, errp=3D0x55b6e497c9= 18 ) at ../qom/object.c:1420 > > > > #8 0x000055b6e3dbd84a in object_property_set_qobject (obj=3D0x55b6= e7c04d40, name=3D0x55b6e41bd6d8 "realized", value=3D0x55b6e74c1890, errp=3D= 0x55b6e497c918 ) > > > > at ../qom/qom-qobject.c:28 > > > > #9 0x000055b6e3db8570 in object_property_set_bool (obj=3D0x55b6e7c= 04d40, name=3D0x55b6e41bd6d8 "realized", value=3Dfalse, errp=3D0x55b6e497c9= 18 ) at ../qom/object.c:1489 > > > > #10 0x000055b6e3daf2b5 in qdev_unrealize (dev=3D0x55b6e7c04d40) at = =2E./hw/core/qdev.c:306 > > > > #11 0x000055b6e3db509d in qdev_simple_device_unplug_cb (hotplug_dev= =3D0x55b6e81c3630, dev=3D0x55b6e7c04d40, errp=3D0x7ffec5519200) at ../hw/co= re/qdev-hotplug.c:72 > > > > #12 0x000055b6e3c520f9 in virtio_scsi_hotunplug (hotplug_dev=3D0x55= b6e81c3630, dev=3D0x55b6e7c04d40, errp=3D0x7ffec5519200) at ../hw/scsi/virt= io-scsi.c:1065 > > > > #13 0x000055b6e3db4dec in hotplug_handler_unplug (plug_handler=3D0x= 55b6e81c3630, plugged_dev=3D0x55b6e7c04d40, errp=3D0x7ffec5519200) at ../hw= /core/hotplug.c:56 > > > > #14 0x000055b6e3a28f84 in qdev_unplug (dev=3D0x55b6e7c04d40, errp= =3D0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935 > > > > #15 0x000055b6e3a290fa in qmp_device_del (id=3D0x55b6e74c1760 "scsi= 0", errp=3D0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955 > > > > #16 0x000055b6e3fb0a5f in qmp_marshal_device_del (args=3D0x7f61cc00= 5eb0, ret=3D0x7f61d5a8ae38, errp=3D0x7f61d5a8ae40) at qapi/qapi-commands-qd= ev.c:114 > > > > #17 0x000055b6e3fd52e1 in do_qmp_dispatch_bh (opaque=3D0x7f61d5a8ae= 08) at ../qapi/qmp-dispatch.c:128 > > > > #18 0x000055b6e4007b9e in aio_bh_call (bh=3D0x55b6e7dea730) at ../u= til/async.c:155 > > > > #19 0x000055b6e4007d2e in aio_bh_poll (ctx=3D0x55b6e72447c0) at ../= util/async.c:184 > > > > #20 0x000055b6e3fe3b45 in aio_dispatch (ctx=3D0x55b6e72447c0) at ..= /util/aio-posix.c:421 > > > > #21 0x000055b6e4009544 in aio_ctx_dispatch (source=3D0x55b6e72447c0= , callback=3D0x0, user_data=3D0x0) at ../util/async.c:326 > > > > #22 0x00007f61ddc14c7f in g_main_dispatch (context=3D0x55b6e7244b20= ) at ../glib/gmain.c:3454 > > > > #23 g_main_context_dispatch (context=3D0x55b6e7244b20) at ../glib/g= main.c:4172 > > > > #24 0x000055b6e400a7e8 in glib_pollfds_poll () at ../util/main-loop= =2Ec:290 > > > > #25 0x000055b6e400a0c2 in os_host_main_loop_wait (timeout=3D0) at .= =2E/util/main-loop.c:313 > > > > #26 0x000055b6e4009fa2 in main_loop_wait (nonblocking=3D0) at ../ut= il/main-loop.c:592 > > > > #27 0x000055b6e3a3047b in qemu_main_loop () at ../softmmu/runstate.= c:731 > > > > #28 0x000055b6e3dab27d in qemu_default_main () at ../softmmu/main.c= :37 > > > > #29 0x000055b6e3dab2b8 in main (argc=3D24, argv=3D0x7ffec55196a8) a= t ../softmmu/main.c:48 > > > > (gdb) p jobs > > > > $4 =3D {lh_first =3D 0x0} > > >=20 > > > I wasn't able to reproduce this with gcc 13.1.1 or clang 16.0.1: > > >=20 > > > $ tests/qemu-iotests/check -qcow2 040 > > >=20 > > > Any suggestions on how to reproduce the issue? > >=20 > > It happens consistently for me with the same command line, both with gcc > > and clang. > >=20 > > gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4) > > clang version 15.0.7 (Fedora 15.0.7-2.fc37) > >=20 > > Maybe there is a semantic merge conflict? I have applied the series on > > top of master (05d50ba2d4) and my block branch (88f81f7bc8). >=20 > I can't find 88f81f7bc8 but rebased on repo.or.cz/qemu/kevin.git block > (4514dac7f2e9) and the test passes here. >=20 > I rebased on qemu.git/master (05d50ba2d4) and it also passes. >=20 > Please let me know if the following tree (a0ff680a72f6) works on your > machine: > https://gitlab.com/stefanha/qemu/-/tree/remove-aio_disable_external Fails in the same way. So I tried to debug this myself now. The problem is that iterating the jobs in blockdev_mark_auto_del() is incorrect: job_cancel_locked() frees the job and then block_job_next_locked() is a use after free. It also drops job_mutex temporarily and polls, so even switching to a *_FOREACH_SAFE style loop won't fix this. I guess we have to restart the whole search from the start after a job_cancel_locked() because the list might look very different after the call. Now, of course, how this is related to your patch and why it doesn't trigger before it, is still less than clear. What I found out is that adding the scsi_device_purge_requests() is enough to crash it. Maybe it's related to the blk_drain() inside of it. That the job finishes earlier during the unplug now or something like that. Anyway, changing blockdev_mark_auto_del() fixes it. I'll send a patch. Kevin --Sh4VljrAVjcetdM0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEE3D3rFZqa+V09dFb+fwmycsiPL9YFAmRSSEAACgkQfwmycsiP L9Z4jRAAvTT64p9bQdNSCCIFrqkkGEZS5J/8ud4wvKBgYacJ8pOY30Z2B3dA78bz ik5KMZlaTa4GXFwlKGNn/OUGiB3ivuhXeIUHXv+pw8F9sCQy4WJDpeO6WPDtQ6Os swWWC3Uv+cnfLQH8dZjf4yoLY4hYACt683Ptml960LIYhpX/AXjdnOv6f/tj3gXO q+mxfLwTi5S08Kkq08e8sXbqCwSiTPXX106MDdk/oMD3MxzUmqu6qFxrf9n6Yp5i kI1rJD0VR64ScA58lJlFnAdZGWL7d9kX5WUbZ+x27lLay4Uel4cmY9AIsUWThnHZ fMk2Sol1aUKSEAlVdHL5vWbQF/UyYe+KB2tn++4HJZ/ojVBL1GNdnWgmk+ZPXGfI BtIc5+h7Qsa2uXUX2gJQYP7j6Y/EBQMAT0oTlG2v+CxS+1MrSa4O9ufnx1c9hoLF FOUU2CT4vrJlYI281yWZ5R6pZrm0ZJMHAGStCJH4/ayo1xVj/5gPdffIRksFwUzy yfia4Pis1nU9ET5riOk4WTCHz44BgIPvkDSYe1aJMS9XT3DM6MsZm9shuUMueLBd 0H5rU7ScgGB6ZLJLOQN0m84AfHOrxSgo7+bdAmXriAYvgDwr9B6fc50cDUBOEGNo zDb0PBH7R3fZT9rEpHlQi4t3SOBkDV3L1toGvWHYymLPkzOC3/w= =9639 -----END PGP SIGNATURE----- --Sh4VljrAVjcetdM0--