[PATCH v2 0/5] migration/remus: bug fix and cleanup

[PATCH v2 0/5] migration/remus: bug fix and cleanup

[Wen Congyang]

Wen Congyang (5):
  remus: don't call stream_continue() when doing failover
  remus: don't write xenstore data if it fails
  tools/libxc: don't send end record if remus fails
  tools/libxc: error handling for the postcopy() callback
  tools/libxl: remove unused function libxl__domain_save_device_model()

 tools/libxc/xc_sr_save.c         |  6 ++-
 tools/libxl/libxl_dom.c          | 91 ----------------------------------------
 tools/libxl/libxl_internal.h     |  3 --
 tools/libxl/libxl_stream_read.c  | 21 +++++++---
 tools/libxl/libxl_stream_write.c |  8 +++-
 5 files changed, 26 insertions(+), 103 deletions(-)

-- 
2.5.0

[PATCH v2 1/5] remus: don't call stream_continue() when doing failover

[Wen Congyang]

stream_continue() is used for migration to read emulator
xenstore data and emulator context. For remus, if we do
failover, we have read it in the checkpoint cycle, and
we only need to complete the stream.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxl/libxl_stream_read.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index 258dec4..65219d5 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -758,6 +758,9 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
     libxl__stream_read_state *stream = &dcs->srs;
     STATE_AO_GC(dcs->ao);
 
+    /* convenience aliases */
+    const int checkpointed_stream = dcs->restore_params.checkpointed_stream;
+
     if (rc)
         goto err;
 
@@ -777,11 +780,19 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
      * If the stream is not still alive, we must not continue any work.
      */
     if (libxl__stream_read_inuse(stream)) {
-        /*
-         * Libxc has indicated that it is done with the stream.  Resume reading
-         * libxl records from it.
-         */
-        stream_continue(egc, stream);
+        if (checkpointed_stream) {
+            /*
+             * Failover from primary. Domain state is currently at a
+             * consistent checkpoint, ready to go.
+             */
+            stream_complete(egc, stream, 0);
+        } else {
+            /*
+             * Libxc has indicated that it is done with the stream.
+             * Resume reading libxl records from it.
+             */
+            stream_continue(egc, stream);
+        }
     }
 }
 
-- 
2.5.0

[PATCH v2 2/5] remus: don't write xenstore data if it fails

[Wen Congyang]

For example: if the secondary host is down, and we fail to send the data to
the secondary host. xc_domain_save() returns 0. So in the function
libxl__xc_domain_save_done(), rc is 0(the helper program exits normally),
and retval is 0(it is xc_domain_save()'s return value). In such case, we
just need to complete the stream.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl_stream_write.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 80d9208..ee9c53a 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -354,8 +354,12 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
      * alive, and check_all_finished() may have torn it down around us.
      * If the stream is not still alive, we must not continue any work.
      */
-    if (libxl__stream_write_inuse(stream))
-        write_emulator_xenstore_record(egc, stream);
+    if (libxl__stream_write_inuse(stream)) {
+        if (dss->remus)
+            stream_complete(egc, stream, 0);
+        else
+            write_emulator_xenstore_record(egc, stream);
+    }
 }
 
 static void write_emulator_xenstore_record(libxl__egc *egc,
-- 
2.5.0

[PATCH v2 3/5] tools/libxc: don't send end record if remus fails

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_sr_save.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index cefcef5..76ebb34 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -790,7 +790,7 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
 
             rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
             if ( rc <= 0 )
-                ctx->save.checkpointed = false;
+                goto err;
         }
     } while ( ctx->save.checkpointed );
 
-- 
2.5.0

[PATCH v2 4/5] tools/libxc: error handling for the postcopy() callback

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxc/xc_sr_save.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 76ebb34..8090fcf 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -786,7 +786,9 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
             if ( rc )
                 goto err;
 
-            ctx->save.callbacks->postcopy(ctx->save.callbacks->data);
+            rc = ctx->save.callbacks->postcopy(ctx->save.callbacks->data);
+            if ( rc <= 0 )
+                goto err;
 
             rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
             if ( rc <= 0 )
-- 
2.5.0

[PATCH v2 5/5] tools/libxl: remove unused function libxl__domain_save_device_model()

[Wen Congyang]

After the commit d77570e7, libxl__domain_save_device_model() can
be dropped.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl_dom.c      | 91 --------------------------------------------
 tools/libxl/libxl_internal.h |  3 --
 2 files changed, 94 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 47971a9..2269998 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1785,97 +1785,6 @@ static void stream_done(libxl__egc *egc,
     domain_save_done(egc, sws->dss, rc);
 }
 
