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

From: Wen Congyang <wency@cn.fujitsu.com>
To: Ian Campbell <ian.campbell@citrix.com>,
	xen devel <xen-devel@lists.xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Wei Liu <wei.liu2@citrix.com>,
	Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v3 1/5] remus: don't call stream_continue() when doing failover
Date: Tue, 12 Jan 2016 09:36:42 +0800	[thread overview]
Message-ID: <569458AA.7070601@cn.fujitsu.com> (raw)
In-Reply-To: <1452270039.26438.40.camel@citrix.com>

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.

> 
> 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.

> 
> 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.

Thanks
Wen Congyang

> 
>> +             */
>> +            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);
>> +        }
>>      }
>>  }
>>  
> 