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

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

[Fam Zheng]

v3: Address Paolo's comments:
    - scsi_req_canceled -> scsi_req_cancel_complete
    - Drop unnecessary changes in scsi_req_alloc and scsi_req_unref.
    - Update comment of scsi_req_cancel_async.
    - Add notifier only if not null.
    - Use slice allocator for VirtIOSCSICancelNotifier and
      VirtIOSCSICancelTracker.
    - Use int return value (0/-EINPROGRESS) for virtio_scsi_do_tmf.

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 (7):
  scsi: Drop scsi_req_abort
  scsi-generic: Handle canceled request in scsi_command_complete
  scsi-bus: Unify request unref in scsi_req_cancel
  scsi: Drop SCSIReqOps.cancel_io
  scsi: Introduce scsi_req_cancel_complete
  scsi: Introduce scsi_req_cancel_async
  virtio-scsi: Handle TMF request cancellation asynchronously

 hw/scsi/scsi-bus.c     | 40 ++++++++++++++++--------
 hw/scsi/scsi-disk.c    | 59 ++++++++++-------------------------
 hw/scsi/scsi-generic.c | 37 ++++++----------------
 hw/scsi/spapr_vscsi.c  | 11 +++++--
 hw/scsi/virtio-scsi.c  | 85 +++++++++++++++++++++++++++++++++++++++++++++-----
 include/hw/scsi/scsi.h |  5 ++-
 6 files changed, 142 insertions(+), 95 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/7] scsi: Drop scsi_req_abort

[Fam Zheng]

The only user of this function is spapr_vscsi.c. We can convert to
scsi_req_cancel plus adding a check in vscsi_request_cancelled.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-bus.c    | 15 ---------------
 hw/scsi/spapr_vscsi.c | 11 ++++++++---
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index af293b5..f90a204 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1736,21 +1736,6 @@ void scsi_req_cancel(SCSIRequest *req)
     scsi_req_unref(req);
 }
 
-void scsi_req_abort(SCSIRequest *req, int status)
-{
-    if (!req->enqueued) {
-        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_complete(req, status);
-    scsi_req_unref(req);
-}
-
 static int scsi_ua_precedence(SCSISense sense)
 {
     if (sense.key != UNIT_ATTENTION) {
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);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/7] scsi-generic: Handle canceled request in scsi_command_complete

[Fam Zheng]

Now that we always called the cb in bdrv_aio_cancel, let's make scsi-generic
callbacks check io_canceled flag similarly to scsi-disk.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-generic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 20587b4..2a73a43 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -93,6 +93,9 @@ static void scsi_command_complete(void *opaque, int ret)
     SCSIGenericReq *r = (SCSIGenericReq *)opaque;
 
     r->req.aiocb = NULL;
+    if (r->req.io_canceled) {
+        goto done;
+    }
     if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
         r->req.sense_len = r->io_header.sb_len_wr;
     }
@@ -133,6 +136,7 @@ static void scsi_command_complete(void *opaque, int ret)
             r, r->req.tag, status);
 
     scsi_req_complete(&r->req, status);
+done:
     if (!r->req.io_canceled) {
         scsi_req_unref(&r->req);
     }
@@ -186,8 +190,7 @@ static void scsi_read_complete(void * opaque, int ret)
     int len;
 
     r->req.aiocb = NULL;
-    if (ret) {
-        DPRINTF("IO error ret %d\n", ret);
+    if (ret || r->req.io_canceled) {
         scsi_command_complete(r, ret);
         return;
     }
@@ -246,8 +249,7 @@ static void scsi_write_complete(void * opaque, int ret)
 
     DPRINTF("scsi_write_complete() ret = %d\n", ret);
     r->req.aiocb = NULL;
-    if (ret) {
-        DPRINTF("IO error\n");
+    if (ret || r->req.io_canceled) {
         scsi_command_complete(r, ret);
         return;
     }
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/7] 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. We did this because we didn't know whether AIO CB will be called or not
during the cancelling, so we set the io_canceled flag before calling it, and
skip unref in the potentially called callbacks, which is not very nice.

Now, bdrv_aio_cancel has a stricter contract that the completion callbacks are
always called, so we can remove the checks of req->io_canceled and just unref
it in callbacks.

It will also make implementing asynchronous cancellation easier.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-disk.c    | 37 ++++++++-----------------------------
 hw/scsi/scsi-generic.c | 13 ++-----------
 2 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 9645d01..2e45752 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -113,11 +113,6 @@ static void scsi_cancel_io(SCSIRequest *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;
 }
@@ -197,9 +192,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)
@@ -246,9 +239,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)
@@ -280,9 +271,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)
@@ -320,9 +309,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.  */
@@ -363,9 +350,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.  */
@@ -481,9 +466,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)
@@ -1582,9 +1565,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);
 }
 