-static void save_device_model_datacopier_done(libxl__egc *egc,
-     libxl__datacopier_state *dc, int rc, int onwrite, int errnoval);
-
-void libxl__domain_save_device_model(libxl__egc *egc,
-                                     libxl__domain_suspend_state *dss,
-                                     libxl__save_device_model_cb *callback)
-{
-    STATE_AO_GC(dss->ao);
-    struct stat st;
-    uint32_t qemu_state_len;
-    int rc;
-
-    dss->save_dm_callback = callback;
-
-    /* Convenience aliases */
-    const char *const filename = dss->dm_savefile;
-    const int fd = dss->fd;
-
-    libxl__datacopier_state *dc = &dss->save_dm_datacopier;
-    memset(dc, 0, sizeof(*dc));
-    dc->readwhat = GCSPRINTF("qemu save file %s", filename);
-    dc->ao = ao;
-    dc->readfd = -1;
-    dc->writefd = fd;
-    dc->maxsz = INT_MAX;
-    dc->bytes_to_read = -1;
-    dc->copywhat = GCSPRINTF("qemu save file for domain %"PRIu32, dss->domid);
-    dc->writewhat = "save/migration stream";
-    dc->callback = save_device_model_datacopier_done;
-
-    dc->readfd = open(filename, O_RDONLY);
-    if (dc->readfd < 0) {
-        LOGE(ERROR, "unable to open %s", dc->readwhat);
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    if (fstat(dc->readfd, &st))
-    {
-        LOGE(ERROR, "unable to fstat %s", dc->readwhat);
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    if (!S_ISREG(st.st_mode)) {
-        LOG(ERROR, "%s is not a plain file!", dc->readwhat);
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    qemu_state_len = st.st_size;
-    LOG(DEBUG, "%s is %d bytes", dc->readwhat, qemu_state_len);
-
-    rc = libxl__datacopier_start(dc);
-    if (rc) goto out;
-
-    libxl__datacopier_prefixdata(egc, dc,
-                                 QEMU_SIGNATURE, strlen(QEMU_SIGNATURE));
-
-    libxl__datacopier_prefixdata(egc, dc,
-                                 &qemu_state_len, sizeof(qemu_state_len));
-    return;
-
- out:
-    save_device_model_datacopier_done(egc, dc, rc, -1, EIO);
-}
-
-static void save_device_model_datacopier_done(libxl__egc *egc,
-     libxl__datacopier_state *dc, int our_rc, int onwrite, int errnoval)
-{
-    libxl__domain_suspend_state *dss =
-        CONTAINER_OF(dc, *dss, save_dm_datacopier);
-    STATE_AO_GC(dss->ao);
-
-    /* Convenience aliases */
-    const char *const filename = dss->dm_savefile;
-    int rc;
-
-    libxl__datacopier_kill(dc);
-
-    if (dc->readfd >= 0) {
-        close(dc->readfd);
-        dc->readfd = -1;
-    }
-
-    rc = libxl__remove_file(gc, filename);
-    if (!our_rc) our_rc = rc;
-
-    dss->save_dm_callback(egc, dss, our_rc);
-}
-
 static void libxl__remus_teardown(libxl__egc *egc,
                                   libxl__domain_suspend_state *dss,
                                   int rc);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a556a38..630172b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3498,9 +3498,6 @@ static inline bool libxl__save_helper_inuse(const libxl__save_helper_state *shs)
 /* Each time the dm needs to be saved, we must call suspend and then save */
 _hidden int libxl__domain_suspend_device_model(libxl__gc *gc,
                                            libxl__domain_suspend_state *dss);
-_hidden void libxl__domain_save_device_model(libxl__egc *egc,
-                                     libxl__domain_suspend_state *dss,
-                                     libxl__save_device_model_cb *callback);
 
 _hidden const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid);
 
-- 
2.5.0

Re: [PATCH v2 4/5] tools/libxc: error handling for the postcopy() callback

[Andrew Cooper]

On 05/01/2016 02:06, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 2/5] remus: don't write xenstore data if it fails

[Andrew Cooper]

On 05/01/2016 02:06, Wen Congyang wrote:

"if it fails" is ambiguous.  What I think you mean is "remus: resume
immediately if libxl__xc_domain_save_done() completes"

> For example: if the secondary host is down, and we fail to send the data to
> the secondary host. xc_domain_save() returns 0. So in the function
> libxl__xc_domain_save_done(), rc is 0(the helper program exits normally),
> and retval is 0(it is xc_domain_save()'s return value). In such case, we
> just need to complete the stream.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  tools/libxl/libxl_stream_write.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
> index 80d9208..ee9c53a 100644
> --- a/tools/libxl/libxl_stream_write.c
> +++ b/tools/libxl/libxl_stream_write.c
> @@ -354,8 +354,12 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
>       * alive, and check_all_finished() may have torn it down around us.
>       * If the stream is not still alive, we must not continue any work.
>       */
> -    if (libxl__stream_write_inuse(stream))
> -        write_emulator_xenstore_record(egc, stream);
> +    if (libxl__stream_write_inuse(stream)) {
> +        if (dss->remus)

Please put a small comment here as well.  Perhaps

"For remus, if libxl__xc_domain_save_done() completes, there was an
error sending data to the secondary.  Resume the primary ASAP."

~Andrew

> +            stream_complete(egc, stream, 0);
> +        else
> +            write_emulator_xenstore_record(egc, stream);
> +    }
>  }
>  
>  static void write_emulator_xenstore_record(libxl__egc *egc,

Re: [PATCH v2 2/5] remus: don't write xenstore data if it fails

[Wen Congyang]

On 01/05/2016 05:27 PM, Andrew Cooper wrote:
> On 05/01/2016 02:06, Wen Congyang wrote:
> 
> "if it fails" is ambiguous.  What I think you mean is "remus: resume
> immediately if libxl__xc_domain_save_done() completes"
> 
>> For example: if the secondary host is down, and we fail to send the data to
>> the secondary host. xc_domain_save() returns 0. So in the function
>> libxl__xc_domain_save_done(), rc is 0(the helper program exits normally),
>> and retval is 0(it is xc_domain_save()'s return value). In such case, we
>> just need to complete the stream.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  tools/libxl/libxl_stream_write.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
>> index 80d9208..ee9c53a 100644
>> --- a/tools/libxl/libxl_stream_write.c
>> +++ b/tools/libxl/libxl_stream_write.c
>> @@ -354,8 +354,12 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
>>       * alive, and check_all_finished() may have torn it down around us.
>>       * If the stream is not still alive, we must not continue any work.
>>       */
>> -    if (libxl__stream_write_inuse(stream))
>> -        write_emulator_xenstore_record(egc, stream);
>> +    if (libxl__stream_write_inuse(stream)) {
>> +        if (dss->remus)
> 
> Please put a small comment here as well.  Perhaps
> 
> "For remus, if libxl__xc_domain_save_done() completes, there was an
> error sending data to the secondary.  Resume the primary ASAP."

OK, I will update it in the next version.

Thanks
Wen Congyang

> 
> ~Andrew
> 
>> +            stream_complete(egc, stream, 0);
>> +        else
>> +            write_emulator_xenstore_record(egc, stream);
>> +    }
>>  }
>>  
>>  static void write_emulator_xenstore_record(libxl__egc *egc,
> 
> 
> 
> .
> 

Re: [PATCH v2 5/5] tools/libxl: remove unused function libxl__domain_save_device_model()

[Wei Liu]

On Tue, Jan 05, 2016 at 10:06:21AM +0800, Wen Congyang wrote:
> After the commit d77570e7, libxl__domain_save_device_model() can
> be dropped.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

This is incomplete as far as I can tell.

There are several references to the said function:

$ git grep libxl__domain_save_device_model

libxl.c:     * On suspend libxl__domain_save_device_model will have already
libxl_dom.c:void libxl__domain_save_device_model(libxl__egc *egc,
libxl_internal.h:    /* private for libxl__domain_save_device_model */
libxl_internal.h:_hidden void libxl__domain_save_device_model(libxl__egc *egc,

I think you haven't handled the instances showed up in line 1 and line
3.

Wei.

Re: [PATCH v2 5/5] tools/libxl: remove unused function libxl__domain_save_device_model()

[Wen Congyang]

在 2016/1/6 22:10, Wei Liu 写道:
> On Tue, Jan 05, 2016 at 10:06:21AM +0800, Wen Congyang wrote:
>> After the commit d77570e7, libxl__domain_save_device_model() can
>> be dropped.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>
> This is incomplete as far as I can tell.
>
> There are several references to the said function:
>
> $ git grep libxl__domain_save_device_model
>
> libxl.c:     * On suspend libxl__domain_save_device_model will have already

libxl__domain_save_device_model() will remove the dm_savefile. But we 
don't remove
it when using migration v2.

> libxl_dom.c:void libxl__domain_save_device_model(libxl__egc *egc,
> libxl_internal.h:    /* private for libxl__domain_save_device_model */
> libxl_internal.h:_hidden void libxl__domain_save_device_model(libxl__egc *egc,
>
> I think you haven't handled the instances showed up in line 1 and line
> 3.

Yes, I will fix it in the next version.
Thanks
Wen Congyang

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

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