Fix device too big bug in mainline?

Fix device too big bug in mainline?

[Valerie Aurora]

Hi all,

Currently, e2fsprogs will fail to create a file system if the
underlying device is "too big" - even if we specify a number of blocks
to use that is in range:

https://bugzilla.redhat.com/show_bug.cgi?id=485236

This is fixed in the current pu branch, but more as a side effect of
an enormous 64-bit rewrite.

Ted, any plans to pull this into mainline?

-VAL

Re: Fix device too big bug in mainline?

[Theodore Tso]

On Thu, Jul 30, 2009 at 05:53:02PM -0400, Valerie Aurora wrote:
> Hi all,
> 
> Currently, e2fsprogs will fail to create a file system if the
> underlying device is "too big" - even if we specify a number of blocks
> to use that is in range:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=485236
> 
> This is fixed in the current pu branch, but more as a side effect of
> an enormous 64-bit rewrite.
> 
> Ted, any plans to pull this into mainline?

We have a special case as of v1.41.4 so that if someone creates a 16TB
partition, we'll treat it as having 16TB - 1 minus blocks.

Yes, I'm working on merging the 64-bit patches into mainline; and so
far we have about 25% or so of the patches merged into the master
branch.  It's been somewhat slow going, since I've many other things
on my plate, and because I've wanted to do a lot of QA while doing the
merge.  I've found more than a few bugs simply by doing code
inspection while merging the patches one at a time.

How much do we care about this specific bug as something that needs to
be fixed ASAP?  We already have something for a 16TB logical volume,
since that is what is most likely to be created with lvcreate.  But do
we consider it a common case where someone creates a 32TB logical
volume, but intends to create a 16TB (minus 1 block) filesystem, that
needs to be urgently fixed?

            					- Ted

Re: Fix device too big bug in mainline?

[Theodore Tso]

In case people are wondering why it's taking so long to merge the
64-bit patch series, let me show one patch as exhibit 'A' about how
not to submit patches to me (or Linus, or any other upstream
maintainer):

Note the lack of an adequate patch description.  Also note how this
patch mushes together 2 or 3 semantic changes into a single patch,
making it extremely difficult to audit.  Even worse are the patches
where what should be a single semantic change is spread out across
multiple patches.

This is why I can't just merge the 64-bit patch series blindly; I'd
have no idea whether or not the result would be good or not because
it's near-impossible to audit.

What I have been doing is gradually extracting out bits and pieces,
combining patches where necessary, separate patches where
appropriately, and then gradually merging patches into the mainline.
But it's slow work that requires meticulous checking, both in terms of
checking to make sure the tree builds after each patch and passes
"make check", as well as making sure I don't drop anything.   

In addition, more than once I've found places where not all of the
places that needed converting to some new interfaces, such as
ext2fs_blocks_count() had been done.  Maybe no one had noticed, but
I'd prefer to catch such problems at merge-time, not when a user
complains about their filesystem getting corrupted and files a bug in
Red Hat's or SLES's bugzilla....


I also would greatly prefer it if people who submit patches to me obey
basic patch and code formatting guidelines.  Things like this are
really uncool:

-				fs->group_desc[i].bg_free_blocks_count =
-					free_array[i];
+				ext2fs_bg_free_blocks_count_set(fs, i, free_array[i])
+					;

Please try to keep code lines wrapped at 72-76 characters, and ixnay
on a single semicolon or one or two close parathesis on a line with
nothing else.

Clean patches get merged more quickly; dirty patches that don't get
cleaned up mean that either I have to try to ask the original patch
submitter to clean them up (and I thought I had made it clear to Val
what my expectations were in terms of clean patches), or I have to
find the time to clean them up myself.  In my experience, cleaning
this up *now* will end up costing us less time than trying to find
bugs in the merged code later.

					- Ted

temporary checkin; about to do checksum conversion

From: Valerie Aurora Henson <vaurora@redhat.com>

