Re: [PATCH 1/9] extents for ext4

On Wed, 09 Aug 2006 18:20:26 -0700
Mingming Cao <cmm@xxxxxxxxxx> wrote:


Add extent map support to ext4. Patch from Alex Tomas.

On disk extents format:
/*
* this is extent on-disk structure
* it's used at the bottom of the tree
*/
struct ext3_extent {
__le32 ee_block; /* first logical block extent covers */
__le16 ee_len; /* number of blocks covered by extent */
__le16 ee_start_hi; /* high 16 bits of physical block */
__le32 ee_start; /* low 32 bigs of physical block */
};


From a quick scan:

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

- Far, far too many inlines

- overly-terse variable naming

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

- Needs kmalloc()->kzalloc() conversion

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

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

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

- It has about 7,000 instances of

if ((lhs = expression)) {

whereas the preferred coding style is

lhs = expression;
if (lhs) {

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


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