Re: geom_raid5 inclusion in HEAD?

On tir, nov 06, 2007 at 11:33:42 -0800, Arne W�rner wrote:
> --- John Nielsen <john_at_jnielsen.net> wrote:
> > I would like to see this get wider testing and eventually be included
> > as well. A few questions (some of which I know the answer to but would
> > like to have you field for the list):
> > 
> > Where can I get/test/review the code?
> >
> http://wgboome.homepage.t-online.de./geom_raid5.tbz
>   aka TOS (the original *ah*eh*... maybe B4 would be a nice name, too)
>   the most stable version
>   but slower than TNG/PP, if there r many requests within a short time,
>     that could be combined...
> http://wgboome.homepage.t-online.de./geom_raid5-eff.tbz
>   aka TNG
>   quite memory hungry, but faster than without any "-..."
> http://wgboome.homepage.t-online.de./geom_raid5-pp.tbz
>   pp = double-plus (I have a little thing with "1984" (by Orwell or so))
>   not as memory hungry as "-tng" and possibly not so fast...
>   but more memory hungry than without any "-..."
>   I like PP's code best (it's the cleanest of all three)...
> 
[...]
> I would say correctness has not been tested as much as necessary...
> Pawel's test with gmirror on top of a graid5 device and some trustworthy device
> would be quite good for the first...
> But maybe fluffles did that test already... I was somehow not so interested in
> graid5 in the last weeks...
> 
> > Are there any threads on the freebsd-geom or other mailing lists that
> > would be enlightening? Has a call for testing and/or code review been
> > made on that list recently?
> > 
> Not recently... And: "No"...
> IIRC they believed, that it is too much code and too complex code... Maybe that
> was the polite form of "chaotic/un-maintain-able code"... :-))
> 
Well, first I'd like to say I like the effort and the concept of the geom_raid5
class itself, and it's definately something useful. But I agree with the point 
that is some very complex code. I took some quick sweeps over the code, and the 
general impression I got was:

- Many style(9) issues.
- Lack of documentation. There are many small comments, but there is little
  description on top of functions describing their purpose and what they do.
  This makes it hard to get into it for reviewers and other developers.
- As to the code logic itself I was a bit sceptic about having the malloc saving   
  queue. Does it really improve performance that much? It's just the sort of 
  thing that could easily lead to bugs.
- I also wonder a bit why you use two worker threads, as this also increases
  complexity (but again, does it improve performance to the point that it's
  worth it?).

And last but not least: All of this have to be reviewed before going into the 
tree, and there are not many people who can do that right now. However, I
really like your work and would gladly help improving it.

-- 
Ulf Lilleengen