Signed-off-by: Valerie Aurora Henson <vaurora@redhat.com>
---
 debugfs/debugfs.c         |   38 +++++++++++++++++++-----------------
 e2fsck/journal.c          |    2 +-
 e2fsck/pass5.c            |   12 +++++-----
 e2fsck/super.c            |   47 ++++++++++++++++++++++-----------------------
 lib/ext2fs/alloc_stats.c  |   15 ++++++-------
 lib/ext2fs/alloc_tables.c |    6 ++--
 lib/ext2fs/closefs.c      |   12 ++++++----
 lib/ext2fs/csum.c         |    2 +-
 lib/ext2fs/ext2fs.h       |    2 +-
 lib/ext2fs/initialize.c   |   16 +++++++-------
 lib/ext2fs/openfs.c       |    2 +-
 lib/ext2fs/swapfs.c       |   14 ++++++++++++-
 misc/mke2fs.c             |    2 +-
 misc/tune2fs.c            |   10 ++++----
 resize/resize2fs.c        |   26 ++++++++++++------------
 15 files changed, 110 insertions(+), 96 deletions(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index 63b9a44..befbb5b 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -307,7 +307,6 @@ void do_show_super_stats(int argc, char *argv[])
 {
 	dgrp_t	i;
 	FILE 	*out;
-	struct ext2_group_desc *gdp;
 	int	c, header_only = 0;
 	int	numdirs = 0, first, gdt_csum;
 
@@ -340,27 +339,30 @@ void do_show_super_stats(int argc, char *argv[])
 
 	gdt_csum = EXT2_HAS_RO_COMPAT_FEATURE(current_fs->super,
 					      EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
-	gdp = &current_fs->group_desc[0];
-	for (i = 0; i < current_fs->group_desc_count; i++, gdp++) {
-		fprintf(out, " Group %2d: block bitmap at %u, "
-		        "inode bitmap at %u, "
-		        "inode table at %u\n"
+	for (i = 0; i < current_fs->group_desc_count; i++) {
+		fprintf(out, " Group %2d: block bitmap at %llu, "
+		        "inode bitmap at %llu, "
+		        "inode table at %llu\n"
 		        "           %d free %s, "
 		        "%d free %s, "
 		        "%d used %s%s",
-		        i, gdp->bg_block_bitmap,
-		        gdp->bg_inode_bitmap, gdp->bg_inode_table,
-		        gdp->bg_free_blocks_count,
-		        gdp->bg_free_blocks_count != 1 ? "blocks" : "block",
-		        gdp->bg_free_inodes_count,
-		        gdp->bg_free_inodes_count != 1 ? "inodes" : "inode",
-		        gdp->bg_used_dirs_count,
-		        gdp->bg_used_dirs_count != 1 ? "directories"
-				: "directory", gdt_csum ? ", " : "\n");
+		        i, ext2fs_block_bitmap_loc(current_fs, i),
+		        ext2fs_inode_bitmap_loc(current_fs, i),
+			ext2fs_inode_table_loc(current_fs, i),
+		        ext2fs_bg_free_blocks_count(current_fs, i),
+		        ext2fs_bg_free_blocks_count(current_fs, i) != 1 ?
+			"blocks" : "block",
+		        ext2fs_bg_free_inodes_count(current_fs, i),
+		        ext2fs_bg_free_inodes_count(current_fs, i) != 1 ?
+			"inodes" : "inode",
+		        ext2fs_bg_used_dirs_count(current_fs, i),
+		        ext2fs_bg_used_dirs_count(current_fs, i) != 1 ? "directories"
+ 				: "directory", gdt_csum ? ", " : "\n");
 		if (gdt_csum)
 			fprintf(out, "%d unused %s\n",
-				gdp->bg_itable_unused,
-				gdp->bg_itable_unused != 1 ? "inodes":"inode");
+				ext2fs_bg_itable_unused(current_fs, i),
+				ext2fs_bg_itable_unused(current_fs, i) != 1 ?
+				"inodes" : "inode");
 		first = 1;
 		print_bg_opts(current_fs, i, EXT2_BG_INODE_UNINIT, "Inode not init",
 			      &first, out);
@@ -368,7 +370,7 @@ void do_show_super_stats(int argc, char *argv[])
 			      &first, out);
 		if (gdt_csum) {
 			fprintf(out, "%sChecksum 0x%04x",
-				first ? "           [":", ", gdp->bg_checksum);
+				first ? "           [":", ", ext2fs_bg_checksum(current_fs, i));
 			first = 0;
 		}
 		if (!first)
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index dd56e7a..f5fdef5 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -1011,7 +1011,7 @@ void e2fsck_move_ext3_journal(e2fsck_t ctx)
 	group = ext2fs_group_of_ino(fs, ino);
 	ext2fs_unmark_inode_bitmap2(fs->inode_map, ino);
 	ext2fs_mark_ib_dirty(fs);
-	fs->group_desc[group].bg_free_inodes_count++;
+	ext2fs_bg_free_inodes_count_set(fs, group, ext2fs_bg_free_inodes_count(fs, group) + 1);
 	ext2fs_group_desc_csum_set(fs, group);
 	fs->super->s_free_inodes_count++;
 	return;
diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index 6cdbd6b..9ac4324 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -343,8 +343,8 @@ redo_counts:
 
 			if (fix_problem(ctx, PR_5_FREE_BLOCK_COUNT_GROUP,
 					&pctx)) {
-				fs->group_desc[i].bg_free_blocks_count =
-					free_array[i];
+				ext2fs_bg_free_blocks_count_set(fs, i, free_array[i])
+					;
 				ext2fs_mark_super_dirty(fs);
 			} else
 				ext2fs_unmark_valid(fs);
@@ -573,8 +573,8 @@ do_counts:
 			pctx.ino2 = free_array[i];
 			if (fix_problem(ctx, PR_5_FREE_INODE_COUNT_GROUP,
 					&pctx)) {
-				fs->group_desc[i].bg_free_inodes_count =
-					free_array[i];
+				ext2fs_bg_free_inodes_count_set(fs, i, free_array[i])
+					;
 				ext2fs_mark_super_dirty(fs);
 			} else
 				ext2fs_unmark_valid(fs);
@@ -586,8 +586,8 @@ do_counts:
 
 			if (fix_problem(ctx, PR_5_FREE_DIR_COUNT_GROUP,
 					&pctx)) {
-				fs->group_desc[i].bg_used_dirs_count =
-					dir_array[i];
+				ext2fs_bg_used_dirs_count_set(fs, i, dir_array[i])
+					;
 				ext2fs_mark_super_dirty(fs);
 			} else
 				ext2fs_unmark_valid(fs);
diff --git a/e2fsck/super.c b/e2fsck/super.c
index c269b0e..a1fb878 100644
--- a/e2fsck/super.c
+++ b/e2fsck/super.c
@@ -458,7 +458,6 @@ void check_super_block(e2fsck_t ctx)
 	ext2_filsys fs = ctx->fs;
 	blk64_t	first_block, last_block;
 	struct ext2_super_block *sb = fs->super;
-	struct ext2_group_desc *gd;
 	problem_t	problem;
 	blk64_t	blocks_per_group = fs->super->s_blocks_per_group;
 	__u32	bpg_max;
@@ -587,7 +586,7 @@ void check_super_block(e2fsck_t ctx)
 
 	csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
 					       EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
-	for (i = 0, gd=fs->group_desc; i < fs->group_desc_count; i++, gd++) {
+	for (i = 0; i < fs->group_desc_count; i++) {
 		pctx.group = i;
 
 		if (!EXT2_HAS_INCOMPAT_FEATURE(fs->super,
@@ -627,61 +626,61 @@ void check_super_block(e2fsck_t ctx)
 			ctx->invalid_inode_table_flag[i]++;
 			ctx->invalid_bitmaps++;
 		}
-		free_blocks += gd->bg_free_blocks_count;
-		free_inodes += gd->bg_free_inodes_count;
+		free_blocks += ext2fs_bg_free_blocks_count(fs, i);
+		free_inodes += ext2fs_bg_free_inodes_count(fs, i);
 
-		if ((gd->bg_free_blocks_count > sb->s_blocks_per_group) ||
-		    (gd->bg_free_inodes_count > sb->s_inodes_per_group) ||
-		    (gd->bg_used_dirs_count > sb->s_inodes_per_group))
+		if ((ext2fs_bg_free_blocks_count(fs, i) > sb->s_blocks_per_group) ||
+		    (ext2fs_bg_free_inodes_count(fs, i) > sb->s_inodes_per_group) ||
+		    (ext2fs_bg_used_dirs_count(fs, i) > sb->s_inodes_per_group))
 			ext2fs_unmark_valid(fs);
 
 		should_be = 0;
 		if (!ext2fs_group_desc_csum_verify(fs, i)) {
 			if (fix_problem(ctx, PR_0_GDT_CSUM, &pctx)) {
-				gd->bg_flags &=	~(EXT2_BG_BLOCK_UNINIT |
-				                  EXT2_BG_INODE_UNINIT);
-				gd->bg_itable_unused = 0;
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_INODE_UNINIT);
+				ext2fs_bg_itable_unused_set(fs, i, 0);
 				should_be = 1;
 			}
 			ext2fs_unmark_valid(fs);
 		}
 
 		if (!csum_flag &&
-		    (gd->bg_flags &(EXT2_BG_BLOCK_UNINIT|EXT2_BG_INODE_UNINIT)||
-		     gd->bg_itable_unused != 0)){
+		    (ext2fs_bg_flag_test(fs, i, EXT2_BG_BLOCK_UNINIT) ||
+		     ext2fs_bg_flag_test(fs, i, EXT2_BG_INODE_UNINIT) ||
+		     ext2fs_bg_itable_unused(fs, i) != 0)){
 			if (fix_problem(ctx, PR_0_GDT_UNINIT, &pctx)) {
-				gd->bg_flags &= ~(EXT2_BG_BLOCK_UNINIT |
-						  EXT2_BG_INODE_UNINIT);
-				gd->bg_itable_unused = 0;
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_INODE_UNINIT);
 				should_be = 1;
 			}
 			ext2fs_unmark_valid(fs);
 		}
 
 		if (i == fs->group_desc_count - 1 &&
-		    gd->bg_flags & EXT2_BG_BLOCK_UNINIT) {
+		    ext2fs_bg_flag_test(fs, i, EXT2_BG_BLOCK_UNINIT)) {
 			if (fix_problem(ctx, PR_0_BB_UNINIT_LAST, &pctx)) {
-				gd->bg_flags &= ~EXT2_BG_BLOCK_UNINIT;
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
 				should_be = 1;
 			}
 			ext2fs_unmark_valid(fs);
 		}
 
-		if (gd->bg_flags & EXT2_BG_BLOCK_UNINIT &&
-		    !(gd->bg_flags & EXT2_BG_INODE_UNINIT)) {
+		if (ext2fs_bg_flag_test(fs, i, EXT2_BG_BLOCK_UNINIT) &&
+		    !ext2fs_bg_flag_test(fs, i, EXT2_BG_INODE_UNINIT)) {
 			if (fix_problem(ctx, PR_0_BB_UNINIT_IB_INIT, &pctx)) {
-				gd->bg_flags &= ~EXT2_BG_BLOCK_UNINIT;
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
 				should_be = 1;
 			}
 			ext2fs_unmark_valid(fs);
 		}
 
 		if (csum_flag &&
-		    (gd->bg_itable_unused > gd->bg_free_inodes_count ||
-		     gd->bg_itable_unused > sb->s_inodes_per_group)) {
-			pctx.blk = gd->bg_itable_unused;
+		    (ext2fs_bg_itable_unused(fs, i) > ext2fs_bg_free_inodes_count(fs, i) ||
+		     ext2fs_bg_itable_unused(fs, i) > sb->s_inodes_per_group)) {
+			pctx.blk = ext2fs_bg_itable_unused(fs, i);
 			if (fix_problem(ctx, PR_0_GDT_ITABLE_UNUSED, &pctx)) {
-				gd->bg_itable_unused = 0;
+				ext2fs_bg_itable_unused_set(fs, i, 0);
 				should_be = 1;
 			}
 			ext2fs_unmark_valid(fs);
diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
index d254998..5423c30 100644
--- a/lib/ext2fs/alloc_stats.c
+++ b/lib/ext2fs/alloc_stats.c
@@ -31,13 +31,13 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
 		ext2fs_mark_inode_bitmap2(fs->inode_map, ino);
 	else
 		ext2fs_unmark_inode_bitmap2(fs->inode_map, ino);
-	fs->group_desc[group].bg_free_inodes_count -= inuse;
+	ext2fs_bg_free_inodes_count_set(fs, group, ext2fs_bg_free_inodes_count(fs, group) - inuse);
 	if (isdir)
-		fs->group_desc[group].bg_used_dirs_count += inuse;
+		ext2fs_bg_used_dirs_count_set(fs, group, ext2fs_bg_used_dirs_count(fs, group) + inuse);
 
 	/* We don't strictly need to be clearing the uninit flag if inuse < 0
 	 * (i.e. freeing inodes) but it also means something is bad. */
-	fs->group_desc[group].bg_flags &= ~EXT2_BG_INODE_UNINIT;
+	ext2fs_bg_flag_clear(fs, group, EXT2_BG_INODE_UNINIT);
 	if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
 				       EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
 		ext2_ino_t first_unused_inode =	fs->super->s_inodes_per_group -
@@ -45,9 +45,8 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
 			group * fs->super->s_inodes_per_group + 1;
 
 		if (ino >= first_unused_inode)
-			fs->group_desc[group].bg_itable_unused =
-				group * fs->super->s_inodes_per_group +
-				fs->super->s_inodes_per_group - ino;
+			ext2fs_bg_itable_unused_set(fs, group, group * fs->super->s_inodes_per_group + fs->super->s_inodes_per_group - ino)
+				;
 		ext2fs_group_desc_csum_set(fs, group);
 	}
 
@@ -76,8 +75,8 @@ void ext2fs_block_alloc_stats2(ext2_filsys fs, blk64_t blk, int inuse)
 		ext2fs_mark_block_bitmap2(fs->block_map, blk);
 	else
 		ext2fs_unmark_block_bitmap2(fs->block_map, blk);
-	fs->group_desc[group].bg_free_blocks_count -= inuse;
-	fs->group_desc[group].bg_flags &= ~EXT2_BG_BLOCK_UNINIT;
+	ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group) - inuse);
+	ext2fs_bg_flag_clear(fs, group, EXT2_BG_BLOCK_UNINIT);
 	ext2fs_group_desc_csum_set(fs, group);
 
 	ext2fs_free_blocks_count_add(fs->super, -inuse);
diff --git a/lib/ext2fs/alloc_tables.c b/lib/ext2fs/alloc_tables.c
index b218e7f..2f691ac 100644
--- a/lib/ext2fs/alloc_tables.c
+++ b/lib/ext2fs/alloc_tables.c
@@ -141,7 +141,7 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 		ext2fs_block_bitmap_loc_set(fs, group, new_blk);
 		if (flexbg_size) {
 			dgrp_t gr = ext2fs_group_of_blk2(fs, new_blk);
-			fs->group_desc[gr].bg_free_blocks_count--;
+			ext2fs_bg_free_blocks_count_set(fs, gr, ext2fs_bg_free_blocks_count(fs, gr) - 1);
 			ext2fs_free_blocks_count_add(fs->super, -1);
 			ext2fs_bg_flag_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
 			ext2fs_group_desc_csum_set(fs, gr);
@@ -169,7 +169,7 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 		ext2fs_inode_bitmap_loc_set(fs, group, new_blk);
 		if (flexbg_size) {
 			dgrp_t gr = ext2fs_group_of_blk2(fs, new_blk);
-			fs->group_desc[gr].bg_free_blocks_count--;
+			ext2fs_bg_free_blocks_count_set(fs, gr, ext2fs_bg_free_blocks_count(fs, gr) - 1);
 			ext2fs_free_blocks_count_add(fs->super, -1);
 			ext2fs_bg_flag_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
 			ext2fs_group_desc_csum_set(fs, gr);
@@ -203,7 +203,7 @@ errcode_t ext2fs_allocate_group_table(ext2_filsys fs, dgrp_t group,
 			ext2fs_mark_block_bitmap2(bmap, blk);
 			if (flexbg_size) {
 				dgrp_t gr = ext2fs_group_of_blk2(fs, blk);
-				fs->group_desc[gr].bg_free_blocks_count--;
+				ext2fs_bg_free_blocks_count_set(fs, gr, ext2fs_bg_free_blocks_count(fs, gr) - 1);
 				ext2fs_free_blocks_count_add(fs->super, -1);
 				ext2fs_bg_flag_clear(fs, gr, EXT2_BG_BLOCK_UNINIT);
 				ext2fs_group_desc_csum_set(fs, gr);
diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c
index dd24856..e2523c3 100644
--- a/lib/ext2fs/closefs.c
+++ b/lib/ext2fs/closefs.c
@@ -270,7 +270,7 @@ errcode_t ext2fs_flush_with_progress(ext2_filsys fs, const char *message)
 	struct ext2_super_block *super_shadow = 0;
 	struct ext2_group_desc *group_shadow = 0;
 #ifdef WORDS_BIGENDIAN
-	struct ext2_group_desc *s, *t;
+	char		*s, *t;
 	dgrp_t		j;
 #endif
 	char	*group_ptr;
@@ -297,10 +297,12 @@ errcode_t ext2fs_flush_with_progress(ext2_filsys fs, const char *message)
 	       fs->desc_blocks);
 
 	/* swap the group descriptors */
-	for (j=0, s=fs->group_desc, t=group_shadow;
-	     j < fs->group_desc_count; j++, t++, s++) {
-		*t = *s;
-		ext2fs_swap_group_desc(t);
+	t = group_shadow;
+	for (j=0; j < fs->group_desc_count; j++) {
+		s = (char *) ext2fs_group_desc(fs, j);
+		memcpy(t, s, fs->group_desc_size);
+		ext2fs_swap_group_desc(fs, t);
+		t += fs->group_desc_size;
 	}
 #else
 	super_shadow = fs->super;
diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index 7c79397..5612de1 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -43,7 +43,7 @@ STATIC __u16 ext2fs_group_desc_csum(ext2_filsys fs, dgrp_t group)
 		struct ext2_group_desc swabdesc = *desc;
 
 		/* Have to swab back to little-endian to do the checksum */
-		ext2fs_swap_group_desc(&swabdesc);
+		ext2fs_swap_group_desc(fs, &swabdesc);
 		desc = &swabdesc;
 
 		group = ext2fs_swab32(group);
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 7c6a9d9..a4396a4 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1279,7 +1279,7 @@ extern void ext2fs_swap_ext_attr_header(struct ext2_ext_attr_header *to_header,
 extern void ext2fs_swap_ext_attr_entry(struct ext2_ext_attr_entry *to_entry,
 				       struct ext2_ext_attr_entry *from_entry);
 extern void ext2fs_swap_super(struct ext2_super_block * super);
-extern void ext2fs_swap_group_desc(struct ext2_group_desc *gdp);
+extern void ext2fs_swap_group_desc(ext2_filsys, struct ext2_group_desc *gdp);
 extern void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
 				   struct ext2_inode_large *f, int hostorder,
 				   int bufsize);
diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
index 35d076a..c1b3ca9 100644
--- a/lib/ext2fs/initialize.c
+++ b/lib/ext2fs/initialize.c
@@ -413,13 +413,13 @@ ipg_retry:
 		 */
 		if (csum_flag) {
 			if (i != fs->group_desc_count - 1)
-				fs->group_desc[i].bg_flags |=
-					EXT2_BG_BLOCK_UNINIT;
-			fs->group_desc[i].bg_flags |= EXT2_BG_INODE_UNINIT;
+				ext2fs_bg_flag_set(fs, i, EXT2_BG_BLOCK_UNINIT)
+					;
+			ext2fs_bg_flag_set(fs, i, EXT2_BG_INODE_UNINIT);
 			numblocks = super->s_inodes_per_group;
 			if (i == 0)
 				numblocks -= super->s_first_ino;
-			fs->group_desc[i].bg_itable_unused = numblocks;
+			ext2fs_bg_itable_unused_set(fs, i, numblocks);
 		}
 		numblocks = ext2fs_reserve_super_and_bgd(fs, i, fs->block_map);
 		if (fs->super->s_log_groups_per_flex)
@@ -428,10 +428,10 @@ ipg_retry:
 		ext2fs_free_blocks_count_set(super,
 					     ext2fs_free_blocks_count(super) +
 					     numblocks);
-		fs->group_desc[i].bg_free_blocks_count = numblocks;
-		fs->group_desc[i].bg_free_inodes_count =
-			fs->super->s_inodes_per_group;
-		fs->group_desc[i].bg_used_dirs_count = 0;
+		ext2fs_bg_free_blocks_count_set(fs, i, numblocks);
+		ext2fs_bg_free_inodes_count_set(fs, i, fs->super->s_inodes_per_group)
+			;
+		ext2fs_bg_used_dirs_count_set(fs, i, 0);
 		ext2fs_group_desc_csum_set(fs, i);
 	}
 
diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index 5e49608..ddb3ede 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -334,7 +334,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
 #ifdef WORDS_BIGENDIAN
 		gdp = (struct ext2_group_desc *) dest;
 		for (j=0; j < groups_per_block; j++)
-			ext2fs_swap_group_desc(gdp++);
+			ext2fs_swap_group_desc(fs, gdp++);
 #endif
 		dest += fs->blocksize;
 	}
diff --git a/lib/ext2fs/swapfs.c b/lib/ext2fs/swapfs.c
index 42bc01e..25af281 100644
--- a/lib/ext2fs/swapfs.c
+++ b/lib/ext2fs/swapfs.c
@@ -78,8 +78,9 @@ void ext2fs_swap_super(struct ext2_super_block * sb)
 
 }
 
-void ext2fs_swap_group_desc(struct ext2_group_desc *gdp)
+void ext2fs_swap_group_desc(ext2_filsys fs, struct ext2_group_desc *gdp)
 {
+	// Do the 32-bit parts first
 	gdp->bg_block_bitmap = ext2fs_swab32(gdp->bg_block_bitmap);
 	gdp->bg_inode_bitmap = ext2fs_swab32(gdp->bg_inode_bitmap);
 	gdp->bg_inode_table = ext2fs_swab32(gdp->bg_inode_table);
@@ -89,6 +90,17 @@ void ext2fs_swap_group_desc(struct ext2_group_desc *gdp)
 	gdp->bg_flags = ext2fs_swab16(gdp->bg_flags);
 	gdp->bg_itable_unused = ext2fs_swab16(gdp->bg_itable_unused);
 	gdp->bg_checksum = ext2fs_swab16(gdp->bg_checksum);
+	// Now do the 64-bit parts if we need 'em
+	if (fs->super->s_desc_size >= EXT2_MIN_DESC_SIZE_64BIT) {
+		struct ext4_group_desc *gdp4 = (struct ext4_group_desc *) gdp;
+		gdp4->bg_block_bitmap_hi = ext2fs_swab32(gdp4->bg_block_bitmap_hi);
+		gdp4->bg_inode_bitmap_hi = ext2fs_swab32(gdp4->bg_inode_bitmap_hi);
+		gdp4->bg_inode_table_hi = ext2fs_swab32(gdp4->bg_inode_table_hi);
+		gdp4->bg_free_blocks_count_hi = ext2fs_swab16(gdp4->bg_free_blocks_count_hi);
+		gdp4->bg_free_inodes_count_hi = ext2fs_swab16(gdp4->bg_free_inodes_count_hi);
+		gdp4->bg_used_dirs_count_hi = ext2fs_swab16(gdp4->bg_used_dirs_count_hi);
+		gdp4->bg_itable_unused_hi = ext2fs_swab16(gdp4->bg_itable_unused_hi);
+	}
 }
 
 void ext2fs_swap_ext_attr_header(struct ext2_ext_attr_header *to_header,
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 0cbfef3..9f311bd 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -273,7 +273,7 @@ _("Warning: the backup superblock/group descriptors at block %u contain\n"
 						group_block);
 				group_bad++;
 				group = ext2fs_group_of_blk(fs, group_block+j);
-				fs->group_desc[group].bg_free_blocks_count++;
+				ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group) + 1);
 				ext2fs_group_desc_csum_set(fs, group);
 				ext2fs_free_blocks_count_add(fs->super, 1);
 			}
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 24d03d9..de1b2fd 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -261,7 +261,7 @@ static int release_blocks_proc(ext2_filsys fs, blk_t *blocknr,
 	block = *blocknr;
 	ext2fs_unmark_block_bitmap(fs->block_map, block);
 	group = ext2fs_group_of_blk(fs, block);
-	fs->group_desc[group].bg_free_blocks_count++;
+	ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group) + 1);
 	ext2fs_group_desc_csum_set(fs, group);
 	ext2fs_free_blocks_count_add(fs->super, 1);
 	return 0;
