Re: [PATCH 1/9] extents for ext4

[Alex Tomas <alex@clusterfs.com>]

>>>>> Andrew Morton (AM) writes:

 >> From a quick scan:

 AM> - The code is very poorly commented.  I'd want to spend a lot of time
 AM>   reviewing this implementation, but not in its present state.  

what sort of comments are you expecting?

 AM> - Far, far too many inlines

probably, I'll review the code in this regard

 AM> - overly-terse variable naming

same

 AM> - There are several places which appear to be putting block numbers into
 AM>   an `int'.

same

 AM> - Needs kmalloc()->kzalloc() conversion

OK

 AM> - replace all brelse() calls with put_bh().  Because brelse() is
 AM>   old-fashioned, has a weird name and neelessly permits a NULL arg.

 AM>   In fact it would be beter to convert JBD and ext3 to put_bh before
 AM>   copying it all over.

OK

 AM> - The open-coded __clear_bit(BH_New, ...) in ext4_ext_get_blocks is a bit
 AM>   nasty.  We can live with nasty, but are we sure that it isn't buggy??

I believe it isn't buggy -- it applies to non-shared var.
it also showed minor improvement on SMP.

 AM> - It has about 7,000 instances of

 AM> 	if ((lhs = expression)) {

 AM>   whereas the preferred coding style is

 AM> 	lhs = expression;
 AM> 	if (lhs) {

OK

 AM> - The existing comments could benefit from some rework by a native English
 AM>   speaker.

could someone assist here, please?

thanks, Alex

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642