Re: [PATCH v10 01/31] tools/libxl: introduce libxl__domain_restore_device_model to load qemu state

From: Wen Congyang <wency@cn.fujitsu.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Dong Eddie <eddie.dong@intel.com>,
	xen devel <xen-devel@lists.xen.org>,
	Anthony Perard <anthony.perard@citrix.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v10 01/31] tools/libxl: introduce libxl__domain_restore_device_model to load qemu state
Date: Fri, 26 Feb 2016 09:55:48 +0800	[thread overview]
Message-ID: <56CFB0A4.3020002@cn.fujitsu.com> (raw)
In-Reply-To: <20160225155341.GA4235@citrix.com>

On 02/25/2016 11:53 PM, Wei Liu wrote:
> On Mon, Feb 22, 2016 at 10:52:05AM +0800, Wen Congyang wrote:
>> In normal migration, the qemu state is passed to qemu as a parameter.
>> With COLO, secondary vm is running. So we will do the following steps
>> at every checkpoint:
>> 1. suspend both primary vm and secondary vm
>> 2. sync the state
>> 3. resume both primary vm and secondary vm
>> Primary will send qemu's state in step2, and secondary's qemu should
>> read it and restore the state before it is resumed. We can not pass
>> the state to qemu as a parameter because secondary QEMU already started
> 
> is already started.
> 
>> at this point, so we introduce libxl__domain_restore_device_model() to
>> do it. This API MUST be called before resuming secondary vm.
>>
> 
> The last sentence is of no relevancy of this function itself, so I would
> just remove it if this patch will be resent in its current form.
> 
> And some comments below.
> 
>> Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  tools/libxl/libxl_dom_save.c | 20 ++++++++++++++++++++
>>  tools/libxl/libxl_internal.h |  4 ++++
>>  tools/libxl/libxl_qmp.c      | 10 ++++++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
>> index 4eb7960..7d345f9 100644
>> --- a/tools/libxl/libxl_dom_save.c
>> +++ b/tools/libxl/libxl_dom_save.c
>> @@ -512,6 +512,26 @@ int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
>>      return rc;
>>  }
>>  
>> +int libxl__domain_restore_device_model(libxl__gc *gc, uint32_t domid,
>> +                                       const char *restore_file)
>> +{
>> +    int rc;
>> +
>> +    switch (libxl__device_model_version_running(gc, domid)) {
>> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
>> +        /* Will never be supported. */
>> +        rc = ERROR_INVAL;
>> +        break;
>> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>> +        rc = libxl__qmp_restore(gc, domid, restore_file);
>> +        break;
>> +    default:
>> +        rc = ERROR_INVAL;
>> +    }
>> +
>> +    return rc;
>> +}
>> +
> 
> The function name suggests that it should be universally applied to all
> device model cases and all places that involves restoring device model.
> This is not the case in this series.  I search for this function in the
> rest of this series, it seems to be only used by COLO (in patch #10).
> 
> It's also unclear where the other half libxl__domain_save_device_model
> is.  I don't think this is your problem, though.
> 
> Based on the two reasons above, I would say let's not have this function
> at all.  You can call libxl__qmp_restore in COLO code directly.
> 
> Does this sound plausible?
> 
> If you agree, this patch can be turned into introduction of
> libxl__qmp_restore.

Yes, it is only used by COLO. I will update it in the next version.

Thanks
Wen Congyang

> 
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index a1aae97..5320e5e 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1119,6 +1119,8 @@ _hidden int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>>                                   const char *old_name, const char *new_name,
>>                                   xs_transaction_t trans);
>>  
>> +_hidden int libxl__domain_restore_device_model(libxl__gc *gc, uint32_t domid,
>> +                                               const char *restore_file);
>>  _hidden int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid);
>>  
>>  _hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
>> @@ -1762,6 +1764,8 @@ _hidden int libxl__qmp_stop(libxl__gc *gc, int domid);
>>  _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
>>  /* Save current QEMU state into fd. */
>>  _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename);
>> +/* Load current QEMU state from file. */
>> +_hidden int libxl__qmp_restore(libxl__gc *gc, int domid, const char *filename);
>>  /* Set dirty bitmap logging status */
>>  _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable);
>>  _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk);
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> index 714038b..eec8a44 100644
>> --- a/tools/libxl/libxl_qmp.c
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -905,6 +905,16 @@ int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename)
>>                             NULL, NULL);
>>  }
>>  
>> +int libxl__qmp_restore(libxl__gc *gc, int domid, const char *state_file)
>> +{
>> +    libxl__json_object *args = NULL;
>> +
>> +    qmp_parameters_add_string(gc, &args, "filename", state_file);
>> +
>> +    return qmp_run_command(gc, domid, "xen-load-devices-state", args,
>> +                           NULL, NULL);
>> +}
>> +
> 
> This looks OK.
> 
>>  static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp,
>>                        char *device, char *target, char *arg)
>>  {
>> -- 
>> 2.5.0
>>
>>
>>
> 
> 
> .
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel