| Message ID | 1495176212-14446-5-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Peter Xu (peterx@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 <peterx@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > 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(&mis->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 / dgilbert@redhat.com / Manchester, UK
Peter Xu <peterx@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 <peterx@redhat.com> > --- > 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 <quintela@redhat.com> 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(&mis->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 <peterx@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 <peterx@redhat.com> You can also submmit this and previous patch alone and I will integrate them. thanks, Juan.
On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote: > Peter Xu <peterx@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 <peterx@redhat.com> > > --- > > 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 <quintela@redhat.com> > > > 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(&mis->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 <peterx@redhat.com> wrote: > On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote: >> Peter Xu <peterx@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 <peterx@redhat.com> >> > --- >> > 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 <quintela@redhat.com> >> >> >> 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.
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 <peterx@redhat.com> > --- > 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.
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 <peterx@redhat.com> > > --- > > 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.
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(&mis->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);
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 <peterx@redhat.com> --- migration/migration.c | 8 +++++++- migration/postcopy-ram.c | 1 - 2 files changed, 7 insertions(+), 2 deletions(-)