[Qemu-devel] [PATCH 0/3] virtio-scsi: Asynchronous cancellation

[Qemu-devel] [PATCH 0/3] virtio-scsi: Asynchronous cancellation

[Fam Zheng]

This series changes VIRTIO_SCSI_T_TMF_ABORT_TASK and
VIRTIO_SCSI_T_TMF_ABORT_TASK_SET emulation to asynchronous by making use of
bdrv_aio_cancel_async.

Before, when guest cancels a SCSI command, we use a nested poll loop to wait
until the request is cancelled or completed before returning. This blocks the
whole vm and makes the guest unresponsive if the backend block device takes
time to complete it, possibly because of slow IO, throttling, network issue,
etc..

Now we return to the guest to allow vcpus to run before completing the TMF, and
only after all the requests have been canceled, we notify the guest about the
completing of the TMF command.




Fam Zheng (3):
  scsi-bus: Unify request unref in scsi_req_cancel
  scsi: Introduce scsi_req_cancel_async
  virtio-scsi: Handle TMF request cancellation asynchronously

 hw/scsi/scsi-bus.c     | 78 +++++++++++++++++++++++++++++++++++++++-----------
 hw/scsi/scsi-disk.c    | 59 +++++++++++---------------------------
 hw/scsi/scsi-generic.c | 28 ++++--------------
 hw/scsi/virtio-scsi.c  | 45 ++++++++++++++++++++++++-----
 include/hw/scsi/scsi.h | 16 ++++++++++-
 5 files changed, 136 insertions(+), 90 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] scsi-bus: Unify request unref in scsi_req_cancel

[Fam Zheng]

Before, scsi_req_cancel will take ownership of the canceled request and
unref it. This is because we didn't know if AIO CB will be called or not
during the cancelling, so we set the io_canceled flag before calling it,
and skip to unref in the potentially called callbacks, which is not
very nice.

Now, bdrv_aio_cancel has a stricter contract that the completion
callback is always called, so we can remove the special case of
req->io_canceled.

It will also make implementing asynchronous cancellation easier.

Also, as the existing SCSIReqOps.cancel_io implementations are exactly
the same, we can move the code to bus for now as we are touching them in
this patch.

All AIO callbacks in devices, those will be called because of
bdrv_aio_cancel, should call scsi_req_canceled if req->io_canceled is
set. That's the uniform place we release the reference we grabbed in
scsi_req_cancel and notify buses.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-bus.c     | 30 ++++++++++++-------------
 hw/scsi/scsi-disk.c    | 59 ++++++++++++++------------------------------------
 hw/scsi/scsi-generic.c | 28 +++++-------------------
 include/hw/scsi/scsi.h |  2 +-
 4 files changed, 37 insertions(+), 82 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 954c607..74172cc 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1613,7 +1613,6 @@ void scsi_req_continue(SCSIRequest *req)
 {
     if (req->io_canceled) {
         trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
-        return;
     }
     trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
     if (req->cmd.mode == SCSI_XFER_TO_DEV) {
@@ -1631,7 +1630,6 @@ void scsi_req_data(SCSIRequest *req, int len)
     uint8_t *buf;
     if (req->io_canceled) {
         trace_scsi_req_data_canceled(req->dev->id, req->lun, req->tag, len);
-        return;
     }
     trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
     assert(req->cmd.mode != SCSI_XFER_NONE);
@@ -1716,35 +1714,37 @@ void scsi_req_complete(SCSIRequest *req, int status)
     scsi_req_unref(req);
 }
 
+/* Called by the devices when the request is canceled. */
+void scsi_req_canceled(SCSIRequest *req)
+{
+    assert(req->io_canceled);
+    if (req->bus->info->cancel) {
+        req->bus->info->cancel(req);
+    }
+    scsi_req_unref(req);
+}
+
 void scsi_req_cancel(SCSIRequest *req)
 {
     trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
-    if (!req->enqueued) {
+    if (req->io_canceled) {
         return;
     }
     scsi_req_ref(req);
     scsi_req_dequeue(req);
     req->io_canceled = true;
-    if (req->ops->cancel_io) {
-        req->ops->cancel_io(req);
+    if (req->aiocb) {
+        bdrv_aio_cancel(req->aiocb);
     }
-    if (req->bus->info->cancel) {
-        req->bus->info->cancel(req);
-    }
-    scsi_req_unref(req);
 }
 
 void scsi_req_abort(SCSIRequest *req, int status)
 {
-    if (!req->enqueued) {
+    if (req->io_canceled) {
         return;
     }
     scsi_req_ref(req);
-    scsi_req_dequeue(req);
-    req->io_canceled = true;
-    if (req->ops->cancel_io) {
-        req->ops->cancel_io(req);
-    }
+    scsi_req_cancel(req);
     scsi_req_complete(req, status);
     scsi_req_unref(req);
 }
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e34a544..56d7e6d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -105,23 +105,6 @@ static void scsi_check_condition(SCSIDiskReq *r, SCSISense sense)
     scsi_req_complete(&r->req, CHECK_CONDITION);
 }
 
-/* Cancel a pending data transfer.  */
-static void scsi_cancel_io(SCSIRequest *req)
-{
-    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
-
-    DPRINTF("Cancel tag=0x%x\n", req->tag);
-    if (r->req.aiocb) {
-        bdrv_aio_cancel(r->req.aiocb);
-
-        /* This reference was left in by scsi_*_data.  We take ownership of
-         * it the moment scsi_req_cancel is called, independent of whether
-         * bdrv_aio_cancel completes the request or not.  */
-        scsi_req_unref(&r->req);
-    }
-    r->req.aiocb = NULL;
-}
-
 static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t size)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
@@ -185,6 +168,7 @@ static void scsi_aio_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
     bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -197,9 +181,7 @@ static void scsi_aio_complete(void *opaque, int ret)
     scsi_req_complete(&r->req, GOOD);
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 static bool scsi_is_cmd_fua(SCSICommand *cmd)
@@ -233,6 +215,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
 
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -245,9 +228,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
     scsi_req_complete(&r->req, GOOD);
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 static void scsi_dma_complete_noio(void *opaque, int ret)
@@ -260,6 +241,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
         bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     }
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -279,9 +261,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
     }
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 static void scsi_dma_complete(void *opaque, int ret)
@@ -302,6 +282,7 @@ static void scsi_read_complete(void * opaque, int ret)
     r->req.aiocb = NULL;
     bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -319,9 +300,7 @@ static void scsi_read_complete(void * opaque, int ret)
     scsi_req_data(&r->req, r->qiov.size);
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 /* Actually issue a read to the block device.  */
@@ -336,6 +315,7 @@ static void scsi_do_read(void *opaque, int ret)
         bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     }
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -361,9 +341,7 @@ static void scsi_do_read(void *opaque, int ret)
     }
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 /* Read more data from scsi device into buffer.  */
@@ -456,6 +434,7 @@ static void scsi_write_complete(void * opaque, int ret)
         bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     }
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -478,9 +457,7 @@ static void scsi_write_complete(void * opaque, int ret)
     }
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 static void scsi_write_data(SCSIRequest *req)
@@ -1548,6 +1525,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
 
     r->req.aiocb = NULL;
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -1577,9 +1555,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
     scsi_req_complete(&r->req, GOOD);
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
     g_free(data);
 }
 
@@ -1649,6 +1625,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
     bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     if (r->req.io_canceled) {
+        scsi_req_canceled(&r->req);
         goto done;
     }
 
@@ -1672,9 +1649,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
     scsi_req_complete(&r->req, GOOD);
 
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
     qemu_vfree(data->iov.iov_base);
     g_free(data);
 }
@@ -2337,7 +2312,6 @@ 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,
 };
 
@@ -2347,7 +2321,6 @@ static const SCSIReqOps scsi_disk_dma_reqops = {
     .send_command = scsi_disk_dma_command,
     .read_data    = scsi_read_data,
     .write_data   = scsi_write_data,
-    .cancel_io    = scsi_cancel_io,
     .get_buf      = scsi_get_buf,
     .load_request = scsi_disk_load_request,
     .save_request = scsi_disk_save_request,
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 20587b4..5bde909 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -132,27 +132,12 @@ static void scsi_command_complete(void *opaque, int ret)
     DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
             r, r->req.tag, status);
 
-    scsi_req_complete(&r->req, status);
     if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
