Re: [patch] 48bit extents in e2fsprogs

[Andreas Dilger <adilger@clusterfs.com>]

On Sep 15, 2006  14:05 +0200, Alexandre Ratchov wrote:
> here is a patch that fixes 48bit extents in e2fsprogs. Basically, it wraps
> acces to 48bit filds of extent indexes and leaves in macros, so 32bit fields
> are no more (mis)used, see EXT3_EE_START and EXT3_EI_LEAF macros definitions.

This looks mostly good...  Some minor comments.
- please wrap lines at 80 columns
- the check for ee_start_hi and ei_leaf_hi fields (PR_1_EXTENT_HI) needs to
  be fixed (I don't see it changed here) so that it considers that an error
  only if INCOMPAT_64BIT flag is set and the filesystem is > 2^32 blocks.
  That is in e2fsck_ext_block_verify()
- (FYI) In my definition of PR_1_EXTENT_HI I recently added the PR_PREEN_NOMSG
  flag because users were confused about the "High 16 bits of extent/index
  block set" message even though it is harmless for 32-bit filesystems.
- In my patch (due to Ted's upstream repository changes) I've renamed
  everything to be ext4_* and EXT4_*.  Ted renamed EXT3_EXTENTS_FL to be
  EXT4_EXTENTS_FL, and I'm guessing he'll do the same with the rest...

>  	if (ix) {
> -		/* FIXME: 48-bit support */
>  		if (ex->ee_block < ix->ei_block)

My bad...

> @@ -298,15 +294,16 @@ int block_iterate_extents(struct ext3_ex
>  			}
> -			ctx->errcode = ext2fs_read_ext_block(ctx->fs,
> -							     ix->ei_leaf,
> -							     block_buf);
> +			ctx->errcode = ext2fs_read_ext_block(
> +				ctx->fs, EXT3_EI_LEAF(ix), block_buf);

I think the original code is more consistent with the e2fsprogs coding style.

As always, many thanks for he good work.


Can you also please change your patch series NOT to add s_*_count_hi in
the wrong place in 16-blk-64bit, and then change it back in 64bit-fixsb?
That is very dangerous if the patch series is partially used and just
adds confusion when reviewing the patches.

Also, the same 16-blk-64bit patch uses EXT2_FEATURE_RO_COMPAT_64BIT, but
later this is changed in 20-blk-64bit-compat to be INCOMPAT_64BIT.  I
suspect Ted will want to call this EXT4_FEATURE_INCOMPAT_64BIT in the end
(maybe he can comment on what the preferred names are for all the new flags).

Can you please fix this in the original patches instead of adding a later
patch that fixes the previous patches?

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.