Patchwork [10/22] migration: Refactor and simplify error checking in migrate_fd_put_ready

login
register
mail settings
Submitter Juan Quintela
Date Feb. 23, 2011, 12:44 a.m.
Message ID <92b9322a569eca234df0b64afb38f5f9cc4f2726.1298421307.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/84026/
State New
Headers show

Comments

Juan Quintela - Feb. 23, 2011, 12:44 a.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)
Yoshiaki Tamura - Feb. 23, 2011, 9:51 a.m.
2011/2/23 Juan Quintela <quintela@redhat.com>:
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index f015e02..641df9f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>
>     DPRINTF("iterate\n");
>     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
> -        int state;
>         int old_vm_running = vm_running;
>
>         DPRINTF("done iterating\n");
>         vm_stop(VMSTOP_MIGRATE);
>
> -        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
> -            if (old_vm_running) {
> -                vm_start();
> -            }
> -            state = MIG_STATE_ERROR;
> +        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
> +            migrate_fd_error(s);
>         } else {
> -            state = MIG_STATE_COMPLETED;
> +            if (migrate_fd_cleanup(s) < 0) {
> +                migrate_fd_error(s);
> +            } else {
> +                s->state = MIG_STATE_COMPLETED;
> +                notifier_list_notify(&migration_state_notifiers);
> +            }
>         }
> -        if (migrate_fd_cleanup(s) < 0) {
> +        if (s->get_status(s) != MIG_STATE_COMPLETED) {
>             if (old_vm_running) {
>                 vm_start();
>             }

This part, although it's not fair to ask you, but calling
vm_start when != MIG_STATE_COMPLETED terrifies me because just
failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
split brain between src/dst.  Although I haven't encountered this
situation, just having stopped VMs on both sides is safer.

Thanks,

Yoshi

> -            state = MIG_STATE_ERROR;
>         }
> -        s->state = state;
> -        notifier_list_notify(&migration_state_notifiers);
>     }
>  }
>
> --
> 1.7.4
>
>
>
Juan Quintela - Feb. 23, 2011, 10:08 a.m.
Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration.c |   20 +++++++++-----------
>>  1 files changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index f015e02..641df9f 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>>
>>     DPRINTF("iterate\n");
>>     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
>> -        int state;
>>         int old_vm_running = vm_running;
>>
>>         DPRINTF("done iterating\n");
>>         vm_stop(VMSTOP_MIGRATE);
>>
>> -        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
>> -            if (old_vm_running) {
>> -                vm_start();
>> -            }
>> -            state = MIG_STATE_ERROR;
>> +        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
>> +            migrate_fd_error(s);
>>         } else {
>> -            state = MIG_STATE_COMPLETED;
>> +            if (migrate_fd_cleanup(s) < 0) {
>> +                migrate_fd_error(s);
>> +            } else {
>> +                s->state = MIG_STATE_COMPLETED;
>> +                notifier_list_notify(&migration_state_notifiers);
>> +            }
>>         }
>> -        if (migrate_fd_cleanup(s) < 0) {
>> +        if (s->get_status(s) != MIG_STATE_COMPLETED) {
>>             if (old_vm_running) {
>>                 vm_start();
>>             }
>
> This part, although it's not fair to ask you, but calling
> vm_start when != MIG_STATE_COMPLETED terrifies me because just
> failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
> split brain between src/dst.  Although I haven't encountered this
> situation, just having stopped VMs on both sides is safer.

I see your pain. I am not happy at all, but this was integrated by
Anthony to fix this bug:

commit 41ef56e61153d7bd27d34a634633bb51b1c5988d
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Wed Jun 2 14:55:25 2010 -0500

    migration: respect exit status with exec:

 This fixes https://bugs.launchpad.net/qemu/+bug/391879


I think that it fixes that bug, but it makes me un-easy to restart vm if
there is a failure in migrate_fd_cleanup().  As I didn't wanted to
change behaviour with this series, I left it as it was.

Next on ToDo list is to do something sensible with errors, just now we
are not very good at handling them.

Later, Juan.
Yoshiaki Tamura - Feb. 23, 2011, 11:36 a.m.
2011/2/23 Juan Quintela <quintela@redhat.com>:
> Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>  migration.c |   20 +++++++++-----------
>>>  1 files changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/migration.c b/migration.c
>>> index f015e02..641df9f 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>>>
>>>     DPRINTF("iterate\n");
>>>     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
>>> -        int state;
>>>         int old_vm_running = vm_running;
>>>
>>>         DPRINTF("done iterating\n");
>>>         vm_stop(VMSTOP_MIGRATE);
>>>
>>> -        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
>>> -            if (old_vm_running) {
>>> -                vm_start();
>>> -            }
>>> -            state = MIG_STATE_ERROR;
>>> +        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
>>> +            migrate_fd_error(s);
>>>         } else {
>>> -            state = MIG_STATE_COMPLETED;
>>> +            if (migrate_fd_cleanup(s) < 0) {
>>> +                migrate_fd_error(s);
>>> +            } else {
>>> +                s->state = MIG_STATE_COMPLETED;
>>> +                notifier_list_notify(&migration_state_notifiers);
>>> +            }
>>>         }
>>> -        if (migrate_fd_cleanup(s) < 0) {
>>> +        if (s->get_status(s) != MIG_STATE_COMPLETED) {
>>>             if (old_vm_running) {
>>>                 vm_start();
>>>             }
>>
>> This part, although it's not fair to ask you, but calling
>> vm_start when != MIG_STATE_COMPLETED terrifies me because just
>> failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
>> split brain between src/dst.  Although I haven't encountered this
>> situation, just having stopped VMs on both sides is safer.
>
> I see your pain. I am not happy at all, but this was integrated by
> Anthony to fix this bug:
>
> commit 41ef56e61153d7bd27d34a634633bb51b1c5988d
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Wed Jun 2 14:55:25 2010 -0500
>
>    migration: respect exit status with exec:
>
>  This fixes https://bugs.launchpad.net/qemu/+bug/391879
>

Thanks for the link.  I don't know IIUC, why stopping the VM was
a problem?  The essential thing is that we need to introduce a
flag that whether user wants to continue a VM when something goes
wrong during live migration.  Deciding only with old_vm_running is
wrong.

> I think that it fixes that bug, but it makes me un-easy to restart vm if
> there is a failure in migrate_fd_cleanup().  As I didn't wanted to
> change behaviour with this series, I left it as it was.

I agree with keeping the behavior unchanged.

> Next on ToDo list is to do something sensible with errors, just now we
> are not very good at handling them.

Yeah.  If we introduce Kemari, the migration code becomes more
important because it'll be part of the normal VM execution
path :)

Thanks,

Yoshi

>
> Later, Juan.
>
>

Patch

diff --git a/migration.c b/migration.c
index f015e02..641df9f 100644
--- a/migration.c
+++ b/migration.c
@@ -361,28 +361,26 @@  static void migrate_fd_put_ready(void *opaque)

     DPRINTF("iterate\n");
     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
-        int state;
         int old_vm_running = vm_running;

         DPRINTF("done iterating\n");
         vm_stop(VMSTOP_MIGRATE);

-        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
-            if (old_vm_running) {
-                vm_start();
-            }
-            state = MIG_STATE_ERROR;
+        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
+            migrate_fd_error(s);
         } else {
-            state = MIG_STATE_COMPLETED;
+            if (migrate_fd_cleanup(s) < 0) {
+                migrate_fd_error(s);
+            } else {
+                s->state = MIG_STATE_COMPLETED;
+                notifier_list_notify(&migration_state_notifiers);
+            }
         }
-        if (migrate_fd_cleanup(s) < 0) {
+        if (s->get_status(s) != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
                 vm_start();
             }
-            state = MIG_STATE_ERROR;
         }
-        s->state = state;
-        notifier_list_notify(&migration_state_notifiers);
     }
 }