Re: [Ext2-devel] [PATCH 1/9] extents for ext4

From: Alex Tomas
Date: Thu Aug 10 2006 - 05:25:48 EST

>>>>> 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


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


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


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.


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) {


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@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at