[Qemu-devel] [PATCH 0/2] Fix double loadvm failure

[Qemu-devel] [PATCH 0/2] Fix double loadvm failure

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

lp: https://bugs.launchpad.net/qemu/+bug/1712564

Reported-by: Stafford Horne <shorne@gmail.com>

Dr. David Alan Gilbert (2):
  migration: Reset rather than destroy main_thread_load_event
  snapshot/tests: Try loadvm twice

 migration/migration.c      | 2 +-
 tests/qemu-iotests/068     | 2 +-
 tests/qemu-iotests/068.out | 4 ++++
 3 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

migration_incoming_state_destroy doesn't really destroy, it cleans up.
After a loadvm it's called, but the loadvm command can be run twice,
and so destroying an init-once mutex breaks on the second loadvm.

Reported-by: Stafford Horne <shorne@gmail.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index c3fe0ed9ca..a625551ce5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
         mis->from_src_file = NULL;
     }
 
-    qemu_event_destroy(&mis->main_thread_load_event);
+    qemu_event_reset(&mis->main_thread_load_event);
 }
 
 static void migrate_generate_event(int new_state)
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Peter Maydell]

On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> migration_incoming_state_destroy doesn't really destroy, it cleans up.
> After a loadvm it's called, but the loadvm command can be run twice,
> and so destroying an init-once mutex breaks on the second loadvm.
>
> Reported-by: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index c3fe0ed9ca..a625551ce5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
>          mis->from_src_file = NULL;
>      }
>
> -    qemu_event_destroy(&mis->main_thread_load_event);
> +    qemu_event_reset(&mis->main_thread_load_event);
>  }

Is it worth doing more here? For instance:
 * rename the function to something that better reflects
   what it's doing
 * make sure it actually goes back to the state it's in
   after you first call migration_incoming_get_current()
   (eg setting mis_current.state, zeroing various fields)
 * maybe instead of a "get current state" that does a
   reset if it's not been called before and a "destroy"
   that does cleanup stuff (like telling the source we've
   stopped) and also resetting back to clean state, we
   could structure this with a function that does
   "give me a clean completely reset state" which you
   must call first, then use get_current() purely to
   get the current state with no 'reset on first call'
   semantics, and finally a "complete" function that just
   does the "tell source we've stopped" and close
   resources we no longer need  ?

PS, in migration_incoming_get_current() we do
        mis_current.state = MIGRATION_STATUS_NONE;
        memset(&mis_current, 0, sizeof(MigrationIncomingState));

and the first line there is pointless because the memset
blasts zeroes over it anyway.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > migration_incoming_state_destroy doesn't really destroy, it cleans up.
> > After a loadvm it's called, but the loadvm command can be run twice,
> > and so destroying an init-once mutex breaks on the second loadvm.
> >
> > Reported-by: Stafford Horne <shorne@gmail.com>
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/migration.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index c3fe0ed9ca..a625551ce5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
> >          mis->from_src_file = NULL;
> >      }
> >
> > -    qemu_event_destroy(&mis->main_thread_load_event);
> > +    qemu_event_reset(&mis->main_thread_load_event);
> >  }
> 
> Is it worth doing more here?

In the longer-term, yes;  it seemed right just to get
this bug fixed first though.

> For instance:
>  * rename the function to something that better reflects
>    what it's doing
>  * make sure it actually goes back to the state it's in
>    after you first call migration_incoming_get_current()
>    (eg setting mis_current.state, zeroing various fields)
>  * maybe instead of a "get current state" that does a
>    reset if it's not been called before and a "destroy"
>    that does cleanup stuff (like telling the source we've
>    stopped) and also resetting back to clean state, we
>    could structure this with a function that does
>    "give me a clean completely reset state" which you
>    must call first, then use get_current() purely to
>    get the current state with no 'reset on first call'
>    semantics, and finally a "complete" function that just
>    does the "tell source we've stopped" and close
>    resources we no longer need  ?

