| Message ID | 1456109555-28299-2-git-send-email-wency@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > /* > * 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 > > >
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 >> >> >> > > > . >
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; +} + /* * 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); +} + static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp, char *device, char *target, char *arg) {