Date: Wed, 7 Nov 2007 22:06:35 +0100 From: Ulf Lilleengen <lulf@stud.ntnu.no> To: Arne@stud.ntnu.no, W@stud.ntnu.no Cc: John Nielsen <john@jnielsen.net>, freebsd-current@freebsd.org, "fluffles.net" <bsd@fluffles.net> Subject: Re: geom_raid5 inclusion in HEAD? Message-ID: <20071107210635.GA12736@stud.ntnu.no> In-Reply-To: <18792.71957.qm@web30311.mail.mud.yahoo.com> References: <20071107170941.GA21274@stud.ntnu.no> <18792.71957.qm@web30311.mail.mud.yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On ons, nov 07, 2007 at 10:57:18 -0800, Arne W�rner wrote:
> --- Ulf Lilleengen <lulf@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071107210635.GA12736>