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

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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/