-}
-
-/* Cancel a pending data transfer.  */
-static void scsi_cancel_io(SCSIRequest *req)
-{
-    SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
-
-    DPRINTF("Cancel tag=0x%x\n", req->tag);
-    if (r->req.aiocb) {
-        bdrv_aio_cancel(r->req.aiocb);
-
-        /* This reference was left in by scsi_*_data.  We take ownership of
-         * it independent of whether bdrv_aio_cancel completes the request
-         * or not.  */
-        scsi_req_unref(&r->req);
+        scsi_req_complete(&r->req, status);
+    } else {
+        scsi_req_canceled(&r->req);
     }
-    r->req.aiocb = NULL;
+    scsi_req_unref(&r->req);
 }
 
 static int execute_command(BlockDriverState *bdrv,
@@ -211,9 +196,7 @@ static void scsi_read_complete(void * opaque, int ret)
         bdrv_set_guest_block_size(s->conf.bs, s->blocksize);
 
         scsi_req_data(&r->req, len);
-        if (!r->req.io_canceled) {
-            scsi_req_unref(&r->req);
-        }
+        scsi_req_unref(&r->req);
     }
 }
 
@@ -465,7 +448,6 @@ const SCSIReqOps scsi_generic_req_ops = {
     .send_command = scsi_send_command,
     .read_data    = scsi_read_data,
     .write_data   = scsi_write_data,
-    .cancel_io    = scsi_cancel_io,
     .get_buf      = scsi_get_buf,
     .load_request = scsi_generic_load_request,
     .save_request = scsi_generic_save_request,
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 2e3a8f9..25a5617 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -123,7 +123,6 @@ struct SCSIReqOps {
     int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
     void (*read_data)(SCSIRequest *req);
     void (*write_data)(SCSIRequest *req);
-    void (*cancel_io)(SCSIRequest *req);
     uint8_t *(*get_buf)(SCSIRequest *req);
 
     void (*save_request)(QEMUFile *f, SCSIRequest *req);
@@ -259,6 +258,7 @@ void scsi_req_complete(SCSIRequest *req, int status);
 uint8_t *scsi_req_get_buf(SCSIRequest *req);
 int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
 void scsi_req_abort(SCSIRequest *req, int status);
+void scsi_req_canceled(SCSIRequest *req);
 void scsi_req_cancel(SCSIRequest *req);
 void scsi_req_retry(SCSIRequest *req);
 void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] scsi: Introduce scsi_req_cancel_async

[Fam Zheng]

Devices can call this function to start an asynchronous cancellation.
The bus->info->cancel will be called later.

Two fields are added to SCSIRequest to respectively keep track of:

 1) The list of (TMF) requests that are waiting for this request to be
    canceled.

 2) The number of (IO) requests that this request (as a TMF request) is
    waiting for.

So when scsi_req_cancel_async is called, the tmf request is tracked with
these fields. When cancel is done, we check if we should notify the bus.

If this is the last canceled request this tmf request is waiting for
(cancel_dep_count drops to 0), we call .cancel_dep_complete so the bus
can complete it.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-bus.c     | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/scsi/scsi.h | 14 ++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 74172cc..47ad0b4 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -552,7 +552,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     SCSIBus *bus = scsi_bus_from_device(d);
     BusState *qbus = BUS(bus);
 
-    req = g_malloc0(reqops->size);
+    req = g_malloc0(reqops ? reqops->size : sizeof(SCSIRequest));
     req->refcount = 1;
     req->bus = bus;
     req->dev = d;
@@ -564,6 +564,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     req->ops = reqops;
     object_ref(OBJECT(d));
     object_ref(OBJECT(qbus->parent));
+    QTAILQ_INIT(&req->cancel_deps);
     trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
     return req;
 }
@@ -1598,7 +1599,7 @@ void scsi_req_unref(SCSIRequest *req)
         if (bus->info->free_request && req->hba_private) {
             bus->info->free_request(bus, req->hba_private);
         }
