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

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

[Fam Zheng]

v2: Address Paolo's comments:
    - Drop scsi_req_abort.
    - Split patch 1.
    - Use NotifierList.
    - Track the number of cancelled requests in virtio-scsi.

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_canceled
  scsi: Introduce scsi_req_cancel_async
  virtio-scsi: Handle TMF request cancellation asynchronously

 hw/scsi/scsi-bus.c     | 48 +++++++++++++++++++----------
 hw/scsi/scsi-disk.c    | 59 ++++++++++-------------------------
 hw/scsi/scsi-generic.c | 37 ++++++----------------
 hw/scsi/spapr_vscsi.c  | 11 +++++--
 hw/scsi/virtio-scsi.c  | 84 +++++++++++++++++++++++++++++++++++++++++++++-----
 include/hw/scsi/scsi.h |  5 ++-
 6 files changed, 146 insertions(+), 98 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 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 v2 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 v2 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 v2 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 v2 5/7] scsi: Introduce scsi_req_canceled

[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..fca4a9e 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_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);
@@ -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..c029d8c 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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&r->req);
         goto done;
     }
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7e85047..50c98b0 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_canceled(&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..0fa2a8a 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_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 v2 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_canceled we notify them.

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

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index fca4a9e..c1b05a1 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -553,9 +553,10 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     BusState *qbus = BUS(bus);
     const int memset_off = offsetof(SCSIRequest, sense)
                            + sizeof(req->sense);
+    size_t reqops_size = reqops ? reqops->size : sizeof(SCSIRequest);
 
-    req = g_slice_alloc(reqops->size);
-    memset((uint8_t *)req + memset_off, 0, reqops->size - memset_off);
+    req = g_slice_alloc(reqops_size);
+    memset((uint8_t *)req + memset_off, 0, reqops_size - memset_off);
     req->refcount = 1;
     req->bus = bus;
     req->dev = d;
@@ -566,6 +567,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;
 }
