Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
On Wed, May 03, 2023 at 01:40:49PM +0200, Kevin Wolf wrote:
> 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_external()
> > > > > > API because it does not fit in a multi-queue block layer world where
> > > > > > many AioContexts may be submitting requests to the same disk.
> > > > > >
> > > > > > 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 guest
> > > > > > driver is submitting I/O.
> > > > > >
> > > > > > Ensure virtio_scsi_hotunplug() is safe as follows:
> > > > > >
> > > > > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> > > > > >device_set_realized() calls qatomic_set(>realized, false) so
> > > > > >that future scsi_device_get() calls return NULL because they
> > > > > > exclude
> > > > > >SCSIDevices with realized=false.
> > > > > >
> > > > > >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
> > > > > > against
> > > > > >new requests!
> > > > > >
> > > > > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize()
> > > > > > so
> > > > > >that in-flight requests are cancelled synchronously. This ensures
> > > > > >that no in-flight requests remain once
> > > > > > qdev_simple_device_unplug_cb()
> > > > > >returns.
> > > > > >
> > > > > > Thanks to these two conditions we don't need aio_disable_external()
> > > > > > anymore.
> > > > > >
> > > > > > Cc: Zhengui Li
> > > > > > Reviewed-by: Paolo Bonzini
> > > > > > Reviewed-by: Daniil Tatianin
> > > > > > Signed-off-by: Stefan Hajnoczi
> > > > >
> > > > > qemu-iotests 040 starts failing for me after this patch, with what
> > > > > looks
> > > > > like a use-after-free error of some kind.
> > > > >
> > > > > (gdb) bt
> > > > > #0 0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at
> > > > > ../job.c:238
> > > > > #1 0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at
> > > > > ../blockjob.c:41
> > > > > #2 0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570)
> > > > > at ../blockjob.c:54
> > > > > #3 0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0)
> > > > > at ../blockdev.c:157
> > > > > #4 0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40)
> > > > > at ../hw/scsi/scsi-bus.c:303
> > > > > #5 0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40,
> > > > > value=false, errp=0x55b6e497c918 ) at
> > > > > ../hw/core/qdev.c:599
> > > > > #6 0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40,
> > > > > v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized",
> > > > > opaque=0x55b6e7246d20, errp=0x55b6e497c918 )
> > > > > at ../qom/object.c:2285
> > > > > #7 0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40,
> > > > > name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918
> > > > > ) at ../qom/object.c:1420
> > > > > #8 0x55b6e3dbd84a in object_property_set_qobject
> > > > > (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized",
> > > > > value=0x55b6e74c1890, errp=0x55b6e497c918 )
> > > > > at ../qom/qom-qobject.c:28
> > > > > #9 0x55b6e3db8570 in object_property_set_bool
> > > > > (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized", value=false,
> > > > > errp=0x55b6e497c918 ) at ../qom/object.c:1489
> > > > > #10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at
> > > > > ../hw/core/qdev.c:306
> > > > > #11 0x55b6e3db509d in qdev_simple_device_unplug_cb
> > > > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200)
> > > > > at ../hw/core/qdev-hotplug.c:72
> > > > > #12 0x55b6e3c520f9 in virtio_scsi_hotunplug
> > > > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200)
> > > > > at ../hw/scsi/virtio-scsi.c:1065
> > > > > #13 0x55b6e3db4dec in hotplug_handler_unplug
> > > > > (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40,
> > > > > errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> > > > > #14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40,
> > > > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> > > > > #15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0",
> > > > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> > > > > #16 0x55b6e3fb0a5f in qmp_marshal_device_del
> > > > >
Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
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_external()
> > > > > API because it does not fit in a multi-queue block layer world where
> > > > > many AioContexts may be submitting requests to the same disk.
> > > > >
> > > > > 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 guest
> > > > > driver is submitting I/O.
> > > > >
> > > > > Ensure virtio_scsi_hotunplug() is safe as follows:
> > > > >
> > > > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> > > > >device_set_realized() calls qatomic_set(>realized, false) so
> > > > >that future scsi_device_get() calls return NULL because they
> > > > > exclude
> > > > >SCSIDevices with realized=false.
> > > > >
> > > > >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
> > > > > against
> > > > >new requests!
> > > > >
> > > > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
> > > > >that in-flight requests are cancelled synchronously. This ensures
> > > > >that no in-flight requests remain once
> > > > > qdev_simple_device_unplug_cb()
> > > > >returns.
> > > > >
> > > > > Thanks to these two conditions we don't need aio_disable_external()
> > > > > anymore.
> > > > >
> > > > > Cc: Zhengui Li
> > > > > Reviewed-by: Paolo Bonzini
> > > > > Reviewed-by: Daniil Tatianin
> > > > > Signed-off-by: Stefan Hajnoczi
> > > >
> > > > qemu-iotests 040 starts failing for me after this patch, with what looks
> > > > like a use-after-free error of some kind.
> > > >
> > > > (gdb) bt
> > > > #0 0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at
> > > > ../job.c:238
> > > > #1 0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at
> > > > ../blockjob.c:41
> > > > #2 0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570)
> > > > at ../blockjob.c:54
> > > > #3 0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0)
> > > > at ../blockdev.c:157
> > > > #4 0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at
> > > > ../hw/scsi/scsi-bus.c:303
> > > > #5 0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40,
> > > > value=false, errp=0x55b6e497c918 ) at ../hw/core/qdev.c:599
> > > > #6 0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40,
> > > > v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized",
> > > > opaque=0x55b6e7246d20, errp=0x55b6e497c918 )
> > > > at ../qom/object.c:2285
> > > > #7 0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40,
> > > > name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918
> > > > ) at ../qom/object.c:1420
> > > > #8 0x55b6e3dbd84a in object_property_set_qobject
> > > > (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized",
> > > > value=0x55b6e74c1890, errp=0x55b6e497c918 )
> > > > at ../qom/qom-qobject.c:28
> > > > #9 0x55b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40,
> > > > name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918
> > > > ) at ../qom/object.c:1489
> > > > #10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at
> > > > ../hw/core/qdev.c:306
> > > > #11 0x55b6e3db509d in qdev_simple_device_unplug_cb
> > > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200)
> > > > at ../hw/core/qdev-hotplug.c:72
> > > > #12 0x55b6e3c520f9 in virtio_scsi_hotunplug
> > > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200)
> > > > at ../hw/scsi/virtio-scsi.c:1065
> > > > #13 0x55b6e3db4dec in hotplug_handler_unplug
> > > > (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40,
> > > > errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> > > > #14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40,
> > > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> > > > #15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0",
> > > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> > > > #16 0x55b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0,
> > > > ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at
> > > > qapi/qapi-commands-qdev.c:114
> > > > #17 0x55b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at
> > > > ../qapi/qmp-dispatch.c:128
> > > > #18 0x55b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at
> >
Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
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_external()
> > > > API because it does not fit in a multi-queue block layer world where
> > > > many AioContexts may be submitting requests to the same disk.
> > > >
> > > > 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 guest
> > > > driver is submitting I/O.
> > > >
> > > > Ensure virtio_scsi_hotunplug() is safe as follows:
> > > >
> > > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> > > >device_set_realized() calls qatomic_set(>realized, false) so
> > > >that future scsi_device_get() calls return NULL because they exclude
> > > >SCSIDevices with realized=false.
> > > >
> > > >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 against
> > > >new requests!
> > > >
> > > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
> > > >that in-flight requests are cancelled synchronously. This ensures
> > > >that no in-flight requests remain once qdev_simple_device_unplug_cb()
> > > >returns.
> > > >
> > > > Thanks to these two conditions we don't need aio_disable_external()
> > > > anymore.
> > > >
> > > > Cc: Zhengui Li
> > > > Reviewed-by: Paolo Bonzini
> > > > Reviewed-by: Daniil Tatianin
> > > > Signed-off-by: Stefan Hajnoczi
> > >
> > > qemu-iotests 040 starts failing for me after this patch, with what looks
> > > like a use-after-free error of some kind.
> > >
> > > (gdb) bt
> > > #0 0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at
> > > ../job.c:238
> > > #1 0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at
> > > ../blockjob.c:41
> > > #2 0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570) at
> > > ../blockjob.c:54
> > > #3 0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0) at
> > > ../blockdev.c:157
> > > #4 0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at
> > > ../hw/scsi/scsi-bus.c:303
> > > #5 0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40,
> > > value=false, errp=0x55b6e497c918 ) at ../hw/core/qdev.c:599
> > > #6 0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40,
> > > v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized", opaque=0x55b6e7246d20,
> > > errp=0x55b6e497c918 )
> > > at ../qom/object.c:2285
> > > #7 0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40,
> > > name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918
> > > ) at ../qom/object.c:1420
> > > #8 0x55b6e3dbd84a in object_property_set_qobject
> > > (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized",
> > > value=0x55b6e74c1890, errp=0x55b6e497c918 )
> > > at ../qom/qom-qobject.c:28
> > > #9 0x55b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40,
> > > name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918
> > > ) at ../qom/object.c:1489
> > > #10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at
> > > ../hw/core/qdev.c:306
> > > #11 0x55b6e3db509d in qdev_simple_device_unplug_cb
> > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at
> > > ../hw/core/qdev-hotplug.c:72
> > > #12 0x55b6e3c520f9 in virtio_scsi_hotunplug
> > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at
> > > ../hw/scsi/virtio-scsi.c:1065
> > > #13 0x55b6e3db4dec in hotplug_handler_unplug
> > > (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40,
> > > errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> > > #14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40,
> > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> > > #15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0",
> > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> > > #16 0x55b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0,
> > > ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at qapi/qapi-commands-qdev.c:114
> > > #17 0x55b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at
> > > ../qapi/qmp-dispatch.c:128
> > > #18 0x55b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at
> > > ../util/async.c:155
> > > #19 0x55b6e4007d2e in aio_bh_poll (ctx=0x55b6e72447c0) at
> > > ../util/async.c:184
> > > #20 0x55b6e3fe3b45 in aio_dispatch (ctx=0x55b6e72447c0) at
> > > ../util/aio-posix.c:421
> > > #21 0x55b6e4009544 in aio_ctx_dispatch
Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
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_external()
> > > API because it does not fit in a multi-queue block layer world where
> > > many AioContexts may be submitting requests to the same disk.
> > >
> > > 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 guest
> > > driver is submitting I/O.
> > >
> > > Ensure virtio_scsi_hotunplug() is safe as follows:
> > >
> > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> > >device_set_realized() calls qatomic_set(>realized, false) so
> > >that future scsi_device_get() calls return NULL because they exclude
> > >SCSIDevices with realized=false.
> > >
> > >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 against
> > >new requests!
> > >
> > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
> > >that in-flight requests are cancelled synchronously. This ensures
> > >that no in-flight requests remain once qdev_simple_device_unplug_cb()
> > >returns.
> > >
> > > Thanks to these two conditions we don't need aio_disable_external()
> > > anymore.
> > >
> > > Cc: Zhengui Li
> > > Reviewed-by: Paolo Bonzini
> > > Reviewed-by: Daniil Tatianin
> > > Signed-off-by: Stefan Hajnoczi
> >
> > qemu-iotests 040 starts failing for me after this patch, with what looks
> > like a use-after-free error of some kind.
> >
> > (gdb) bt
> > #0 0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at ../job.c:238
> > #1 0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at
> > ../blockjob.c:41
> > #2 0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570) at
> > ../blockjob.c:54
> > #3 0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0) at
> > ../blockdev.c:157
> > #4 0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at
> > ../hw/scsi/scsi-bus.c:303
> > #5 0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40,
> > value=false, errp=0x55b6e497c918 ) at ../hw/core/qdev.c:599
> > #6 0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40,
> > v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized", opaque=0x55b6e7246d20,
> > errp=0x55b6e497c918 )
> > at ../qom/object.c:2285
> > #7 0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40,
> > name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918
> > ) at ../qom/object.c:1420
> > #8 0x55b6e3dbd84a in object_property_set_qobject (obj=0x55b6e7c04d40,
> > name=0x55b6e41bd6d8 "realized", value=0x55b6e74c1890, errp=0x55b6e497c918
> > )
> > at ../qom/qom-qobject.c:28
> > #9 0x55b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40,
> > name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918
> > ) at ../qom/object.c:1489
> > #10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at
> > ../hw/core/qdev.c:306
> > #11 0x55b6e3db509d in qdev_simple_device_unplug_cb
> > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at
> > ../hw/core/qdev-hotplug.c:72
> > #12 0x55b6e3c520f9 in virtio_scsi_hotunplug
> > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at
> > ../hw/scsi/virtio-scsi.c:1065
> > #13 0x55b6e3db4dec in hotplug_handler_unplug
> > (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40,
> > errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> > #14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40,
> > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> > #15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0",
> > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> > #16 0x55b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0,
> > ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at qapi/qapi-commands-qdev.c:114
> > #17 0x55b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at
> > ../qapi/qmp-dispatch.c:128
> > #18 0x55b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at
> > ../util/async.c:155
> > #19 0x55b6e4007d2e in aio_bh_poll (ctx=0x55b6e72447c0) at
> > ../util/async.c:184
> > #20 0x55b6e3fe3b45 in aio_dispatch (ctx=0x55b6e72447c0) at
> > ../util/aio-posix.c:421
> > #21 0x55b6e4009544 in aio_ctx_dispatch (source=0x55b6e72447c0,
> > callback=0x0, user_data=0x0) at ../util/async.c:326
> > #22 0x7f61ddc14c7f in g_main_dispatch (context=0x55b6e7244b20) at
> > ../glib/gmain.c:3454
> > #23 g_main_context_dispatch (context=0x55b6e7244b20) at ../glib/gmain.c:4172
Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
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_external()
> > API because it does not fit in a multi-queue block layer world where
> > many AioContexts may be submitting requests to the same disk.
> >
> > 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 guest
> > driver is submitting I/O.
> >
> > Ensure virtio_scsi_hotunplug() is safe as follows:
> >
> > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> >device_set_realized() calls qatomic_set(>realized, false) so
> >that future scsi_device_get() calls return NULL because they exclude
> >SCSIDevices with realized=false.
> >
> >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 against
> >new requests!
> >
> > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
> >that in-flight requests are cancelled synchronously. This ensures
> >that no in-flight requests remain once qdev_simple_device_unplug_cb()
> >returns.
> >
> > Thanks to these two conditions we don't need aio_disable_external()
> > anymore.
> >
> > Cc: Zhengui Li
> > Reviewed-by: Paolo Bonzini
> > Reviewed-by: Daniil Tatianin
> > Signed-off-by: Stefan Hajnoczi
>
> qemu-iotests 040 starts failing for me after this patch, with what looks
> like a use-after-free error of some kind.
>
> (gdb) bt
> #0 0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at ../job.c:238
> #1 0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at
> ../blockjob.c:41
> #2 0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570) at
> ../blockjob.c:54
> #3 0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0) at
> ../blockdev.c:157
> #4 0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at
> ../hw/scsi/scsi-bus.c:303
> #5 0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40,
> value=false, errp=0x55b6e497c918 ) at ../hw/core/qdev.c:599
> #6 0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40,
> v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized", opaque=0x55b6e7246d20,
> errp=0x55b6e497c918 )
> at ../qom/object.c:2285
> #7 0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40,
> name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918
> ) at ../qom/object.c:1420
> #8 0x55b6e3dbd84a in object_property_set_qobject (obj=0x55b6e7c04d40,
> name=0x55b6e41bd6d8 "realized", value=0x55b6e74c1890, errp=0x55b6e497c918
> )
> at ../qom/qom-qobject.c:28
> #9 0x55b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40,
> name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918
> ) at ../qom/object.c:1489
> #10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at
> ../hw/core/qdev.c:306
> #11 0x55b6e3db509d in qdev_simple_device_unplug_cb
> (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at
> ../hw/core/qdev-hotplug.c:72
> #12 0x55b6e3c520f9 in virtio_scsi_hotunplug (hotplug_dev=0x55b6e81c3630,
> dev=0x55b6e7c04d40, errp=0x7ffec5519200) at ../hw/scsi/virtio-scsi.c:1065
> #13 0x55b6e3db4dec in hotplug_handler_unplug
> (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40,
> errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> #14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40,
> errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> #15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0",
> errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> #16 0x55b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0,
> ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at qapi/qapi-commands-qdev.c:114
> #17 0x55b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at
> ../qapi/qmp-dispatch.c:128
> #18 0x55b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at
> ../util/async.c:155
> #19 0x55b6e4007d2e in aio_bh_poll (ctx=0x55b6e72447c0) at
> ../util/async.c:184
> #20 0x55b6e3fe3b45 in aio_dispatch (ctx=0x55b6e72447c0) at
> ../util/aio-posix.c:421
> #21 0x55b6e4009544 in aio_ctx_dispatch (source=0x55b6e72447c0,
> callback=0x0, user_data=0x0) at ../util/async.c:326
> #22 0x7f61ddc14c7f in g_main_dispatch (context=0x55b6e7244b20) at
> ../glib/gmain.c:3454
> #23 g_main_context_dispatch (context=0x55b6e7244b20) at ../glib/gmain.c:4172
> #24 0x55b6e400a7e8 in glib_pollfds_poll () at ../util/main-loop.c:290
> #25 0x55b6e400a0c2 in os_host_main_loop_wait (timeout=0) at
> ../util/main-loop.c:313
> #26 0x55b6e4009fa2 in main_loop_wait (nonblocking=0) at
>
Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> This patch is part of an effort to remove the aio_disable_external()
> API because it does not fit in a multi-queue block layer world where
> many AioContexts may be submitting requests to the same disk.
>
> 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 guest
> driver is submitting I/O.
>
> Ensure virtio_scsi_hotunplug() is safe as follows:
>
> 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
>device_set_realized() calls qatomic_set(>realized, false) so
>that future scsi_device_get() calls return NULL because they exclude
>SCSIDevices with realized=false.
>
>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 against
>new requests!
>
> 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
>that in-flight requests are cancelled synchronously. This ensures
>that no in-flight requests remain once qdev_simple_device_unplug_cb()
>returns.
>
> Thanks to these two conditions we don't need aio_disable_external()
> anymore.
>
> Cc: Zhengui Li
> Reviewed-by: Paolo Bonzini
> Reviewed-by: Daniil Tatianin
> Signed-off-by: Stefan Hajnoczi
qemu-iotests 040 starts failing for me after this patch, with what looks
like a use-after-free error of some kind.
(gdb) bt
#0 0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at ../job.c:238
#1 0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at
../blockjob.c:41
#2 0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570) at
../blockjob.c:54
#3 0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0) at
../blockdev.c:157
#4 0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at
../hw/scsi/scsi-bus.c:303
#5 0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40, value=false,
errp=0x55b6e497c918 ) at ../hw/core/qdev.c:599
#6 0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40,
v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized", opaque=0x55b6e7246d20,
errp=0x55b6e497c918 )
at ../qom/object.c:2285
#7 0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40,
name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918
) at ../qom/object.c:1420
#8 0x55b6e3dbd84a in object_property_set_qobject (obj=0x55b6e7c04d40,
name=0x55b6e41bd6d8 "realized", value=0x55b6e74c1890, errp=0x55b6e497c918
)
at ../qom/qom-qobject.c:28
#9 0x55b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40,
name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918 )
at ../qom/object.c:1489
#10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at
../hw/core/qdev.c:306
#11 0x55b6e3db509d in qdev_simple_device_unplug_cb
(hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at
../hw/core/qdev-hotplug.c:72
#12 0x55b6e3c520f9 in virtio_scsi_hotunplug (hotplug_dev=0x55b6e81c3630,
dev=0x55b6e7c04d40, errp=0x7ffec5519200) at ../hw/scsi/virtio-scsi.c:1065
#13 0x55b6e3db4dec in hotplug_handler_unplug (plug_handler=0x55b6e81c3630,
plugged_dev=0x55b6e7c04d40, errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
#14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40, errp=0x7ffec55192e0)
at ../softmmu/qdev-monitor.c:935
#15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0",
errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
#16 0x55b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0,
ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at qapi/qapi-commands-qdev.c:114
#17 0x55b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at
../qapi/qmp-dispatch.c:128
#18 0x55b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at ../util/async.c:155
#19 0x55b6e4007d2e in aio_bh_poll (ctx=0x55b6e72447c0) at
../util/async.c:184
#20 0x55b6e3fe3b45 in aio_dispatch (ctx=0x55b6e72447c0) at
../util/aio-posix.c:421
#21 0x55b6e4009544 in aio_ctx_dispatch (source=0x55b6e72447c0,
callback=0x0, user_data=0x0) at ../util/async.c:326
#22 0x7f61ddc14c7f in g_main_dispatch (context=0x55b6e7244b20) at
../glib/gmain.c:3454
#23 g_main_context_dispatch (context=0x55b6e7244b20) at ../glib/gmain.c:4172
#24 0x55b6e400a7e8 in glib_pollfds_poll () at ../util/main-loop.c:290
#25 0x55b6e400a0c2 in os_host_main_loop_wait (timeout=0) at
../util/main-loop.c:313
#26 0x55b6e4009fa2 in main_loop_wait (nonblocking=0) at
../util/main-loop.c:592
#27 0x55b6e3a3047b in qemu_main_loop () at ../softmmu/runstate.c:731
#28 0x55b6e3dab27d in qemu_default_main () at ../softmmu/main.c:37
#29 0x55b6e3dab2b8 in main (argc=24, argv=0x7ffec55196a8) at
../softmmu/main.c:48
(gdb) p jobs
$4 =
[PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug
This patch is part of an effort to remove the aio_disable_external()
API because it does not fit in a multi-queue block layer world where
many AioContexts may be submitting requests to the same disk.
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 guest
driver is submitting I/O.
Ensure virtio_scsi_hotunplug() is safe as follows:
1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
device_set_realized() calls qatomic_set(>realized, false) so
that future scsi_device_get() calls return NULL because they exclude
SCSIDevices with realized=false.
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 against
new requests!
2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
that in-flight requests are cancelled synchronously. This ensures
that no in-flight requests remain once qdev_simple_device_unplug_cb()
returns.
Thanks to these two conditions we don't need aio_disable_external()
anymore.
Cc: Zhengui Li
Reviewed-by: Paolo Bonzini
Reviewed-by: Daniil Tatianin
Signed-off-by: Stefan Hajnoczi
---
hw/scsi/scsi-disk.c | 1 +
hw/scsi/virtio-scsi.c | 3 ---
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 97c9b1c8cd..e01bd84541 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2522,6 +2522,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
static void scsi_unrealize(SCSIDevice *dev)
{
+scsi_device_purge_requests(dev, SENSE_CODE(RESET));
del_boot_device_lchs(>qdev, NULL);
}
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 000961446c..a02f9233ec 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -1061,11 +1061,8 @@ static void virtio_scsi_hotunplug(HotplugHandler
*hotplug_dev, DeviceState *dev,
VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
VirtIOSCSI *s = VIRTIO_SCSI(vdev);
SCSIDevice *sd = SCSI_DEVICE(dev);
-AioContext *ctx = s->ctx ?: qemu_get_aio_context();
-aio_disable_external(ctx);
qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
-aio_enable_external(ctx);
if (s->ctx) {
virtio_scsi_acquire(s);
--
2.39.2
