From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48863) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoosn-0008KL-4e for qemu-devel@nongnu.org; Wed, 21 Oct 2015 04:36:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zoosj-0003sC-O8 for qemu-devel@nongnu.org; Wed, 21 Oct 2015 04:36:28 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:54019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoosi-0003rl-Ie for qemu-devel@nongnu.org; Wed, 21 Oct 2015 04:36:25 -0400 References: <1441182199-8328-1-git-send-email-zhang.zhanghailiang@huawei.com> <1441182199-8328-9-git-send-email-zhang.zhanghailiang@huawei.com> <20151019095422.GB2462@work-vm> <5625FBC3.9040505@huawei.com> <20151020193256.GD2520@work-vm> From: zhanghailiang Message-ID: <56274DF3.1020509@huawei.com> Date: Wed, 21 Oct 2015 16:33:55 +0800 MIME-Version: 1.0 In-Reply-To: <20151020193256.GD2520@work-vm> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v9 08/32] COLO/migration: establish a new communication path from destination to source 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/21 3:32, Dr. David Alan Gilbert wrote: > * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >> On 2015/10/19 17:54, Dr. David Alan Gilbert wrote: >>> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >>>> Add a new member 'to_src_file' to MigrationIncomingState and a >>>> new member 'from_dst_file' to MigrationState. >>>> They will be used for returning messages from destination to source. >>>> It will also be used by post-copy migration. >>>> >>>> Signed-off-by: zhanghailiang >>>> Signed-off-by: Li Zhijian >>>> Cc: Dr. David Alan Gilbert >>>> --- >>>> include/migration/migration.h | 3 ++- >>>> migration/colo.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 45 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/migration/migration.h b/include/migration/migration.h >>>> index 6488e03..0c94103 100644 >>>> --- a/include/migration/migration.h >>>> +++ b/include/migration/migration.h >>>> @@ -50,7 +50,7 @@ typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head; >>>> /* State for the incoming migration */ >>>> struct MigrationIncomingState { >>>> QEMUFile *from_src_file; >>>> - >>>> + QEMUFile *to_src_file; >>>> int state; >>>> >>>> bool have_colo_incoming_thread; >>>> @@ -74,6 +74,7 @@ struct MigrationState >>>> QemuThread thread; >>>> QEMUBH *cleanup_bh; >>>> QEMUFile *to_dst_file; >>>> + QEMUFile *from_dst_file; >>>> int parameters[MIGRATION_PARAMETER_MAX]; >>>> >>>> int state; >>>> diff --git a/migration/colo.c b/migration/colo.c >>>> index a341eee..5f4fb20 100644 >>>> --- a/migration/colo.c >>>> +++ b/migration/colo.c >>>> @@ -39,6 +39,20 @@ bool migration_incoming_in_colo_state(void) >>>> static void *colo_thread(void *opaque) >>>> { >>>> MigrationState *s = opaque; >>>> + int fd, ret = 0; >>>> + >>>> + /* Dup the fd of to_dst_file */ >>>> + fd = dup(qemu_get_fd(s->to_dst_file)); >>>> + if (fd == -1) { >>>> + ret = -errno; >>>> + goto out; >>>> + } >>>> + s->from_dst_file = qemu_fopen_socket(fd, "rb"); >>> >>> In my postcopy code I add the return-path opening as a new >>> method on QEMUFile, that way if we get a return path working >>> on another transport (RDMA which I hope to do) then it >>> works; have a look at 'Return path: Open a return path on QEMUFile for sockets' >>> >> >> I have looked at it. That's a good solution, we use the same fd for return path, and >> i don't have to call qemu_file_shutdown two times in failover process. >> >>>> + if (!s->from_dst_file) { >>>> + ret = -EINVAL; >>>> + error_report("Open QEMUFile failed!"); >>> >>> In errors, try to give detail of where a problem was; >>> e.g. 'colo_thread: Open QEMUFile from_dst failed'. >>> >> >> OK. I will fix it in next version. >> >>>> + goto out; >>>> + } >>>> >>>> qemu_mutex_lock_iothread(); >>>> vm_start(); >>>> @@ -47,9 +61,17 @@ static void *colo_thread(void *opaque) >>>> >>>> /*TODO: COLO checkpoint savevm loop*/ >>>> >>>> +out: >>>> + if (ret < 0) { >>>> + error_report("Detect some error: %s", strerror(-ret)); >>>> + } >>> >>> Again, best to say where the error happened. >>> >> >> Hmm, it is a little difficult to say where exactly this error happened here, >> what i can do is to error out in the place where the error happened. >> Here is only a summary for the error. > > OK, just add the function name then, e.g. > error_report("%s: %s", __func__, strerror(-ret)); > > it just helps when you're looking at a log file, to understand where it came > from; especially if the person who sent you the log file didn't tell you much :-) > OK, that's a good idea, will fix in next version. Thanks. >> >>> >>>> migrate_set_state(&s->state, MIGRATION_STATUS_COLO, >>>> MIGRATION_STATUS_COMPLETED); >>>> >>>> + if (s->from_dst_file) { >>>> + qemu_fclose(s->from_dst_file); >>>> + } >>>> + >>>> qemu_mutex_lock_iothread(); >>>> qemu_bh_schedule(s->cleanup_bh); >>>> qemu_mutex_unlock_iothread(); >>>> @@ -86,12 +108,33 @@ void colo_init_checkpointer(MigrationState *s) >>>> void *colo_process_incoming_thread(void *opaque) >>>> { >>>> MigrationIncomingState *mis = opaque; >>>> + int fd, ret = 0; >>>> >>>> migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE, >>>> MIGRATION_STATUS_COLO); >>>> >>>> + fd = dup(qemu_get_fd(mis->from_src_file)); >>>> + if (fd < 0) { >>>> + ret = -errno; >>>> + goto out; >>>> + } >>>> + mis->to_src_file = qemu_fopen_socket(fd, "wb"); >>>> + if (!mis->to_src_file) { >>>> + ret = -EINVAL; >>>> + error_report("Can't open incoming channel!"); >>>> + goto out; >>>> + } >>> >>> Same as above. >> >> OK. Will fix it. >> >> Thanks, >> zhanghailiang >>> >>>> /* TODO: COLO checkpoint restore loop */ >>>> >>>> +out: >>>> + if (ret < 0) { >>> >>>> + error_report("colo incoming thread will exit, detect error: %s", >>>> + strerror(-ret)); >>>> + } >>>> + >>>> + if (mis->to_src_file) { >>>> + qemu_fclose(mis->to_src_file); >>>> + } >>>> migration_incoming_exit_colo(); >>>> >>>> return NULL; >>>> -- >>>> 1.8.3.1 >>>> >>>> >>> -- >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >>> >>> . >>> >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > . >