Patchwork [6/6] Continue a migrated vm is a bad idea

login
register
mail settings
Submitter Juan Quintela
Date Aug. 19, 2009, 2:07 a.m.
Message ID <927bb26f5425c58febd666db8fec67f55a229b5c.1250646771.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/31622/
State Superseded
Headers show

Comments

Juan Quintela - Aug. 19, 2009, 2:07 a.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    1 +
 monitor.c   |    6 ++++++
 sysemu.h    |    1 +
 vl.c        |    1 +
 4 files changed, 9 insertions(+), 0 deletions(-)
Avi Kivity - Aug. 20, 2009, 8:40 a.m.
On 08/19/2009 05:07 AM, Juan Quintela wrote:
> index ea5c33a..762a9a5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
>   {
>       struct bdrv_iterate_context context = { mon, 0 };
>
> +    if (unlikely(vm_has_migrated)) {
> +        monitor_printf(mon, "this vm has migrated to other machine\n");
> +        monitor_printf(mon, "You have to load other vm\n");
> +        return;
> +    }
>    

You don't know whether the guest is running on the other side.  It may 
be that the control program started the other side stopped, and after 
migration completes decides which side to continue running.
Juan Quintela - Aug. 20, 2009, 9:10 a.m.
Avi Kivity <avi@redhat.com> wrote:
> On 08/19/2009 05:07 AM, Juan Quintela wrote:
>> index ea5c33a..762a9a5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
>>   {
>>       struct bdrv_iterate_context context = { mon, 0 };
>>
>> +    if (unlikely(vm_has_migrated)) {
>> +        monitor_printf(mon, "this vm has migrated to other machine\n");
>> +        monitor_printf(mon, "You have to load other vm\n");
>> +        return;
>> +    }
>>    
>
> You don't know whether the guest is running on the other side.  It may
> be that the control program started the other side stopped, and after
> migration completes decides which side to continue running.

Only works if migration suceeded and was done in _non_ autostart node.
Have to debug this very bug.  User thought that he had done _nothing_
on the destination side, and that then stoping it, and continue in the
source side was a good idea.

He didn't understand why he had disk corruption, obviosly qemu had a
bug.  I haven't found a better approach to fix this bug.  If you have a
better idea of what to do in this case, I am all ears.

The reason that I created the patch is that if the user didn't got it
right, it gets disk corruption.

Later, Juan.
Avi Kivity - Aug. 20, 2009, 9:28 a.m.
On 08/20/2009 12:10 PM, Juan Quintela wrote:
> Avi Kivity<avi@redhat.com>  wrote:
>    
>> On 08/19/2009 05:07 AM, Juan Quintela wrote:
>>      
>>> index ea5c33a..762a9a5 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
>>>    {
>>>        struct bdrv_iterate_context context = { mon, 0 };
>>>
>>> +    if (unlikely(vm_has_migrated)) {
>>> +        monitor_printf(mon, "this vm has migrated to other machine\n");
>>> +        monitor_printf(mon, "You have to load other vm\n");
>>> +        return;
>>> +    }
>>>
>>>        
>> You don't know whether the guest is running on the other side.  It may
>> be that the control program started the other side stopped, and after
>> migration completes decides which side to continue running.
>>      
> Only works if migration suceeded and was done in _non_ autostart node.
>    

Well, you're disabling this method.

> Have to debug this very bug.  User thought that he had done _nothing_
> on the destination side, and that then stoping it, and continue in the
> source side was a good idea.
>
> He didn't understand why he had disk corruption, obviosly qemu had a
> bug.  I haven't found a better approach to fix this bug.  If you have a
> better idea of what to do in this case, I am all ears.
>
> The reason that I created the patch is that if the user didn't got it
> right, it gets disk corruption.
>    

There are tons of ways to cause disk corruption running multiple qemus.  
This only solves one of them.

I suggest taking locks on the disk images instead (and adding an option 
not to lock for raw images).

Patch

diff --git a/migration.c b/migration.c
index ee64d41..f91401e 100644
--- a/migration.c
+++ b/migration.c
@@ -275,6 +275,7 @@  void migrate_fd_put_ready(void *opaque)
             state = MIG_STATE_ERROR;
         } else {
             state = MIG_STATE_COMPLETED;
+            vm_has_migrated = 1;
         }
         migrate_fd_cleanup(s);
         s->state = state;
diff --git a/monitor.c b/monitor.c
index ea5c33a..762a9a5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -567,6 +567,11 @@  static void do_cont(Monitor *mon)
 {
     struct bdrv_iterate_context context = { mon, 0 };

+    if (unlikely(vm_has_migrated)) {
+        monitor_printf(mon, "this vm has migrated to other machine\n");
+        monitor_printf(mon, "You have to load other vm\n");
+        return;
+    }
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err)
@@ -1780,6 +1785,7 @@  static void do_loadvm(Monitor *mon, const char *name)
 {
     int saved_vm_running  = vm_running;

+    vm_has_migrated = 0;
     vm_stop(0);

     if (load_vmstate(mon, name) >= 0 && saved_vm_running)
diff --git a/sysemu.h b/sysemu.h
index e0338ce..7c3fdc1 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -18,6 +18,7 @@  extern const char *bios_name;
 char *qemu_find_file(int type, const char *name);

 extern int vm_running;
+extern int vm_has_migrated;
 extern const char *qemu_name;
 extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
diff --git a/vl.c b/vl.c
index bf2468e..d39295a 100644
--- a/vl.c
+++ b/vl.c
@@ -188,6 +188,7 @@  ram_addr_t ram_size;
 int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int vm_running;
+int vm_has_migrated;
 int autostart;
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */