diff mbox

[1/4] scsi-disk: Implement rerror option

Message ID 1288019856-16649-2-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Oct. 25, 2010, 3:17 p.m. UTC 
This implements the rerror option for SCSI disks.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c     |    2 +-
 hw/scsi-disk.c |   91 ++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 63 insertions(+), 30 deletions(-)

Comments

Stefan Hajnoczi Oct. 28, 2010, 9:12 a.m. UTC | #1 
On Mon, Oct 25, 2010 at 05:17:33PM +0200, Kevin Wolf wrote:
> This implements the rerror option for SCSI disks.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c     |    2 +-
>  hw/scsi-disk.c |   91 ++++++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 63 insertions(+), 30 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ff7602b..160fa84 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -326,7 +326,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
>  
>      on_read_error = BLOCK_ERR_REPORT;
>      if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
> -        if (type != IF_IDE && type != IF_VIRTIO && type != IF_NONE) {
> +        if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
>              fprintf(stderr, "rerror is no supported by this format\n");

This is a good opportunity to fix the "is *no* supported" typo in the error message.

>              return NULL;
>          }
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 9628b39..55ba558 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -41,7 +41,10 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
>  #define SCSI_DMA_BUF_SIZE    131072
>  #define SCSI_MAX_INQUIRY_LEN 256
>  
> -#define SCSI_REQ_STATUS_RETRY 0x01
> +#define SCSI_REQ_STATUS_RETRY           0x01
> +#define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
> +#define SCSI_REQ_STATUS_RETRY_READ      0x00
> +#define SCSI_REQ_STATUS_RETRY_WRITE     0x02
>  
>  typedef struct SCSIDiskState SCSIDiskState;
>  
> @@ -70,6 +73,8 @@ struct SCSIDiskState
>      char *serial;
>  };
>  
> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
> +
>  static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
>          uint32_t lun)
>  {
> @@ -127,34 +132,30 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
>  static void scsi_read_complete(void * opaque, int ret)
>  {
>      SCSIDiskReq *r = (SCSIDiskReq *)opaque;
> +    int n;
>  
>      r->req.aiocb = NULL;
>  
>      if (ret) {
> -        DPRINTF("IO error\n");
> -        r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
> -        scsi_command_complete(r, CHECK_CONDITION, NO_SENSE);
> -        return;
> +        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
> +            return;
> +        }
>      }
> +
>      DPRINTF("Data ready tag=0x%x len=%zd\n", r->req.tag, r->iov.iov_len);
>  
> +    n = r->iov.iov_len / 512;
> +    r->sector += n;
> +    r->sector_count -= n;
>      r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->iov.iov_len);
>  }
>  
> -/* Read more data from scsi device into buffer.  */
> -static void scsi_read_data(SCSIDevice *d, uint32_t tag)
> +
> +static void scsi_read_request(SCSIDiskReq *r)
>  {
> -    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
> -    SCSIDiskReq *r;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>      uint32_t n;
>  
> -    r = scsi_find_request(s, tag);
> -    if (!r) {
> -        BADF("Bad read tag 0x%x\n", tag);
> -        /* ??? This is the wrong error.  */
> -        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
> -        return;
> -    }
>      if (r->sector_count == (uint32_t)-1) {
>          DPRINTF("Read buf_len=%zd\n", r->iov.iov_len);
>          r->sector_count = 0;
> @@ -177,14 +178,34 @@ static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>                                scsi_read_complete, r);
>      if (r->req.aiocb == NULL)
>          scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
> -    r->sector += n;
> -    r->sector_count -= n;
>  }
>  
> -static int scsi_handle_write_error(SCSIDiskReq *r, int error)
> +/* Read more data from scsi device into buffer.  */
> +static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>  {
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
> +    SCSIDiskReq *r;
> +
> +    r = scsi_find_request(s, tag);
> +    if (!r) {
> +        BADF("Bad read tag 0x%x\n", tag);
> +        /* ??? This is the wrong error.  */
> +        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
> +        return;
> +    }
> +
> +    if (r->req.aiocb) {
> +        BADF("Data transfer already in progress\n");
> +    }

Can this be triggered from the guest?  If yes, then we need to (optionally) cancel the request with an error and then return.  If no, then this should be an assert.

> +
> +    scsi_read_request(r);
> +}
> +
> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> +{
> +    int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> -    BlockErrorAction action = bdrv_get_on_error(s->bs, 0);
> +    BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
>  
>      if (action == BLOCK_ERR_IGNORE) {
>          bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
> @@ -193,10 +214,16 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int error)
>  
>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>              || action == BLOCK_ERR_STOP_ANY) {
> -        r->status |= SCSI_REQ_STATUS_RETRY;
> +
> +        type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
> +        r->status |= SCSI_REQ_STATUS_RETRY | type;
> +
>          bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
>          vm_stop(0);
>      } else {
> +        if (type == SCSI_REQ_STATUS_RETRY_READ) {
> +            r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
> +        }
>          scsi_command_complete(r, CHECK_CONDITION,
>                  HARDWARE_ERROR);
>          bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);