@@ -1678,9 +1659,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);
 }
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 2a73a43..e92b418 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -137,9 +137,7 @@ static void scsi_command_complete(void *opaque, int ret)
 
     scsi_req_complete(&r->req, status);
 done:
-    if (!r->req.io_canceled) {
-        scsi_req_unref(&r->req);
-    }
+    scsi_req_unref(&r->req);
 }
 
 /* Cancel a pending data transfer.  */
@@ -150,11 +148,6 @@ static void scsi_cancel_io(SCSIRequest *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);
     }
     r->req.aiocb = NULL;
 }
@@ -214,9 +207,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);
     }
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v3 4/7] scsi: Drop SCSIReqOps.cancel_io

[Fam Zheng]

The only two implementations are identical to each other, with nothing specific
to device: they only call bdrv_aio_cancel with the SCSIRequest.aiocb.

Let's move it to scsi-bus.

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

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f90a204..764f6cf 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1727,8 +1727,8 @@ void scsi_req_cancel(SCSIRequest *req)
     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);
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 2e45752..ef13e66 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -105,18 +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);
-    }
-    r->req.aiocb = NULL;
-}
-
 static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t size)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
@@ -2325,7 +2313,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,
 };
 
@@ -2335,7 +2322,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 e92b418..7e85047 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -140,18 +140,6 @@ done:
     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);
-    }
-    r->req.aiocb = NULL;
-}
-
 static int execute_command(BlockDriverState *bdrv,
                            SCSIGenericReq *r, int direction,
 			   BlockDriverCompletionFunc *complete)
@@ -458,7 +446,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 6271ad3..1118107 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -130,7 +130,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);
-- 
1.9.3

[Qemu-devel] [PATCH v3 5/7] scsi: Introduce scsi_req_cancel_complete

[Fam Zheng]

Let the aio cb do the clean up and notification job after scsi_req_cancel, in
preparation for asynchronous cancellation.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-bus.c     | 14 ++++++++++----
 hw/scsi/scsi-disk.c    |  8 ++++++++
 hw/scsi/scsi-generic.c |  1 +
 include/hw/scsi/scsi.h |  1 +
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 764f6cf..c91db63 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1718,6 +1718,16 @@ void scsi_req_complete(SCSIRequest *req, int status)
     scsi_req_unref(req);
 }
 
+/* Called by the devices when the request is canceled. */
+void scsi_req_cancel_complete(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);
@@ -1730,10 +1740,6 @@ void scsi_req_cancel(SCSIRequest *req)
     if (req->aiocb) {
         bdrv_aio_cancel(req->aiocb);
     }
-    if (req->bus->info->cancel) {
-        req->bus->info->cancel(req);
-    }
-    scsi_req_unref(req);
 }
 
 static int scsi_ua_precedence(SCSISense sense)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ef13e66..7a7938a 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -168,6 +168,7 @@ static void scsi_aio_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
     block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
@@ -214,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_cancel_complete(&r->req);
         goto done;
     }
 
@@ -240,6 +242,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
         block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
     }
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
@@ -280,6 +283,7 @@ static void scsi_read_complete(void * opaque, int ret)
     r->req.aiocb = NULL;
     block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
@@ -312,6 +316,7 @@ static void scsi_do_read(void *opaque, int ret)
         block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
     }
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
@@ -432,6 +437,7 @@ static void scsi_write_complete(void * opaque, int ret)
         block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
     }
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
@@ -1524,6 +1530,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
 
     r->req.aiocb = NULL;
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
@@ -1623,6 +1630,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
     block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7e85047..01bca08 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -94,6 +94,7 @@ static void scsi_command_complete(void *opaque, int ret)
 
     r->req.aiocb = NULL;
     if (r->req.io_canceled) {
+        scsi_req_cancel_complete(&r->req);
         goto done;
     }
     if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 1118107..a75a7c8 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -265,6 +265,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_cancel_complete(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 v3 6/7] scsi: Introduce scsi_req_cancel_async

[Fam Zheng]

Devices will call this function to start an asynchronous cancellation. The
bus->info->cancel will be called after the request is canceled.

