Re: [Ext2-devel] [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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/