-        if (req->ops->free_req) {
+        if (req->ops && req->ops->free_req) {
             req->ops->free_req(req);
         }
         object_unref(OBJECT(req->dev));
@@ -1717,13 +1718,58 @@ void scsi_req_complete(SCSIRequest *req, int status)
 /* Called by the devices when the request is canceled. */
 void scsi_req_canceled(SCSIRequest *req)
 {
+    SCSIRequestReference *ref, *next;
     assert(req->io_canceled);
     if (req->bus->info->cancel) {
         req->bus->info->cancel(req);
     }
+    QTAILQ_FOREACH_SAFE(ref, &req->cancel_deps, next, next) {
+        SCSIRequest *r = ref->req;
+        assert(r->cancel_dep_count);
+        r->cancel_dep_count--;
+        if (!r->cancel_dep_count && req->bus->info->cancel_dep_complete) {
+            req->bus->info->cancel_dep_complete(r);
+        }
+        QTAILQ_REMOVE(&req->cancel_deps, ref, next);
+        scsi_req_unref(r);
+        g_free(ref);
+    }
     scsi_req_unref(req);
 }
 
+static void scsi_req_cancel_dep_add(SCSIRequest *req, SCSIRequest *tmf_req)
+{
+    SCSIRequestReference *ref = g_new0(SCSIRequestReference, 1);
+    ref->req = tmf_req;
+    tmf_req->cancel_dep_count++;
+    scsi_req_ref(tmf_req);
+    QTAILQ_INSERT_TAIL(&req->cancel_deps, ref, next);
+}
+
+/* Cancel @req asynchronously.
+ * @tmf_req is added to @req's cancellation dependency list, the bus will be
+ * notified with @tmf_req when all the requests it depends on are canceled.
+ *
+ * @red:     The request to cancel.
+ * @tmf_req: The tmf request which cancels @req.
+ * */
+void scsi_req_cancel_async(SCSIRequest *req, SCSIRequest *tmf_req)
+{
+    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    if (tmf_req) {
+        scsi_req_cancel_dep_add(req, tmf_req);
+    }
+    if (req->io_canceled) {
+        return;
+    }
+    scsi_req_ref(req);
+    scsi_req_dequeue(req);
+    req->io_canceled = true;
+    if (req->aiocb) {
+        bdrv_aio_cancel_async(req->aiocb);
+    }
+}
+
 void scsi_req_cancel(SCSIRequest *req)
 {
     trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 25a5617..d0e0fb4 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -42,6 +42,13 @@ struct SCSICommand {
     enum SCSIXferMode mode;
 };
 
+struct SCSIRequest;
+
+typedef struct SCSIRequestReference {
+    struct SCSIRequest *req;
+    QTAILQ_ENTRY(SCSIRequestReference) next;
+} SCSIRequestReference;
+
 struct SCSIRequest {
     SCSIBus           *bus;
     SCSIDevice        *dev;
@@ -62,6 +69,11 @@ struct SCSIRequest {
     bool retry;
     void *hba_private;
     QTAILQ_ENTRY(SCSIRequest) next;
+
+    /* List of requests that depend on this one */
+    QTAILQ_HEAD(, SCSIRequestReference) cancel_deps;
+    /* The number of requests this one depends on */
+    int cancel_dep_count;
 };
 
 #define TYPE_SCSI_DEVICE "scsi-device"
@@ -137,6 +149,7 @@ struct SCSIBusInfo {
     void (*transfer_data)(SCSIRequest *req, uint32_t arg);
     void (*complete)(SCSIRequest *req, uint32_t arg, size_t resid);
     void (*cancel)(SCSIRequest *req);
+    void (*cancel_dep_complete)(SCSIRequest *req);
     void (*hotplug)(SCSIBus *bus, SCSIDevice *dev);
     void (*hot_unplug)(SCSIBus *bus, SCSIDevice *dev);
     void (*change)(SCSIBus *bus, SCSIDevice *dev, SCSISense sense);
@@ -260,6 +273,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
 void scsi_req_abort(SCSIRequest *req, int status);
 void scsi_req_canceled(SCSIRequest *req);
 void scsi_req_cancel(SCSIRequest *req);
+void scsi_req_cancel_async(SCSIRequest *req, SCSIRequest *tmf_req);
 void scsi_req_retry(SCSIRequest *req);
 void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
 void scsi_device_set_ua(SCSIDevice *sdev, SCSISense sense);
-- 
1.9.3

[Qemu-devel] [PATCH 3/3] virtio-scsi: Handle TMF request cancellation asynchronously

[Fam Zheng]

For VIRTIO_SCSI_T_TMF_ABORT_TASK and VIRTIO_SCSI_T_TMF_ABORT_TASK_SET,
use scsi_req_cancel_async to start the cancellation. In
virtio_scsi_handle_ctrl, wait for virtio_scsi_cancel_dep_complete before
completing the request.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 86aba88..323140e 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -226,12 +226,27 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     return req;
 }
 
-static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
+static void virtio_scsi_cancel_req(SCSIDevice *d, SCSIRequest *req,
+                                   VirtIOSCSIReq *tmf_req)
+{
+    if (!tmf_req->sreq) {
+        /* Allocate a dummy sreq in order to keep track of the
+         * dependency */
+        tmf_req->sreq = scsi_req_alloc(NULL, NULL, 0, 0, req);
+    }
+    scsi_req_cancel_async(req, tmf_req->sreq);
+}
+
+/* Return true if the request is ready to be completed and return to guest;
+ * false if the request will be completed (by some other events) later, for
+ * example in the case of async cancellation. */
+static bool virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
     SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
     SCSIRequest *r, *next;
     BusChild *kid;
     int target;
+    bool ret = true;
 
     /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE".  */
     req->resp.tmf.response = VIRTIO_SCSI_S_OK;
@@ -264,7 +279,8 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
                  */
                 req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
             } else {
-                scsi_req_cancel(r);
+                virtio_scsi_cancel_req(d, r, req);
+                ret = false;
             }
         }
         break;
@@ -299,7 +315,8 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
                     req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
                     break;
                 } else {
-                    scsi_req_cancel(r);
+                    virtio_scsi_cancel_req(d, r, req);
+                    ret = false;
                 }
             }
         }
