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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

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>

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