Yes; an init/current/clean does make sense;  one of my comments
on one of Peter's patchsets was to point out the
migration_incoming_get_current isn't thread safe if you're
not sure you've already called it, so it could do with fixing.

There has also been a few things that have wanted to gather stats
that are available after the end of an incoming migration;
so we don't really want to destroy that state, we just want
to get rid of anything temporary.

You could argue that this thread_load_event is best init'd
at the start of the incoming migration and then destroyed at the
end.

> PS, in migration_incoming_get_current() we do
>         mis_current.state = MIGRATION_STATUS_NONE;
>         memset(&mis_current, 0, sizeof(MigrationIncomingState));
> 
> and the first line there is pointless because the memset
> blasts zeroes over it anyway.

Hmm yes.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Peter Xu]

On Fri, Aug 25, 2017 at 04:51:29PM +0100, Dr. David Alan Gilbert wrote:

[...]

> > PS, in migration_incoming_get_current() we do
> >         mis_current.state = MIGRATION_STATUS_NONE;
> >         memset(&mis_current, 0, sizeof(MigrationIncomingState));
> > 
> > and the first line there is pointless because the memset
> > blasts zeroes over it anyway.
> 
> Hmm yes.

I'll include this cleanup within my (future) patch to init incoming
migration object in migration_object_init() as well.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Stafford Horne]

On Fri, Aug 25, 2017 at 03:19:39PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> migration_incoming_state_destroy doesn't really destroy, it cleans up.
> After a loadvm it's called, but the loadvm command can be run twice,
> and so destroying an init-once mutex breaks on the second loadvm.
> 
> Reported-by: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index c3fe0ed9ca..a625551ce5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
>          mis->from_src_file = NULL;
>      }
>  
> -    qemu_event_destroy(&mis->main_thread_load_event);
> +    qemu_event_reset(&mis->main_thread_load_event);
>  }
>  
>  static void migrate_generate_event(int new_state)

For what its worth:

Tested-by: Stafford Horne <shorne@gmail.com>

Thanks, I saw the mail from Peter as well, I agree it makes sense to not
call the parent method destroy.  But this works for me at the moment.

-Stafford

Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event

[Peter Xu]

On Fri, Aug 25, 2017 at 03:19:39PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> migration_incoming_state_destroy doesn't really destroy, it cleans up.
> After a loadvm it's called, but the loadvm command can be run twice,
> and so destroying an init-once mutex breaks on the second loadvm.
> 
> Reported-by: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[Qemu-devel] [PATCH 2/2] snapshot/tests: Try loadvm twice

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

It's legal to loadvm twice, modify the existing save/loadvm test
to do it twice.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/qemu-iotests/068     | 2 +-
 tests/qemu-iotests/068.out | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index cfa0f2aed5..e7fca6a494 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -78,7 +78,7 @@ for extra_args in \
     # Give qemu some time to boot before saving the VM state
     { sleep 1; printf "savevm 0\nquit\n"; } | _qemu $extra_args
     # Now try to continue from that VM state (this should just work)
-    echo quit | _qemu $extra_args -loadvm 0
+    { sleep 1; printf "loadvm 0\nloadvm 0\nquit\n"; } | _qemu $extra_args -S
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out
index aa063cf711..f07a938a38 100644
--- a/tests/qemu-iotests/068.out
+++ b/tests/qemu-iotests/068.out
@@ -7,6 +7,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
 QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) loadvm 0
+(qemu) loadvm 0
 (qemu) quit
 
 === Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
@@ -16,5 +18,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
 QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) loadvm 0
+(qemu) loadvm 0
 (qemu) quit
 *** done
-- 
2.13.5

Re: [Qemu-devel] [PATCH 2/2] snapshot/tests: Try loadvm twice

[Peter Xu]

On Fri, Aug 25, 2017 at 03:19:40PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> It's legal to loadvm twice, modify the existing save/loadvm test
> to do it twice.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

(a question below though)