@@ -323,20 +340,22 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         break;
     }
 
-    return;
+    return ret;
 
 incorrect_lun:
     req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
-    return;
+    return ret;
 
 fail:
     req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+    return ret;
 }
 
 static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
     VirtIOSCSIReq *req;
+    bool should_complete = true;
 
     while ((req = virtio_scsi_pop_req(s, vq))) {
         int type;
@@ -353,7 +372,7 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
                                       sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
                 virtio_scsi_bad_req();
             } else {
-                virtio_scsi_do_tmf(s, req);
+                should_complete = virtio_scsi_do_tmf(s, req);
             }
 
         } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
@@ -366,7 +385,9 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
                 req->resp.an.response = VIRTIO_SCSI_S_OK;
             }
         }
-        virtio_scsi_complete_req(req);
+        if (should_complete) {
+            virtio_scsi_complete_req(req);
+        }
     }
 }
 
@@ -437,6 +458,15 @@ static QEMUSGList *virtio_scsi_get_sg_list(SCSIRequest *r)
     return &req->qsgl;
 }
 
+static void virtio_scsi_cancel_dep_complete(SCSIRequest *r)
+{
+    VirtIOSCSIReq *req = r->hba_private;
+
+    scsi_req_unref(req->sreq);
+    req->sreq = NULL;
+    virtio_scsi_complete_req(req);
+}
+
 static void virtio_scsi_request_cancelled(SCSIRequest *r)
 {
     VirtIOSCSIReq *req = r->hba_private;
@@ -679,6 +709,7 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
 
     .complete = virtio_scsi_command_complete,
     .cancel = virtio_scsi_request_cancelled,
+    .cancel_dep_complete = virtio_scsi_cancel_dep_complete,
     .change = virtio_scsi_change,
     .hotplug = virtio_scsi_hotplug,
     .hot_unplug = virtio_scsi_hot_unplug,
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/3] scsi-bus: Unify request unref in scsi_req_cancel

[Paolo Bonzini]

