Re: [PATCH 1/9] extents for ext4

Andrew Morton
2006-08-10T06:40:10Z

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.