[Qemu-devel] [PATCH 0/4] scsi-disk: Complete rerror/werror support

[Qemu-devel] [PATCH 0/4] scsi-disk: Complete rerror/werror support

[Kevin Wolf]

SCSI disks currently only support werror for write requests. This series adds
rerror for failed read requests and respects werror also for flushes.

Kevin Wolf (4):
  scsi-disk: Implement rerror option
  block: Allow bdrv_flush to return errors
  scsi-disk: Complete failed requests in scsi_disk_emulate_command
  scsi-disk: Implement werror for flushes

 block.c           |   21 +++++++--
 block.h           |    2 +-
 block/blkdebug.c  |    4 +-
 block/blkverify.c |    4 +-
 block/cow.c       |    4 +-
 block/qcow.c      |    4 +-
 block/qcow2.c     |    4 +-
 block/raw-posix.c |    4 +-
 block/raw-win32.c |    9 +++-
 block/raw.c       |    4 +-
 block/vdi.c       |    4 +-
 block/vmdk.c      |    4 +-
 block_int.h       |    2 +-
 blockdev.c        |    2 +-
 hw/scsi-disk.c    |  129 ++++++++++++++++++++++++++++++++++++-----------------
 15 files changed, 134 insertions(+), 67 deletions(-)

-- 
1.7.2.3

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

[Kevin Wolf]

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");
             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);
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/4] block: Allow bdrv_flush to return errors

[Kevin Wolf]

This changes bdrv_flush to return 0 on success and -errno in case of failure.
It's a requirement for implementing proper error handle in users of bdrv_flush.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c           |   21 +++++++++++++++++----
 block.h           |    2 +-
 block/blkdebug.c  |    4 ++--
 block/blkverify.c |    4 ++--
 block/cow.c       |    4 ++--
 block/qcow.c      |    4 ++--
 block/qcow2.c     |    4 ++--
 block/raw-posix.c |    4 ++--
 block/raw-win32.c |    9 ++++++++-
 block/raw.c       |    4 ++--
 block/vdi.c       |    4 ++--
 block/vmdk.c      |    4 ++--
 block_int.h       |    2 +-
 13 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/block.c b/block.c
index 985d0b7..6b505fb 100644
--- a/block.c
+++ b/block.c
@@ -1453,14 +1453,27 @@ const char *bdrv_get_device_name(BlockDriverState *bs)
     return bs->device_name;
 }
 
-void bdrv_flush(BlockDriverState *bs)
+int bdrv_flush(BlockDriverState *bs)
 {
     if (bs->open_flags & BDRV_O_NO_FLUSH) {
-        return;
+        return 0;
+    }
+
+    if (bs->drv && bs->drv->bdrv_flush) {
+        return bs->drv->bdrv_flush(bs);
     }
 
-    if (bs->drv && bs->drv->bdrv_flush)
-        bs->drv->bdrv_flush(bs);
+    /*
+     * Some block drivers always operate in either writethrough or unsafe mode
+     * and don't support bdrv_flush therefore. Usually qemu doesn't know how
+     * the server works (because the behaviour is hardcoded or depends on
+     * server-side configuration), so we can't ensure that everything is safe
+     * on disk. Returning an error doesn't work because that would break guests
+     * even if the server operates in writethrough mode.
+     *
+     * Let's hope the user knows what he's doing.
+     */
+    return 0;
 }
 
 void bdrv_flush_all(void)
diff --git a/block.h b/block.h
index a4facf2..78ecfac 100644
--- a/block.h
+++ b/block.h
@@ -142,7 +142,7 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque);
 
 /* Ensure contents are flushed to disk.  */
-void bdrv_flush(BlockDriverState *bs);
+int bdrv_flush(BlockDriverState *bs);
 void bdrv_flush_all(void);
 void bdrv_close_all(void);
 
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 4d6ff0a..cd9eb80 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -397,9 +397,9 @@ static void blkdebug_close(BlockDriverState *bs)
     }
 }
 
-static void blkdebug_flush(BlockDriverState *bs)
+static int blkdebug_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
diff --git a/block/blkverify.c b/block/blkverify.c
index b2a12fe..0a8d691 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,12 +116,12 @@ static void blkverify_close(BlockDriverState *bs)
     s->test_file = NULL;
 }
 
-static void blkverify_flush(BlockDriverState *bs)
+static int blkverify_flush(BlockDriverState *bs)
 {
     BDRVBlkverifyState *s = bs->opaque;
 
     /* Only flush test file, the raw file is not important */
-    bdrv_flush(s->test_file);
+    return bdrv_flush(s->test_file);
 }
 
 static int64_t blkverify_getlength(BlockDriverState *bs)
diff --git a/block/cow.c b/block/cow.c
index eedcc48..4cf543c 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -282,9 +282,9 @@ exit:
     return ret;
 }
 
-static void cow_flush(BlockDriverState *bs)
+static int cow_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static QEMUOptionParameter cow_create_options[] = {
diff --git a/block/qcow.c b/block/qcow.c
index 816103d..9cd547d 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -910,9 +910,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
+static int qcow_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *qcow_aio_flush(BlockDriverState *bs,
diff --git a/block/qcow2.c b/block/qcow2.c
index b816d87..537c479 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1148,9 +1148,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
+static int qcow_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *qcow_aio_flush(BlockDriverState *bs,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index d0393e0..d0960b8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -734,10 +734,10 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     return result;
 }
 
-static void raw_flush(BlockDriverState *bs)
+static int raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    qemu_fdatasync(s->fd);
+    return qemu_fdatasync(s->fd);
 }
 
 
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 503ed39..7f32778 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -150,7 +150,14 @@ static int raw_write(BlockDriverState *bs, int64_t sector_num,
 static void raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    FlushFileBuffers(s->hfile);
+    int ret;
+
+    ret = FlushFileBuffers(s->hfile);
+    if (ret != 0) {
+        return -EIO;
+    }
+
+    return 0;
 }
 
 static void raw_close(BlockDriverState *bs)
diff --git a/block/raw.c b/block/raw.c
index 9108779..1980deb 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -39,9 +39,9 @@ static void raw_close(BlockDriverState *bs)
 {
 }
 
-static void raw_flush(BlockDriverState *bs)
+static int raw_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *raw_aio_flush(BlockDriverState *bs,
diff --git a/block/vdi.c b/block/vdi.c
index f72633c..3b51e53 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -900,10 +900,10 @@ static void vdi_close(BlockDriverState *bs)
 {
 }
 
-static void vdi_flush(BlockDriverState *bs)
+static int vdi_flush(BlockDriverState *bs)
 {
     logout("\n");
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 2d4ba42..872aeba 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -823,9 +823,9 @@ static void vmdk_close(BlockDriverState *bs)
     qemu_free(s->l2_cache);
 }
 
-static void vmdk_flush(BlockDriverState *bs)
+static int vmdk_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 
diff --git a/block_int.h b/block_int.h
index 87e60b8..3c3adb5 100644
--- a/block_int.h
+++ b/block_int.h
@@ -59,7 +59,7 @@ struct BlockDriver {
                       const uint8_t *buf, int nb_sectors);
     void (*bdrv_close)(BlockDriverState *bs);
     int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
-    void (*bdrv_flush)(BlockDriverState *bs);
+    int (*bdrv_flush)(BlockDriverState *bs);
     int (*bdrv_is_allocated)(BlockDriverState *bs, int64_t sector_num,
                              int nb_sectors, int *pnum);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/4] scsi-disk: Complete failed requests in scsi_disk_emulate_command

[Kevin Wolf]

This pulls the request completion for error cases from the caller to
scsi_disk_emulate_command. This should not change semantics, but allows to
reuse scsi_handle_write_error() for flushes in the next patch.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi-disk.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 55ba558..7418f0b 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -785,8 +785,9 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
     return toclen;
 }
 
