[Qemu-devel] [PATCH] Make a separate accounting for block I/O error

[Mark Wu]

This patch excludes I/O requests ending with error from the
accounting of operations, bytes and time, and adds accounting
for errors separately.It could make the statistics more accurate
and record the number of I/O failure.

Signed-off-by: Mark Wu <address@hidden>
---
 block.c         |   30 +++++++++++++++++++++++-------
 block.h         |    2 +-
 block_int.h     |    1 +
 hw/ide/ahci.c   |    2 +-
 hw/ide/atapi.c  |    6 +++---
 hw/ide/core.c   |   12 ++++++++----
 hw/ide/macio.c  |    4 ++--
 hw/scsi-disk.c  |    6 +++---
 hw/virtio-blk.c |    6 +++---
 hw/xen_disk.c   |    2 +-
 qmp-commands.hx |   21 +++++++++++++++++++++
 11 files changed, 67 insertions(+), 25 deletions(-)

diff --git a/block.c b/block.c
index 061b4cc..95f2cd4 100644
--- a/block.c
+++ b/block.c
@@ -1969,6 +1969,9 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
                         " wr_total_time_ns=%" PRId64
                         " rd_total_time_ns=%" PRId64
                         " flush_total_time_ns=%" PRId64
+                        " rd_errors=%" PRId64
+                        " wr_errors=%" PRId64
+                        " flush_errors=%" PRId64
                         "\n",
                         qdict_get_int(qdict, "rd_bytes"),
                         qdict_get_int(qdict, "wr_bytes"),
@@ -1977,7 +1980,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
                         qdict_get_int(qdict, "flush_operations"),
                         qdict_get_int(qdict, "wr_total_time_ns"),
                         qdict_get_int(qdict, "rd_total_time_ns"),
-                        qdict_get_int(qdict, "flush_total_time_ns"));
+                        qdict_get_int(qdict, "flush_total_time_ns"),
+                        qdict_get_int(qdict, "rd_errors"),
+                        qdict_get_int(qdict, "wr_errors"),
+                        qdict_get_int(qdict, "flush_errors"));
 }
 
 void bdrv_stats_print(Monitor *mon, const QObject *data)
@@ -1999,7 +2005,10 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
                              "'flush_operations': %" PRId64 ","
                              "'wr_total_time_ns': %" PRId64 ","
                              "'rd_total_time_ns': %" PRId64 ","
-                             "'flush_total_time_ns': %" PRId64
+                             "'flush_total_time_ns': %" PRId64 ","
+                             "'rd_errors': %" PRId64 ","
+                             "'wr_errors': %" PRId64 ","
+                             "'flush_errors': %" PRId64
                              "} }",
                              bs->nr_bytes[BDRV_ACCT_READ],
                              bs->nr_bytes[BDRV_ACCT_WRITE],
@@ -2010,7 +2019,10 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
                              bs->nr_ops[BDRV_ACCT_FLUSH],
                              bs->total_time_ns[BDRV_ACCT_WRITE],
                              bs->total_time_ns[BDRV_ACCT_READ],
-                             bs->total_time_ns[BDRV_ACCT_FLUSH]);
+                             bs->total_time_ns[BDRV_ACCT_FLUSH],
+                             bs->nr_errors[BDRV_ACCT_READ],
+                             bs->nr_errors[BDRV_ACCT_WRITE],
+                             bs->nr_errors[BDRV_ACCT_FLUSH]);
     dict  = qobject_to_qdict(res);
 
     if (*bs->device_name) {
@@ -3250,13 +3262,17 @@ bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie 
*cookie, int64_t bytes,
 }
 
 void
-bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
+bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie, int error)
 {
     assert(cookie->type < BDRV_MAX_IOTYPE);
 
-    bs->nr_bytes[cookie->type] += cookie->bytes;
-    bs->nr_ops[cookie->type]++;
-    bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
+    if (error < 0) {
+        bs->nr_errors[cookie->type]++;
+    } else {
+        bs->nr_bytes[cookie->type] += cookie->bytes;
+        bs->nr_ops[cookie->type]++;
+        bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
+    }
 }
 
 int bdrv_img_create(const char *filename, const char *fmt,
diff --git a/block.h b/block.h
index e77988e..2f33aca 100644
--- a/block.h
+++ b/block.h
@@ -309,7 +309,7 @@ typedef struct BlockAcctCookie {
 
 void bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie,
         int64_t bytes, enum BlockAcctType type);
-void bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie);
+void bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie, int error);
 
 typedef enum {
     BLKDBG_L1_UPDATE,
diff --git a/block_int.h b/block_int.h
index f2f4f2d..8c1a3cb 100644
--- a/block_int.h
+++ b/block_int.h
@@ -183,6 +183,7 @@ struct BlockDriverState {
     /* I/O stats (display with "info blockstats"). */
     uint64_t nr_bytes[BDRV_MAX_IOTYPE];
     uint64_t nr_ops[BDRV_MAX_IOTYPE];
+    uint64_t nr_errors[BDRV_MAX_IOTYPE];
     uint64_t total_time_ns[BDRV_MAX_IOTYPE];
     uint64_t wr_highest_sector;
 
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1c7e3a0..1b8ee10 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -744,7 +744,7 @@ static void ncq_cb(void *opaque, int ret)
     DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n",
             ncq_tfs->tag);
 
-    bdrv_acct_done(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct);
+    bdrv_acct_done(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct, ret);
     qemu_sglist_destroy(&ncq_tfs->sglist);
     ncq_tfs->used = 0;
 }
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 3f909c3..1305515 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -112,12 +112,12 @@ static int cd_read_sector(IDEState *s, int lba, uint8_t 
*buf, int sector_size)
     case 2048:
         bdrv_acct_start(s->bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
         ret = bdrv_read(s->bs, (int64_t)lba << 2, buf, 4);
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(s->bs, &s->acct, ret);
         break;
     case 2352:
         bdrv_acct_start(s->bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
         ret = bdrv_read(s->bs, (int64_t)lba << 2, buf + 16, 4);
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(s->bs, &s->acct, ret);
         if (ret < 0)
             return ret;
         cd_data_to_raw(buf, lba);
@@ -361,7 +361,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 
     return;
 eot:
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->bs, &s->acct, ret);
     s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_INT);
     ide_set_inactive(s);
 }
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9ec1310..98922e4 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -477,7 +477,7 @@ void ide_sector_read(IDEState *s)
 
         bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
         ret = bdrv_read(s->bs, sector_num, s->io_buffer, n);
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(s->bs, &s->acct, ret);
         if (ret != 0) {
             if (ide_handle_rw_error(s, -ret,
                 BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ))
@@ -557,6 +557,10 @@ handle_rw_error:
         else if (s->dma_cmd == IDE_DMA_TRIM)
             op |= BM_STATUS_RETRY_TRIM;
 
+        if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
+            bdrv_acct_done(s->bs, &s->acct, ret);
+        }
+
         if (ide_handle_rw_error(s, -ret, op)) {
             return;
         }
@@ -616,7 +620,7 @@ handle_rw_error:
 
 eot:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(s->bs, &s->acct, ret);
     }
     ide_set_inactive(s);
 }
@@ -666,7 +670,7 @@ void ide_sector_write(IDEState *s)
 
     bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
     ret = bdrv_write(s->bs, sector_num, s->io_buffer, n);
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->bs, &s->acct, ret);
 
     if (ret != 0) {
         if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY))
@@ -703,6 +707,7 @@ static void ide_flush_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
 
+    bdrv_acct_done(s->bs, &s->acct, ret);
     if (ret < 0) {
         /* XXX: What sector number to set here? */
         if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
@@ -710,7 +715,6 @@ static void ide_flush_cb(void *opaque, int ret)
         }
     }
 
-    bdrv_acct_done(s->bs, &s->acct);
     s->status = READY_STAT | SEEK_STAT;
     ide_set_irq(s->bus);
 }
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 37b8239..11602d3 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -94,7 +94,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     return;
 
 done:
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->bs, &s->acct, ret);
     io->dma_end(opaque);
     return;
 }