Il 18/09/2014 04:36, Fam Zheng ha scritto:
> Before, scsi_req_cancel will take ownership of the canceled request and
> unref it. This is because we didn't know if AIO CB will be called or not
> during the cancelling, so we set the io_canceled flag before calling it,
> and skip to unref in the potentially called callbacks, which is not
> very nice.
> 
> Now, bdrv_aio_cancel has a stricter contract that the completion
> callback is always called, so we can remove the special case of
> req->io_canceled.
> 
> It will also make implementing asynchronous cancellation easier.
> 
> Also, as the existing SCSIReqOps.cancel_io implementations are exactly
> the same, we can move the code to bus for now as we are touching them in
> this patch.
> 
> All AIO callbacks in devices, those will be called because of
> bdrv_aio_cancel, should call scsi_req_canceled if req->io_canceled is
> set. That's the uniform place we release the reference we grabbed in
> scsi_req_cancel and notify buses.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/scsi-bus.c     | 30 ++++++++++++-------------
>  hw/scsi/scsi-disk.c    | 59 ++++++++++++++------------------------------------
>  hw/scsi/scsi-generic.c | 28 +++++-------------------
>  include/hw/scsi/scsi.h |  2 +-
>  4 files changed, 37 insertions(+), 82 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 954c607..74172cc 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1613,7 +1613,6 @@ void scsi_req_continue(SCSIRequest *req)
>  {
>      if (req->io_canceled) {
>          trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
> -        return;

I think this is incorrect; same in scsi_req_data.

As discussed on IRC, scsi-generic is the case where this happens.  We
can change it to use the same io_canceled handling as everyone else, so
that the subsequent changes are more mechanical.

>      }
>      trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
>      if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> @@ -1631,7 +1630,6 @@ void scsi_req_data(SCSIRequest *req, int len)
>      uint8_t *buf;
>      if (req->io_canceled) {
>          trace_scsi_req_data_canceled(req->dev->id, req->lun, req->tag, len);
> -        return;
>      }
>      trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
>      assert(req->cmd.mode != SCSI_XFER_NONE);
> @@ -1716,35 +1714,37 @@ void scsi_req_complete(SCSIRequest *req, int status)
>      scsi_req_unref(req);
>  }
>  
> +/* Called by the devices when the request is canceled. */
> +void scsi_req_canceled(SCSIRequest *req)
> +{
> +    assert(req->io_canceled);
> +    if (req->bus->info->cancel) {
> +        req->bus->info->cancel(req);
> +    }
> +    scsi_req_unref(req);
> +}

I think this patch does a bit too much.  I would do it like this:

- first, as mentioned above, make scsi-generic follow the usual pattern with

    if (req->io_canceled) {
        goto done;
    }
    ...
done:
    if (!req->io_canceled)
        scsi_req_unref(&r->req);
    }

- second, remove the scsi_req_unref from the scsi_cancel_io
implementations, and remove the "if" statement around

done:
    if (!r->req.io_canceled) {
        scsi_req_unref(&r->req);
    }

We can do this now that the callback is always invoked, and it
simplifies the reference counting quite a bit.

- third, uninline scsi_cancel_io and introduce scsi_req_canceled