Devices will probably need to track a separate TMF request that triggers this
cancellation, and wait until the cancellation is done before completing it. So
we store a notifier list in SCSIRequest and in scsi_req_cancel_complete we
notify them.

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

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index c91db63..df7585a 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -566,6 +566,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     req->ops = reqops;
     object_ref(OBJECT(d));
     object_ref(OBJECT(qbus->parent));
+    notifier_list_init(&req->cancel_notifiers);
     trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
     return req;
 }
@@ -1725,9 +1726,31 @@ void scsi_req_cancel_complete(SCSIRequest *req)
     if (req->bus->info->cancel) {
         req->bus->info->cancel(req);
     }
+    notifier_list_notify(&req->cancel_notifiers, req);
     scsi_req_unref(req);
 }
 
+/* Cancel @req asynchronously. @notifier is added to @req's cancellation
+ * notifier list, the bus will be notified the requests cancellation is
+ * completed.
+ * */
+void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
+{
+    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    if (notifier) {
+        notifier_list_add(&req->cancel_notifiers, notifier);
+    }
+    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 a75a7c8..c47dc53 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -5,6 +5,7 @@
 #include "block/block.h"
 #include "hw/block/block.h"
 #include "sysemu/sysemu.h"
+#include "qemu/notify.h"
 
 #define MAX_SCSI_DEVS	255
 
@@ -53,6 +54,7 @@ struct SCSIRequest {
     void              *hba_private;
     size_t            resid;
     SCSICommand       cmd;
+    NotifierList      cancel_notifiers;
 
     /* Note:
      * - fields before sense are initialized by scsi_req_alloc;
@@ -267,6 +269,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
 void scsi_req_abort(SCSIRequest *req, int status);
 void scsi_req_cancel_complete(SCSIRequest *req);
 void scsi_req_cancel(SCSIRequest *req);
+void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier);
 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 v3 7/7] 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.

Because each tmf command may cancel multiple requests, we need to use a
counter to track the number of remaining requests we still need to wait
for.

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

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index fa36e23..7a6b71a 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -208,12 +208,40 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     return req;
 }
 
-static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
+typedef struct {
+    VirtIOSCSIReq  *tmf_req;
+    int            remaining;
+} VirtIOSCSICancelTracker;
+
+typedef struct {
+    Notifier                 notifier;
+    VirtIOSCSICancelTracker  *tracker;
+} VirtIOSCSICancelNotifier;
+
+static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
+{
+    VirtIOSCSICancelNotifier *n = container_of(notifier,
+                                               VirtIOSCSICancelNotifier,
+                                               notifier);
+
+    if (--n->tracker->remaining == 0) {
+        virtio_scsi_complete_req(n->tracker->tmf_req);
+        g_slice_free(VirtIOSCSICancelTracker, n->tracker);
+    }
+    g_slice_free(VirtIOSCSICancelNotifier, n);
+}
+
+/* Return 0 if the request is ready to be completed and return to guest;
+ * -EINPROGRESS if the request is submitted and will be completed later, in the
+ *  case of async cancellation. */
+static int 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;
+    int ret = 0;
+    int cancel_count;
 
     if (s->dataplane_started && bdrv_get_aio_context(d->conf.bs) != s->ctx) {
         aio_context_acquire(s->ctx);
@@ -251,7 +279,18 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
                  */
                 req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
             } else {
-                scsi_req_cancel(r);
+                VirtIOSCSICancelNotifier *notifier;
+                VirtIOSCSICancelTracker *tracker;
+
+                notifier           = g_slice_new(VirtIOSCSICancelNotifier);
+                notifier->notifier.notify
+                                   = virtio_scsi_cancel_notify;
+                tracker            = g_slice_new(VirtIOSCSICancelTracker);
+                tracker->tmf_req   = req;
+                tracker->remaining = 1;
+                notifier->tracker  = tracker;
+                scsi_req_cancel_async(r, &notifier->notifier);
+                ret = -EINPROGRESS;
             }
         }
         break;
@@ -277,6 +316,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
             goto incorrect_lun;
         }
+        cancel_count = 0;
         QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
             if (r->hba_private) {
                 if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
@@ -286,10 +326,35 @@ 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);
+                    /* Before we actually cancel any requests in the next for
+                     * loop, let's count them.  This way, if the bus starts
+                     * calling back to the notifier even before we finish the
+                     * loop, the counter, which value is already seen in
+                     * virtio_scsi_cancel_notify, will prevent us from
+                     * completing the tmf too quickly. */
+                    cancel_count++;
                 }
             }
         }
