hello,
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.
the patch have to be applied on top of last patch set i've sent, see:
http://www.bullopensource.org/ext4/20060908/
tested on x86_64 on 20TB device.
cheers,
-- Alexandre
Signed-off-by: Alexandre Ratchov <[email protected]>
Index: e2fsprogs-1.39/lib/ext2fs/extents.c
===================================================================
--- e2fsprogs-1.39.orig/lib/ext2fs/extents.c 2006-09-14 19:16:03.000000000 +0200
+++ e2fsprogs-1.39/lib/ext2fs/extents.c 2006-09-14 20:28:16.000000000 +0200
@@ -72,8 +72,7 @@ errcode_t ext2fs_extent_verify(ext2_fils
struct ext3_extent_idx *ix, int ix_len)
{
show_extent(ex);
- /* FIXME: 48-bit support */
- if (ex->ee_start > fs->super->s_blocks_count)
+ if (EXT3_EE_START(ex) > EXT2_BLOCKS_COUNT(fs->super))
return EXT2_ET_EXTENT_LEAF_BAD;
if (ex->ee_len == 0)
@@ -84,19 +83,17 @@ errcode_t ext2fs_extent_verify(ext2_fils
if (ex->ee_block == 0)
return EXT2_ET_EXTENT_LEAF_BAD;
- /* FIXME: 48-bit support */
/* extents must be in logical offset order */
if (ex->ee_block < ex_prev->ee_block + ex_prev->ee_len)
return EXT2_ET_EXTENT_LEAF_BAD;
/* extents must not overlap physical blocks */
- if ((ex->ee_start < ex_prev->ee_start + ex_prev->ee_len) &&
- (ex->ee_start + ex->ee_len > ex_prev->ee_start))
+ if ((EXT3_EE_START(ex) < EXT3_EE_START(ex_prev) + ex_prev->ee_len) &&
+ (EXT3_EE_START(ex) + ex->ee_len > EXT3_EE_START(ex_prev)))
return EXT2_ET_EXTENT_LEAF_BAD;
}
if (ix) {
- /* FIXME: 48-bit support */
if (ex->ee_block < ix->ei_block)
return EXT2_ET_EXTENT_LEAF_BAD;
@@ -111,8 +108,7 @@ errcode_t ext2fs_extent_index_verify(ext
struct ext3_extent_idx *ix_prev)
{
show_index(ix);
- /* FIXME: 48-bit support */
- if (ix->ei_leaf > fs->super->s_blocks_count)
+ if (EXT3_EI_LEAF(ix) > EXT2_BLOCKS_COUNT(fs->super))
return EXT2_ET_EXTENT_INDEX_BAD;
if (ix_prev == NULL)
@@ -164,10 +160,9 @@ errcode_t ext2fs_extent_split(struct ext
++eh->eh_entries;
ex->ee_len = count;
- /* FIXME: 48-bit support */
ex_new->ee_len -= count;
ex_new->ee_block += count;
- ex_new->ee_start += count;
+ EXT3_EE_START_SET(ex_new, EXT3_EE_START(ex_new) + count);
return 0;
}
@@ -200,7 +195,7 @@ int block_iterate_extents(struct ext3_ex
for (i = 0; i < eh->eh_entries; i++, ex++) {
show_extent(ex);
for (j = 0; j < ex->ee_len; j++) {
- block_address = ex->ee_start + j;
+ block_address = EXT3_EE_START(ex) + j;
flags = (*ctx->func)(ctx->fs, &block_address,
(ex->ee_block + j),
ref_block, i,
@@ -214,15 +209,14 @@ int block_iterate_extents(struct ext3_ex
#ifdef EXT_DEBUG
printf("ugh, extent leaf changed: "
- "block was %u+%u = %u, now %u\n",
- ex->ee_start, j,
- ex->ee_start + j, block_address);
+ "block was %llu+%llu = %llu, now %llu\n",
+ EXT3_EE_START(ex), j,
+ EXT3_EE_START(ex) + j, block_address);
#endif
- /* FIXME: 48-bit support */
if (ex_prev &&
block_address ==
- ex_prev->ee_start + ex_prev->ee_len &&
+ EXT3_EE_START(ex_prev) + ex_prev->ee_len &&
ex->ee_block + j ==
ex_prev->ee_block + ex_prev->ee_len) {
/* can merge block with prev extent */
@@ -235,7 +229,8 @@ int block_iterate_extents(struct ext3_ex
i--; ex--;
break;
} else {
- ex->ee_start++;
+ EXT3_EE_START_SET(
+ ex, EXT3_EE_START(ex) + 1);
ex->ee_block++;
j--;
}
@@ -244,7 +239,7 @@ int block_iterate_extents(struct ext3_ex
} else if (ex->ee_len == 1) {
/* single-block extent is easy -
* change extent directly */
- ex->ee_start = block_address;
+ EXT3_EE_START_SET(ex, block_address);
ret |= BLOCK_CHANGED;
} else if (ext2fs_extent_split(eh, ex, j)) {
@@ -266,7 +261,7 @@ int block_iterate_extents(struct ext3_ex
ret |= BLOCK_ABORT |BLOCK_ERROR;
return ret;
}
- ex->ee_start = block_address;
+ EXT3_EE_START_SET(ex, block_address);
ret |= BLOCK_CHANGED;
} else {
@@ -277,7 +272,7 @@ int block_iterate_extents(struct ext3_ex
ret |= BLOCK_ABORT |BLOCK_ERROR;
return ret;
}
- ex->ee_start = block_address;
+ EXT3_EE_START_SET(ex, block_address);
ret |= BLOCK_CHANGED;
}
}
@@ -287,6 +282,7 @@ int block_iterate_extents(struct ext3_ex
char *block_buf;
struct ext3_extent_idx *ix;
struct ext3_extent_header *nh;
+ blk_t leaf;
ret = ext2fs_get_mem(ctx->fs->blocksize, &block_buf);
if (ret)
@@ -298,15 +294,16 @@ int block_iterate_extents(struct ext3_ex
show_index(ix);
if (!(ctx->flags & BLOCK_FLAG_DEPTH_TRAVERSE) &&
!(ctx->flags & BLOCK_FLAG_DATA_ONLY)) {
- ret |= (*ctx->func)(ctx->fs, &ix->ei_leaf,
+ leaf = EXT3_EI_LEAF(ix);
+ ret |= (*ctx->func)(ctx->fs, &leaf,
BLOCK_COUNT_IND, ref_block,
i, ctx->priv_data);
+ EXT3_EI_LEAF_SET(ix, leaf);
if (ret & BLOCK_ABORT)
goto free_buf;
}
- 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);
if (ctx->errcode) {
ret |= BLOCK_ERROR;
goto free_buf;
@@ -316,21 +313,23 @@ int block_iterate_extents(struct ext3_ex
ret |= BLOCK_ERROR;
goto free_buf;
}
- flags = block_iterate_extents(nh, ix->ei_leaf, i, ctx);
+ flags = block_iterate_extents(nh, EXT3_EI_LEAF(ix), i, ctx);
if (flags & BLOCK_CHANGED)
ctx->errcode =
- ext2fs_write_ext_block(ctx->fs,
- ix->ei_leaf,
- block_buf);
+ ext2fs_write_ext_block(
+ ctx->fs, EXT3_EI_LEAF(ix), block_buf);
if (flags & BLOCK_ABORT) {
ret |= BLOCK_ABORT;
goto free_buf;
}
if ((ctx->flags & BLOCK_FLAG_DEPTH_TRAVERSE) &&
- !(ctx->flags & BLOCK_FLAG_DATA_ONLY))
- ret |= (*ctx->func)(ctx->fs, &ix->ei_leaf,
+ !(ctx->flags & BLOCK_FLAG_DATA_ONLY)) {
+ leaf = EXT3_EI_LEAF(ix);
+ ret |= (*ctx->func)(ctx->fs, &leaf,
BLOCK_COUNT_IND, ref_block,
i, ctx->priv_data);
+ EXT3_EI_LEAF_SET(ix, leaf);
+ }
}
free_buf:
Index: e2fsprogs-1.39/lib/ext2fs/ext3_extents.h
===================================================================
--- e2fsprogs-1.39.orig/lib/ext2fs/ext3_extents.h 2006-09-08 19:04:46.000000000 +0200
+++ e2fsprogs-1.39/lib/ext2fs/ext3_extents.h 2006-09-14 20:29:13.000000000 +0200
@@ -75,6 +75,13 @@ struct ext3_extent {
__u32 ee_start; /* low 32 bigs of physical block */
};
+#define EXT3_EE_START(e) (((blk_t)(e)->ee_start_hi << 32) + (e)->ee_start)
+#define EXT3_EE_START_SET(e,blk) \
+do { \
+ (e)->ee_start = (__u32)(blk); \
+ (e)->ee_start_hi = (blk) >> 32; \
+} while(0)
+
/*
* this is index on-disk structure
* it's used at all the levels, but the bottom
@@ -87,6 +94,13 @@ struct ext3_extent_idx {
__u16 ei_unused;
};
+#define EXT3_EI_LEAF(ix) (((blk_t)(ix)->ei_leaf_hi << 32) + (ix)->ei_leaf)
+#define EXT3_EI_LEAF_SET(e,blk) \
+do { \
+ (ix)->ei_leaf = (__u32)(blk); \
+ (ix)->ei_leaf_hi = (blk) >> 32; \
+} while(0)
+
/*
* each block (leaves and indexes), even inode-stored has header
*/
Index: e2fsprogs-1.39/lib/ext2fs/bmap.c
===================================================================
--- e2fsprogs-1.39.orig/lib/ext2fs/bmap.c 2006-09-08 19:04:46.000000000 +0200
+++ e2fsprogs-1.39/lib/ext2fs/bmap.c 2006-09-14 20:22:20.000000000 +0200
@@ -46,7 +46,7 @@ static errcode_t block_bmap_extents(stru
for (i = 0; i < eh->eh_entries; i++, ex++) {
if ((ex->ee_block <= block) &&
(block < ex->ee_block + ex->ee_len)) {
- *phys_blk = ex->ee_start +
+ *phys_blk = EXT3_EE_START(ex) +
(block - ex->ee_block);
return 0;
}
@@ -68,7 +68,7 @@ static errcode_t block_bmap_extents(stru
if (ix->ei_block < block)
continue;
- ret = io_channel_read_blk(fs->io, ix->ei_leaf, 1,
+ ret = io_channel_read_blk(fs->io, EXT3_EI_LEAF(ix), 1,
block_buf);
if (ret) {
ret = BLOCK_ERROR;
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.
On Fri, Sep 15, 2006 at 10:57:43AM -0600, Andreas Dilger wrote:
> 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.
just to be sure to get it right: we allow 32bit file-systems to have extents
with _hi bits set, right? (and *_hi are ignored)
so we can't simply assume that extents are always 48bit, in which case it
would be enough to just check that they are inside the block group (that
would detect extents with *_hi set as corrupt).
> - 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 the 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?
>
yes of course; i'll try take the latest e2fsprogs (-wip) and update all my
patches following your suggestions... clean-up, re-ording, coding style,
naming, etc.
thanks for your comments, they are very helpful for me
-- Alexandre
On Sep 15, 2006 20:11 +0200, Alexandre Ratchov wrote:
> On Fri, Sep 15, 2006 at 10:57:43AM -0600, Andreas Dilger wrote:
> > - 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.
>
> just to be sure to get it right: we allow 32bit file-systems to have extents
> with _hi bits set, right? (and *_hi are ignored)
Well, this is an undesirable side-effect of the old 32-bit extents code.
e2fsck should just clear the _hi fields for 32-bit filesystems. For
larger filesystems it should assume they are valid. That means that the
PR_1_EXTENT_HI checks should just be skipped for > 32-bit filesystems.
> so we can't simply assume that extents are always 48bit, in which case it
> would be enough to just check that they are inside the block group (that
> would detect extents with *_hi set as corrupt).
For now I wouldn't change the ext4 code. The latest CFS e2fsprogs and kernel
code clears the _hi fields correctly, and we can get our users to run an
e2fsck before they ever start using ext4-based code (which likely won't be
for a year or more) if they have a filesystem which still has _hi values set.
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.