[PATCH RFC 0/6] add full off-line resize2fs support for flex_bg file systems

[PATCH RFC 0/6] add full off-line resize2fs support for flex_bg file systems

[Theodore Ts'o]

This addresses the corruption problems reported by George Spelvin when
we created a file system with a non-default number of reserved gdt
blocks using the -E resize=NNN option.  It does this by adding full
support for flex_bg file systems, so that we don't have to worry about
allocating overlapping metadata tables while we are growing the number
of block group descriptor blocks, in the case where the resize inode
does not have enough reserved blocks, or is not present altogether.

Theodore Ts'o (6):
  mke2fs: fix crash when parsing "-E resize=NNN" with "-O 64bit"
  resize2fs: reserve fs metadata blocks first in blocks_to_move()
  resize2fs: reserve all metadata blocks for flex_bg file systems
  resize2fs: handle bg descriptors which overlap with other bg's
    metadata
  resize2fs: allow resizing flex_bg && !resize_inode file systems
  resize2fs: create optimized flex_bg block groups

 misc/mke2fs.c      |   8 +++++
 resize/main.c      |  22 ------------
 resize/resize2fs.c | 102 ++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 73 insertions(+), 59 deletions(-)

-- 
1.7.12.rc0.22.gcdd159b

[PATCH 1/6] mke2fs: fix crash when parsing "-E resize=NNN" with "-O 64bit"

[Theodore Ts'o]

If the 64-bit file system feature is enabled, then mke2fs would crash
due to a divide-by-zero error caused by s_desc_size not being
initialized yet.

Reported-by: George Spelvin <linux@horizon.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 misc/mke2fs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 7ec8cc2..0f9a299 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1873,6 +1873,14 @@ profile_error:
 
 	blocksize = EXT2_BLOCK_SIZE(&fs_param);
 
+	/*
+	 * Initialize s_desc_size so that the parse_extended_opts()
+	 * can correctly handle "-E resize=NNN" if the 64-bit option
+	 * is set.
+	 */
+	if (fs_param.s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT)
+		fs_param.s_desc_size = EXT2_MIN_DESC_SIZE_64BIT;
+
 	/* This check should happen beyond the last assignment to blocksize */
 	if (blocksize > sys_page_size) {
 		if (!force) {
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 2/6] resize2fs: reserve fs metadata blocks first in blocks_to_move()

[Theodore Ts'o]

This is the first commit to add support for off-line resizing using
flex_bg without the assist of using the resize_inode to reserve gdt
blocks.  This functionality has been broken up into separate commits
which are hopefully obviously correct to make them easier to review
for correctness.

In this first step, we break up the for loop at the end of
blocks_to_move() so that we first mark all of the metadata blocks
which don't need to be moved in the reserve_blocks bitmap, and then
try to allocate the metadata blocks are new or which need to moved
second.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 resize/resize2fs.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index ce8ed91..c5d8a23 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -874,6 +874,7 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 	 * gets interesting....
 	 */
 	meta_bg_size = EXT2_DESC_PER_BLOCK(fs->super);
+	/* first reserve all of the existing fs meta blocks */
 	for (i = 0; i < max_groups; i++) {
 		has_super = ext2fs_bg_has_super(fs, i);
 		if (has_super)
@@ -899,11 +900,6 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 						  group_blk + has_super);
 		}
 
-		if (ext2fs_inode_table_loc(fs, i) &&
-		    ext2fs_inode_bitmap_loc(fs, i) &&
-		    ext2fs_block_bitmap_loc(fs, i))
-			goto next_group;
-
 		/*
 		 * Reserve the existing meta blocks that we know
 		 * aren't to be moved.
@@ -920,9 +916,17 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 				ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
 							 blk);
 
+		group_blk += rfs->new_fs->super->s_blocks_per_group;
+	}
+
+	/* Allocate the missing data structures */
+	for (i = 0; i < max_groups; i++) {
+		if (ext2fs_inode_table_loc(fs, i) &&
+		    ext2fs_inode_bitmap_loc(fs, i) &&
+		    ext2fs_block_bitmap_loc(fs, i))
+			continue;
+
 		/*
-		 * Allocate the missing data structures
-		 *
 		 * XXX We have a problem with FLEX_BG and off-line
 		 * resizing where we are growing the size of the
 		 * filesystem.  ext2fs_allocate_group_table() will try
@@ -977,7 +981,7 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 		 * block relocation phase.
 		 */
 		if (ext2fs_inode_table_loc(fs, i) == ext2fs_inode_table_loc(old_fs, i))
-			goto next_group; /* inode table not moved */
+			continue;	/* inode table not moved */
 
 		rfs->needed_blocks += fs->inode_blocks_per_group;
 
@@ -1002,9 +1006,6 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 		for (blk = ext2fs_inode_table_loc(rfs->old_fs, i), j=0;
 		     j < fs->inode_blocks_per_group ; j++, blk++)
 			ext2fs_mark_block_bitmap2(rfs->reserve_blocks, blk);

[PATCH 3/6] resize2fs: reserve all metadata blocks for flex_bg file systems

[Theodore Ts'o]

For flex_bg file systems, if we need to relocate an allocation bitmap
or inode table, we need to make sure that all metadata blocks have
been reserved, lest we end up overwriting a metadata block belonging
to a different block group.

This change fixes the following test case:

rm -f foo.img; touch foo.img
truncate -s 32G foo.img
mke2fs -F -t ext4 -E resize=12582912 foo.img
e2fsck -f foo.img
truncate -s 64G foo.img
./resize2fs foo.img
e2fsck -fy foo.img

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 resize/resize2fs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index c5d8a23..9702645 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -783,6 +783,7 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 	ext2_filsys 	fs, old_fs;
 	ext2fs_block_bitmap	meta_bmap;
 	__u32		save_incompat_flag;
+	int		flex_bg;
 
 	fs = rfs->new_fs;
 	old_fs = rfs->old_fs;
@@ -874,6 +875,8 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 	 * gets interesting....
 	 */
 	meta_bg_size = EXT2_DESC_PER_BLOCK(fs->super);
+	flex_bg = fs->super->s_feature_incompat &
+		EXT4_FEATURE_INCOMPAT_FLEX_BG;
 	/* first reserve all of the existing fs meta blocks */
 	for (i = 0; i < max_groups; i++) {
 		has_super = ext2fs_bg_has_super(fs, i);
@@ -903,18 +906,37 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 		/*
 		 * Reserve the existing meta blocks that we know
 		 * aren't to be moved.
+		 *
+		 * For flex_bg file systems, in order to avoid
+		 * overwriting fs metadata (especially inode table
+		 * blocks) belonging to a different block group when
+		 * we are relocating the inode tables, we need to
+		 * reserve all existing fs metadata blocks.
 		 */
 		if (ext2fs_block_bitmap_loc(fs, i))
 			ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
 				 ext2fs_block_bitmap_loc(fs, i));
+		else if (flex_bg && i < old_fs->group_desc_count)
+			ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
+				 ext2fs_block_bitmap_loc(old_fs, i));
+
 		if (ext2fs_inode_bitmap_loc(fs, i))
 			ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
 				 ext2fs_inode_bitmap_loc(fs, i));
+		else if (flex_bg && i < old_fs->group_desc_count)
+			ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
+				 ext2fs_inode_bitmap_loc(old_fs, i));
+
 		if (ext2fs_inode_table_loc(fs, i))
 			for (blk = ext2fs_inode_table_loc(fs, i), j=0;
 			     j < fs->inode_blocks_per_group ; j++, blk++)
 				ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
 							 blk);
+		else if (flex_bg && i < old_fs->group_desc_count)
+			for (blk = ext2fs_inode_table_loc(old_fs, i), j=0;
+			     j < old_fs->inode_blocks_per_group ; j++, blk++)
+				ext2fs_mark_block_bitmap2(rfs->reserve_blocks,
+							  blk);
 
 		group_blk += rfs->new_fs->super->s_blocks_per_group;
 	}
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 4/6] resize2fs: handle bg descriptors which overlap with other bg's metadata

[Theodore Ts'o]

With flex_bg file systems, bg-specific metadata (i.e., bitmaps and the
inode table blocks) can be located in another block group.  Hence,
when we grow the number of block group descriptors, we need to check
if we need to relocate metadata blocks not just for the block group
where the bgd blocks are located, but in all block groups.

This change fixes the following test case:

rm -f foo.img; touch foo.img
truncate -s 32G foo.img
mke2fs -F -t ext4 -E resize=12582912 foo.img
e2fsck -f foo.img
truncate -s 256G foo.img
./resize2fs foo.img
e2fsck -fy foo.img

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 resize/resize2fs.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index 9702645..159846d 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -745,14 +745,41 @@ static void mark_fs_metablock(ext2_resize_t rfs,
 	if (IS_BLOCK_BM(fs, group, blk)) {
 		ext2fs_block_bitmap_loc_set(fs, group, 0);
 		rfs->needed_blocks++;
-	} else if (IS_INODE_BM(fs, group, blk)) {
+		return;
+	}
+	if (IS_INODE_BM(fs, group, blk)) {
 		ext2fs_inode_bitmap_loc_set(fs, group, 0);
 		rfs->needed_blocks++;
-	} else if (IS_INODE_TB(fs, group, blk)) {
+		return;
+	}
+	if (IS_INODE_TB(fs, group, blk)) {
 		ext2fs_inode_table_loc_set(fs, group, 0);
 		rfs->needed_blocks++;
-	} else if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
-					      EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
+		return;
+	}
+	if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_FLEX_BG) {
+		dgrp_t i;
+
+		for (i=0; i < rfs->old_fs->group_desc_count; i++) {
+			if (IS_BLOCK_BM(fs, i, blk)) {
+				ext2fs_block_bitmap_loc_set(fs, i, 0);
+				rfs->needed_blocks++;
+				return;
+			}
+			if (IS_INODE_BM(fs, i, blk)) {
+				ext2fs_inode_bitmap_loc_set(fs, i, 0);
+				rfs->needed_blocks++;
+				return;
+			}
+			if (IS_INODE_TB(fs, i, blk)) {
+				ext2fs_inode_table_loc_set(fs, i, 0);
+				rfs->needed_blocks++;
+				return;
+			}
+		}
+	}
+	if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
+				       EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
 		   (ext2fs_bg_flags_test(fs, group, EXT2_BG_BLOCK_UNINIT))) {
 		/*
 		 * If the block bitmap is uninitialized, which means
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 5/6] resize2fs: allow resizing flex_bg && !resize_inode file systems

[Theodore Ts'o]

With the bug fixes from the last two commits, resize2fs can now fully
support off-line resizing of file systems with flex_bg even if the
resize_inode feature is not present; so we no longer need to disallow
this combination.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 resize/main.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/resize/main.c b/resize/main.c
index 876dbaa..711e375 100644
--- a/resize/main.c
+++ b/resize/main.c
@@ -438,28 +438,6 @@ int main (int argc, char ** argv)
 				device_name);
 			exit(1);
 		}
-		/*
-		 * XXXX The combination of flex_bg and !resize_inode
-		 * causes major problems for resize2fs, since when the
-		 * group descriptors grow in size this can potentially
-		 * require multiple inode tables to be moved aside to
-		 * make room, and resize2fs chokes rather badly in
-		 * this scenario.  It's a rare combination, except
-		 * when a filesystem is expanded more than a certain
-		 * size, so for now, we'll just prohibit that
-		 * combination.  This is something we should fix
-		 * eventually, though.
-		 */
-		if ((fs->super->s_feature_incompat &
-		     EXT4_FEATURE_INCOMPAT_FLEX_BG) &&
-		    !(fs->super->s_feature_compat &
-		      EXT2_FEATURE_COMPAT_RESIZE_INODE)) {
-			com_err(program_name, 0, _("%s: The combination of "
-				"flex_bg and\n\t!resize_inode features "
-				"is not supported by resize2fs.\n"),
-				device_name);
-			exit(1);
-		}
 		printf(_("Resizing the filesystem on "
 			 "%s to %llu (%dk) blocks.\n"),
 		       device_name, new_size, fs->blocksize / 1024);
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 6/6] resize2fs: create optimized flex_bg block groups