+        if (cancel_count) {
+            VirtIOSCSICancelNotifier *notifier;
+            VirtIOSCSICancelTracker *tracker;
+
+            tracker            = g_slice_new(VirtIOSCSICancelTracker);
+            tracker->tmf_req   = req;
+            tracker->remaining = cancel_count;
+
+            QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
+                if (r->hba_private) {
+                    notifier = g_slice_new(VirtIOSCSICancelNotifier);
+                    notifier->notifier.notify = virtio_scsi_cancel_notify;
+                    notifier->tracker = tracker;
+                    scsi_req_cancel_async(r, &notifier->notifier);
+                }
+            }
+            ret = -EINPROGRESS;
+        }
+
         break;
 
     case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
@@ -310,20 +375,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;
 }
 
 void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
     VirtIODevice *vdev = (VirtIODevice *)s;
     int type;
+    int r = 0;
 
     if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
                 &type, sizeof(type)) < sizeof(type)) {
@@ -337,7 +404,7 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
                     sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
             virtio_scsi_bad_req();
         } else {
-            virtio_scsi_do_tmf(s, req);
+            r = virtio_scsi_do_tmf(s, req);
         }
 
     } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
@@ -350,7 +417,11 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
             req->resp.an.response = VIRTIO_SCSI_S_OK;
         }
     }
-    virtio_scsi_complete_req(req);
+    if (r == 0) {
+        virtio_scsi_complete_req(req);
+    } else {
+        assert(r = -EINPROGRESS);
+    }
 }
 
 static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 6/7] scsi: Introduce scsi_req_cancel_async

[Paolo Bonzini]

Il 25/09/2014 04:20, Fam Zheng ha scritto:
> Devices will call this function to start an asynchronous cancellation. The
> bus->info->cancel will be called after the request is canceled.
> 
> Devices will probably need to track a separate TMF request that triggers this
> cancellation, and wait until the cancellation is done before completing it. So
> we store a notifier list in SCSIRequest and in scsi_req_cancel_complete we
> notify them.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/scsi-bus.c     | 23 +++++++++++++++++++++++
>  include/hw/scsi/scsi.h |  3 +++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index c91db63..df7585a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -566,6 +566,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>      req->ops = reqops;
>      object_ref(OBJECT(d));
>      object_ref(OBJECT(qbus->parent));
> +    notifier_list_init(&req->cancel_notifiers);
>      trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
>      return req;
>  }
> @@ -1725,9 +1726,31 @@ void scsi_req_cancel_complete(SCSIRequest *req)
>      if (req->bus->info->cancel) {
>          req->bus->info->cancel(req);
>      }
> +    notifier_list_notify(&req->cancel_notifiers, req);

I think you also have to call notifier_list_notify from
scsi_req_complete, because a cancelled request might end up being
completed instead of cancelled.

In fact, the next obvious step (enabled by your bdrv_aio_cancel cleanup)
would be to _not_ call scsi_req_cancel_complete if we can report
completion or if there was an I/O error.  This can happen for
scsi-generic, scsi_dma_complete_noio, etc.  Basically, moving the
io_canceled check from the beginning of the completion routine to just
before bdrv_aio_* or dma_aio_* are called.

Paolo