We don't report whether this was a read or a write here?

Stefan
Kevin Wolf Oct. 28, 2010, 9:29 a.m. UTC | #2 
Am 28.10.2010 11:12, schrieb Stefan Hajnoczi:
> On Mon, Oct 25, 2010 at 05:17:33PM +0200, Kevin Wolf wrote:
>> This implements the rerror option for SCSI disks.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  blockdev.c     |    2 +-
>>  hw/scsi-disk.c |   91 ++++++++++++++++++++++++++++++++++++++------------------
>>  2 files changed, 63 insertions(+), 30 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index ff7602b..160fa84 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -326,7 +326,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
>>  
>>      on_read_error = BLOCK_ERR_REPORT;
>>      if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
>> -        if (type != IF_IDE && type != IF_VIRTIO && type != IF_NONE) {
>> +        if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
>>              fprintf(stderr, "rerror is no supported by this format\n");
> 
> This is a good opportunity to fix the "is *no* supported" typo in the error message.
>>              return NULL;
>>          }
>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>> index 9628b39..55ba558 100644
>> --- a/hw/scsi-disk.c
>> +++ b/hw/scsi-disk.c
>> @@ -41,7 +41,10 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
>>  #define SCSI_DMA_BUF_SIZE    131072
>>  #define SCSI_MAX_INQUIRY_LEN 256
>>  
>> -#define SCSI_REQ_STATUS_RETRY 0x01
>> +#define SCSI_REQ_STATUS_RETRY           0x01
>> +#define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
>> +#define SCSI_REQ_STATUS_RETRY_READ      0x00
>> +#define SCSI_REQ_STATUS_RETRY_WRITE     0x02
>>  
>>  typedef struct SCSIDiskState SCSIDiskState;
>>  
>> @@ -70,6 +73,8 @@ struct SCSIDiskState
>>      char *serial;
>>  };
>>  
>> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
>> +
>>  static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
>>          uint32_t lun)
>>  {
>> @@ -127,34 +132,30 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
>>  static void scsi_read_complete(void * opaque, int ret)
>>  {
>>      SCSIDiskReq *r = (SCSIDiskReq *)opaque;
>> +    int n;
>>  
>>      r->req.aiocb = NULL;
>>  
>>      if (ret) {
>> -        DPRINTF("IO error\n");
>> -        r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
>> -        scsi_command_complete(r, CHECK_CONDITION, NO_SENSE);
>> -        return;
>> +        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
>> +            return;
>> +        }
>>      }
>> +
>>      DPRINTF("Data ready tag=0x%x len=%zd\n", r->req.tag, r->iov.iov_len);
>>  
>> +    n = r->iov.iov_len / 512;
>> +    r->sector += n;
>> +    r->sector_count -= n;
>>      r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->iov.iov_len);
>>  }
>>  
>> -/* Read more data from scsi device into buffer.  */
>> -static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>> +
>> +static void scsi_read_request(SCSIDiskReq *r)
>>  {
>> -    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
>> -    SCSIDiskReq *r;
>> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>>      uint32_t n;
>>  
>> -    r = scsi_find_request(s, tag);
>> -    if (!r) {
>> -        BADF("Bad read tag 0x%x\n", tag);
>> -        /* ??? This is the wrong error.  */
>> -        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
>> -        return;
>> -    }
>>      if (r->sector_count == (uint32_t)-1) {
>>          DPRINTF("Read buf_len=%zd\n", r->iov.iov_len);
>>          r->sector_count = 0;
>> @@ -177,14 +178,34 @@ static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>>                                scsi_read_complete, r);
>>      if (r->req.aiocb == NULL)
>>          scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
>> -    r->sector += n;
>> -    r->sector_count -= n;
>>  }
>>  
>> -static int scsi_handle_write_error(SCSIDiskReq *r, int error)
>> +/* Read more data from scsi device into buffer.  */
>> +static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>>  {
>> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
>> +    SCSIDiskReq *r;
>> +
>> +    r = scsi_find_request(s, tag);
>> +    if (!r) {
>> +        BADF("Bad read tag 0x%x\n", tag);
>> +        /* ??? This is the wrong error.  */
>> +        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
>> +        return;
>> +    }
>> +
>> +    if (r->req.aiocb) {
>> +        BADF("Data transfer already in progress\n");
>> +    }
> 
> Can this be triggered from the guest?  If yes, then we need to (optionally) cancel the request with an error and then return.  If no, then this should be an assert.

Honestly, I don't know for sure. It's just the same as what we're doing
in scsi_write_disk.

But after a look at the callers, I think an assert should work.

>> +
>> +    scsi_read_request(r);
>> +}
>> +
>> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>> +{
>> +    int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
>>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> -    BlockErrorAction action = bdrv_get_on_error(s->bs, 0);
>> +    BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
>>  
>>      if (action == BLOCK_ERR_IGNORE) {
>>          bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
>> @@ -193,10 +214,16 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int error)
>>  
>>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>>              || action == BLOCK_ERR_STOP_ANY) {
>> -        r->status |= SCSI_REQ_STATUS_RETRY;
>> +
>> +        type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
>> +        r->status |= SCSI_REQ_STATUS_RETRY | type;
>> +
>>          bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
>>          vm_stop(0);
>>      } else {
>> +        if (type == SCSI_REQ_STATUS_RETRY_READ) {
>> +            r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
>> +        }
>>          scsi_command_complete(r, CHECK_CONDITION,
>>                  HARDWARE_ERROR);
>>          bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);
> 
> We don't report whether this was a read or a write here?

