[Qemu-devel] [PATCH v4 05/24] scsi: reference-count requests

[Paolo Bonzini <pbonzini@redhat.com>]

With the next patch, a device may hold SCSIRequest for an indefinite
time.  Split a rather big patch, and protect against access errors,
by reference counting them.

There is some ugliness in scsi_send_command implementation due to
the need to unref the request when it fails.  This will go away
with the next patches, which move the unref'ing to the devices.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
---
 hw/scsi-bus.c     |   29 ++++++++++++++++++++++-------
 hw/scsi-disk.c    |   21 +++++++++++++--------
 hw/scsi-generic.c |   24 ++++++++++++++++--------
 hw/scsi.h         |    5 +++++
 4 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index f21704f..74b9a74 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -136,6 +136,8 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
     SCSIRequest *req;
 
     req = qemu_mallocz(size);
+    /* Two references: one is passed back to the HBA, one is in d->requests.  */
+    req->refcount = 2;
     req->bus = scsi_bus_from_device(d);
     req->dev = d;
     req->tag = tag;
@@ -159,21 +161,16 @@ SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag)
     return NULL;
 }
 
-static void scsi_req_dequeue(SCSIRequest *req)
+void scsi_req_dequeue(SCSIRequest *req)
 {
     trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
     if (req->enqueued) {
         QTAILQ_REMOVE(&req->dev->requests, req, next);
         req->enqueued = false;
+        scsi_req_unref(req);
     }
 }
 
-void scsi_req_free(SCSIRequest *req)
-{
-    scsi_req_dequeue(req);
-    qemu_free(req);
-}
-
 static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
 {
     switch (cmd[0] >> 5) {
@@ -495,6 +492,22 @@ static const char *scsi_command_name(uint8_t cmd)
     return names[cmd];
 }
 
+SCSIRequest *scsi_req_ref(SCSIRequest *req)
+{
+    req->refcount++;
+    return req;
+}
+
+void scsi_req_unref(SCSIRequest *req)
+{
+    if (--req->refcount == 0) {
+	if (req->dev->info->free_req) {
+            req->dev->info->free_req(req);
+	}
+        qemu_free(req);
+    }
+}
+
 /* Called by the devices when data is ready for the HBA.  The HBA should
    start a DMA operation to read or fill the device's data buffer.
    Once it completes, calling one of req->dev->info->read_data or
@@ -537,10 +550,12 @@ void scsi_req_print(SCSIRequest *req)
 void scsi_req_complete(SCSIRequest *req)
 {
     assert(req->status != -1);
+    scsi_req_ref(req);
     scsi_req_dequeue(req);
     req->bus->ops->complete(req->bus, SCSI_REASON_DONE,
                             req->tag,
                             req->status);
+    scsi_req_unref(req);
 }
 
 static char *scsibus_get_fw_dev_path(DeviceState *dev)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 2b5dc2a..d17f43e 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -95,10 +95,11 @@ static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
     return r;
 }
 
-static void scsi_remove_request(SCSIDiskReq *r)
+static void scsi_free_request(SCSIRequest *req)
 {
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+
     qemu_vfree(r->iov.iov_base);
-    scsi_req_free(&r->req);
 }
 
 static SCSIDiskReq *scsi_find_request(SCSIDiskState *s, uint32_t tag)
@@ -131,7 +132,6 @@ static void scsi_command_complete(SCSIDiskReq *r, int status, int sense)
             r->req.tag, status, sense);
     scsi_req_set_status(r, status, sense);
     scsi_req_complete(&r->req);
-    scsi_remove_request(r);
 }
 
 /* Cancel a pending data transfer.  */
@@ -145,7 +145,7 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
         if (r->req.aiocb)
             bdrv_aio_cancel(r->req.aiocb);
         r->req.aiocb = NULL;
-        scsi_remove_request(r);
+        scsi_req_dequeue(&r->req);
     }
 }
 
@@ -1030,7 +1030,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
                                  uint8_t *buf, int lun)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
-    uint32_t len;
+    int32_t len;
     int is_write;
     uint8_t command;
     uint8_t *outbuf;
@@ -1092,6 +1092,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     case REZERO_UNIT:
         rc = scsi_disk_emulate_command(r, outbuf);
         if (rc < 0) {
+            scsi_req_unref(&r->req);
             return 0;
         }
 
@@ -1178,9 +1179,11 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
         DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
     fail:
         scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+        scsi_req_unref(&r->req);
         return 0;
     illegal_lba:
         scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+        scsi_req_unref(&r->req);
         return 0;
     }
     if (r->sector_count == 0 && r->iov.iov_len == 0) {
@@ -1188,12 +1191,13 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     }
     len = r->sector_count * 512 + r->iov.iov_len;
     if (is_write) {
-        return -len;
+        len = -len;
     } else {
         if (!r->sector_count)
             r->sector_count = -1;
-        return len;
     }
+    scsi_req_unref(&r->req);
+    return len;
 }
 
 static void scsi_disk_purge_requests(SCSIDiskState *s)
