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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-01-12 at 09:36 +0800, Wen Congyang wrote:
> On 01/09/2016 12:20 AM, Ian Campbell wrote:
> > On Fri, 2016-01-08 at 14:38 +0800, Wen Congyang wrote:
> > > 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.
> > 
> > This implies that the stream is currently at a consistent point.
> > Whereas
> > what I think is meant is that things have failed (perhaps halfway
> > through a
> > checkpoint, i.e. not at a consistent state), therefore we stop and
> > continue
> > with the previous fully consistent checkpoint (which may have been
> > earlier
> > in the stream, not at the current point). Is that right?
> 
> The state is always consistent, because we buffer the state until all state are
> received.

I think the comment is misleading, it implies that the state up to and
including the current point is consistent, when really there is buffered
partial state which is not consistent (because it is partial). This
misleadingness is mainly because of the "at a consistent checkpoint"
wording I think.


> 
> > 
> > And what does "ready to go" mean? Does it mean that we will return back
> > to
> > the next higher level or that we go somewhere else first?
> 
> stream's callback will be called to resume the guest.

Please update the comment.

> > 
> > The big comment about flow control at the top of this file doesn't seem to
> > cover the checkpoint case, if it did I suspect I would have found the
> > answers there.
> 
> We buffer the state in xc_sr_restore.c.

Perhaps I was too subtle: Please update the comment about flow control at
the top of the file.

Ian.

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