@@ -165,7 +165,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     return;
 done:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(s->bs, &s->acct, ret);
     }
     io->dma_end(io);
 }
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a980a53..4545d93 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -132,7 +132,7 @@ static void scsi_read_complete(void * opaque, int ret)
 
     if (r->req.aiocb != NULL) {
         r->req.aiocb = NULL;
-        bdrv_acct_done(s->bs, &r->acct);
+        bdrv_acct_done(s->bs, &r->acct, ret);
     }
 
     if (ret) {
@@ -156,7 +156,7 @@ static void scsi_flush_complete(void * opaque, int ret)
 
     if (r->req.aiocb != NULL) {
         r->req.aiocb = NULL;
-        bdrv_acct_done(s->bs, &r->acct);
+        bdrv_acct_done(s->bs, &r->acct, ret);
     }
 
     if (ret < 0) {
@@ -254,7 +254,7 @@ static void scsi_write_complete(void * opaque, int ret)
 
     if (r->req.aiocb != NULL) {
         r->req.aiocb = NULL;
-        bdrv_acct_done(s->bs, &r->acct);
+        bdrv_acct_done(s->bs, &r->acct, ret);
     }
 
     if (ret) {
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index bd63a85..ca7dfa1 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -81,7 +81,6 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, 
int error,
         bdrv_iostatus_set_err(s->bs, error);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
-        bdrv_acct_done(s->bs, &req->acct);
         g_free(req);
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
@@ -94,6 +93,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     VirtIOBlockReq *req = opaque;
 
     trace_virtio_blk_rw_complete(req, ret);
+    bdrv_acct_done(req->dev->bs, &req->acct, ret);
 
     if (ret) {
         int is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
@@ -102,7 +102,6 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     }
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
-    bdrv_acct_done(req->dev->bs, &req->acct);
     g_free(req);
 }
 
@@ -110,6 +109,8 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
 {
     VirtIOBlockReq *req = opaque;
 
+    bdrv_acct_done(req->dev->bs, &req->acct, ret);
+
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, 0)) {
             return;
@@ -117,7 +118,6 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
     }
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
-    bdrv_acct_done(req->dev->bs, &req->acct);
     g_free(req);
 }
 
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 8a9fac4..d654694 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -402,7 +402,7 @@ static void qemu_aio_complete(void *opaque, int ret)
     ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
     ioreq_unmap(ioreq);
     ioreq_finish(ioreq);
-    bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
+    bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct, ret);
     qemu_bh_schedule(ioreq->blkdev->bh);
 }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 34cc353..3d298a0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1215,6 +1215,9 @@ Each json-object contain the following:
     - "flush_total_time_ns": total time spend on cache flushes in nano-seconds 
(json-int)
     - "wr_highest_offset": Highest offset of a sector written since the
                            BlockDriverState has been opened (json-int)
+    - "rd_errors": read errors (json-int)
+    - "wr_errors": write errors (json-int)
+    - "flush_errors": flush errors (json-int)
 - "parent": Contains recursively the statistics of the underlying
             protocol (e.g. the host file for a qcow2 image). If there is
             no underlying protocol, this field is omitted
@@ -1232,6 +1235,9 @@ Example:
                   "wr_highest_offset":3686448128,
                   "wr_bytes":9786368,
                   "wr_operations":751,
+                 "wr_errors":0,
+                 "rd_errors":0,
+                 "flush_errors":0,
                   "rd_bytes":122567168,
                   "rd_operations":36772
                   "wr_total_times_ns":313253456
@@ -1250,6 +1256,9 @@ Example:
                "wr_total_times_ns":313253456
                "rd_total_times_ns":3465673657
                "flush_total_times_ns":49653
+              "wr_errors":0,
+              "rd_errors":0,
+              "flush_errors":0,
             }
          },
          {
@@ -1258,12 +1267,18 @@ Example:
                "wr_highest_offset":0,
                "wr_bytes":0,
                "wr_operations":0,
+              "wr_errors":0,
+              "rd_errors":0,
+               "flush_errors":0,
                "rd_bytes":0,
                "rd_operations":0
                "flush_operations":0,
                "wr_total_times_ns":0
                "rd_total_times_ns":0
                "flush_total_times_ns":0
+              "wr_errors":0,
+              "rd_errors":0,
+              "flush_errors":0,
             }
          },
          {
@@ -1278,6 +1293,9 @@ Example:
                "wr_total_times_ns":0
                "rd_total_times_ns":0
                "flush_total_times_ns":0
+              "wr_errors":0,
+              "rd_errors":0,
+              "flush_errors":0,
             }
          },
          {
@@ -1292,6 +1310,9 @@ Example:
                "wr_total_times_ns":0
                "rd_total_times_ns":0
                "flush_total_times_ns":0
+               "wr_errors":0,
+              "rd_errors":0,
+              "flush_errors":0,
             }
          }
       ]
-- 
1.7.1