> ---
>  tests/qemu-iotests/068     | 2 +-
>  tests/qemu-iotests/068.out | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
> index cfa0f2aed5..e7fca6a494 100755
> --- a/tests/qemu-iotests/068
> +++ b/tests/qemu-iotests/068
> @@ -78,7 +78,7 @@ for extra_args in \
>      # Give qemu some time to boot before saving the VM state
>      { sleep 1; printf "savevm 0\nquit\n"; } | _qemu $extra_args
>      # Now try to continue from that VM state (this should just work)
> -    echo quit | _qemu $extra_args -loadvm 0
> +    { sleep 1; printf "loadvm 0\nloadvm 0\nquit\n"; } | _qemu $extra_args -S

Curious about why there are both "sleep 1" and "-S" added - I thought
"-S" means CPU won't really run, then why wait for one more second?

>  done
>  
>  # success, all done
> diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out
> index aa063cf711..f07a938a38 100644
> --- a/tests/qemu-iotests/068.out
> +++ b/tests/qemu-iotests/068.out
> @@ -7,6 +7,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
>  QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) loadvm 0
> +(qemu) loadvm 0
>  (qemu) quit
>  
>  === Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
> @@ -16,5 +18,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
>  QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) loadvm 0
> +(qemu) loadvm 0
>  (qemu) quit
>  *** done
> -- 
> 2.13.5
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/2] snapshot/tests: Try loadvm twice

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Fri, Aug 25, 2017 at 03:19:40PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > It's legal to loadvm twice, modify the existing save/loadvm test
> > to do it twice.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> (a question below though)
> 
> > ---
> >  tests/qemu-iotests/068     | 2 +-
> >  tests/qemu-iotests/068.out | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
> > index cfa0f2aed5..e7fca6a494 100755
> > --- a/tests/qemu-iotests/068
> > +++ b/tests/qemu-iotests/068
> > @@ -78,7 +78,7 @@ for extra_args in \
> >      # Give qemu some time to boot before saving the VM state
> >      { sleep 1; printf "savevm 0\nquit\n"; } | _qemu $extra_args
> >      # Now try to continue from that VM state (this should just work)
> > -    echo quit | _qemu $extra_args -loadvm 0
> > +    { sleep 1; printf "loadvm 0\nloadvm 0\nquit\n"; } | _qemu $extra_args -S
> 
> Curious about why there are both "sleep 1" and "-S" added - I thought
> "-S" means CPU won't really run, then why wait for one more second?

Mostly because I copied it from the savevm line above :-)

Dave

> >  done
> >  
> >  # success, all done
> > diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out
> > index aa063cf711..f07a938a38 100644
> > --- a/tests/qemu-iotests/068.out
> > +++ b/tests/qemu-iotests/068.out
> > @@ -7,6 +7,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
> >  (qemu) savevm 0
> >  (qemu) quit
> >  QEMU X.Y.Z monitor - type 'help' for more information
> > +(qemu) loadvm 0
> > +(qemu) loadvm 0
> >  (qemu) quit
> >  
> >  === Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
> > @@ -16,5 +18,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
> >  (qemu) savevm 0
> >  (qemu) quit
> >  QEMU X.Y.Z monitor - type 'help' for more information
> > +(qemu) loadvm 0
> > +(qemu) loadvm 0
> >  (qemu) quit
> >  *** done
> > -- 
> > 2.13.5
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/2] Fix double loadvm failure

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> lp: https://bugs.launchpad.net/qemu/+bug/1712564
> 
> Reported-by: Stafford Horne <shorne@gmail.com>
> 
> Dr. David Alan Gilbert (2):
>   migration: Reset rather than destroy main_thread_load_event
>   snapshot/tests: Try loadvm twice
> 
>  migration/migration.c      | 2 +-
>  tests/qemu-iotests/068     | 2 +-
>  tests/qemu-iotests/068.out | 4 ++++
>  3 files changed, 6 insertions(+), 2 deletions(-)

Queued

> -- 
> 2.13.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK