diff mbox

[1/1] vl: change QEMU state machine for system reset

Message ID 1453190362-2127-1-git-send-email-den@openvz.org (mailing list archive)
State New, archived
Headers show

Commit Message

Denis V. Lunev Jan. 19, 2016, 7:59 a.m. UTC 
This patch implements proposal from Paolo to handle system reset when
the guest is not running.

"After a reset, main_loop_should_exit should actually transition
to VM_STATE_PRELAUNCH (*not* RUN_STATE_PAUSED) for *all* states except
RUN_STATE_INMIGRATE, RUN_STATE_SAVE_VM (which I think cannot happen
there) and (of course) RUN_STATE_RUNNING."

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Dmitry Andreev <dandreev@virtuozzo.com>
---
 vl.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Jan. 21, 2016, 10:35 a.m. UTC | #1 
On 19/01/2016 08:59, Denis V. Lunev wrote:
> @@ -612,8 +617,10 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
> +    { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
>  
>      { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
> +    { RUN_STATE_RESTORE_VM, RUN_STATE_PRELAUNCH },
>  
>      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
>      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> @@ -627,20 +634,25 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>  
>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
> +    { RUN_STATE_SAVE_VM, RUN_STATE_PRELAUNCH },

This should not happen; thus I would remove this line.

>      { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
>      { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
> +    { RUN_STATE_SHUTDOWN, RUN_STATE_PRELAUNCH },
>  
>      { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
>      { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
>      { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
>      { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
> +    { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
>  
>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
> +    { RUN_STATE_WATCHDOG, RUN_STATE_PRELAUNCH },
>  
>      { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
>      { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PRELAUNCH },
>  
>      { RUN_STATE__MAX, RUN_STATE__MAX },
>  };
> @@ -1886,8 +1899,10 @@ static bool main_loop_should_exit(void)
>          cpu_synchronize_all_states();
>          qemu_system_reset(VMRESET_REPORT);
>          resume_all_vcpus();
> -        if (runstate_needs_reset()) {
> -            runstate_set(RUN_STATE_PAUSED);
> +        if (!runstate_check(RUN_STATE_RUNNING) &&
> +                !runstate_check(RUN_STATE_INMIGRATE) &&
> +                !runstate_check(RUN_STATE_SAVE_VM)) {

Since SAVE_VM should not happen here, I would leave this check out too.
 If it happens, the lack of a SAVE_VM->PRELAUNCH transition will cause
an assertion failure.

Thanks for the patch!

Paolo

> +            runstate_set(RUN_STATE_PRELAUNCH);
>          }
>      }
>      if (qemu_wakeup_requested()) {
>
Denis V. Lunev Jan. 21, 2016, 12:56 p.m. UTC | #2 
On 01/21/2016 01:35 PM, Paolo Bonzini wrote:
>
> On 19/01/2016 08:59, Denis V. Lunev wrote:
>> @@ -612,8 +617,10 @@ static const RunStateTransition runstate_transitions_def[] = {
>>   
>>       { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>>       { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
>> +    { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
>>   
>>       { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
>> +    { RUN_STATE_RESTORE_VM, RUN_STATE_PRELAUNCH },
>>   
>>       { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
>>       { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
>> @@ -627,20 +634,25 @@ static const RunStateTransition runstate_transitions_def[] = {
>>       { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>>   
>>       { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>> +    { RUN_STATE_SAVE_VM, RUN_STATE_PRELAUNCH },
> This should not happen; thus I would remove this line.
>
>>       { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
>>       { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
>> +    { RUN_STATE_SHUTDOWN, RUN_STATE_PRELAUNCH },
>>   
>>       { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
>>       { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
>>       { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
>>       { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
>> +    { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
>>   
>>       { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>>       { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>> +    { RUN_STATE_WATCHDOG, RUN_STATE_PRELAUNCH },
>>   
>>       { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
>>       { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
>> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PRELAUNCH },
>>   
>>       { RUN_STATE__MAX, RUN_STATE__MAX },
>>   };
>> @@ -1886,8 +1899,10 @@ static bool main_loop_should_exit(void)
>>           cpu_synchronize_all_states();
>>           qemu_system_reset(VMRESET_REPORT);
>>           resume_all_vcpus();
>> -        if (runstate_needs_reset()) {
>> -            runstate_set(RUN_STATE_PAUSED);
>> +        if (!runstate_check(RUN_STATE_RUNNING) &&
>> +                !runstate_check(RUN_STATE_INMIGRATE) &&
>> +                !runstate_check(RUN_STATE_SAVE_VM)) {
> Since SAVE_VM should not happen here, I would leave this check out too.
>   If it happens, the lack of a SAVE_VM->PRELAUNCH transition will cause
> an assertion failure.
>
> Thanks for the patch!
>
> Paolo
>
:) cool. This one is tied to one of our internal bugs thus I have to 
finish it some way.
The rest will be handled in libvirt by my colleague.

I much more worry about bdrv_invalidate_cache(). This problem crashes QEMU
with 100% probability in our stress testing. Manually it crashes 1 times 
out of 5
('virsh managed-save && virsh start' in parallel with
'while /bin/true; do virsh qemu-monitor-command --hmp info block ; done'

Den
diff mbox

Patch

diff --git a/vl.c b/vl.c
index 0172e42..c9e47b0 100644
--- a/vl.c
+++ b/vl.c
@@ -583,6 +583,7 @@  static const RunStateTransition runstate_transitions_def[] = {
     /*     from      ->     to      */
     { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
     { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_INMIGRATE, RUN_STATE_INTERNAL_ERROR },
     { RUN_STATE_INMIGRATE, RUN_STATE_IO_ERROR },
@@ -596,15 +597,19 @@  static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
     { RUN_STATE_IO_ERROR, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_IO_ERROR, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
     { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_POSTMIGRATE, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
     { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
@@ -612,8 +617,10 @@  static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
+    { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
+    { RUN_STATE_RESTORE_VM, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
     { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
@@ -627,20 +634,25 @@  static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
+    { RUN_STATE_SAVE_VM, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
     { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_SHUTDOWN, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
     { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
     { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
     { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
     { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_WATCHDOG, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
     { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PRELAUNCH },
 
     { RUN_STATE__MAX, RUN_STATE__MAX },
 };
@@ -1886,8 +1899,10 @@  static bool main_loop_should_exit(void)
         cpu_synchronize_all_states();
         qemu_system_reset(VMRESET_REPORT);
         resume_all_vcpus();
-        if (runstate_needs_reset()) {
-            runstate_set(RUN_STATE_PAUSED);
+        if (!runstate_check(RUN_STATE_RUNNING) &&
+                !runstate_check(RUN_STATE_INMIGRATE) &&
+                !runstate_check(RUN_STATE_SAVE_VM)) {
+            runstate_set(RUN_STATE_PRELAUNCH);
         }
     }
     if (qemu_wakeup_requested()) {