>  void scsi_req_cancel(SCSIRequest *req)
>  {
>      trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> -    if (!req->enqueued) {
> +    if (req->io_canceled) {
>          return;
>      }
>      scsi_req_ref(req);
>      scsi_req_dequeue(req);
>      req->io_canceled = true;
> -    if (req->ops->cancel_io) {
> -        req->ops->cancel_io(req);
> +    if (req->aiocb) {
> +        bdrv_aio_cancel(req->aiocb);
>      }
> -    if (req->bus->info->cancel) {
> -        req->bus->info->cancel(req);
> -    }
> -    scsi_req_unref(req);
>  }
>  
>  void scsi_req_abort(SCSIRequest *req, int status)
>  {
> -    if (!req->enqueued) {
> +    if (req->io_canceled) {
>          return;
>      }
>      scsi_req_ref(req);
> -    scsi_req_dequeue(req);
> -    req->io_canceled = true;
> -    if (req->ops->cancel_io) {
> -        req->ops->cancel_io(req);
> -    }
> +    scsi_req_cancel(req);

This is a problem, because we would complete the request twice: once
from scsi_req_canceled, once in scsi_req_complete below.

Let's just drop scsi_req_abort.  The only user can be removed like this:

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 048cfc7..ec5dc0d 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -77,8 +77,9 @@ typedef struct vscsi_req {
     SCSIRequest             *sreq;
     uint32_t                qtag; /* qemu tag != srp tag */
     bool                    active;
-    uint32_t                data_len;
     bool                    writing;
+    bool                    dma_error;
+    uint32_t                data_len;
     uint32_t                senselen;
     uint8_t                 sense[SCSI_SENSE_BUF_SIZE];

@@ -536,8 +537,8 @@ static void vscsi_transfer_data(SCSIRequest *sreq,
uint32_t len)
     }
     if (rc < 0) {
         fprintf(stderr, "VSCSI: RDMA error rc=%d!\n", rc);
-        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
-        scsi_req_abort(req->sreq, CHECK_CONDITION);
+        req->dma_error = true;
+        scsi_req_cancel(req->sreq);
         return;
     }

@@ -591,6 +592,10 @@ static void vscsi_request_cancelled(SCSIRequest *sreq)
 {
     vscsi_req *req = sreq->hba_private;

+    if (req->dma_error) {
+        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
+        vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
+    }
     vscsi_put_req(req);
 }


(Yet another separate patch...)

I like this patch.  It is very mechanical, which makes it easier to
review than the size would suggest.  However, splitting it would make
the review even easier. :)

Paolo

>      scsi_req_complete(req, status);
>      scsi_req_unref(req);
>  }
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e34a544..56d7e6d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -105,23 +105,6 @@ static void scsi_check_condition(SCSIDiskReq *r, SCSISense sense)
>      scsi_req_complete(&r->req, CHECK_CONDITION);
>  }
>  
> -/* Cancel a pending data transfer.  */
> -static void scsi_cancel_io(SCSIRequest *req)
> -{
> -    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> -
> -    DPRINTF("Cancel tag=0x%x\n", req->tag);
> -    if (r->req.aiocb) {
> -        bdrv_aio_cancel(r->req.aiocb);
> -
> -        /* This reference was left in by scsi_*_data.  We take ownership of
> -         * it the moment scsi_req_cancel is called, independent of whether
> -         * bdrv_aio_cancel completes the request or not.  */
> -        scsi_req_unref(&r->req);
> -    }
> -    r->req.aiocb = NULL;
> -}
> -
>  static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t size)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> @@ -185,6 +168,7 @@ static void scsi_aio_complete(void *opaque, int ret)
>      r->req.aiocb = NULL;
>      bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -197,9 +181,7 @@ static void scsi_aio_complete(void *opaque, int ret)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static bool scsi_is_cmd_fua(SCSICommand *cmd)
> @@ -233,6 +215,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>  
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -245,9 +228,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static void scsi_dma_complete_noio(void *opaque, int ret)
> @@ -260,6 +241,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
>          bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -279,9 +261,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
>      }
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static void scsi_dma_complete(void *opaque, int ret)
> @@ -302,6 +282,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      r->req.aiocb = NULL;
>      bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -319,9 +300,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      scsi_req_data(&r->req, r->qiov.size);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  /* Actually issue a read to the block device.  */
> @@ -336,6 +315,7 @@ static void scsi_do_read(void *opaque, int ret)
>          bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -361,9 +341,7 @@ static void scsi_do_read(void *opaque, int ret)
>      }
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  /* Read more data from scsi device into buffer.  */
> @@ -456,6 +434,7 @@ static void scsi_write_complete(void * opaque, int ret)
>          bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -478,9 +457,7 @@ static void scsi_write_complete(void * opaque, int ret)
>      }
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static void scsi_write_data(SCSIRequest *req)
> @@ -1548,6 +1525,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
>  
>      r->req.aiocb = NULL;
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -1577,9 +1555,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>      g_free(data);
>  }
>  
> @@ -1649,6 +1625,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
>      r->req.aiocb = NULL;
>      bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -1672,9 +1649,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>      qemu_vfree(data->iov.iov_base);
>      g_free(data);
>  }
> @@ -2337,7 +2312,6 @@ 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,
>  };
>  
> @@ -2347,7 +2321,6 @@ static const SCSIReqOps scsi_disk_dma_reqops = {
>      .send_command = scsi_disk_dma_command,
>      .read_data    = scsi_read_data,
>      .write_data   = scsi_write_data,
> -    .cancel_io    = scsi_cancel_io,
>      .get_buf      = scsi_get_buf,
>      .load_request = scsi_disk_load_request,
>      .save_request = scsi_disk_save_request,
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 20587b4..5bde909 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -132,27 +132,12 @@ static void scsi_command_complete(void *opaque, int ret)
>      DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
>              r, r->req.tag, status);
>  
> -    scsi_req_complete(&r->req, status);
>      if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> -}
> -
> -/* Cancel a pending data transfer.  */
> -static void scsi_cancel_io(SCSIRequest *req)
> -{
> -    SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
> -
> -    DPRINTF("Cancel tag=0x%x\n", req->tag);
> -    if (r->req.aiocb) {
> -        bdrv_aio_cancel(r->req.aiocb);
> -
> -        /* This reference was left in by scsi_*_data.  We take ownership of
> -         * it independent of whether bdrv_aio_cancel completes the request
> -         * or not.  */
> -        scsi_req_unref(&r->req);
> +        scsi_req_complete(&r->req, status);
> +    } else {
> +        scsi_req_canceled(&r->req);
>      }
> -    r->req.aiocb = NULL;
> +    scsi_req_unref(&r->req);
>  }
>  
>  static int execute_command(BlockDriverState *bdrv,
> @@ -211,9 +196,7 @@ static void scsi_read_complete(void * opaque, int ret)
>          bdrv_set_guest_block_size(s->conf.bs, s->blocksize);
>  
>          scsi_req_data(&r->req, len);
> -        if (!r->req.io_canceled) {
> -            scsi_req_unref(&r->req);
> -        }
> +        scsi_req_unref(&r->req);
>      }
>  }
>  
> @@ -465,7 +448,6 @@ const SCSIReqOps scsi_generic_req_ops = {
>      .send_command = scsi_send_command,
>      .read_data    = scsi_read_data,
>      .write_data   = scsi_write_data,
> -    .cancel_io    = scsi_cancel_io,
>      .get_buf      = scsi_get_buf,
>      .load_request = scsi_generic_load_request,
>      .save_request = scsi_generic_save_request,
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 2e3a8f9..25a5617 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -123,7 +123,6 @@ struct SCSIReqOps {
>      int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
>      void (*read_data)(SCSIRequest *req);
>      void (*write_data)(SCSIRequest *req);
> -    void (*cancel_io)(SCSIRequest *req);
>      uint8_t *(*get_buf)(SCSIRequest *req);
>  
>      void (*save_request)(QEMUFile *f, SCSIRequest *req);
> @@ -259,6 +258,7 @@ void scsi_req_complete(SCSIRequest *req, int status);
>  uint8_t *scsi_req_get_buf(SCSIRequest *req);
>  int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
>  void scsi_req_abort(SCSIRequest *req, int status);
> +void scsi_req_canceled(SCSIRequest *req);
>  void scsi_req_cancel(SCSIRequest *req);
>  void scsi_req_retry(SCSIRequest *req);
>  void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
> 

Re: [Qemu-devel] [PATCH 2/3] scsi: Introduce scsi_req_cancel_async

[Paolo Bonzini]

Il 18/09/2014 04:36, Fam Zheng ha scritto:
> +    QTAILQ_FOREACH_SAFE(ref, &req->cancel_deps, next, next) {
> +        SCSIRequest *r = ref->req;
> +        assert(r->cancel_dep_count);
> +        r->cancel_dep_count--;
> +        if (!r->cancel_dep_count && req->bus->info->cancel_dep_complete) {
> +            req->bus->info->cancel_dep_complete(r);
> +        }
> +        QTAILQ_REMOVE(&req->cancel_deps, ref, next);
> +        scsi_req_unref(r);
> +        g_free(ref);
> +    }

I think there is one problem here.

scsi_req_cancel_async can actually be synchronous if you're unlucky
(because bdrv_aio_cancel_async can be synchronous too, for example in
the case of a linux-aio AIOCB).  So you could end up calling
cancel_dep_complete even if the caller intends to cancel more requests.

I think it's better to track the count in virtio-scsi instead.  You can
initialize it similar to bdrv_aio_multiwrite:

    /* Run the aio requests. */
    mcb->num_requests = num_reqs;
    for (i = 0; i < num_reqs; i++) {
        bdrv_co_aio_rw_vector(bs, reqs[i].sector, reqs[i].qiov,
                              reqs[i].nb_sectors, reqs[i].flags,
                              multiwrite_cb, mcb,
                              true);
    }

    return 0;

and decrement the count on every call to the notifier.

This is independent of the choice to make bdrv_aio_cancel_async
semantics stricter.  In the case of bdrv_aio_multiwrite, we know that
bdrv_co_aio_rw_vector is never synchronous, but the code is simply nicer
that way. :)

Paolo

Re: [Qemu-devel] [PATCH 2/3] scsi: Introduce scsi_req_cancel_async

[Paolo Bonzini]

Il 18/09/2014 04:36, Fam Zheng ha scritto:
> @@ -552,7 +552,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>      SCSIBus *bus = scsi_bus_from_device(d);
>      BusState *qbus = BUS(bus);
>  
> -    req = g_malloc0(reqops->size);
> +    req = g_malloc0(reqops ? reqops->size : sizeof(SCSIRequest));

Let's just add a NotifierList to SCSIRequest instead, and pass a
Notifier to scsi_req_cancel_async.  There can be multiple aborts for the
same SCSIRequest, we can leave it to the HBA to create a Notifier for
each TMF.

The notifier can be embedded in a struct to track the request count, as
mentioned in the other message I just sent.

Paolo