[Qemu-devel] [PATCH v2 0/3] virtio-scsi unplug of active device

[Qemu-devel] [PATCH v2 0/3] virtio-scsi unplug of active device

[Eric Farman]

In working with hot-plug/unplug of virtio-scsi devices on s390,
we have occasionally noticed some erratic behavior when an unplug
occurs while I/O is in flight.  Ideally a device is not being used
when it is removed from a guest configuration, but no guarantee
can be made that this will be the case.  And while this scenario
is meant for I/O that occurs during normal use of a device, it
includes the pathological case of an unplug that occurs while the
asynchronous Inquiry loop (initiated by a hotplug) is still ongoing.

Symptoms vary depending on when the unplug is recognized.  Sometimes
a hang occurs, because a reference is not properly released and thus
never reaches zero.  Sometimes a reference is released too early,
allowing the count to go negative and trip an assertion (or more
unpredictable results, if storage is released but still used).

Of course there are many times when things work perfectly, though
that seems to be when the I/O was able to complete in time.  These
patches simply straighten out the completion of I/Os during an
unplug, such that it results in predictable behavior whenever the
device is not idle.

Eric Farman (3):
  scsi: Assign cancel_io vector for scsi disk
  scsi/virtio-scsi: Cleanup of I/Os that never started
  scsi/virtio-scsi: Prevent assertion on missed events

 hw/scsi/scsi-disk.c   |    1 +
 hw/scsi/virtio-scsi.c |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 1/3] scsi: Assign cancel_io vector for scsi disk

[Eric Farman]

Provide the cancel_io vector for disk devices, to ensure that
all asynchronous I/Os are properly cleaned up of their references.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 hw/scsi/scsi-disk.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index bce617c..ee1f5eb 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2306,6 +2306,7 @@ static const SCSIReqOps scsi_disk_emulate_reqops = {
     .send_command = scsi_disk_emulate_command,
     .read_data    = scsi_disk_emulate_read_data,
     .write_data   = scsi_disk_emulate_write_data,
+    .cancel_io    = scsi_cancel_io,
     .get_buf      = scsi_get_buf,
 };
 
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 2/3] scsi/virtio-scsi: Cleanup of I/Os that never started

[Eric Farman]

There is still a small window that occurs when a cancel I/O affects
an asynchronous I/O operation that hasn't started.  In other words,
when the residual data length equals the expected data length.

Today, the routine virtio_scsi_command_complete fails because the
VirtIOSCSIReq pointer (from the hba_private field in SCSIRequest)
was cleared earlier when virtio_scsi_complete_req was called by
the virtio_scsi_request_cancelled routine.  As a result, the
virtio_scsi_command_complete routine needs to simply return when
it is processing a SCSIRequest block that was marked canceled.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 hw/scsi/virtio-scsi.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 6dcdd1b..1da98cd 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -306,6 +306,10 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     VirtIOSCSIReq *req = r->hba_private;
     uint32_t sense_len;
 
+    if (r->io_canceled) {
+        return;
+    }
+
     req->resp.cmd->response = VIRTIO_SCSI_S_OK;
     req->resp.cmd->status = status;
     if (req->resp.cmd->status == GOOD) {
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 3/3] scsi/virtio-scsi: Prevent assertion on missed events

[Eric Farman]

In some cases, an unplug can cause events to be dropped, which
leads to an assertion failure when preparing to notify the guest
kernel.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 hw/scsi/virtio-scsi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 1da98cd..6610b3a 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -520,7 +520,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
     evt->event = event;
     evt->reason = reason;
     if (!dev) {
-        assert(event == VIRTIO_SCSI_T_NO_EVENT);
+        assert(event == VIRTIO_SCSI_T_EVENTS_MISSED);
     } else {
         evt->lun[0] = 1;
         evt->lun[1] = dev->id;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 0/3] virtio-scsi unplug of active device

[Paolo Bonzini]

Il 14/01/2014 20:16, Eric Farman ha scritto:
> In working with hot-plug/unplug of virtio-scsi devices on s390,
> we have occasionally noticed some erratic behavior when an unplug
> occurs while I/O is in flight.  Ideally a device is not being used
> when it is removed from a guest configuration, but no guarantee
> can be made that this will be the case.  And while this scenario
> is meant for I/O that occurs during normal use of a device, it
> includes the pathological case of an unplug that occurs while the
> asynchronous Inquiry loop (initiated by a hotplug) is still ongoing.
> 
> Symptoms vary depending on when the unplug is recognized.  Sometimes
> a hang occurs, because a reference is not properly released and thus
> never reaches zero.  Sometimes a reference is released too early,
> allowing the count to go negative and trip an assertion (or more
> unpredictable results, if storage is released but still used).
> 
> Of course there are many times when things work perfectly, though
> that seems to be when the I/O was able to complete in time.  These
> patches simply straighten out the completion of I/Os during an
> unplug, such that it results in predictable behavior whenever the
> device is not idle.
> 
> Eric Farman (3):
>   scsi: Assign cancel_io vector for scsi disk
>   scsi/virtio-scsi: Cleanup of I/Os that never started
>   scsi/virtio-scsi: Prevent assertion on missed events
> 
>  hw/scsi/scsi-disk.c   |    1 +
>  hw/scsi/virtio-scsi.c |    6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 

Thanks, applied to scsi-next branch.

Paolo