On ons, nov 07, 2007 at 10:57:18 -0800, Arne W�rner wrote:
> --- Ulf Lilleengen wrote:
> > - Many style(9) issues.
> >

> Hmm... Would "cb" help? R some function too long? I tried to comply to Pawel's
> style, but obviously I deviated from it after some weeks...
> Could give me an example of the worst issue?

Well, this might sound picky, but it's still a style issue:
- Parantheses around return values:

static __inline u_int
g_raid5_nvalid(struct g_raid5_softc *sc)
{
u_int i, no;

no = 0;
for (i = 0; i < sc->sc_ndisks; i++)
if (g_raid5_disk_good(sc,i))
no++;

- return no;
+ return (no);
}


- Proper spacing between variable declaration and function body.
struct g_consumer **cpp = cp->private;
+
if (cpp == NULL)
return -1;

- Declaration of variables should be in the top of the function.
struct g_consumer **cpp = cp->private;
if (cpp == NULL)
return -1;
struct g_consumer *rcp = *cpp;
if (rcp == NULL)
return -1;
int dn = cpp - sc->sc_disks;

- Proper indenting when breaking a line, should be 4 spaces etc.

All of this can be found in the style(9) manpage, so I'd rather just suggest
to go through it and make sure it's satisfied.

> > - 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.
> >

> Hmm... Yup...
>
> There r interface functions to the GEOM system: ..._start(), ..._done(),
> ..._create(), and so on...
> Then there r 2 worker threads (one for the graid5 start queue (...worker()) and
> one for the graid5 done queue (...workerD()).
> The other functions r helper functions...
> I could add the function-purpose-comment in PP and then try to merge it to TNG
> and TOS...

I've only looked at TOS so far, but I'll look at PP as well to see how it is.

>
> > - 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.
> >

> Hmm... if I understood correctly, FreeBSD's kernel memory suffers under
> fragmentation, if many big mem areas r needed... There might be even a dead
> lock, if UFS uses 64kb block size... So I thought it would be a good idea to
> avoid those sleeps but "hamster-ing" the big chunks... But I am not sure
> anymore, that it improved performance (but performance was the reason for
> it)...

Hmm, I'm not sure what you mean about this dead lock, but sounds like a weird
thing to having to deadlock because of your filesystem. Maybe this could be
solved in another way, or is this not a graid5-thing at all?

The general thing is that I don't think one should start optimizing for
performance before everything works correctly and having made sure that it
improves performance statistically. (I know this isn't a completely new
project, but still).
>
> > - 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?).
> >

> Hmm... I think so... At least on MP boxes, since both threads do some XOR-ing
> (worker() uses XOR for writing "full-stripes" (where no read is necessary) and
> bcopy; and workerD() uses XOR after the old data/parity has been read)...

First of all, disk I/O is generally much slower than CPU anyway, so I would
doubt that having to use one thread would decrease performance noticeably.
In my ears, this is a good argument for using one thread only. But then
again, this might not be a big deal if it's already there and it's correct.

>
> > 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.
> >

> OK... review sounds good... maybe we should concentrate on PP then (it is quite
> space (in comparison to TNG but not TOS)+time (in comparison to TOS; maybe in
> comparison to TNG, too?) efficient and has a read cache)? Although fluffles
> favors TNG, although it is quite nasty (a write request of size 4KB costs 3
> full stripes ((-1)*) plus 2*128KB... *giggle*)...
>

I'm starting to get busier and busier with exams coming up now, but I'll try
take a look when I can, but don't expect to much Also, as I said, I've
only looked at TOS so far.

--
Ulf Lilleengen
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/lis...reebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"