Whoops, good catch.

Kevin
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index ff7602b..160fa84 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -326,7 +326,7 @@  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     on_read_error = BLOCK_ERR_REPORT;
     if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
-        if (type != IF_IDE && type != IF_VIRTIO && type != IF_NONE) {
+        if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
             fprintf(stderr, "rerror is no supported by this format\n");
             return NULL;
         }
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 9628b39..55ba558 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -41,7 +41,10 @@  do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #define SCSI_DMA_BUF_SIZE    131072
 #define SCSI_MAX_INQUIRY_LEN 256
 
-#define SCSI_REQ_STATUS_RETRY 0x01
+#define SCSI_REQ_STATUS_RETRY           0x01
+#define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
+#define SCSI_REQ_STATUS_RETRY_READ      0x00
+#define SCSI_REQ_STATUS_RETRY_WRITE     0x02
 
 typedef struct SCSIDiskState SCSIDiskState;
 
@@ -70,6 +73,8 @@  struct SCSIDiskState
     char *serial;
 };
 
+static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
+
 static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
         uint32_t lun)
 {
@@ -127,34 +132,30 @@  static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
 static void scsi_read_complete(void * opaque, int ret)
 {
     SCSIDiskReq *r = (SCSIDiskReq *)opaque;
+    int n;
 
     r->req.aiocb = NULL;
 
     if (ret) {
-        DPRINTF("IO error\n");
-        r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
-        scsi_command_complete(r, CHECK_CONDITION, NO_SENSE);
-        return;
+        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
+            return;
+        }
     }
+
     DPRINTF("Data ready tag=0x%x len=%zd\n", r->req.tag, r->iov.iov_len);
 
+    n = r->iov.iov_len / 512;
+    r->sector += n;
+    r->sector_count -= n;
     r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->iov.iov_len);
 }
 
