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

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

On Fri, 2015-07-03 at 15:25 +0100, Andrew Cooper wrote:
> 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'd say that was a feature not a bug, if you are going to be getting
these things thrown back at you from some point onwards then you need to
be prepared to cope (e.g. negotiate).

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

Ian.