[Qemu-devel] [PATCH 0/3] Fail migration on bdrv_flush_all() error

[Qemu-devel] [PATCH 0/3] Fail migration on bdrv_flush_all() error

[Kevin Wolf]

If bdrv_flush_all() returns an error, there is an inconsistency in the
view of an image file between the source and the destination host.
Completing the migration would lead to corruption. Better abort
migration in this case.

Kevin Wolf (3):
  block: Add return value for bdrv_flush_all()
  cpus: Add return value for vm_stop()
  migration: Fail migration on bdrv_flush_all() error

 block.c                 | 10 ++++++++--
 cpus.c                  | 20 +++++++++++++-------
 include/block/block.h   |  2 +-
 include/sysemu/sysemu.h |  4 ++--
 migration.c             | 17 ++++++++++++++---
 stubs/vm-stop.c         |  2 +-
 6 files changed, 39 insertions(+), 16 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] block: Add return value for bdrv_flush_all()

[Kevin Wolf]

bdrv_flush() can fail, and bdrv_flush_all() should return an error as
well if this happens for a block device. It returns the first error
return now, but still at least tries to flush the remaining devices even
in error cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 10 ++++++++--
 include/block/block.h |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 6c493ad..6abdd99 100644
--- a/block.c
+++ b/block.c
@@ -2902,13 +2902,19 @@ int bdrv_get_flags(BlockDriverState *bs)
     return bs->open_flags;
 }
 
-void bdrv_flush_all(void)
+int bdrv_flush_all(void)
 {
     BlockDriverState *bs;
+    int result = 0;
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
-        bdrv_flush(bs);
+        int ret = bdrv_flush(bs);
+        if (ret < 0 && !result) {
+            result = ret;
+        }
     }
+
+    return result;
 }
 
 int bdrv_has_zero_init_1(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index dd8eca1..31c1bbc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -266,7 +266,7 @@ void bdrv_clear_incoming_migration_all(void);
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
-void bdrv_flush_all(void);
+int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain_all(void);
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/3] cpus: Add return value for vm_stop()

[Kevin Wolf]

If flushing the block devices fails, return an error. The VM is stopped
anyway.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 cpus.c                  | 20 +++++++++++++-------
 include/sysemu/sysemu.h |  4 ++--
 stubs/vm-stop.c         |  2 +-
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/cpus.c b/cpus.c
index 20958e5..e15fdcb 100644
--- a/cpus.c
+++ b/cpus.c
@@ -435,17 +435,21 @@ bool cpu_is_stopped(CPUState *cpu)
     return !runstate_is_running() || cpu->stopped;
 }
 
-static void do_vm_stop(RunState state)
+static int do_vm_stop(RunState state)
 {
+    int ret = 0;
+
     if (runstate_is_running()) {
         cpu_disable_ticks();
         pause_all_vcpus();
         runstate_set(state);
         vm_state_notify(0, state);
         bdrv_drain_all();
-        bdrv_flush_all();
+        ret = bdrv_flush_all();
         monitor_protocol_event(QEVENT_STOP, NULL);
     }
+
+    return ret;
 }
 
 static bool cpu_can_run(CPUState *cpu)
@@ -1078,7 +1082,7 @@ void cpu_stop_current(void)
     }
 }
 
-void vm_stop(RunState state)
+int vm_stop(RunState state)
 {
     if (qemu_in_vcpu_thread()) {
         qemu_system_vmstop_request(state);
@@ -1087,19 +1091,21 @@ void vm_stop(RunState state)
          * vm_stop() has been requested.
          */
         cpu_stop_current();
-        return;
+        return 0;
     }
-    do_vm_stop(state);
+
+    return do_vm_stop(state);
 }
 
 /* does a state transition even if the VM is already stopped,
    current state is forgotten forever */
-void vm_stop_force_state(RunState state)
+int vm_stop_force_state(RunState state)
 {
     if (runstate_is_running()) {
-        vm_stop(state);
+        return vm_stop(state);
     } else {
         runstate_set(state);
+        return 0;
     }
 }
 
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 2fb71af..b5e1add 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -35,8 +35,8 @@ void vm_state_notify(int running, RunState state);
 #define VMRESET_REPORT   true
 
 void vm_start(void);