[Theodore Ts'o]

Now that we are reserving all of the bg-specific metadata before we
try to allocate the metadata for the new block groups, we don't have
to temporarily disable the flex_bg feature flag while we allocate the
new metadata blocks --- this allows the newly created block groups to
have a much more optimized layout, instead of fragmenting the inode
table and block/inode bitmaps in sepraate block groups.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 resize/resize2fs.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index 159846d..092cfbd 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -809,7 +809,6 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 	errcode_t	retval;
 	ext2_filsys 	fs, old_fs;
 	ext2fs_block_bitmap	meta_bmap;
-	__u32		save_incompat_flag;
 	int		flex_bg;
 
 	fs = rfs->new_fs;
@@ -975,29 +974,8 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
 		    ext2fs_block_bitmap_loc(fs, i))
 			continue;
 
-		/*
-		 * XXX We have a problem with FLEX_BG and off-line
-		 * resizing where we are growing the size of the
-		 * filesystem.  ext2fs_allocate_group_table() will try
-		 * to reserve the inode table in the desired flex_bg
-		 * location.  However, passing rfs->reserve_blocks
-		 * doesn't work since it only has reserved the blocks
-		 * that will be used in the new block group -- and
-		 * with flex_bg, we can and will allocate the tables
-		 * outside of the block group.  And we can't pass in
-		 * the fs->block_map because it doesn't handle
-		 * overlapping inode table movements right.  So for
-		 * now, we temporarily disable flex_bg to force
-		 * ext2fs_allocate_group_tables() to allocate the bg
-		 * metadata in side the block group, and the restore
-		 * it afterwards.  Ugly, until we can fix this up
-		 * right later.
-		 */
-		save_incompat_flag = fs->super->s_feature_incompat;
-		fs->super->s_feature_incompat &= ~EXT4_FEATURE_INCOMPAT_FLEX_BG;
 		retval = ext2fs_allocate_group_table(fs, i,
 						     rfs->reserve_blocks);
-		fs->super->s_feature_incompat = save_incompat_flag;
 		if (retval)
 			goto errout;
 
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH RFC 0/6] add full off-line resize2fs support for flex_bg file systems

[Theodore Ts'o]

In addition to people reviewing these patches, I'd really appreciate
it if some folks could try doing some testing.  These changes are
included on the pu (proposed update) branch which I've pushed out to
the e2fprogs git tree.  I've done some light testing, and so far
everything works, but it would be great if we could have more people
trying to beat up on these changes.

Thanks!!

						- Ted