From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoS0g-0005km-6Y for qemu-devel@nongnu.org; Tue, 20 Oct 2015 04:11:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoS0c-00084E-UQ for qemu-devel@nongnu.org; Tue, 20 Oct 2015 04:11:06 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:55291) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoS0c-0007y5-2m for qemu-devel@nongnu.org; Tue, 20 Oct 2015 04:11:02 -0400 References: <1441182199-8328-1-git-send-email-zhang.zhanghailiang@huawei.com> <1441182199-8328-7-git-send-email-zhang.zhanghailiang@huawei.com> <20151019091746.GA2462@work-vm> From: zhanghailiang Message-ID: <5625F591.5050601@huawei.com> Date: Tue, 20 Oct 2015 16:04:33 +0800 MIME-Version: 1.0 In-Reply-To: <20151019091746.GA2462@work-vm> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v9 06/32] migration: Integrate COLO checkpoint process into loadvm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: lizhijian@cn.fujitsu.com, quintela@redhat.com, yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org, arei.gonglei@huawei.com, stefanha@redhat.com, amit.shah@redhat.com, yanghy@cn.fujitsu.com On 2015/10/19 17:17, Dr. David Alan Gilbert wrote: > * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >> Switch from normal migration loadvm process into COLO checkpoint process if >> COLO mode is enabled. >> We add three new members to struct MigrationIncomingState, 'have_colo_incoming_thread' >> and 'colo_incoming_thread' record the colo related threads for secondary VM, >> 'migration_incoming_co' records the original migration incoming coroutine. >> >> Signed-off-by: zhanghailiang >> Signed-off-by: Li Zhijian >> Signed-off-by: Yang Hongyang > > Mostly OK, some mostly minor comments, and one question below: > >> --- >> include/migration/colo.h | 7 +++++++ >> include/migration/migration.h | 7 +++++++ >> migration/colo-comm.c | 10 ++++++++++ >> migration/colo.c | 22 ++++++++++++++++++++++ >> migration/migration.c | 33 +++++++++++++++++++++++---------- >> stubs/migration-colo.c | 10 ++++++++++ >> trace-events | 1 + >> 7 files changed, 80 insertions(+), 10 deletions(-) >> >> diff --git a/include/migration/colo.h b/include/migration/colo.h >> index dface19..58849f7 100644 >> --- a/include/migration/colo.h >> +++ b/include/migration/colo.h >> @@ -15,6 +15,8 @@ >> >> #include "qemu-common.h" >> #include "migration/migration.h" >> +#include "block/coroutine.h" >> +#include "qemu/thread.h" >> >> bool colo_supported(void); >> void colo_info_mig_init(void); >> @@ -22,4 +24,9 @@ void colo_info_mig_init(void); >> void colo_init_checkpointer(MigrationState *s); >> bool migration_in_colo_state(void); >> >> +/* loadvm */ >> +bool migration_incoming_enable_colo(void); >> +void migration_incoming_exit_colo(void); >> +void *colo_process_incoming_thread(void *opaque); >> +bool migration_incoming_in_colo_state(void); >> #endif >> diff --git a/include/migration/migration.h b/include/migration/migration.h >> index a62068f..9cdd6b6 100644 >> --- a/include/migration/migration.h >> +++ b/include/migration/migration.h >> @@ -22,6 +22,7 @@ >> #include "migration/vmstate.h" >> #include "qapi-types.h" >> #include "exec/cpu-common.h" >> +#include "block/coroutine.h" >> >> #define QEMU_VM_FILE_MAGIC 0x5145564d >> #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 >> @@ -51,6 +52,12 @@ struct MigrationIncomingState { >> QEMUFile *file; >> >> int state; >> + >> + bool have_colo_incoming_thread; >> + QemuThread colo_incoming_thread; >> + /* The coroutine we should enter (back) after failover */ >> + Coroutine *migration_incoming_co; >> + >> /* See savevm.c */ >> LoadStateEntry_Head loadvm_handlers; >> }; >> diff --git a/migration/colo-comm.c b/migration/colo-comm.c >> index 4330bd8..0808d6c 100644 >> --- a/migration/colo-comm.c >> +++ b/migration/colo-comm.c >> @@ -52,3 +52,13 @@ void colo_info_mig_init(void) >> { >> vmstate_register(NULL, 0, &colo_state, &colo_info); >> } >> + >> +bool migration_incoming_enable_colo(void) >> +{ >> + return colo_info.colo_requested; >> +} >> + >> +void migration_incoming_exit_colo(void) >> +{ >> + colo_info.colo_requested = 0; >> +} >> diff --git a/migration/colo.c b/migration/colo.c >> index 97e64a3..a341eee 100644 >> --- a/migration/colo.c >> +++ b/migration/colo.c >> @@ -13,6 +13,7 @@ >> #include "sysemu/sysemu.h" >> #include "migration/colo.h" >> #include "trace.h" >> +#include "qemu/error-report.h" >> >> static QEMUBH *colo_bh; >> >> @@ -28,6 +29,13 @@ bool migration_in_colo_state(void) >> return (s->state == MIGRATION_STATUS_COLO); >> } >> >> +bool migration_incoming_in_colo_state(void) >> +{ >> + MigrationIncomingState *mis = migration_incoming_get_current(); >> + >> + return (mis && (mis->state == MIGRATION_STATUS_COLO)); > > Can remove outer brackets. > OK. >> +} >> + >> static void *colo_thread(void *opaque) >> { >> MigrationState *s = opaque; >> @@ -74,3 +82,17 @@ void colo_init_checkpointer(MigrationState *s) >> colo_bh = qemu_bh_new(colo_start_checkpointer, s); >> qemu_bh_schedule(colo_bh); >> } >> + >> +void *colo_process_incoming_thread(void *opaque) >> +{ >> + MigrationIncomingState *mis = opaque; >> + >> + migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, >> + MIGRATION_STATUS_COLO); >> + >> + /* TODO: COLO checkpoint restore loop */ >> + >> + migration_incoming_exit_colo(); >> + >> + return NULL; >> +} >> diff --git a/migration/migration.c b/migration/migration.c >> index bee61aa..241689f 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -280,7 +280,28 @@ static void process_incoming_migration_co(void *opaque) >> MIGRATION_STATUS_ACTIVE); >> ret = qemu_loadvm_state(f); >> >> - qemu_fclose(f); >> + if (!ret) { >> + /* Make sure all file formats flush their mutable metadata */ >> + bdrv_invalidate_cache_all(&local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + migrate_decompress_threads_join(); >> + exit(EXIT_FAILURE); >> + } >> + } >> + /* we get colo info, and know if we are in colo mode */ >> + if (!ret && migration_incoming_enable_colo()) { >> + mis->migration_incoming_co = qemu_coroutine_self(); >> + qemu_thread_create(&mis->colo_incoming_thread, "colo incoming", >> + colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE); >> + mis->have_colo_incoming_thread = true; >> + qemu_coroutine_yield(); >> + >> + /* Wait checkpoint incoming thread exit before free resource */ >> + qemu_thread_join(&mis->colo_incoming_thread); >> + } else { >> + qemu_fclose(f); >> + } > > Why is that qemu_fclose(f) in the else ? If you do colo, and do the colo > thread, when the incoming colo thread exits, don't we return here - and if > we return here then don't we want to close f? (I guess this only happens > on a failover to secondary?) > Er,it is a bug, we closed it in colo incoming thread before in old version, but after several version, we lost it. I will fix it in next version. >> free_xbzrle_decoded_buf(); >> migration_incoming_state_destroy(); >> >> @@ -295,18 +316,9 @@ static void process_incoming_migration_co(void *opaque) >> MIGRATION_STATUS_COMPLETED); >> qemu_announce_self(); >> >> - /* Make sure all file formats flush their mutable metadata */ >> - bdrv_invalidate_cache_all(&local_err); >> - if (local_err) { >> - error_report_err(local_err); >> - migrate_decompress_threads_join(); >> - exit(EXIT_FAILURE); >> - } >> - >> /* If global state section was not received or we are in running >> state, we need to obey autostart. Any other state is set with >> runstate_set. */ >> - >> if (!global_state_received() || >> global_state_get_runstate() == RUN_STATE_RUNNING) { >> if (autostart) { >> @@ -740,6 +752,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >> error_setg(errp, QERR_MIGRATION_ACTIVE); >> return; >> } >> + >> if (runstate_check(RUN_STATE_INMIGRATE)) { >> error_setg(errp, "Guest is waiting for an incoming migration"); >> return; >> diff --git a/stubs/migration-colo.c b/stubs/migration-colo.c >> index 51b8f66..c49ee1a 100644 >> --- a/stubs/migration-colo.c >> +++ b/stubs/migration-colo.c >> @@ -22,6 +22,16 @@ bool migration_in_colo_state(void) >> return false; >> } >> >> +bool migration_incoming_in_colo_state(void) >> +{ >> + return false; >> +} >> + >> void colo_init_checkpointer(MigrationState *s) >> { >> } >> + >> +void *colo_process_incoming_thread(void *opaque) >> +{ >> + return NULL; >> +} >> diff --git a/trace-events b/trace-events >> index 487d1c7..352e9c3 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -1474,6 +1474,7 @@ rdma_start_outgoing_migration_after_rdma_source_init(void) "" >> >> # migration/colo.c >> colo_vm_state_change(const char *old, const char *new) "Change '%s' => '%s'" >> +colo_receive_message(const char *msg) "Receive '%s'" > > Should be in a different patch? > This is a residual, i have removed it. Thanks, zhanghailiang > Dave > >> # kvm-all.c >> kvm_ioctl(int type, void *arg) "type 0x%x, arg %p" >> -- >> 1.8.3.1 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > . >