From owner-freebsd-current@FreeBSD.ORG Wed Nov 7 21:06:45 2007 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8855016A421 for ; Wed, 7 Nov 2007 21:06:45 +0000 (UTC) (envelope-from lulf@stud.ntnu.no) Received: from fri.itea.ntnu.no (fri.itea.ntnu.no [129.241.7.60]) by mx1.freebsd.org (Postfix) with ESMTP id 1A0D013C4A6 for ; Wed, 7 Nov 2007 21:06:44 +0000 (UTC) (envelope-from lulf@stud.ntnu.no) Received: from localhost (localhost [127.0.0.1]) by fri.itea.ntnu.no (Postfix) with ESMTP id B17488C6E; Wed, 7 Nov 2007 22:06:16 +0100 (CET) Received: from caracal.stud.ntnu.no (caracal.stud.ntnu.no [129.241.56.185]) by fri.itea.ntnu.no (Postfix) with ESMTP; Wed, 7 Nov 2007 22:06:16 +0100 (CET) Received: by caracal.stud.ntnu.no (Postfix, from userid 2312) id A8D32624109; Wed, 7 Nov 2007 22:06:35 +0100 (CET) Date: Wed, 7 Nov 2007 22:06:35 +0100 From: Ulf Lilleengen To: Arne@stud.ntnu.no, W@stud.ntnu.no Message-ID: <20071107210635.GA12736@stud.ntnu.no> References: <20071107170941.GA21274@stud.ntnu.no> <18792.71957.qm@web30311.mail.mud.yahoo.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <18792.71957.qm@web30311.mail.mud.yahoo.com> User-Agent: Mutt/1.5.9i X-Content-Scanned: with sophos and spamassassin at mailgw.ntnu.no. Cc: John Nielsen , freebsd-current@freebsd.org, "fluffles.net" Subject: Re: geom_raid5 inclusion in HEAD? X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Nov 2007 21:06:45 -0000 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