Re: [PATCH 1/9] extents for ext4

[Andrew Morton <akpm@osdl.org>]

On Wed, 09 Aug 2006 18:20:26 -0700
Mingming Cao <cmm@us.ibm.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/