Fail migration on bdrv_flush_all() error
cover Fail migration on bdrv_flush_all() error
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(-)
Signature · 1 line
1.8.1.4
PATCH 1/3 block: Add return value for bdrv_flush_all()
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.
--- block.c | 10 ++++++++-- include/block/block.h | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-)
block.c +8 -2
@@ -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)
include/block/block.h +1 -1
@@ -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 );
Signature · 1 line
1.8.1.4
PATCH 2/3 cpus: Add return value for vm_stop()
In reply to Kevin WolfIf flushing the block devices fails, return an error. The VM is stopped anyway.
--- cpus.c | 20 +++++++++++++------- include/sysemu/sysemu.h | 4 ++-- stubs/vm-stop.c | 2 +- 3 files changed, 16 insertions(+), 10 deletions(-)
cpus.c +13 -7
@@ -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 ; } }
include/sysemu/sysemu.h +2 -2
@@ -35,8 +35,8 @@ void vm_state_notify(int running, RunState state);#define VMRESET_REPORT truevoid 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,
stubs/vm-stop.c +1 -1
@@ -1,7 +1,7 @@#include "qemu-common.h" #include "sysemu/sysemu.h" -void vm_stop (RunState state) +int vm_stop (RunState state) {abort (); }
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 2/3] cpus: Add return value for vm_stop()
Il 05/07/2013 14:03, Kevin Wolf ha scritto:
Quoted 59 lines
If flushing the block devices fails, return an error. The VM is stopped anyway.
Signed-off-by Kevin Wolf <[email protected]>--- cpus.c | 20 +++++++++++++------- include/sysemu/sysemu.h | 4 ++-- stubs/vm-stop.c | 2 +- 3 files changed, 16 insertions(+), 10 deletions(-)cpus.c +10
-5@@ -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); }
cpus.c void vm_stop(RunState state) · line 1107
Quoted 10 lines
cpus.c +2
-2/* 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
Quoted 33 lines
cpus.c +1
-0+return 0 ; } }include/sysemu/sysemu.h +2
-2@@ -35,8 +35,8 @@ void vm_state_notify(int running, RunState state);#define VMRESET_REPORT truevoid 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,stubs/vm-stop.c +1
-1@@ -1,7 +1,7 @@#include "qemu-common.h" #include "sysemu/sysemu.h" -void vm_stop (RunState state) +int vm_stop (RunState state) {abort (); }
PATCH 3/3 migration: Fail migration on bdrv_flush_all() error
In reply to Kevin WolfIf 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 ...
--- migration.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
migration.c +14 -3
@@ -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 ;
Signature · 1 line
1.8.1.4
Quoted 10 lines · 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 forbdrv_flush_all()
cpus: Add return value forvm_stop()
migration: Fail migration onbdrv_flush_all()error
Quoted 7 lines
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.