[Qemu-devel] [PATCH 0/2] ide: Handle bdrv_aio_flush errors

[Qemu-devel] [PATCH 0/2] ide: Handle bdrv_aio_flush errors

[Kevin Wolf]

Until now, errors returned by bdrv_aio_flush were silently ignored, so that
from a guest point of view, FLUSH would never fail even if the data hasn't made
it to the host disk.

This applies the werror option also to flushes, so that by default errors are
reported back to the guest, and qemu can be configured to stop the VM instead.

Kevin Wolf (2):
  ide: Factor ide_flush_cache out
  ide: Handle flush failure

 hw/ide/core.c     |   24 +++++++++++++++++++-----
 hw/ide/internal.h |    3 ++-
 2 files changed, 21 insertions(+), 6 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/2] ide: Factor ide_flush_cache out

[Kevin Wolf]

The next patch reuses this code, so put it in its own function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5ccb09c..6d8606e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -801,6 +801,15 @@ static void ide_flush_cb(void *opaque, int ret)
     ide_set_irq(s->bus);
 }
 
+static void ide_flush_cache(IDEState *s)
+{
+    if (s->bs) {
+        bdrv_aio_flush(s->bs, ide_flush_cb, s);
+    } else {
+        ide_flush_cb(s, 0);
+    }
+}
+
 static inline void cpu_to_ube16(uint8_t *buf, int val)
 {
     buf[0] = val >> 8;
@@ -2031,10 +2040,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
         case WIN_FLUSH_CACHE:
         case WIN_FLUSH_CACHE_EXT:
-            if (s->bs)
-                bdrv_aio_flush(s->bs, ide_flush_cb, s);
-            else
-                ide_flush_cb(s, 0);
+            ide_flush_cache(s);
             break;
         case WIN_STANDBY:
         case WIN_STANDBY2:
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/2] ide: Handle flush failure

[Kevin Wolf]

Instead of always assuming success for bdrv_aio_flush, actually do something
with the error. This respects the werror option and accordingly ignores the
error, reports it to the guest or stops the VM and retries after cont.

Ignoring the error is trivial, obviously. For stopping the VM and retrying
later old code can be reused, but we need to introduce a new status for "retry
a flush". For reporting to the guest, fortunately the same action is required
as for a failed read/write (status = DRDY | ERR, error = ABRT), so this code
can be reused as well.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c     |   10 +++++++++-
 hw/ide/internal.h |    3 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 6d8606e..bc3e916 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -65,6 +65,7 @@ static void ide_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb);
 static void ide_dma_restart(IDEState *s, int is_read);
 static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret);
 static int ide_handle_rw_error(IDEState *s, int error, int op);
+static void ide_flush_cache(IDEState *s);
 
 static void padstr(char *str, const char *src, int len)
 {
@@ -688,6 +689,8 @@ static void ide_dma_restart_bh(void *opaque)
         } else {
             ide_sector_write(bmdma_active_if(bm));
         }
+    } else if (bm->status & BM_STATUS_RETRY_FLUSH) {
+        ide_flush_cache(bmdma_active_if(bm));
     }
 }
 
@@ -795,7 +798,12 @@ static void ide_flush_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
 
-    /* XXX: how do we signal I/O errors here? */
+    if (ret < 0) {
+        /* XXX: What sector number to set here? */
+        if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
+            return;
+        }
+    }
 
     s->status = READY_STAT | SEEK_STAT;
     ide_set_irq(s->bus);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 4165543..d652e06 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -472,7 +472,8 @@ struct IDEDeviceInfo {
 #define BM_STATUS_INT    0x04
 #define BM_STATUS_DMA_RETRY  0x08
 #define BM_STATUS_PIO_RETRY  0x10
-#define BM_STATUS_RETRY_READ 0x20
+#define BM_STATUS_RETRY_READ  0x20
+#define BM_STATUS_RETRY_FLUSH 0x40
 
 #define BM_CMD_START     0x01
 #define BM_CMD_READ      0x08
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/2] ide: Factor ide_flush_cache out

[Christoph Hellwig]

On Tue, Oct 19, 2010 at 05:08:51PM +0200, Kevin Wolf wrote:
> The next patch reuses this code, so put it in its own function.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

ACK

Re: [Qemu-devel] [PATCH 2/2] ide: Handle flush failure

[Christoph Hellwig]

On Tue, Oct 19, 2010 at 05:08:52PM +0200, Kevin Wolf wrote:
> Instead of always assuming success for bdrv_aio_flush, actually do something
> with the error. This respects the werror option and accordingly ignores the
> error, reports it to the guest or stops the VM and retries after cont.
> 
> Ignoring the error is trivial, obviously. For stopping the VM and retrying
> later old code can be reused, but we need to introduce a new status for "retry
> a flush". For reporting to the guest, fortunately the same action is required
> as for a failed read/write (status = DRDY | ERR, error = ABRT), so this code
> can be reused as well.

ACK