[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 --for 4.6 COLOPre 07/25] libxl/remus: init checkpoint_callback in Remus checkpoint callback



On Thu, 2015-07-16 at 19:00 +0800, Yang Hongyang wrote:
> 
> On 07/16/2015 06:32 PM, Ian Campbell wrote:
> > On Wed, 2015-07-15 at 20:35 +0800, Yang Hongyang wrote:
> >>
> >> On 07/15/2015 08:02 PM, Ian Campbell wrote:
> >>> On Wed, 2015-07-15 at 15:45 +0800, Yang Hongyang wrote:
> >>>> init stream {read/write} state checkpoint_callback in Remus
> >>>> checkpoint callback.
> >>>
> >>> Why? Is this earlier or later than previously? Seems later?
> >>
> >> There's no functional change, it's just refactoring so that we can move
> >> all remus code into one file.
> >
> > That answers the why, thanks.
> >
> > But, it would be a no functional change if the initialisation was moving
> > from e.g. the very start of a function to the caller of that function
> > right before the call or from the end of a function to the caller right
> > after the call.
> >
> > But AFAICT this movement is a bit more than that, e.g. the init of
> > dcs->srs.checkpoint_callback has moved from near the end of
> > domcreate_bootloader_done to
> > libxl__remus_domain_restore_checkpoint_callback before a call to
> > libxl__stream_read_start_checkpoint, which doesn't have the property I
> > describe above, at least not in an obvious way.
> >
> > So there is either a span of time where the callback is no longer
> > initialised when it was before, or it is initialised for a larger span
> > that it was before (with the former having the larger potential for
> > issues).
> >
> > It's also not entirely clear that the new location of the initialisation
> > is traversed on all the same paths as before, or if it happens on fewer
> > paths that those are exactly the ones which matter.
> >
> > Lastly it seems odd to split out the initialisation of only one member
> > of dcs->srs and dcs->sws into a different location to all the others,
> > especially putting it into the checkpoint callback (which is called
> > repeatedly).
> >
> > Perhaps what is really needed here is a new function to initialise a
> > dcs->srs for remus, and another for dcs->sws to be called exactly where
> > the init happens today?
> 
> The checkpoint_callback() only used by remus, you can see from the
> initialisation line:
> dss->sws.checkpoint_callback = remus_checkpoint_stream_written;
> dcs->srs.checkpoint_callback = remus_checkpoint_stream_done;
> 
> These two functions remus_checkpoint_stream_written &
> remus_checkpoint_stream_done only called when libxc call
> chaeckpoint() callback, so there should be no problem
> with the move. Given the fact it's only used by Remus, init
> it in previous place is not a good idea IMO.

I suggested a pair of Remus specific functions to init an sws or srs
stream for Remus, called in the same place, which is not the same thing
as just leaving it there, nor of initialising this state from the
callback itself. You can call that init function on the same condition
as I suppose you will eventually apply to:
    callbacks->checkpoint = libxl__remus_domain_checkpoint_callback;

Or perhaps have the remus (and other checkpoint types if necessary)
setup code fill in two callbacks in a higher level struct which are
called at the appropriate points to init dss->{sws,srs}?

Ian.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.