Re: Some oxenstored improvements (v3)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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@oracle.com> 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@citrix.com>

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.

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@lists.xen.org
http://lists.xen.org/xen-devel