[Qemu-devel] [PATCH 6/6] scsi-disk: handle io_canceled uniformly and correctly

[Paolo Bonzini <pbonzini@redhat.com>]

Always check it immediately after calling bdrv_acct_done, and
always do a "goto done" in case the "done" label has to free
some memory---as is the case for scsi_unmap_complete in the
previous patch.

This patch could fix problems that happen when a request is
split into multiple parts, and one of them is canceled.  Then
the next part is fired, but the HBA's cancellation callbacks have
fired already.  Whether this happens or not, depends on how the
block/ driver implements AIO cancellation.  It it does a simple
bdrv_drain_all() or similar, then it will not have a problem.
If it only cancels the given AIOCB, this scenario could happen.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 6c0ddff..4a0673c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -178,6 +178,9 @@ static void scsi_aio_complete(void *opaque, int ret)
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
     bdrv_acct_done(s->qdev.conf.bs, &r->acct);
+    if (r->req.io_canceled) {
+        goto done;
+    }
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret)) {
@@ -223,6 +226,10 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
 
+    if (r->req.io_canceled) {
+        goto done;
+    }
+
     if (scsi_is_cmd_fua(&r->req.cmd)) {
         bdrv_acct_start(s->qdev.conf.bs, &r->acct, 0, BDRV_ACCT_FLUSH);
         r->req.aiocb = bdrv_aio_flush(s->qdev.conf.bs, scsi_aio_complete, r);
@@ -230,6 +237,8 @@ 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);
     }
@@ -243,6 +252,9 @@ static void scsi_dma_complete(void *opaque, int ret)
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
     bdrv_acct_done(s->qdev.conf.bs, &r->acct);
+    if (r->req.io_canceled) {
+        goto done;
+    }
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret)) {
@@ -274,6 +286,9 @@ static void scsi_read_complete(void * opaque, int ret)
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
     bdrv_acct_done(s->qdev.conf.bs, &r->acct);
+    if (r->req.io_canceled) {
+        goto done;
+    }
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret)) {
@@ -305,6 +320,9 @@ static void scsi_do_read(void *opaque, int ret)
         r->req.aiocb = NULL;
         bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     }
+    if (r->req.io_canceled) {
+        goto done;
+    }
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret)) {
@@ -312,10 +330,6 @@ static void scsi_do_read(void *opaque, int ret)
         }
     }
 
-    if (r->req.io_canceled) {
-        return;
-    }
-
     /* The request is used as the AIO opaque value, so add a ref.  */
     scsi_req_ref(&r->req);
 
@@ -423,6 +437,9 @@ static void scsi_write_complete(void * opaque, int ret)
         r->req.aiocb = NULL;
         bdrv_acct_done(s->qdev.conf.bs, &r->acct);
     }
+    if (r->req.io_canceled) {
+        goto done;
+    }
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret)) {
-- 
1.8.1.2