Re: [PATCH v3 COLOPre 19/26] libxc/migration: Specification update for DIRTY_BITMAP records

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/07/15 12:00, Ian Campbell wrote:
> On Wed, 2015-07-01 at 11:39 +0100, Andrew Cooper wrote:
>> On 01/07/15 11:27, Ian Campbell wrote:
>>> On Wed, 2015-07-01 at 11:16 +0100, Andrew Cooper wrote:
>>>> On 01/07/15 04:07, Yang Hongyang wrote:
>>>>> On 06/30/2015 06:24 PM, Ian Campbell wrote:
>>>>>> On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
>>>>>>> Used by secondary to send it's dirty bitmap to primary under COLO.
>>>>>> This is the backchannel, right?
>>>>> Right.
>>>>>
>>>>>> It seems to me that this ought to be described more clearly as a
>>>>>> separate stream in the opposite direction, rather than looking like just
>>>>>> another record in the forward channel.
>>>>> Agreed, I'm not sure if having this back channel record is eligible,
>>>>> Andy, thoughts?
>>>>>
>>>>>> Does the back channel not also need some sort of negotiation phase where
>>>>>> we check both ends are compatible (i.e. like the forward channel's
>>>>>> header). This might be easier than with the forward channel since you
>>>>>> might assert that the versions must match exactly for COLO to be
>>>>>> possible, that might not be true of some potential future user of the
>>>>>> backchannel though.
>>>>> The negotiation record for COLO is introduced in the following patch
>>>>> on libxl side. But that might be diffrent form what you said here, we
>>>>> don't have a version check currently, if the 2 side doesn't match, for
>>>>> example one has colo feature enabled and the other end do not, the
>>>>> migration will simply fail.
>>>> I do think that each backchannel level needs some kind of initial
>>>> negotiation to confirm everything is set up and working, but I think the
>>>> backchannel should also match the spec for its level, and all contained
>>>> in the single spec document.
>>> In the same spec, sure. It's the presenting it as just another record
>>> mixed in with all the others which I think is a problem.
>> Ah I see.  Yes - this would better be avoided.
>>
>>> At the very least every record should be tagged as either forward,
>>> backward or bidirectional to indicate who can produce and who should
>>> consume it.
>>>
>>> Even better would br if we can convince ourselves there should be no
>>> bidirectional fields, in which case I'd be further inclined to say that
>>> the record space should be explicitly separate. i.e. the backchannel
>>> should be a separate chapter in the doc and the records.
>> I think it would be unwise to rule out the possibility of bidirectional
>> records.  In the case that we get to a position of wanting/needing them,
>> we absolutely don't want a bidirectional record to have different id in
>> the forwards and backwards direction.
> Agreed.
>
> Perhaps we should reserve some space for forward/backward/bidir records
> in the record id space? Bit 31 is already the optional flag so e.g.
> perhaps bit 30 = Backwards and bit 29 = !Forwards.
>
> So a forward mandatory id would be 0x0......., backward would be
> 0x4....... and bidir would be 0x6......., optional bidir would be
> 0xe....... etc?
>
> The weird inversion of Forward is in order to retain the existing record
> ids.

I don't see how this helps.  It will break binary compatibility if an
existing forward record wants to be changed to being bidirectional.

I think that some clear identification in the spec, and perhaps a helper
or two in xc_sr_common.c is going to be the easiest solution to this
problem.

~Andrew