-static int scsi_disk_emulate_command(SCSIRequest *req, uint8_t *outbuf)
+static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
 {
+    SCSIRequest *req = &r->req;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint64_t nb_sectors;
     int buflen = 0;
@@ -944,12 +945,12 @@ static int scsi_disk_emulate_command(SCSIRequest *req, uint8_t *outbuf)
     return buflen;
 
 not_ready:
-    scsi_req_set_status(req, CHECK_CONDITION, NOT_READY);
-    return 0;
+    scsi_command_complete(r, CHECK_CONDITION, NOT_READY);
+    return -1;
 
 illegal_request:
-    scsi_req_set_status(req, CHECK_CONDITION, ILLEGAL_REQUEST);
-    return 0;
+    scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+    return -1;
 }
 
 /* Execute a scsi command.  Returns the length of the data expected by the
@@ -1057,14 +1058,12 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     case REPORT_LUNS:
     case VERIFY:
     case REZERO_UNIT:
-        rc = scsi_disk_emulate_command(&r->req, outbuf);
-        if (rc > 0) {
-            r->iov.iov_len = rc;
-        } else {
-            scsi_req_complete(&r->req);
-            scsi_remove_request(r);
+        rc = scsi_disk_emulate_command(r, outbuf);
+        if (rc < 0) {
             return 0;
         }
+
+        r->iov.iov_len = rc;
         break;
     case READ_6:
     case READ_10:
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/4] scsi-disk: Implement werror for flushes

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi-disk.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 7418f0b..a1976ea 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -45,6 +45,7 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
 #define SCSI_REQ_STATUS_RETRY_READ      0x00
 #define SCSI_REQ_STATUS_RETRY_WRITE     0x02
+#define SCSI_REQ_STATUS_RETRY_FLUSH     0x04
 
 typedef struct SCSIDiskState SCSIDiskState;
 
@@ -74,6 +75,7 @@ struct SCSIDiskState
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
+static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf);
 
 static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
         uint32_t lun)
@@ -317,6 +319,8 @@ static void scsi_dma_restart_bh(void *opaque)
         r = DO_UPCAST(SCSIDiskReq, req, req);
         if (r->status & SCSI_REQ_STATUS_RETRY) {
             int status = r->status;
+            int ret;
+
             r->status &=
                 ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
 
@@ -327,6 +331,11 @@ static void scsi_dma_restart_bh(void *opaque)
             case SCSI_REQ_STATUS_RETRY_WRITE:
                 scsi_write_request(r);
                 break;
+            case SCSI_REQ_STATUS_RETRY_FLUSH:
+                ret = scsi_disk_emulate_command(r, r->iov.iov_base);
+                if (ret == 0) {
+                    scsi_command_complete(r, GOOD, NO_SENSE);
+                }
             }
         }
     }
@@ -791,6 +800,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint64_t nb_sectors;
     int buflen = 0;
+    int ret;
 
     switch (req->cmd.buf[0]) {
     case TEST_UNIT_READY:
@@ -881,7 +891,12 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
         buflen = 8;
 	break;
     case SYNCHRONIZE_CACHE:
-        bdrv_flush(s->bs);
+        ret = bdrv_flush(s->bs);
+        if (ret < 0) {
+            if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
+                return -1;
+            }
+        }
         break;
     case GET_CONFIGURATION:
         memset(outbuf, 0, 8);
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/4] scsi-disk: Implement rerror option

[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.

> +
> +    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

Re: [Qemu-devel] [PATCH 2/4] block: Allow bdrv_flush to return errors

[Stefan Hajnoczi]

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 1/4] scsi-disk: Implement rerror option

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 3/4] scsi-disk: Complete failed requests in scsi_disk_emulate_command

[Stefan Hajnoczi]

On Mon, Oct 25, 2010 at 4:17 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> This pulls the request completion for error cases from the caller to
> scsi_disk_emulate_command. This should not change semantics, but allows to
> reuse scsi_handle_write_error() for flushes in the next patch.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/scsi-disk.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Stefan

Re: [Qemu-devel] [PATCH 4/4] scsi-disk: Implement werror for flushes

[Stefan Hajnoczi]

On Mon, Oct 25, 2010 at 4:17 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/scsi-disk.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Stefan