2009-07-20 03:57:15

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 1/3] libext2fs: Fix regression in ext2fs_extent_set_bmap()

Commit 0dc291611 introduced a regression when unmapping the first
block in an extent. This caused e2fsck -fD to corrupt large
directories if the directory has to shrink by more than one block.
The problem was set_bmap should only go to a next leaf when setting a
first block in an extent, and not when it is unmapping the first block
in an extent.

Addresses-Debian-Bug: #537510

Signed-off-by: "Theodore Ts'o" <[email protected]>
---
lib/ext2fs/extent.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 7fcc2cf..9d7b7de 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -1296,11 +1296,12 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
}
if (retval)
goto done;
+ retval = ext2fs_extent_get(handle,
+ EXT2_EXTENT_NEXT_LEAF,
+ &extent);
+ if (retval)
+ goto done;
}
- retval = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT_LEAF,
- &extent);
- if (retval)
- goto done;
extent.e_pblk++;
extent.e_lblk++;
extent.e_len--;
--
1.6.3.2.1.gb9f7d.dirty



2009-07-20 03:57:15

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 2/3] libext2fs: Improve debugging printf's in extent.c

Comment out less common debugging printf's, and fix some type
warnings. Add high-level debugging printf's for ext2fs_extent_goto(),
ext2fs_extent_insert(), ext2fs_extent_delete(), ext2fs_extent_replace()

