diff mbox

[v3,05/21] scsi: reference-count requests

Message ID 1305630067-2119-6-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 17, 2011, 11 a.m. UTC 
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.  One such access error in fact exists (it
is visible by testing the lsi driver with MALLOC_PERTURB_), and this
patch provides the infrastructure to fix it later.

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>
---
 hw/scsi-bus.c     |   22 ++++++++++++++++++++--
 hw/scsi-disk.c    |   20 +++++++++++++-------
 hw/scsi-generic.c |   23 ++++++++++++++++-------
 hw/scsi.h         |    5 +++++
 4 files changed, 54 insertions(+), 16 deletions(-)

Comments

Christoph Hellwig May 20, 2011, 3:58 p.m. UTC | #1 
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -136,6 +136,7 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
>      SCSIRequest *req;
>  
>      req = qemu_mallocz(size);
> +    req->refcount = 2;
>      req->bus = scsi_bus_from_device(d);
>      req->dev = d;
>      req->tag = tag;

A little comment explaining why we start out with a reference count of 2
would be useful here.  Might be worth making that a top of the function
block comment explaning the function a bit more while you're at it.

>  void scsi_req_free(SCSIRequest *req)
>  {
> -    scsi_req_dequeue(req);
> +    assert(req->refcount == 0);
>      qemu_free(req);
>  }

Is there any reason to keep a free function?  The pattern should be
that people just call the function to decrement the reference count,
and that frees the structure when it hits zero. In the current model
that would mean moving the freeing out of ->free_req into scsi_req_unref,
but that seems pretty sensible anyway.
Paolo Bonzini May 20, 2011, 5:48 p.m. UTC | #2 
On 05/20/2011 05:58 PM, Christoph Hellwig wrote:
>> >    void scsi_req_free(SCSIRequest *req)
>> >    {
>> >  -    scsi_req_dequeue(req);
>> >  +    assert(req->refcount == 0);
>> >        qemu_free(req);
>> >    }
> Is there any reason to keep a free function?

It's internal for SCSIDevice implementation, kind of a "base 
implementation" for free_req

> The pattern should be
> that people just call the function to decrement the reference count,
> and that frees the structure when it hits zero. In the current model
> that would mean moving the freeing out of ->free_req into scsi_req_unref,
> but that seems pretty sensible anyway.

free_req is still needed, because it takes care of freeing the bounce 
buffers or any other allocated data.

Paolo
Paolo Bonzini May 20, 2011, 6:03 p.m. UTC | #3 
On 05/20/2011 07:48 PM, Paolo Bonzini wrote:
>> Is there any reason to keep a free function?
>
> It's internal for SCSIDevice implementation, kind of a "base
> implementation" for free_req
>
>> The pattern should be
>> that people just call the function to decrement the reference count,
>> and that frees the structure when it hits zero. In the current model
>> that would mean moving the freeing out of ->free_req into scsi_req_unref,
>> but that seems pretty sensible anyway.
>
> free_req is still needed, because it takes care of freeing the bounce
> buffers or any other allocated data.

Nevermind, I see what you mean.  Will do.

Paolo
diff mbox

Patch

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 63d9a68..c0bc275 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -136,6 +136,7 @@  SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
     SCSIRequest *req;
 
     req = qemu_mallocz(size);
+    req->refcount = 2;
     req->bus = scsi_bus_from_device(d);
     req->dev = d;
     req->tag = tag;
@@ -159,21 +160,23 @@  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);
+    assert(req->refcount == 0);
     qemu_free(req);
 }
 
+
 static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
 {
     switch (cmd[0] >> 5) {
@@ -495,6 +498,19 @@  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) {
+        req->dev->info->free_req(req);
+    }
+}
+
 void scsi_req_data(SCSIRequest *req, int len)
 {
     trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
@@ -532,10 +548,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..ba7ffa1 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -95,8 +95,10 @@  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);
 }
@@ -131,7 +133,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 +146,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 +1031,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 +1093,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 +1180,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 +1192,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 +1210,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 +1293,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 3db734a..5739067 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -74,8 +74,10 @@  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);
 }
@@ -112,7 +114,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 +128,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 +321,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 +350,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 +377,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 +395,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 +474,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 +566,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 d1753f9..ecf1aeb 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);