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

[Fam Zheng]

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

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

It will also make implementing asynchronous cancellation easier.

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

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

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

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