@@ -1311,8 +1311,8 @@ static errcode_t ext2fs_calculate_summary_stats(ext2_filsys fs)
 		count++;
 		if ((count == fs->super->s_blocks_per_group) ||
 		    (blk == ext2fs_blocks_count(fs->super)-1)) {
-			fs->group_desc[group++].bg_free_blocks_count =
-				group_free;
+			ext2fs_bg_free_blocks_count_set(fs, group++, group_free)
+				;
 			count = 0;
 			group_free = 0;
 		}
@@ -1336,8 +1336,8 @@ static errcode_t ext2fs_calculate_summary_stats(ext2_filsys fs)
 		count++;
 		if ((count == fs->super->s_inodes_per_group) ||
 		    (ino == fs->super->s_inodes_count)) {
-			fs->group_desc[group++].bg_free_inodes_count =
-				group_free;
+			ext2fs_bg_free_inodes_count_set(fs, group++, group_free)
+				;
 			count = 0;
 			group_free = 0;
 		}
diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index 3e2712a..1e84a62 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -465,7 +465,7 @@ retry:
 	} else
 		numblocks = fs->super->s_blocks_per_group;
 	i = old_fs->group_desc_count - 1;
-	fs->group_desc[i].bg_free_blocks_count += (numblocks-old_numblocks);
+	ext2fs_bg_free_blocks_count_set(fs, i, ext2fs_bg_free_blocks_count(fs, i) + (numblocks - old_numblocks));
 	ext2fs_group_desc_csum_set(fs, i);
 
 	/*
@@ -549,10 +549,10 @@ retry:
 			     ext2fs_free_blocks_count(fs->super) - adjblocks);
 		fs->super->s_free_inodes_count +=
 			fs->super->s_inodes_per_group;
-		fs->group_desc[i].bg_free_blocks_count = numblocks;
-		fs->group_desc[i].bg_free_inodes_count =
-			fs->super->s_inodes_per_group;
-		fs->group_desc[i].bg_used_dirs_count = 0;
+		ext2fs_bg_free_blocks_count_set(fs, i, numblocks);
+		ext2fs_bg_free_inodes_count_set(fs, i, fs->super->s_inodes_per_group)
+			;
+		ext2fs_bg_used_dirs_count_set(fs, i, 0);
 		ext2fs_group_desc_csum_set(fs, i);
 
 		retval = ext2fs_allocate_group_table(fs, i, 0);
@@ -1828,14 +1828,14 @@ static errcode_t ext2fs_calculate_summary_stats(ext2_filsys fs)
 		count++;
 		if ((count == fs->super->s_blocks_per_group) ||
 		    (blk == ext2fs_blocks_count(fs->super)-1)) {
-			fs->group_desc[group].bg_free_blocks_count =
-				group_free;
+			ext2fs_bg_free_blocks_count_set(fs, group, group_free)
+				;
 			ext2fs_group_desc_csum_set(fs, group);
 			group++;
 			count = 0;
 			group_free = 0;
-			uninit = (fs->group_desc[group].bg_flags &
-				  EXT2_BG_BLOCK_UNINIT);
+			uninit = (ext2fs_bg_flag_test(fs, group, EXT2_BG_BLOCK_UNINIT)
+				  );
 			ext2fs_super_and_bgd_loc(fs, group, &super_blk,
 						 &old_desc_blk,
 						 &new_desc_blk, 0);
@@ -1868,14 +1868,14 @@ static errcode_t ext2fs_calculate_summary_stats(ext2_filsys fs)
 		count++;
 		if ((count == fs->super->s_inodes_per_group) ||
 		    (ino == fs->super->s_inodes_count)) {
-			fs->group_desc[group].bg_free_inodes_count =
-				group_free;
+			ext2fs_bg_free_inodes_count_set(fs, group, group_free)
+				;
 			ext2fs_group_desc_csum_set(fs, group);
 			group++;
 			count = 0;
 			group_free = 0;
-			uninit = (fs->group_desc[group].bg_flags &
-				  EXT2_BG_INODE_UNINIT);
+			uninit = (ext2fs_bg_flag_test(fs, group, EXT2_BG_INODE_UNINIT)
+				  );
 		}
 	}
 	fs->super->s_free_inodes_count = total_free;

Re: Fix device too big bug in mainline?

[Theodore Tso]

On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> temporary checkin; about to do checksum conversion
> 
> From: Valerie Aurora Henson <vaurora@redhat.com>
> 
> Signed-off-by: Valerie Aurora Henson <vaurora@redhat.com>

BTW, while I was painstakingly picking apart this patch, separating it
into its constiuent pieces, I found the following bug in it:

diff --git a/e2fsck/super.c b/e2fsck/super.c
index c269b0e..a1fb878 100644
--- a/e2fsck/super.c
+++ b/e2fsck/super.c
	....
 			if (fix_problem(ctx, PR_0_GDT_UNINIT, &pctx)) {
-				gd->bg_flags &= ~(EXT2_BG_BLOCK_UNINIT |
-						  EXT2_BG_INODE_UNINIT);
-				gd->bg_itable_unused = 0;
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
+				ext2fs_bg_flag_clear (fs, i, EXT2_BG_INODE_UNINIT);
 				should_be = 1;
 			}


This patch hunk (buried deep within the 800+ lines of the "temporary
checkin; about to do checksum conversion" patch) removed this line:

				gd->bg_itable_unused = 0;

... but failed to replace it with this line:

				ext2fs_bg_itable_unused_set(fs, i, 0);

This is *why* I insist on auditable patches, and why I can't just
blindly merge the 64-bit branch.  When multiple semantic changes are
mushed up all together in one gigundo patch, it's really easy to miss
omissions like this, and since we don't have a regression test to test
this specific repair, we would have never noticed.

And no, it's no fun for me to be picking through this patch set at on
a Saturday evening on a Summer evening; I'd rather be in Lenox,
enjoying a concert at Tanglewood.  But someone has got to do it...

	     	      		     	     	     - Ted

Re: Fix device too big bug in mainline?

[Valerie Aurora]

On Sat, Aug 01, 2009 at 08:28:33PM -0400, Theodore Tso wrote:
> On Thu, Jul 30, 2009 at 05:53:02PM -0400, Valerie Aurora wrote:
> > Hi all,
> > 
> > Currently, e2fsprogs will fail to create a file system if the
> > underlying device is "too big" - even if we specify a number of blocks
> > to use that is in range:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=485236
> > 
> > This is fixed in the current pu branch, but more as a side effect of
> > an enormous 64-bit rewrite.
> > 
> > Ted, any plans to pull this into mainline?
> 
> We have a special case as of v1.41.4 so that if someone creates a 16TB
> partition, we'll treat it as having 16TB - 1 minus blocks.
> 
> Yes, I'm working on merging the 64-bit patches into mainline; and so
> far we have about 25% or so of the patches merged into the master
> branch.  It's been somewhat slow going, since I've many other things
> on my plate, and because I've wanted to do a lot of QA while doing the
> merge.  I've found more than a few bugs simply by doing code
> inspection while merging the patches one at a time.
> 
> How much do we care about this specific bug as something that needs to
> be fixed ASAP?  We already have something for a 16TB logical volume,
> since that is what is most likely to be created with lvcreate.  But do
> we consider it a common case where someone creates a 32TB logical
> volume, but intends to create a 16TB (minus 1 block) filesystem, that
> needs to be urgently fixed?

I've only talked to one customer who ran into this problem, and they
were testing, not doing anything in production.  The bug is pretty
easy to workaround - just partition your device or shrink your logical
volume.  Obviously, there's no point in having a partition larger than
16TB without the 64-bit feature since you can't grow the file system
to larger than that.  On the other hand, it offends my sensibilities
to have a known bug in a release.

To me, it seems reasonable to wait to fix this bug until the 64-bit
stuff goes in, especially since you'd have to write a fix from
scratch, since you can't backport the 64-bit version.  However, you
are the maintainer; if you think it should be fixed sooner then I'll
go ahead and write it.

-VAL

Re: Fix device too big bug in mainline?

[Valerie Aurora]

On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> In case people are wondering why it's taking so long to merge the
> 64-bit patch series, let me show one patch as exhibit 'A' about how
> not to submit patches to me (or Linus, or any other upstream
> maintainer):

I apologize for the misunderstanding!  These patches have (clearly)
never been reviewed and were not intended for merging into mainline
without a thorough reorganization.  It's simply unfortunate that we
are all busy and no one has had time to review them in the last
several months.  I am very grateful you have time to review them now!

The main difficulty with the 64-bit patch set is choosing patch
boundaries and granularity.  I'll try to summarize the issues I ran
into as briefly as possible.

The straightforward approach goes like so:

1. Add 64-bit version function foo2() (foo() is the 32-bit)
  (make check should pass here)
2. Convert all instances of foo() to foo2()
  (make check should pass here)
3. Repeat for bar(), baz(), etc.

In the ideal world, every patch would represent a single semantic
change, and every patch would result in a correct, compilable,
testable code change.

Two major problems with this scheme arise in the e2fsprogs code base:

1. The foo2() and bar() interfaces are incompatible, requiring a
non-trivial amount of glue code to convert between the output of
foo2() and the input of bar() and vice versa.  By "non-trivial" I
mean that the number of lines of glue code may exceed the number of
lines of code of the foo() -> foo2() transition itself.

2. While the code passes "make check" on 32-bit test cases at each
patch boundary, we can't check correctness of the 64-bit case until
the transition is complete for each testable binary unit.  Thus, we
currently have to rely on code inspection alone to track down bugs
that only affect the 64-bit case.  For example, Jose's first 64-bit
patch was posted around March 2008; it was not (and could not be)
tested above 32 bits until I ran the first 64-bit mke2fs sometime in
the fall of 2008.

The result is that the straightforward approach requires a great deal
more effort than usual to convert a semantic patch boundary to a "make
check" boundary.  Someone who knows the e2fsprogs code base very well
(such as yourself) might be able to write the necessary glue code
fairly swiftly and with few bugs.  However, every other developer in
the world (including myself) might spend as much time writing and
debugging glue code as the original patches themselves - only to find
that they chose the wrong patch boundary and must throw away that
code.

Hence, my decision to wait for your review and comments before even
the most trivial reorganization of the patch set.  I apologize if I
chose wrongly!

Some notes on improving 64-bit debugging at patch boundaries:

Currently, the "make check" test suite doesn't include any
infrastructure for 64-bit test devices.  This is particularly painful
to implement because many file systems (including ext4) don't support
files with > 2^32 blocks.  During development, I hacked together a
test system using loop devices backed by sparse files on an XFS file
system - clearly not a long-term solution.  Eric has suggested using
md to build 64-bit devices out of multiple 32-bit files backing loop
devices.  Some 64-bit tests require 64-bit userland and take many
gigabytes of memory (I had to get another 4GB to bring my test machine
up to 8GB before I could test > 2^32 block file systems).  Compressed
bitmap support would certainly help.  Overall, creating a 64-bit test
infrastructure is a significant project and should definitely get your
input before going forward.

Thanks for your time,

-VAL

spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Valerie Aurora]

On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> 
> I also would greatly prefer it if people who submit patches to me obey
> basic patch and code formatting guidelines.  Things like this are
> really uncool:
> 
> -				fs->group_desc[i].bg_free_blocks_count =
> -					free_array[i];
> +				ext2fs_bg_free_blocks_count_set(fs, i, free_array[i])
> +					;

Ah, that's left over from an spatch bug that Julia Lawall (cc'd)
kindly fixed immediately after I reported it.  It won't happen if you
use the current spatch.  My apologies for missing this one during
review!

I think spatch could probably also wrap lines automatically when
making semantic patches - Julia?

I'm curious what you think of this proposal: Redo all the foo() ->
foo2() patches in the entire 64-bit series as a semantic patches.
This would also fix this kind of cut and paste bug:

+                               ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
+                               ext2fs_bg_flag_clear (fs, i, EXT2_BG_INODE_UNINIT);

I fixed several of these in the existing 64-bit code when I took it
over, so I assume more lurk undiscovered and would be revealed if we
redid them with spatch.

Julia, would you and/or your students be interested in helping?  I
think you're running out of bugs in the kernel and e2fsprogs would be
another excellent showcase for spatch/Coccinelle. :)

-VAL

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Valerie Aurora]

On Mon, Aug 03, 2009 at 04:27:40PM -0400, Valerie Aurora wrote:
> On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> > 
> > I also would greatly prefer it if people who submit patches to me obey
> > basic patch and code formatting guidelines.  Things like this are
> > really uncool:
> > 
> > -				fs->group_desc[i].bg_free_blocks_count =
> > -					free_array[i];
> > +				ext2fs_bg_free_blocks_count_set(fs, i, free_array[i])
> > +					;
> 
> Ah, that's left over from an spatch bug that Julia Lawall (cc'd)
> kindly fixed immediately after I reported it.  It won't happen if you
> use the current spatch.  My apologies for missing this one during
> review!
> 
> I think spatch could probably also wrap lines automatically when
> making semantic patches - Julia?
> 
> I'm curious what you think of this proposal: Redo all the foo() ->
> foo2() patches in the entire 64-bit series as a semantic patches.
> This would also fix this kind of cut and paste bug:
> 
> +                               ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
> +                               ext2fs_bg_flag_clear (fs, i, EXT2_BG_INODE_UNINIT);
> 
> I fixed several of these in the existing 64-bit code when I took it
> over, so I assume more lurk undiscovered and would be revealed if we
> redid them with spatch.
> 
> Julia, would you and/or your students be interested in helping?  I
> think you're running out of bugs in the kernel and e2fsprogs would be
> another excellent showcase for spatch/Coccinelle. :)

I just remembered another reason to convert to spatch: For various
reasons, I ignored several parts of e2fsprogs while doing the 64-bit
conversion.  Some of those should remain unconverted (dead code or
32-bit only code) but others should be converted.  For example, I
found some unconverted block group descriptor references in resize2fs
last week - I remember having some reason for doing it at the time but
I don't recall what it was now since it's been a few months.  Sorry!

-VAL

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Julia Lawall]

On Mon, 3 Aug 2009, Valerie Aurora wrote:

> On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> > 
> > I also would greatly prefer it if people who submit patches to me obey
> > basic patch and code formatting guidelines.  Things like this are
> > really uncool:
> > 
> > -				fs->group_desc[i].bg_free_blocks_count =
> > -					free_array[i];
> > +				ext2fs_bg_free_blocks_count_set(fs, i, free_array[i])
> > +					;
> 
> Ah, that's left over from an spatch bug that Julia Lawall (cc'd)
> kindly fixed immediately after I reported it.  It won't happen if you
> use the current spatch.  My apologies for missing this one during
> review!
> 
> I think spatch could probably also wrap lines automatically when
> making semantic patches - Julia?

It doesn't now, but it is a good point that it probably could.  The pretty 
printing works at the token level, and thus one knows the size of 
everything.  I will look into it.

> I'm curious what you think of this proposal: Redo all the foo() ->
> foo2() patches in the entire 64-bit series as a semantic patches.
> This would also fix this kind of cut and paste bug:
> 
> +                               ext2fs_bg_flag_clear (fs, i, EXT2_BG_BLOCK_UNINIT);
> +                               ext2fs_bg_flag_clear (fs, i, EXT2_BG_INODE_UNINIT);
> 
> I fixed several of these in the existing 64-bit code when I took it
> over, so I assume more lurk undiscovered and would be revealed if we
> redid them with spatch.
> 
> Julia, would you and/or your students be interested in helping?  I
> think you're running out of bugs in the kernel and e2fsprogs would be
> another excellent showcase for spatch/Coccinelle. :)

Actually, my PhD student Jesper (added to CC list) could generate semantic 
patches for such things automatically.  Where can he find the normal 
patches?

julia

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Theodore Tso]

On Mon, Aug 03, 2009 at 04:27:40PM -0400, Valerie Aurora wrote:
> I'm curious what you think of this proposal: Redo all the foo() ->
> foo2() patches in the entire 64-bit series as a semantic patches.

Well, certainly assembling all of the 64-bit changes into a single
patch is a good thing; I'm currently doing it by hand, by rearranging
patches and transplanting patch hunks around.  The reason why I
hesitate about automating things is that when then have to follow up
the patch with one that fixes up the types of various variables, and
also printf format statements, etc.

In addition, one challenge that has to be taken into account is that
once we start making wholesale changes, we start breaking the rest of
the patches in the patch queue.  So there are probably some changes
that we probably want to do by hand, and merge them into the tree
first.  This includes the bitmap changes (which I've simplified to the
point where we don't need make the *_bitmap -> *_bitmap64 type changes
any more), the filesystem swap code (which I think has an ABI change
and I want to fix slightly differently), the 64-bit dblist code (which
I want to look at more closely) and the progress meter code (which
again I want to clean up their API first).

So we probably need to merge some patches by hand before we get to the
changes that can be done via spatch.  Otherwise, if we start making
redoing thesemantic patch changes right away, the concern is that
we'll miss some of the more subtle changes that are contained within
the patch series.

If you want to start preparing for the semantic patches by preparing
and testing the receipes, and then helping to flag those patches that
contain some changes that contain some changes that can't be applied
via spatch, that would be helpful.

Does that sound like a plan?

						-Ted

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Valerie Aurora]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On Tue, Aug 04, 2009 at 10:48:46AM -0400, Theodore Tso wrote:
> 
> If you want to start preparing for the semantic patches by preparing
> and testing the receipes, and then helping to flag those patches that
> contain some changes that contain some changes that can't be applied
> via spatch, that would be helpful.
> 
> Does that sound like a plan?

That sounds great.  We won't be able to automate everything, but after
this exercise, I bet spatch will be able to automate most of it.

Jesper, the patches you'll be interested in can be found in the
shared-64bit branch of:

git://git.kernel.org/pub/scm/fs/ext2/val/e2fsprogs.git

If getting these out of git is at any trouble at all, please ask and
I'll send them as plain patches.  The obvious candidates are things
like:

Author: Valerie Aurora Henson <vaurora@redhat.com>
Date:   Tue Feb 3 13:15:19 2009 -0800

    parse_num_blocks() -> parse_num_blocks2()

More difficult and interesting projects include:

Author: Valerie Aurora Henson <vaurora@redhat.com>
Date:   Tue Feb 3 13:15:19 2009 -0800

    New accessor functions for block group descriptor variables.

And the following three patches.  I attached a (huge) .cocci script
that I used to do part of that conversion.  I'm looking forward to
your professional version of it. :) This is a good patch for testing
newline fixup features.

-VAL

[-- Attachment #2: group_desc.cocci --]
[-- Type: text/plain, Size: 5190 bytes --]

// Checksum
@@
ext2_filsys fs;
expression group, flag;
@@

-fs->group_desc[group].bg_checksum
+ext2fs_bg_checksum(fs, group)


// Get/set block/inode counts
@@
ext2_filsys fs;
expression group;
identifier blk;
expression E;
@@
(
// Free blocks
-blk = fs->group_desc[group].bg_free_blocks_count
+blk = ext2fs_bg_free_blocks_count(fs, group)
|
-fs->group_desc[group].bg_free_blocks_count = E
+ext2fs_bg_free_blocks_count_set(fs, group, E)
|
// Free inodes
-blk = fs->group_desc[group].bg_free_inodes_count
+blk = ext2fs_bg_free_inodes_count(fs, group)
|
-fs->group_desc[group].bg_free_inodes_count = E
+ext2fs_bg_free_inodes_count_set(fs, group, E)
|
// Used dirs
-blk = fs->group_desc[group].bg_used_dirs_count
+blk = ext2fs_bg_used_dirs_count(fs, group)
|
-fs->group_desc[group].bg_used_dirs_count = E
+ext2fs_bg_used_dirs_count_set(fs, group, E)
|
// Unused inode table blocks
-blk = fs->group_desc[group].bg_itable_unused
+blk = ext2fs_bg_itable_unused(fs, group)
|
-fs->group_desc[group].bg_itable_unused = E
+ext2fs_bg_itable_unused_set(fs, group, E)
)

// Increment/decrement block/inode counts
// I tried to create an isomorphism for this, but I failed.
@@
ext2_filsys fs;
expression group, i;
@@
(
// Free blocks
-fs->group_desc[group].bg_free_blocks_count++
+ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)+1)
|
-fs->group_desc[group].bg_free_blocks_count--
+ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)-1)
|
-fs->group_desc[group].bg_free_blocks_count += i
+ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)+i)
|
-fs->group_desc[group].bg_free_blocks_count -= i
+ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)-i)
|
// Free inodes
-fs->group_desc[group].bg_free_inodes_count++
+ext2fs_bg_free_inodes_count_set(fs, group, ext2fs_bg_free_inodes_count(fs, group)+1)
|
-fs->group_desc[group].bg_free_inodes_count--
+ext2fs_bg_free_inodes_count_set(fs, group, ext2fs_bg_free_inodes_count(fs, group)-1)
|
-fs->group_desc[group].bg_free_inodes_count += i
+ext2fs_bg_free_inodes_count_set(fs, group, ext2fs_bg_free_inodes_count(fs, group)+i)
|
-fs->group_desc[group].bg_free_inodes_count -= i
+ext2fs_bg_free_inodes_count_set(fs, group, ext2fs_bg_free_inodes_count(fs, group)-i)
|
// Used dirs
-fs->group_desc[group].bg_used_dirs_count++
+ext2fs_bg_used_dirs_count_set(fs, group, ext2fs_bg_used_dirs_count(fs, group)+1)
|
-fs->group_desc[group].bg_used_dirs_count--
+ext2fs_bg_used_dirs_count_set(fs, group, ext2fs_bg_used_dirs_count(fs, group)-1)
|
-fs->group_desc[group].bg_used_dirs_count += i
+ext2fs_bg_used_dirs_count_set(fs, group, ext2fs_bg_used_dirs_count(fs, group)+i)
|
-fs->group_desc[group].bg_used_dirs_count -= i
+ext2fs_bg_used_dirs_count_set(fs, group, ext2fs_bg_used_dirs_count(fs, group)-i)
|
// Unused inode table blocks
-fs->group_desc[group].bg_itable_unused++
+ext2fs_bg_itable_unused_set(fs, group, ext2fs_bg_itable_unused(fs, group)+1)
|
-fs->group_desc[group].bg_itable_unused--
+ext2fs_bg_itable_unused_set(fs, group, ext2fs_bg_itable_unused(fs, group)-1)
|
-fs->group_desc[group].bg_itable_unused += i
+ext2fs_bg_itable_unused_set(fs, group, ext2fs_bg_itable_unused(fs, group)+i)
|
-fs->group_desc[group].bg_itable_unused -= i
+ext2fs_bg_itable_unused_set(fs, group, ext2fs_bg_itable_unused(fs, group)-i)
)

// Get a pointer to a whole dang descriptor
@@
ext2_filsys fs;
expression group;
@@
-&fs->group_desc[group]
+ext2fs_group_desc(fs, fs->group_desc, group)

// Block group flags
@@
ext2_filsys fs;
expression group, flag;
@@
(
-fs->group_desc[group].bg_flags & flag
+ext2fs_bg_flag_test(fs, group, flag)
|
-fs->group_desc[group].bg_flags &= ~flag
+ext2fs_bg_flag_clear(fs, group, flag)
|
-fs->group_desc[group].bg_flags |= flag
+ext2fs_bg_flag_set(fs, group, flag)
|
-fs->group_desc[group].bg_flags = 0
+ext2fs_bg_flags_clear(fs, group, 0)
)

// Block group bitmap/table locations
@@
ext2_filsys fs;
expression group, E;
@@
(
-fs->group_desc[group].bg_block_bitmap = E
+ext2fs_block_bitmap_loc_set(fs, group, E)
|
-fs->group_desc[group].bg_block_bitmap
+ext2fs_block_bitmap_loc(fs, group)
|
-fs->group_desc[group].bg_inode_bitmap = E
+ext2fs_inode_bitmap_loc_set(fs, group, E)
|
-fs->group_desc[group].bg_inode_bitmap
+ext2fs_inode_bitmap_loc(fs, group)
|
-fs->group_desc[group].bg_inode_table = E
+ext2fs_inode_table_loc_set(fs, group, E)
|
-fs->group_desc[group].bg_inode_table
+ext2fs_inode_table_loc(fs, group)
|
-fs->group_desc[group].bg_free_blocks = E
+ext2fs_bg_free_blocks_count_set(fs, group, E)
|
-fs->group_desc[group].bg_free_blocks_count
+ext2fs_bg_free_blocks_count(fs, group)
|
-fs->group_desc[group].bg_free_inodes_count = E
+ext2fs_bg_free_inodes_count_set(fs, group, E)
|
-fs->group_desc[group].bg_free_inodes_count
+ext2fs_bg_free_inodes_count(fs, group)
|
-fs->group_desc[group].bg_used_dirs_count = E
+ext2fs_bg_used_dirs_count_set(fs, group, E)
|
-fs->group_desc[group].bg_used_dirs_count
+ext2fs_bg_used_dirs_count(fs, group)
|
-fs->group_desc[group].bg_itable_unused = E
+ext2fs_bg_itable_unused_set(fs, group, E)
|
-fs->group_desc[group].bg_itable_unused
+ext2fs_bg_itable_unused(fs, group)
)

Re: Fix device too big bug in mainline?

[Valerie Aurora]

On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> In case people are wondering why it's taking so long to merge the
> 64-bit patch series, let me show one patch as exhibit 'A' about how
> not to submit patches to me (or Linus, or any other upstream
> maintainer):

Oh, geez, those are an old patch set!  I did go back and fix the
temporary commits and dangly semi-colons, plus reimplemented progress
meters the way you wanted:

http://osdir.com/ml/linux-ext4/2009-02/msg00591.html

I just forgot because I sent it back in February. :( I hope you can
merge those changes without a lot of trouble.

-VAL

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Andreas Dilger]

Semantic patches...  a very interesting idea.

On Aug 04, 2009  14:18 -0400, Valerie Aurora wrote:
> // Free blocks
> -fs->group_desc[group].bg_free_blocks_count++
> +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)+1)
> |
> -fs->group_desc[group].bg_free_blocks_count--
> +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)-1)
> |
> -fs->group_desc[group].bg_free_blocks_count += i
> +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)+i)
> |
> -fs->group_desc[group].bg_free_blocks_count -= i
> +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)-i)

I wonder if it makes more sense for ext2fs to export functions like
ext2fs_bg_free_blocks_count_add() and ext2fs_bg_free_blocks_count_sub()?

> -fs->group_desc[group].bg_flags & flag
> +ext2fs_bg_flag_test(fs, group, flag)
> |
> -fs->group_desc[group].bg_flags &= ~flag
> +ext2fs_bg_flag_clear(fs, group, flag)
> |
> -fs->group_desc[group].bg_flags |= flag
> +ext2fs_bg_flag_set(fs, group, flag)
> |
> -fs->group_desc[group].bg_flags = 0
> +ext2fs_bg_flags_clear(fs, group, 0)

This last one looks like an error.  To clear the flags you should
use ext2fs_bg_flags_set(fs, group, 0), otherwise you are "clearing
no flags".


Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Valerie Aurora]

On Tue, Aug 04, 2009 at 01:24:08PM -0600, Andreas Dilger wrote:
> Semantic patches...  a very interesting idea.
> 
> On Aug 04, 2009  14:18 -0400, Valerie Aurora wrote:
> > // Free blocks
> > -fs->group_desc[group].bg_free_blocks_count++
> > +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)+1)
> > |
> > -fs->group_desc[group].bg_free_blocks_count--
> > +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)-1)
> > |
> > -fs->group_desc[group].bg_free_blocks_count += i
> > +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)+i)
> > |
> > -fs->group_desc[group].bg_free_blocks_count -= i
> > +ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group)-i)
> 
> I wonder if it makes more sense for ext2fs to export functions like
> ext2fs_bg_free_blocks_count_add() and ext2fs_bg_free_blocks_count_sub()?

I am agnostic.

> > -fs->group_desc[group].bg_flags = 0
> > +ext2fs_bg_flags_clear(fs, group, 0)
> 
> This last one looks like an error.  To clear the flags you should
> use ext2fs_bg_flags_set(fs, group, 0), otherwise you are "clearing
> no flags".

The code does the right thing:

void ext2fs_bg_flags_clear(ext2_filsys fs, dgrp_t group, __u16 bg_flags)
{
        if (fs->super->s_desc_size >= EXT2_MIN_DESC_SIZE_64BIT) {
                struct ext4_group_desc *gdp;
                gdp = (struct ext4_group_desc *) (fs->group_desc) + group;

                gdp->bg_flags = 0;
                return;
        }

        fs->group_desc[group].bg_flags = 0;
}

The problem is that this function is stupidly named:

ext2fs_bg_flag_clear() - does what you think
ext2fs_bg_flags_clear() - does above
              ^

It should be removed and replaced by ext2fs_bg_flags_set(), I agree.

-VAL

Re: spatch for 64-bit e2fsprogs (was Re: Fix device too big bug in mainline?)

[Theodore Tso]

On Tue, Aug 04, 2009 at 02:18:51PM -0400, Valerie Aurora wrote:
> On Tue, Aug 04, 2009 at 10:48:46AM -0400, Theodore Tso wrote:
> > 
> > If you want to start preparing for the semantic patches by preparing
> > and testing the receipes, and then helping to flag those patches that
> > contain some changes that contain some changes that can't be applied
> > via spatch, that would be helpful.
> > 
> > Does that sound like a plan?
> 
> That sounds great.  We won't be able to automate everything, but after
> this exercise, I bet spatch will be able to automate most of it.
> 
> Jesper, the patches you'll be interested in can be found in the
> shared-64bit branch of:
> 
> git://git.kernel.org/pub/scm/fs/ext2/val/e2fsprogs.git

This is actually *very* out of date.  It's based off of e2fsprogs
1.41.4, and doesn't have the latest bug fixes or my efforts to clean
up the patches.

Please use either the pu branch of e2fsprogs:

   git://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git
   http://www.kernel.org/pub/scm/fs/ext2/e2fsprogs.git

or, the e2fsprogs 64-bit patch queue, which can be found here:

   http://github.com/tytso/e2fsprogs-64bit/tree/master
   git://github.com/tytso/e2fsprogs-64bit.git

Val, I had offered you write access into this patch queue back when I
first rebased the e2fsprogs patches to 1.41.4, and you declined,
saying you preferred not to work that way.  The offer is still open; I
find that patch queue to be a **far** better way of working,
especially if the goal is to refactor and clean up the patches.
Simply using a git branch doesn't work at all; I provide the pu branch
as a convenience, but it is constantly getting rewound as I clean up,
refactor, reorder, and gradually merge patches into mainline.

Currently I have merged around third of the changes into the e2fsprogs
master branch, and the patches have been significantly improved.  The
new bitmap code has been cleaned up significantly, and is almost ready
for mainline.

						- Ted

Re: Fix device too big bug in mainline?

[Theodore Tso]

On Tue, Aug 04, 2009 at 02:28:11PM -0400, Valerie Aurora wrote:
> On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> > In case people are wondering why it's taking so long to merge the
> > 64-bit patch series, let me show one patch as exhibit 'A' about how
> > not to submit patches to me (or Linus, or any other upstream
> > maintainer):
> 
> Oh, geez, those are an old patch set!  I did go back and fix the
> temporary commits and dangly semi-colons, plus reimplemented progress
> meters the way you wanted:
> 
> http://osdir.com/ml/linux-ext4/2009-02/msg00591.html

Oh, I see what happened.  It looks like you fixed some of that up in
the "shared-64bit-handover" branch, but I didn't see that one, since
you checked in the bug fixes and patches which Nick Dokus submitted
into the "shared-64bit" branch.  Hence, the shared-64bit branch had
commits in it dating from May, 2009, while the shared-64bit-handover
branch had commits dating from February, 2009.  Hence, I started work
using the shared-64bit branch instead of the shared-64bit-handover
branch.

This is, by the way, why I believe using a patch queue is a
****much**** better way of working, instead of using multiple git
branches.  I'm guessing that when Nick started submitting patches, you
got confused and applied his patches onto the wrong branch, and so of
course I used the latest, new version of the 64-bit branch --- which
meant that I got Nick's fixes, but not your cleanups.   Argh....

I wish I had figured this out much earlier, since at this point, I've
done so much work using the non-cleaned-up patches, including merging
around a third of the patches into e2fsprogs mainline, that it's
probably not worth it to go back.

						- Ted

Re: Fix device too big bug in mainline?

[Valerie Aurora]

On Tue, Aug 04, 2009 at 04:41:22PM -0400, Theodore Tso wrote:
> On Tue, Aug 04, 2009 at 02:28:11PM -0400, Valerie Aurora wrote:
> > On Sat, Aug 01, 2009 at 10:22:09PM -0400, Theodore Tso wrote:
> > > In case people are wondering why it's taking so long to merge the
> > > 64-bit patch series, let me show one patch as exhibit 'A' about how
> > > not to submit patches to me (or Linus, or any other upstream
> > > maintainer):
> > 
> > Oh, geez, those are an old patch set!  I did go back and fix the
> > temporary commits and dangly semi-colons, plus reimplemented progress
> > meters the way you wanted:
> > 
> > http://osdir.com/ml/linux-ext4/2009-02/msg00591.html
> 
> Oh, I see what happened.  It looks like you fixed some of that up in
> the "shared-64bit-handover" branch, but I didn't see that one, since
> you checked in the bug fixes and patches which Nick Dokus submitted
> into the "shared-64bit" branch.  Hence, the shared-64bit branch had
> commits in it dating from May, 2009, while the shared-64bit-handover
> branch had commits dating from February, 2009.  Hence, I started work
> using the shared-64bit branch instead of the shared-64bit-handover
> branch.

Yeah, it's pretty hard to keep track of which branch does what after
several months go by.  Maybe next time we can sync up first if it's
been a while since the last review/pull request.

-VAL

Re: Fix device too big bug in mainline?

[Theodore Tso]

On Tue, Aug 04, 2009 at 05:29:37PM -0400, Valerie Aurora wrote:
> 
> Yeah, it's pretty hard to keep track of which branch does what after
> several months go by.  Maybe next time we can sync up first if it's
> been a while since the last review/pull request.
> 

Well, if you are planning on doing more e2fsprogs work, why not work
against a shared patch queue which is under git control?  That way we
don't need to do explicit sync ups.

Once we have more than one person with write access into the patch
queue, if someone plans to make changes that involve reorganizing or
refactoring patches, they should send e-mail to take a "lock" on the
patch queue.  I normally work on the patch queue in concentrated
bursts, usually in the evenings or on the weekends, so if you're going
to "take the lock" to work on the patch queue during the day, we're
not likely to collide with each other.

I believe this will be a far more efficient way of working together,
which won't require massive "sync up" efforts.

					- Ted

Re: Fix device too big bug in mainline?

[Theodore Tso]

On Tue, Aug 04, 2009 at 02:28:11PM -0400, Valerie Aurora wrote:
> Oh, geez, those are an old patch set!  I did go back and fix the
> temporary commits and dangly semi-colons, plus reimplemented progress
> meters the way you wanted:

I just pulled in the reimplemented progress meter from the
shared-64bits-handover patches and merged it into e2fsprogs patch
queue.

While I was testing it, I noticed that it was buggy; it was printing
progress reports of the form:

Allocating group tables:  3/ 0

This was because the printf format being used was %d/%d, but the
arguments being printed were unsigned long long's.  Now fixed.

						- Ted