Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Peter Xu]

On Mon, Jun 05, 2017 at 03:22:24PM -0500, Eric Blake wrote:
> On 05/19/2017 01:43 AM, Peter Xu wrote:
> > We were do the shutting off only for postcopy. Now we do this as long as
> > the source return path is there.
> > 
> > Moving the cleanup of from_src_file there too.
> > 
> > Signed-off-by: Peter Xu 
> > ---
> >  migration/migration.c| 8 +++-
> >  migration/postcopy-ram.c | 1 -
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> This commit causes a regression in qemu-iotests 68:
> 
> $ cd tests/qemu-iotests
> $ ./check -qcow2 68
> ...
> 068 1s ... - output mismatch (see 068.out.bad)
> --- /home/eblake/qemu-tmp2/tests/qemu-iotests/068.out 2017-05-30
> 09:27:26.795821748 -0500
> +++ 068.out.bad   2017-06-05 15:21:18.566816816 -0500
> @@ -6,6 +6,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
> +./common.config: line 107:  1912 Segmentation fault  (core dumped)
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done
> Failures: 068
> Failed 1 of 1 tests
> 
> I didn't investigate further; but am hoping you'll be able to fix the
> segfault and get the test working again.

Sorry for that! I will have a look.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Eric Blake]

On 05/19/2017 01:43 AM, Peter Xu wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
> 
> Moving the cleanup of from_src_file there too.
> 
> Signed-off-by: Peter Xu 
> ---
>  migration/migration.c| 8 +++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)

This commit causes a regression in qemu-iotests 68:

$ cd tests/qemu-iotests
$ ./check -qcow2 68
...
068 1s ... - output mismatch (see 068.out.bad)
--- /home/eblake/qemu-tmp2/tests/qemu-iotests/068.out   2017-05-30
09:27:26.795821748 -0500
+++ 068.out.bad 2017-06-05 15:21:18.566816816 -0500
@@ -6,6 +6,8 @@
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
+./common.config: line 107:  1912 Segmentation fault  (core dumped)
( if [ -n "${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Failures: 068
Failed 1 of 1 tests

I didn't investigate further; but am hoping you'll be able to fix the
segfault and get the test working again.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Juan Quintela]

Peter Xu  wrote:
> On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote:
>> Peter Xu  wrote:
>> > We were do the shutting off only for postcopy. Now we do this as long as
>> > the source return path is there.
>> >
>> > Moving the cleanup of from_src_file there too.
>> >
>> > Signed-off-by: Peter Xu 
>> > ---
>> >  migration/migration.c| 8 +++-
>> >  migration/postcopy-ram.c | 1 -
>> >  2 files changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 92617fc..a4006b4 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>> >  struct MigrationIncomingState *mis = migration_incoming_get_current();
>> >  
>> >  if (mis->to_src_file) {
>> > +/* Tell source that we are done */
>> > +migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) 
>> > != 0);
>> 
>> Reviewed-by: Juan Quintela 
>> 
>> 
>> I think this one belongs to previous patch (with accompaining line from 
>> below).
>> But just if you want to change it.
>
> I separated it since these two patches were actually doing different
> things:
>
> - previous patch fixed one possible leak, while
>
> - this patch postponed MIG_RP_MSG_SHUT a bit to the end, and let it
>   not depending on postcopy, but the return path itself (so that we
>   can enable the return path even without postcopy then)
>
> Meanwhile, there might be problem if we just put this single line into
> previous patch, since this line depends on below change [1]
> (from_src_file should better be closed after this
> qemu_file_get_error() call). So... I would still prefer to separate
> them using current way. Even if we really want to merge them, I would
> prefer directly squashing current patch into previous one.

ok, it is up to you.

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Peter Xu]

On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote:
> Peter Xu  wrote:
> > We were do the shutting off only for postcopy. Now we do this as long as
> > the source return path is there.
> >
> > Moving the cleanup of from_src_file there too.
> >
> > Signed-off-by: Peter Xu 
> > ---
> >  migration/migration.c| 8 +++-
> >  migration/postcopy-ram.c | 1 -
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 92617fc..a4006b4 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
> >  struct MigrationIncomingState *mis = migration_incoming_get_current();
> >  
> >  if (mis->to_src_file) {
> > +/* Tell source that we are done */
> > +migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) 
> > != 0);
> 
> Reviewed-by: Juan Quintela 
> 
> 
> I think this one belongs to previous patch (with accompaining line from 
> below).
> But just if you want to change it.

I separated it since these two patches were actually doing different
things:

- previous patch fixed one possible leak, while

- this patch postponed MIG_RP_MSG_SHUT a bit to the end, and let it
  not depending on postcopy, but the return path itself (so that we
  can enable the return path even without postcopy then)

Meanwhile, there might be problem if we just put this single line into
previous patch, since this line depends on below change [1]
(from_src_file should better be closed after this
qemu_file_get_error() call). So... I would still prefer to separate
them using current way. Even if we really want to merge them, I would
prefer directly squashing current patch into previous one.

Thanks,

> 
> >  qemu_fclose(mis->to_src_file);
> >  mis->to_src_file = NULL;
> >  }
> >  
> > +if (mis->from_src_file) {
> > +qemu_fclose(mis->from_src_file);
> > +mis->from_src_file = NULL;
> > +}
> > +

[1]

> >  qemu_event_destroy(>main_thread_load_event);
> >  loadvm_free_handlers(mis);
> >  }
> > @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
> >  exit(EXIT_FAILURE);
> >  }
> >  
> > -qemu_fclose(f);
> >  free_xbzrle_decoded_buf();
> >  
> >  mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index a0489f6..57aa208 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -320,7 +320,6 @@ int 
> > postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> >  }
> >  
> >  postcopy_state_set(POSTCOPY_INCOMING_END);
> > -migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 
> > 0);
> >  
> >  if (mis->postcopy_tmp_page) {
> >  munmap(mis->postcopy_tmp_page, mis->largest_page_size);

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Juan Quintela]

Peter Xu  wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
>
> Moving the cleanup of from_src_file there too.
>
> Signed-off-by: Peter Xu 

You can also submmit this and previous patch alone and I will integrate
them.

thanks, Juan.

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Juan Quintela]

Peter Xu  wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
>
> Moving the cleanup of from_src_file there too.
>
> Signed-off-by: Peter Xu 
> ---
>  migration/migration.c| 8 +++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 92617fc..a4006b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>  struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
>  if (mis->to_src_file) {
> +/* Tell source that we are done */
> +migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 
> 0);

Reviewed-by: Juan Quintela 


I think this one belongs to previous patch (with accompaining line from below).
But just if you want to change it.

>  qemu_fclose(mis->to_src_file);
>  mis->to_src_file = NULL;
>  }
>  
> +if (mis->from_src_file) {
> +qemu_fclose(mis->from_src_file);
> +mis->from_src_file = NULL;
> +}
> +
>  qemu_event_destroy(>main_thread_load_event);
>  loadvm_free_handlers(mis);
>  }
> @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
>  exit(EXIT_FAILURE);
>  }
>  
> -qemu_fclose(f);
>  free_xbzrle_decoded_buf();
>  
>  mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a0489f6..57aa208 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState 
> *mis)
>  }
>  
>  postcopy_state_set(POSTCOPY_INCOMING_END);
> -migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>  
>  if (mis->postcopy_tmp_page) {
>  munmap(mis->postcopy_tmp_page, mis->largest_page_size);

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
> 
> Moving the cleanup of from_src_file there too.
> 
> Signed-off-by: Peter Xu 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/migration.c| 8 +++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 92617fc..a4006b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>  struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
>  if (mis->to_src_file) {
> +/* Tell source that we are done */
> +migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 
> 0);
>  qemu_fclose(mis->to_src_file);
>  mis->to_src_file = NULL;
>  }
>  
> +if (mis->from_src_file) {
> +qemu_fclose(mis->from_src_file);
> +mis->from_src_file = NULL;
> +}
> +
>  qemu_event_destroy(>main_thread_load_event);
>  loadvm_free_handlers(mis);
>  }
> @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
>  exit(EXIT_FAILURE);
>  }
>  
> -qemu_fclose(f);
>  free_xbzrle_decoded_buf();
>  
>  mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a0489f6..57aa208 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState 
> *mis)
>  }
>  
>  postcopy_state_set(POSTCOPY_INCOMING_END);
> -migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>  
>  if (mis->postcopy_tmp_page) {
>  munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK