Re: geom_raid5 inclusion in HEAD?

On ons, nov 07, 2007 at 10:57:18 -0800, Arne W�rner wrote:
> --- Ulf Lilleengen <lulf_at_stud.ntnu.no> 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 ((<number of disks>-1)*<stripe size>) 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