-void vm_stop(RunState state);
-void vm_stop_force_state(RunState state);
+int vm_stop(RunState state);
+int vm_stop_force_state(RunState state);
 
 typedef enum WakeupReason {
     QEMU_WAKEUP_REASON_OTHER = 0,
diff --git a/stubs/vm-stop.c b/stubs/vm-stop.c
index 4568935..f82c897 100644
--- a/stubs/vm-stop.c
+++ b/stubs/vm-stop.c
@@ -1,7 +1,7 @@
 #include "qemu-common.h"
 #include "sysemu/sysemu.h"
 
-void vm_stop(RunState state)
+int vm_stop(RunState state)
 {
     abort();
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/3] migration: Fail migration on bdrv_flush_all() error

[Kevin Wolf]

If bdrv_flush_all() returns an error, there is an inconsistency in the
view of an image file between the source and the destination host.
Completing the migration would lead to corruption. Better abort
migration in this case.

To reproduce this case, try the following (ensures that there is
something to flush, and then fails that flush):

$ qemu-img create -f qcow2 test.qcow2 1G
$ cat blkdebug.cfg
[inject-error]
event = "flush_to_os"
errno = "5"
$ qemu-system-x86_64 -hda blkdebug:blkdebug.cfg:test.qcow2 -monitor stdio
(qemu) qemu-io ide0-hd0 "write 0 4k"
(qemu) migrate ...

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 migration.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index a704d48..f7ae061 100644
--- a/migration.c
+++ b/migration.c
@@ -528,15 +528,26 @@ static void *migration_thread(void *opaque)
             if (pending_size && pending_size >= max_size) {
                 qemu_savevm_state_iterate(s->file);
             } else {
+                int ret;
+
                 DPRINTF("done iterating\n");
                 qemu_mutex_lock_iothread();
                 start_time = qemu_get_clock_ms(rt_clock);
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
                 old_vm_running = runstate_is_running();
-                vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-                qemu_file_set_rate_limit(s->file, INT_MAX);
-                qemu_savevm_state_complete(s->file);
+
+                ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+                if (ret >= 0) {
+                    qemu_file_set_rate_limit(s->file, INT_MAX);
+                    qemu_savevm_state_complete(s->file);
+                }
                 qemu_mutex_unlock_iothread();
+
+                if (ret < 0) {
+                    migrate_finish_set_state(s, MIG_STATE_ERROR);
+                    break;
+                }
+
                 if (!qemu_file_get_error(s->file)) {
                     migrate_finish_set_state(s, MIG_STATE_COMPLETED);
                     break;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/3] cpus: Add return value for vm_stop()

[Paolo Bonzini]

Il 05/07/2013 14:03, Kevin Wolf ha scritto:
> If flushing the block devices fails, return an error. The VM is stopped
> anyway.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  cpus.c                  | 20 +++++++++++++-------
>  include/sysemu/sysemu.h |  4 ++--
>  stubs/vm-stop.c         |  2 +-
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 20958e5..e15fdcb 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -435,17 +435,21 @@ bool cpu_is_stopped(CPUState *cpu)
>      return !runstate_is_running() || cpu->stopped;
>  }
>  
> -static void do_vm_stop(RunState state)
> +static int do_vm_stop(RunState state)
>  {
> +    int ret = 0;
> +
>      if (runstate_is_running()) {
>          cpu_disable_ticks();
>          pause_all_vcpus();
>          runstate_set(state);
>          vm_state_notify(0, state);
>          bdrv_drain_all();
> -        bdrv_flush_all();
> +        ret = bdrv_flush_all();
>          monitor_protocol_event(QEVENT_STOP, NULL);
>      }
> +
> +    return ret;
>  }
>  
>  static bool cpu_can_run(CPUState *cpu)
> @@ -1078,7 +1082,7 @@ void cpu_stop_current(void)
>      }
>  }
>  
> -void vm_stop(RunState state)
> +int vm_stop(RunState state)
>  {
>      if (qemu_in_vcpu_thread()) {
>          qemu_system_vmstop_request(state);
> @@ -1087,19 +1091,21 @@ void vm_stop(RunState state)
>           * vm_stop() has been requested.
>           */
>          cpu_stop_current();
> -        return;
> +        return 0;
>      }
> -    do_vm_stop(state);
> +
> +    return do_vm_stop(state);
>  }
>  
>  /* does a state transition even if the VM is already stopped,
>     current state is forgotten forever */
> -void vm_stop_force_state(RunState state)
> +int vm_stop_force_state(RunState state)
>  {
>      if (runstate_is_running()) {
> -        vm_stop(state);
> +        return vm_stop(state);
>      } else {
>          runstate_set(state);

I think you should add a bdrv_flush_all() here.  Otherwise, you could
migrate a stopped VM that has failed to flush (not that unlikely if the
VM was stopped due to ENOSPC, for example).

Overall these patches fix the problem and they are good, but I even
wonder if the failure to flush should change the runstate to "I/O error"
and trigger a monitor event.

In the end, the "runstate" design is showing some problems...  I found
this discussion: http://patchwork.ozlabs.org/patch/117606/ and if you
search for "Paolo Bonzini - Oct. 6, 2011, 11:14 a.m." you can find my
humble proposal.

Paolo

> +        return 0;
>      }
>  }
>  
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 2fb71af..b5e1add 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -35,8 +35,8 @@ void vm_state_notify(int running, RunState state);
>  #define VMRESET_REPORT   true
>  
>  void vm_start(void);
> -void vm_stop(RunState state);
> -void vm_stop_force_state(RunState state);
> +int vm_stop(RunState state);
> +int vm_stop_force_state(RunState state);
>  
>  typedef enum WakeupReason {
>      QEMU_WAKEUP_REASON_OTHER = 0,
> diff --git a/stubs/vm-stop.c b/stubs/vm-stop.c
> index 4568935..f82c897 100644
> --- a/stubs/vm-stop.c
> +++ b/stubs/vm-stop.c
> @@ -1,7 +1,7 @@
>  #include "qemu-common.h"
>  #include "sysemu/sysemu.h"
>  
> -void vm_stop(RunState state)
> +int vm_stop(RunState state)
>  {
>      abort();
>  }
> 

Re: [Qemu-devel] [PATCH 0/3] Fail migration on bdrv_flush_all() error

[Stefan Hajnoczi]

On Fri, Jul 05, 2013 at 02:03:50PM +0200, Kevin Wolf wrote:
> If bdrv_flush_all() returns an error, there is an inconsistency in the
> view of an image file between the source and the destination host.
> Completing the migration would lead to corruption. Better abort
> migration in this case.
> 
> Kevin Wolf (3):
>   block: Add return value for bdrv_flush_all()
>   cpus: Add return value for vm_stop()
>   migration: Fail migration on bdrv_flush_all() error
> 
>  block.c                 | 10 ++++++++--
>  cpus.c                  | 20 +++++++++++++-------
>  include/block/block.h   |  2 +-
>  include/sysemu/sysemu.h |  4 ++--
>  migration.c             | 17 ++++++++++++++---
>  stubs/vm-stop.c         |  2 +-
>  6 files changed, 39 insertions(+), 16 deletions(-)

Modulo Paolo's comments.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>