Re: [Xen-devel] Some oxenstored improvements (v3)

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Oct 08, 2014 at 12:59:32PM +0000, Dave Scott wrote:
> Hi,
> 
> On 7 Oct 2014, at 14:24, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> 
> > On Mon, Sep 29, 2014 at 08:55:09PM +0000, Dave Scott wrote:
> >> Hi,
> >> 
> >> On 29 Sep 2014, at 15:37, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
> >> wrote:
> >> 
> >>> On Thu, Sep 25, 2014 at 06:34:53PM +0100, Zheng Li wrote:
> >>>> This is mainly about removing the 1024 fds limitation in the current 
> >>>> oxenstored
> >>>> implementation. We also fixed some bugs and made some perf improvements 
> >>>> along
> >>>> the way.
> >>>> 
> >>>> This is v3. The first 6 patches are the same as in v2. For patch 7/8/9, 
> >>>> we
> >>>> 
> >>>> * Add a safe net mechanism for ill-behaved legacy clients
> >>>> * Make some performance improvement to reduce syslog workload
> >>>> * Small refactoring on patch 7/8 to accomodate the new changes
> >>> 
> >>> That all looks quite nice but I have no experience with OCaml.
> >>> 
> >>> I fear that this patchset will have wait until an OCaml expert can
> >>> review the code :-(
> >> 
> >> Iâve just read through the v3 patches and am happy with them. So Iâm happy 
> >> to give them an
> >> 
> >>  Acked-by: David Scott <dave.scott@xxxxxxxxxx>
> > 
> > Excellent!
> >> 
> >> I think this patch:
> >> 
> >> [PATCH v3 4/9] oxenstored: catch the error when a connection is already 
> >> deleted
> >> 
> >> is a useful bug fix â this might explain why oxenstored has occasionally 
> >> âdisappearedâ in mysterious circumstances. Zheng: could this patch be 
> >> taken by itself? If so I think the release should definitely have it.
> >> 
> >> The rest of the patches will help scalability a lot but arenât critical 
> >> fixes. They would (IMHO) increase the quality of the release though.
> > 
> > The bug-fix I believe should go in Xen 4.5.
> > 
> > 
> > In regards to the rest (help scalability) I am worried that:
> > - This is rather unknown code for me (OCaml) but then so is the ARM 
> > assembler
> >   (I am slowly digging through the manual) - so I am falling back
> >   here on the maintainers perspective. Dave says OK, so that means
> >   we are OK.
> > - Now on the other hand this is core code. If this goes belly up we a
> >   screwed - and if this introduces races, it will be quite a headache
> >   to troubleshoot down to this.
> > 
> > In terms of the positive aspects:
> > - It will improve the code quality.
> > - It improves scalability.
> > 
> > Dave, when you said "am happy with them" (and giving it an Acked-by) - does
> > that mean you dug through the code carefully with a microscope looking for
> > ways to break it? Or was it more of a cursory view?
> > 
> > Asking because if it is the first then an 'Reviewed-by' is more apt
> > and it also means that the chance of the code going 'belly-up' is lesser.
> > 
> > If you are comfortable giving an 'Reviewed-by' then I believe
> > it can go in Xen 4.5 (and lowers the 'belly-up' chance). If you don't
> > have the cycles then I believe it should go in Xen 4.6 to give it some
> > "soak" time.
> 
> Iâve re-examined the patches and canât see anything wrong with them. I talked
> to Zheng off-list about how the patches were tested. Zheng has run a lot of
> test sequences, many of them are very xenstore-intensive. Reassuringly he
> found a bug â not in his patches, but in en earlier version of my âxenstore
> reconnectionâ patch (subsequently fixed).
> 
> So Iâm confident to say:
> 
> Reviewed-by: David Scott <dave.scott@xxxxxxxxxx>

Excellent, hence:

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Thank you!
> 
> Cheers,
> Dave
> 
> 
> > 
> > If there are any doubts or if the author is not too keen on debugging
> > issues during the next couple of months (say, if you have vacation
> > planned or such) - if of course there are bugs - we can also postpone
> > these patches and put them in Xen 4.6 and have a more time to sort
> > issues out.
> > 
> > Thank you!
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
> 

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