| Message ID | 20230504195327.695107-6-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: remove aio_disable_external() API | expand |
Am 04.05.2023 um 21:53 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(&dev->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. scsi_device_unrealize() already contains a call to I think you mean scsi_qdev_unrealize(). Can be fixed while applying. > scsi_device_purge_requests() 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 <lizhengui@huawei.com> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Kevin
On Tue, May 09, 2023 at 08:55:14PM +0200, Kevin Wolf wrote: > Am 04.05.2023 um 21:53 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(&dev->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. scsi_device_unrealize() already contains a call to > > I think you mean scsi_qdev_unrealize(). Can be fixed while applying. Yes, it should be scsi_qdev_unrealize(). I'll review your other comments and fix this if I need to respin. Stefan
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index ae314af3de..c1a7ea9ae2 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -1091,7 +1091,6 @@ 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(); VirtIOSCSIEventInfo info = { .event = VIRTIO_SCSI_T_TRANSPORT_RESET, .reason = VIRTIO_SCSI_EVT_RESET_REMOVED, @@ -1101,9 +1100,7 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, }, }; - aio_disable_external(ctx); qdev_simple_device_unplug_cb(hotplug_dev, dev, errp); - aio_enable_external(ctx); if (s->ctx) { virtio_scsi_acquire(s);
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(&dev->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. scsi_device_unrealize() already contains a call to scsi_device_purge_requests() 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 <lizhengui@huawei.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/scsi/virtio-scsi.c | 3 --- 1 file changed, 3 deletions(-)