>      scsi_req_unref(req);
>  }
>  
> +/* Cancel @req asynchronously. @notifier is added to @req's cancellation
> + * notifier list, the bus will be notified the requests cancellation is
> + * completed.
> + * */
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
> +{
> +    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> +    if (notifier) {
> +        notifier_list_add(&req->cancel_notifiers, notifier);
> +    }
> +    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 a75a7c8..c47dc53 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -5,6 +5,7 @@
>  #include "block/block.h"
>  #include "hw/block/block.h"
>  #include "sysemu/sysemu.h"
> +#include "qemu/notify.h"
>  
>  #define MAX_SCSI_DEVS	255
>  
> @@ -53,6 +54,7 @@ struct SCSIRequest {
>      void              *hba_private;
>      size_t            resid;
>      SCSICommand       cmd;
> +    NotifierList      cancel_notifiers;
>  
>      /* Note:
>       * - fields before sense are initialized by scsi_req_alloc;
> @@ -267,6 +269,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
>  void scsi_req_abort(SCSIRequest *req, int status);
>  void scsi_req_cancel_complete(SCSIRequest *req);
>  void scsi_req_cancel(SCSIRequest *req);
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier);
>  void scsi_req_retry(SCSIRequest *req);
>  void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
>  void scsi_device_set_ua(SCSIDevice *sdev, SCSISense sense);
> 

Re: [Qemu-devel] [PATCH v3 5/7] scsi: Introduce scsi_req_cancel_complete

[Paolo Bonzini]

Il 25/09/2014 04:20, Fam Zheng ha scritto:
> Let the aio cb do the clean up and notification job after scsi_req_cancel, in
> preparation for asynchronous cancellation.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/scsi-bus.c     | 14 ++++++++++----
>  hw/scsi/scsi-disk.c    |  8 ++++++++
>  hw/scsi/scsi-generic.c |  1 +
>  include/hw/scsi/scsi.h |  1 +
>  4 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 764f6cf..c91db63 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1718,6 +1718,16 @@ void scsi_req_complete(SCSIRequest *req, int status)
>      scsi_req_unref(req);
>  }
>  
> +/* Called by the devices when the request is canceled. */
> +void scsi_req_cancel_complete(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);
> @@ -1730,10 +1740,6 @@ void scsi_req_cancel(SCSIRequest *req)
>      if (req->aiocb) {
>          bdrv_aio_cancel(req->aiocb);
>      }
> -    if (req->bus->info->cancel) {
> -        req->bus->info->cancel(req);
> -    }
> -    scsi_req_unref(req);
>  }
>  
>  static int scsi_ua_precedence(SCSISense sense)
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index ef13e66..7a7938a 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -168,6 +168,7 @@ static void scsi_aio_complete(void *opaque, int ret)
>      r->req.aiocb = NULL;
>      block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -214,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_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -240,6 +242,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
>          block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -280,6 +283,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      r->req.aiocb = NULL;
>      block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -312,6 +316,7 @@ static void scsi_do_read(void *opaque, int ret)
>          block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -432,6 +437,7 @@ static void scsi_write_complete(void * opaque, int ret)
>          block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -1524,6 +1530,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
>  
>      r->req.aiocb = NULL;
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> @@ -1623,6 +1630,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
>      r->req.aiocb = NULL;
>      block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>  
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7e85047..01bca08 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -94,6 +94,7 @@ static void scsi_command_complete(void *opaque, int ret)
>  
>      r->req.aiocb = NULL;
>      if (r->req.io_canceled) {
> +        scsi_req_cancel_complete(&r->req);
>          goto done;
>      }
>      if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 1118107..a75a7c8 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -265,6 +265,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_cancel_complete(SCSIRequest *req);
>  void scsi_req_cancel(SCSIRequest *req);
>  void scsi_req_retry(SCSIRequest *req);
>  void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v3 6/7] scsi: Introduce scsi_req_cancel_async

[Fam Zheng]

On Thu, 09/25 10:51, Paolo Bonzini wrote:
> Il 25/09/2014 04:20, Fam Zheng ha scritto:
> > Devices will call this function to start an asynchronous cancellation. The
> > bus->info->cancel will be called after the request is canceled.
> > 
> > Devices will probably need to track a separate TMF request that triggers this
> > cancellation, and wait until the cancellation is done before completing it. So
> > we store a notifier list in SCSIRequest and in scsi_req_cancel_complete we
> > notify them.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/scsi/scsi-bus.c     | 23 +++++++++++++++++++++++
> >  include/hw/scsi/scsi.h |  3 +++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > index c91db63..df7585a 100644
> > --- a/hw/scsi/scsi-bus.c
> > +++ b/hw/scsi/scsi-bus.c
> > @@ -566,6 +566,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
> >      req->ops = reqops;
> >      object_ref(OBJECT(d));
> >      object_ref(OBJECT(qbus->parent));
> > +    notifier_list_init(&req->cancel_notifiers);
> >      trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
> >      return req;
> >  }
> > @@ -1725,9 +1726,31 @@ void scsi_req_cancel_complete(SCSIRequest *req)
> >      if (req->bus->info->cancel) {
> >          req->bus->info->cancel(req);
> >      }
> > +    notifier_list_notify(&req->cancel_notifiers, req);
> 
> I think you also have to call notifier_list_notify from
> scsi_req_complete, because a cancelled request might end up being
> completed instead of cancelled.

Yes, will update the series.

> 
> In fact, the next obvious step (enabled by your bdrv_aio_cancel cleanup)
> would be to _not_ call scsi_req_cancel_complete if we can report
> completion or if there was an I/O error.  This can happen for
> scsi-generic, scsi_dma_complete_noio, etc.  Basically, moving the
> io_canceled check from the beginning of the completion routine to just
> before bdrv_aio_* or dma_aio_* are called.

Okay.

Thanks,
Fam