Signed-off-by: "Theodore Ts'o" <[email protected]>
---
lib/ext2fs/extent.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-------
1 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 9d7b7de..c47fbcc 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -104,7 +104,7 @@ static void dbg_print_extent(char *desc, struct ext2fs_extent *extent)
{
if (desc)
printf("%s: ", desc);
- printf("extent: lblk %llu--%llu, len %lu, pblk %llu, flags: ",
+ printf("extent: lblk %llu--%llu, len %u, pblk %llu, flags: ",
extent->e_lblk, extent->e_lblk + extent->e_len - 1,
extent->e_len, extent->e_pblk);
if (extent->e_flags & EXT2_EXTENT_FLAGS_LEAF)
@@ -321,7 +321,7 @@ retry:
return EXT2_ET_EXTENT_NO_NEXT;
}
if (op != EXT2_EXTENT_NEXT_SIB) {
-#ifdef DEBUG
+#ifdef DEBUG_GET_EXTENT
printf("<<<< OP = %s\n",
(op == EXT2_EXTENT_DOWN) ? "down" :
((op == EXT2_EXTENT_UP) ? "up" : "unknown"));
@@ -352,7 +352,7 @@ retry:
return EXT2_ET_EXTENT_NO_PREV;
}
if (op != EXT2_EXTENT_PREV_SIB) {
-#ifdef DEBUG
+#ifdef DEBUG_GET_EXTENT
printf("<<<< OP = %s\n",
(op == EXT2_EXTENT_DOWN_AND_LAST) ? "down/last" :
((op == EXT2_EXTENT_UP) ? "up" : "unknown"));
@@ -366,7 +366,7 @@ retry:
op = EXT2_EXTENT_DOWN;
else
op = EXT2_EXTENT_LAST_SIB;
-#ifdef DEBUG
+#ifdef DEBUG_GET_EXTENT
printf("<<<< OP = %s\n",
(op == EXT2_EXTENT_DOWN) ? "down" : "last_sib");
#endif
@@ -481,7 +481,7 @@ retry:
if (handle->level < handle->max_depth)
path->visit_num = 1;
}
-#ifdef DEBUG
+#ifdef DEBUG_GET_EXTENT
printf("Down to level %d/%d, end_blk=%llu\n",
handle->level, handle->max_depth,
path->end_blk);
@@ -495,7 +495,7 @@ retry:
return EXT2_ET_NO_CURRENT_NODE;

extent->e_flags = 0;
-#ifdef DEBUG
+#ifdef DEBUG_GET_EXTENT
printf("(Left %d)\n", path->left);
#endif

@@ -628,7 +628,14 @@ static errcode_t extent_goto(ext2_extent_handle_t handle,
return EXT2_ET_OP_NOT_SUPPORTED;
}

+#ifdef DEBUG
+ printf("goto extent ino %u, level %d, %llu\n", handle->ino,
+ leaf_level, blk);
+#endif
+
+#ifdef DEBUG_GOTO_EXTENTS
dbg_print_extent("root", &extent);
+#endif
while (1) {
if (handle->max_depth - handle->level == leaf_level) {
/* block is in this &extent */
@@ -658,7 +665,9 @@ static errcode_t extent_goto(ext2_extent_handle_t handle,
if (retval)
return retval;

+#ifdef DEBUG_GOTO_EXTENTS
dbg_print_extent("next", &extent);
+#endif
if (blk == extent.e_lblk)
goto go_down;
if (blk > extent.e_lblk)
@@ -669,7 +678,9 @@ static errcode_t extent_goto(ext2_extent_handle_t handle,
if (retval)
return retval;

+#ifdef DEBUG_GOTO_EXTENTS
dbg_print_extent("prev", &extent);
+#endif

go_down:
retval = ext2fs_extent_get(handle, EXT2_EXTENT_DOWN,
@@ -677,7 +688,9 @@ static errcode_t extent_goto(ext2_extent_handle_t handle,
if (retval)
return retval;

+#ifdef DEBUG_GOTO_EXTENTS
dbg_print_extent("down", &extent);
+#endif
}
}

@@ -765,6 +778,11 @@ errcode_t ext2fs_extent_replace(ext2_extent_handle_t handle,
if (!path->curr)
return EXT2_ET_NO_CURRENT_NODE;

+#ifdef DEBUG
+ printf("extent replace: %u ", handle->ino);
+ dbg_print_extent(0, extent);
+#endif
+
if (handle->level == handle->max_depth) {
ex = path->curr;

@@ -922,7 +940,8 @@ static errcode_t extent_node_split(ext2_extent_handle_t handle)
goto done;

#ifdef DEBUG
- printf("will copy to new node at block %lu\n", new_node_pblk);
+ printf("will copy to new node at block %lu\n",
+ (unsigned long) new_node_pblk);
#endif

/* Copy data into new block buffer */
@@ -1040,6 +1059,11 @@ errcode_t ext2fs_extent_insert(ext2_extent_handle_t handle, int flags,
if (!handle->path)
return EXT2_ET_NO_CURRENT_NODE;

+#ifdef DEBUG
+ printf("extent insert: %u ", handle->ino);
+ dbg_print_extent(0, extent);
+#endif
+
path = handle->path + handle->level;

if (path->entries >= path->max_entries) {
@@ -1132,6 +1156,11 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,

EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EXTENT_HANDLE);

+#ifdef DEBUG
+ printf("set_bmap ino %u log %lld phys %lld flags %d\n",
+ handle->ino, logical, physical, flags);
+#endif
+
if (!(handle->fs->flags & EXT2_FLAG_RW))
return EXT2_ET_RO_FILSYS;

@@ -1364,6 +1393,19 @@ errcode_t ext2fs_extent_delete(ext2_extent_handle_t handle, int flags)
if (!handle->path)
return EXT2_ET_NO_CURRENT_NODE;

+#ifdef DEBUG
+ {
+ struct ext2fs_extent extent;
+
+ retval = ext2fs_extent_get(handle, EXT2_EXTENT_CURRENT,
+ &extent);
+ if (retval == 0) {
+ printf("extent delete %u ", handle->ino);
+ dbg_print_extent(0, &extent);
+ }
+ }
+#endif
+
path = handle->path + handle->level;
if (!path->curr)
return EXT2_ET_NO_CURRENT_NODE;
@@ -1886,7 +1928,8 @@ void do_goto_block(int argc, char **argv)
retval = extent_goto(current_handle, level, (blk64_t) blk);

if (retval) {
- com_err(argv[0], retval, "while trying to go to block %lu, level %d",
+ com_err(argv[0], retval,
+ "while trying to go to block %u, level %d",
blk, level);
return;
}
--
1.6.3.2.1.gb9f7d.dirty


2009-07-20 03:57:15

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 3/3] libext2fs: Avoid creating unneeded new extents in ext2fs_extent_set_bmap()

Avoiding inserting a new extent if it is possible to merge the new
block to the beginning or the end of the previous or next extent.

Signed-off-by: "Theodore Ts'o" <[email protected]>
---
lib/ext2fs/extent.c | 118 ++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 93 insertions(+), 25 deletions(-)

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index c47fbcc..1505447 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -1146,11 +1146,14 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
int mapped = 1; /* logical is mapped? */
int orig_height;
int extent_uninit = 0;
+ int prev_uninit = 0;
+ int next_uninit = 0;
int new_uninit = 0;
int max_len = EXT_INIT_MAX_LEN;
+ int has_prev, has_next;
blk64_t orig_lblk;
struct extent_path *path;
- struct ext2fs_extent extent;
+ struct ext2fs_extent extent, next_extent, prev_extent;
struct ext2fs_extent newextent;
struct ext2_extent_info info;

@@ -1222,12 +1225,44 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
/*
* This may be the extent *before* the requested logical,
* if it's currently unmapped.
+ *
+ * Get the previous and next leaf extents, if they are present.
*/
retval = ext2fs_extent_get(handle, EXT2_EXTENT_CURRENT, &extent);
if (retval)
goto done;
if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
extent_uninit = 1;
+ retval = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT_LEAF, &next_extent);
+ if (retval) {
+ has_next = 0;
+ if (retval != EXT2_ET_EXTENT_NO_NEXT)
+ goto done;
+ } else {
+ dbg_print_extent("set_bmap: next_extent",
+ &next_extent);
+ has_next = 1;
+ if (next_extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
+ next_uninit = 1;
+ }
+ retval = ext2fs_extent_goto(handle, logical);
+ if (retval && retval != EXT2_ET_EXTENT_NOT_FOUND)
+ goto done;
+ retval = ext2fs_extent_get(handle, EXT2_EXTENT_PREV_LEAF, &prev_extent);
+ if (retval) {
+ has_prev = 0;
+ if (retval != EXT2_ET_EXTENT_NO_PREV)
+ goto done;
+ } else {
+ has_prev = 1;
+ dbg_print_extent("set_bmap: prev_extent",
+ &prev_extent);
+ if (prev_extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
+ prev_uninit = 1;
+ }
+ retval = ext2fs_extent_goto(handle, logical);
+ if (retval && retval != EXT2_ET_EXTENT_NOT_FOUND)
+ goto done;

/* check if already pointing to the requested physical */
if (mapped && (new_uninit == extent_uninit) &&
@@ -1248,6 +1283,28 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
((int) extent.e_len < max_len-1)) {
extent.e_len++;
retval = ext2fs_extent_replace(handle, 0, &extent);
+ } else if ((logical == extent.e_lblk - 1) &&
+ (physical == extent.e_pblk - 1) &&
+ (new_uninit == extent_uninit) &&
+ ((int) extent.e_len < max_len - 1)) {
+ extent.e_len++;
+ extent.e_lblk--;
+ extent.e_pblk--;
+ retval = ext2fs_extent_replace(handle, 0, &extent);
+ } else if (has_next &&
+ (logical == next_extent.e_lblk - 1) &&
+ (physical == next_extent.e_pblk - 1) &&
+ (new_uninit == next_uninit) &&
+ ((int) next_extent.e_len < max_len - 1)) {
+ retval = ext2fs_extent_get(handle,
+ EXT2_EXTENT_NEXT_LEAF,
+ &next_extent);
+ if (retval)
+ goto done;
+ next_extent.e_len++;
+ next_extent.e_lblk--;
+ next_extent.e_pblk--;
+ retval = ext2fs_extent_replace(handle, 0, &next_extent);
} else if (logical < extent.e_lblk)
retval = ext2fs_extent_insert(handle, 0, &newextent);
else
@@ -1279,14 +1336,32 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
#ifdef DEBUG
printf("(re/un)mapping last block in extent\n");
#endif
- /* Make sure insert works before replacing old extent */
if (physical) {
- retval = ext2fs_extent_insert(handle,
- EXT2_EXTENT_INSERT_AFTER, &newextent);
+ if (has_next &&
+ (logical == (next_extent.e_lblk - 1)) &&
+ (physical == (next_extent.e_pblk - 1)) &&
+ (new_uninit == next_uninit) &&
+ ((int) next_extent.e_len < max_len - 1)) {
+ retval = ext2fs_extent_get(handle,
+ EXT2_EXTENT_NEXT_LEAF, &next_extent);
+ if (retval)
+ goto done;
+ next_extent.e_len++;
+ next_extent.e_lblk--;
+ next_extent.e_pblk--;
+ retval = ext2fs_extent_replace(handle, 0,
+ &next_extent);
+ if (retval)
+ goto done;
+ } else
+ retval = ext2fs_extent_insert(handle,
+ EXT2_EXTENT_INSERT_AFTER, &newextent);
if (retval)
goto done;
/* Now pointing at inserted extent; move back to prev */
- retval = ext2fs_extent_goto(handle, logical - 1);
+ retval = ext2fs_extent_get(handle,
+ EXT2_EXTENT_PREV_LEAF,
+ &extent);
if (retval)
goto done;
}
@@ -1299,30 +1374,23 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
printf("(re/un)mapping first block in extent\n");
#endif
if (physical) {
- retval = ext2fs_extent_get(handle,
- EXT2_EXTENT_PREV_LEAF,
- &extent);
- if (extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT)
- extent_uninit = 1;
- if (retval == EXT2_ET_EXTENT_NO_PREV) {
- retval = ext2fs_extent_goto(handle, logical);
+ if (has_prev &&
+ (logical == (prev_extent.e_lblk +
+ prev_extent.e_len)) &&
+ (physical == (prev_extent.e_pblk +
+ prev_extent.e_len)) &&
+ (new_uninit == prev_uninit) &&
+ ((int) prev_extent.e_len < max_len-1)) {
+ retval = ext2fs_extent_get(handle,
+ EXT2_EXTENT_PREV_LEAF, &prev_extent);
if (retval)
goto done;
- retval = ext2fs_extent_insert(handle,
- 0, &newextent);
- } else if (retval)
- goto done;
- else if ((logical == extent.e_lblk + extent.e_len) &&
- (physical == extent.e_pblk + extent.e_len) &&
- (new_uninit == extent_uninit) &&
- ((int) extent.e_len < max_len-1)) {
- extent.e_len++;
+ prev_extent.e_len++;
retval = ext2fs_extent_replace(handle, 0,
- &extent);
- } else {
+ &prev_extent);
+ } else
retval = ext2fs_extent_insert(handle,
- EXT2_EXTENT_INSERT_AFTER, &newextent);
- }
+ 0, &newextent);
if (retval)
goto done;
retval = ext2fs_extent_get(handle,
--
1.6.3.2.1.gb9f7d.dirty