-/* Read more data from scsi device into buffer.  */
-static void scsi_read_data(SCSIDevice *d, uint32_t tag)
+
+static void scsi_read_request(SCSIDiskReq *r)
 {
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
-    SCSIDiskReq *r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     uint32_t n;
 
-    r = scsi_find_request(s, tag);
-    if (!r) {
-        BADF("Bad read tag 0x%x\n", tag);
-        /* ??? This is the wrong error.  */
-        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
-        return;
-    }
     if (r->sector_count == (uint32_t)-1) {
         DPRINTF("Read buf_len=%zd\n", r->iov.iov_len);
         r->sector_count = 0;
@@ -177,14 +178,34 @@  static void scsi_read_data(SCSIDevice *d, uint32_t tag)
                               scsi_read_complete, r);
     if (r->req.aiocb == NULL)
         scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
-    r->sector += n;
-    r->sector_count -= n;
 }
 
-static int scsi_handle_write_error(SCSIDiskReq *r, int error)
+/* Read more data from scsi device into buffer.  */
+static void scsi_read_data(SCSIDevice *d, uint32_t tag)
 {
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
+    SCSIDiskReq *r;
+
+    r = scsi_find_request(s, tag);
+    if (!r) {
+        BADF("Bad read tag 0x%x\n", tag);
+        /* ??? This is the wrong error.  */
+        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+        return;
+    }
+
+    if (r->req.aiocb) {
+        BADF("Data transfer already in progress\n");
+    }
+
+    scsi_read_request(r);
+}
+
+static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
+{
+    int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-    BlockErrorAction action = bdrv_get_on_error(s->bs, 0);
+    BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
 
     if (action == BLOCK_ERR_IGNORE) {
         bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
@@ -193,10 +214,16 @@  static int scsi_handle_write_error(SCSIDiskReq *r, int error)
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
-        r->status |= SCSI_REQ_STATUS_RETRY;
+
+        type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
+        r->status |= SCSI_REQ_STATUS_RETRY | type;
+
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
         vm_stop(0);
     } else {
+        if (type == SCSI_REQ_STATUS_RETRY_READ) {
+            r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
+        }
         scsi_command_complete(r, CHECK_CONDITION,
                 HARDWARE_ERROR);
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);
@@ -214,8 +241,9 @@  static void scsi_write_complete(void * opaque, int ret)
     r->req.aiocb = NULL;
 
     if (ret) {
-        if (scsi_handle_write_error(r, -ret))
+        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_WRITE)) {
             return;
+        }
     }
 
     n = r->iov.iov_len / 512;
@@ -288,8 +316,18 @@  static void scsi_dma_restart_bh(void *opaque)
     QTAILQ_FOREACH(req, &s->qdev.requests, next) {
         r = DO_UPCAST(SCSIDiskReq, req, req);
         if (r->status & SCSI_REQ_STATUS_RETRY) {
-            r->status &= ~SCSI_REQ_STATUS_RETRY;
-            scsi_write_request(r); 
+            int status = r->status;
+            r->status &=
+                ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
+
+            switch (status & SCSI_REQ_STATUS_RETRY_TYPE_MASK) {
+            case SCSI_REQ_STATUS_RETRY_READ:
+                scsi_read_request(r);
+                break;
+            case SCSI_REQ_STATUS_RETRY_WRITE:
+                scsi_write_request(r);
+                break;
+            }
         }
     }
 }
@@ -1152,11 +1190,6 @@  static int scsi_disk_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
-        error_report("Device doesn't support drive option rerror");
-        return -1;
-    }
-
     if (!s->serial) {
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(s->bs);