@@ -1205,7 +1209,7 @@ static void scsi_disk_purge_requests(SCSIDiskState *s)
         if (r->req.aiocb) {
             bdrv_aio_cancel(r->req.aiocb);
         }
-        scsi_remove_request(r);
+        scsi_req_dequeue(&r->req);
     }
 }
 
@@ -1288,6 +1292,7 @@ static SCSIDeviceInfo scsi_disk_info = {
     .qdev.reset   = scsi_disk_reset,
     .init         = scsi_disk_initfn,
     .destroy      = scsi_destroy,
+    .free_req     = scsi_free_request,
     .send_command = scsi_send_command,
     .read_data    = scsi_read_data,
     .write_data   = scsi_write_data,
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index f09458b..7e4831c 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -74,10 +74,11 @@ static SCSIGenericReq *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lu
     return DO_UPCAST(SCSIGenericReq, req, req);
 }
 
-static void scsi_remove_request(SCSIGenericReq *r)
+static void scsi_free_request(SCSIRequest *req)
 {
+    SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
+
     qemu_free(r->buf);
-    scsi_req_free(&r->req);
 }
 
 static SCSIGenericReq *scsi_find_request(SCSIGenericState *s, uint32_t tag)
@@ -112,7 +113,6 @@ static void scsi_command_complete(void *opaque, int ret)
             r, r->req.tag, r->req.status);
 
     scsi_req_complete(&r->req);
-    scsi_remove_request(r);
 }
 
 /* Cancel a pending data transfer.  */
@@ -127,7 +127,7 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
         if (r->req.aiocb)
             bdrv_aio_cancel(r->req.aiocb);
         r->req.aiocb = NULL;
-        scsi_remove_request(r);
+        scsi_req_dequeue(&r->req);
     }
 }
 
@@ -320,6 +320,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     SCSIGenericReq *r;
     SCSIBus *bus;
     int ret;
+    int32_t len;
 
     if (cmd[0] != REQUEST_SENSE &&
         (lun != s->lun || (cmd[1] >> 5) != s->lun)) {
@@ -348,7 +349,8 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
 
     if (-1 == scsi_req_parse(&r->req, cmd)) {
         BADF("Unsupported command length, command %x\n", cmd[0]);
-        scsi_remove_request(r);
+        scsi_req_dequeue(&r->req);
+        scsi_req_unref(&r->req);
         return 0;
     }
     scsi_req_fixup(&r->req);
@@ -374,8 +376,10 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
         ret = execute_command(s->bs, r, SG_DXFER_NONE, scsi_command_complete);
         if (ret == -1) {
             scsi_command_complete(r, -EINVAL);
+            scsi_req_unref(&r->req);
             return 0;
         }
+        scsi_req_unref(&r->req);
         return 0;
     }
 
@@ -390,10 +394,13 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     r->len = r->req.cmd.xfer;
     if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
         r->len = 0;
-        return -r->req.cmd.xfer;
+        len = -r->req.cmd.xfer;
+    } else {
+        len = r->req.cmd.xfer;
     }
 
-    return r->req.cmd.xfer;
+    scsi_req_unref(&r->req);
+    return len;
 }
 
 static int get_blocksize(BlockDriverState *bdrv)
@@ -466,7 +473,7 @@ static void scsi_generic_purge_requests(SCSIGenericState *s)
         if (r->req.aiocb) {
             bdrv_aio_cancel(r->req.aiocb);
         }
-        scsi_remove_request(r);
+        scsi_req_dequeue(&r->req);
     }
 }
 
@@ -558,6 +565,7 @@ static SCSIDeviceInfo scsi_generic_info = {
     .qdev.reset   = scsi_generic_reset,
     .init         = scsi_generic_initfn,
     .destroy      = scsi_destroy,
+    .free_req     = scsi_free_request,
     .send_command = scsi_send_command,
     .read_data    = scsi_read_data,
     .write_data   = scsi_write_data,
diff --git a/hw/scsi.h b/hw/scsi.h
index 20cf397..7d97dfa 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -29,6 +29,7 @@ enum SCSIXferMode {
 typedef struct SCSIRequest {
     SCSIBus           *bus;
     SCSIDevice        *dev;
+    uint32_t          refcount;
     uint32_t          tag;
     uint32_t          lun;
     uint32_t          status;
@@ -65,6 +66,7 @@ struct SCSIDeviceInfo {
     DeviceInfo qdev;
     scsi_qdev_initfn init;
     void (*destroy)(SCSIDevice *s);
+    void (*free_req)(SCSIRequest *req);
     int32_t (*send_command)(SCSIDevice *s, uint32_t tag, uint8_t *buf,
                             int lun);
     void (*read_data)(SCSIDevice *s, uint32_t tag);
@@ -103,6 +105,9 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
 SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
 SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag);
 void scsi_req_free(SCSIRequest *req);
+void scsi_req_dequeue(SCSIRequest *req);
+SCSIRequest *scsi_req_ref(SCSIRequest *req);
+void scsi_req_unref(SCSIRequest *req);
 
 int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
 void scsi_req_print(SCSIRequest *req);
-- 
1.7.4.4