@@ -1600,7 +1602,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));
@@ -1725,9 +1727,32 @@ void scsi_req_canceled(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.
+ * @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, Notifier *notifier)
+{
+    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    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 0fa2a8a..6a051df 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_canceled(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 v2 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index fa36e23..9bd7d8a 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_free(n->tracker);
+    }
+    g_free(n);
+}
+
+/* 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;
+    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_new(VirtIOSCSICancelNotifier, 1);
+                notifier->notifier.notify
+                                   = virtio_scsi_cancel_notify;
+                tracker            = g_new(VirtIOSCSICancelTracker, 1);
+                tracker->tmf_req   = req;
+                tracker->remaining = 1;
+                notifier->tracker  = tracker;
+                scsi_req_cancel_async(r, &notifier->notifier);
+                ret = false;
             }
         }
         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,36 @@ 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_new(VirtIOSCSICancelTracker, 1);
+            tracker->tmf_req   = req;
+            tracker->remaining = cancel_count;
+
+            QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
+                if (r->hba_private) {
+                    notifier           = g_new(VirtIOSCSICancelNotifier, 1);
+                    notifier->notifier.notify
+                                       = virtio_scsi_cancel_notify;
+                    notifier->tracker  = tracker;
+                    scsi_req_cancel_async(r, &notifier->notifier);
+                }
+            }
+            ret = false;
+        }
+
         break;
 
     case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
@@ -310,20 +376,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;
+    bool should_complete = true;
 
     if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
                 &type, sizeof(type)) < sizeof(type)) {
@@ -337,7 +405,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);
+            should_complete = virtio_scsi_do_tmf(s, req);
         }
 
     } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
@@ -350,7 +418,9 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
             req->resp.an.response = VIRTIO_SCSI_S_OK;
         }
     }
-    virtio_scsi_complete_req(req);
+    if (should_complete) {
+        virtio_scsi_complete_req(req);
+    }
 }
 
 static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
-- 
1.9.3

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

[Paolo Bonzini]

Il 24/09/2014 10:27, Fam Zheng ha scritto:
> v2: Address Paolo's comments:
>     - Drop scsi_req_abort.
>     - Split patch 1.
>     - Use NotifierList.
>     - Track the number of cancelled requests in virtio-scsi.
> 
> 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_canceled
>   scsi: Introduce scsi_req_cancel_async
>   virtio-scsi: Handle TMF request cancellation asynchronously
> 
>  hw/scsi/scsi-bus.c     | 48 +++++++++++++++++++----------
>  hw/scsi/scsi-disk.c    | 59 ++++++++++-------------------------
>  hw/scsi/scsi-generic.c | 37 ++++++----------------
>  hw/scsi/spapr_vscsi.c  | 11 +++++--
>  hw/scsi/virtio-scsi.c  | 84 +++++++++++++++++++++++++++++++++++++++++++++-----
>  include/hw/scsi/scsi.h |  5 ++-
>  6 files changed, 146 insertions(+), 98 deletions(-)
> 

I didn't review 5-7 yet, and they are more complicated so I'll start by
applying the nice cleanup part (1-4).

Thanks!

Paolo

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

[Paolo Bonzini]

Il 24/09/2014 10:27, 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_canceled we notify them.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

There are some remnants of the old implementation.  Apart from that it
looks good.

Paolo

> ---
>  hw/scsi/scsi-bus.c     | 31 ++++++++++++++++++++++++++++---
>  include/hw/scsi/scsi.h |  3 +++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index fca4a9e..c1b05a1 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -553,9 +553,10 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>      BusState *qbus = BUS(bus);
>      const int memset_off = offsetof(SCSIRequest, sense)
>                             + sizeof(req->sense);
> +    size_t reqops_size = reqops ? reqops->size : sizeof(SCSIRequest);
>  
> -    req = g_slice_alloc(reqops->size);
> -    memset((uint8_t *)req + memset_off, 0, reqops->size - memset_off);
> +    req = g_slice_alloc(reqops_size);
> +    memset((uint8_t *)req + memset_off, 0, reqops_size - memset_off);
>      req->refcount = 1;
>      req->bus = bus;
>      req->dev = d;

This is not needed anymore.

> @@ -566,6 +567,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;
>  }
> @@ -1600,7 +1602,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) {

Same here.

>              req->ops->free_req(req);
>          }
>          object_unref(OBJECT(req->dev));
> @@ -1725,9 +1727,32 @@ void scsi_req_canceled(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.
> + * @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.

@tmf_req doesn't exist anymore... :)

> + * @red:     The request to cancel.
> + * @tmf_req: The tmf request which cancels @req.
> + * */
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
> +{
> +    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> +    notifier_list_add(&req->cancel_notifiers, notifier);

Perhaps wrap with "if (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 0fa2a8a..6a051df 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_canceled(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 v2 7/7] virtio-scsi: Handle TMF request cancellation asynchronously

[Paolo Bonzini]

Il 24/09/2014 10:27, Fam Zheng ha scritto:
> 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index fa36e23..9bd7d8a 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;

What about putting "remaining" directly in VirtIOSCSIReq?

> +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_free(n->tracker);
> +    }
> +    g_free(n);
> +}
> +
> +/* 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)

Perhaps return 0/-EINPROGRESS so that it's easier to remember the
calling convention?

>  {
>      SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
>      SCSIRequest *r, *next;
>      BusChild *kid;
>      int target;
> +    bool ret = true;
> +    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_new(VirtIOSCSICancelNotifier, 1);

Slice allocator?

> +                notifier->notifier.notify
> +                                   = virtio_scsi_cancel_notify;
> +                tracker            = g_new(VirtIOSCSICancelTracker, 1);

Same here if you keep VirtIOSCSICancelTracker.

> +                tracker->tmf_req   = req;
> +                tracker->remaining = 1;
> +                notifier->tracker  = tracker;
> +                scsi_req_cancel_async(r, &notifier->notifier);
> +                ret = false;
>              }
>          }
>          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,36 @@ 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_new(VirtIOSCSICancelTracker, 1);

Same as above.

> +            tracker->tmf_req   = req;
> +            tracker->remaining = cancel_count;
> +
> +            QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> +                if (r->hba_private) {
> +                    notifier           = g_new(VirtIOSCSICancelNotifier, 1);

Same as above.

> +                    notifier->notifier.notify
> +                                       = virtio_scsi_cancel_notify;
> +                    notifier->tracker  = tracker;
> +                    scsi_req_cancel_async(r, &notifier->notifier);
> +                }
> +            }
> +            ret = false;
> +        }
> +
>          break;
>  
>      case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
> @@ -310,20 +376,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;
> +    bool should_complete = true;
>  
>      if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
>                  &type, sizeof(type)) < sizeof(type)) {
> @@ -337,7 +405,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);
> +            should_complete = virtio_scsi_do_tmf(s, req);
>          }
>  
>      } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
> @@ -350,7 +418,9 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
>              req->resp.an.response = VIRTIO_SCSI_S_OK;
>          }
>      }
> -    virtio_scsi_complete_req(req);
> +    if (should_complete) {
> +        virtio_scsi_complete_req(req);
> +    }
>  }
>  
>  static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> 

Very nice apart from these comments.

Paolo

Re: [Qemu-devel] [PATCH v2 5/7] scsi: Introduce scsi_req_canceled

[Paolo Bonzini]

Il 24/09/2014 10:27, Fam Zheng ha scritto:
> Let the aio cb do the clean up and notification job after scsi_req_cancel, in
> preparation for asynchronous cancellation.

What about renaming it to scsi_req_cancel_complete?

Paolo

> 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..fca4a9e 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_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);
> @@ -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..c029d8c 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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&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_canceled(&r->req);
>          goto done;
>      }
>  
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7e85047..50c98b0 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_canceled(&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..0fa2a8a 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_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 v2 7/7] virtio-scsi: Handle TMF request cancellation asynchronously

[Fam Zheng]

On Wed, 09/24 13:18, Paolo Bonzini wrote:
> Il 24/09/2014 10:27, Fam Zheng ha scritto:
> > 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 77 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index fa36e23..9bd7d8a 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;
> 
> What about putting "remaining" directly in VirtIOSCSIReq?

It's rarely used, so I preferred managing it here.

> 
> > +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_free(n->tracker);
> > +    }
> > +    g_free(n);
> > +}
> > +
> > +/* 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)
> 
> Perhaps return 0/-EINPROGRESS so that it's easier to remember the
> calling convention?

OK!

> 
> >  {
> >      SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
> >      SCSIRequest *r, *next;
> >      BusChild *kid;
> >      int target;
> > +    bool ret = true;
> > +    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_new(VirtIOSCSICancelNotifier, 1);
> 
> Slice allocator?

Cancellation is not in the fast path, but I can do it.

> 
> > +                notifier->notifier.notify
> > +                                   = virtio_scsi_cancel_notify;
> > +                tracker            = g_new(VirtIOSCSICancelTracker, 1);
> 
> Same here if you keep VirtIOSCSICancelTracker.
> 
> > +                tracker->tmf_req   = req;
> > +                tracker->remaining = 1;
> > +                notifier->tracker  = tracker;
> > +                scsi_req_cancel_async(r, &notifier->notifier);
> > +                ret = false;
> >              }
> >          }
> >          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,36 @@ 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_new(VirtIOSCSICancelTracker, 1);
> 
> Same as above.
> 
> > +            tracker->tmf_req   = req;
> > +            tracker->remaining = cancel_count;
> > +
> > +            QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> > +                if (r->hba_private) {
> > +                    notifier           = g_new(VirtIOSCSICancelNotifier, 1);
> 
> Same as above.
> 
> > +                    notifier->notifier.notify
> > +                                       = virtio_scsi_cancel_notify;
> > +                    notifier->tracker  = tracker;
> > +                    scsi_req_cancel_async(r, &notifier->notifier);
> > +                }
> > +            }
> > +            ret = false;
> > +        }
> > +
> >          break;
> >  
> >      case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
> > @@ -310,20 +376,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;
> > +    bool should_complete = true;
> >  
> >      if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
> >                  &type, sizeof(type)) < sizeof(type)) {
> > @@ -337,7 +405,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);
> > +            should_complete = virtio_scsi_do_tmf(s, req);
> >          }
> >  
> >      } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
> > @@ -350,7 +418,9 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
> >              req->resp.an.response = VIRTIO_SCSI_S_OK;
> >          }
> >      }
> > -    virtio_scsi_complete_req(req);
> > +    if (should_complete) {
> > +        virtio_scsi_complete_req(req);
> > +    }
> >  }
> >  
> >  static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > 
> 
> Very nice apart from these comments.

Thanks for reviewing!

Fam

Re: [Qemu-devel] [PATCH v2 7/7] virtio-scsi: Handle TMF request cancellation asynchronously

[Paolo Bonzini]

Il 25/09/2014 04:01, Fam Zheng ha scritto:
>>> > > -static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
>>> > > +typedef struct {
>>> > > +    VirtIOSCSIReq  *tmf_req;
>>> > > +    int            remaining;
>>> > > +} VirtIOSCSICancelTracker;
>> > 
>> > What about putting "remaining" directly in VirtIOSCSIReq?
> It's rarely used, so I preferred managing it here.
> 

It complicates the code though.  If you really feel like economizing
space, put it in a union with "QTAILQ_ENTRY(VirtIOSCSIReq) next;".

Paolo