Fix device too big bug in mainline?

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

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

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 <[email protected]>

---
 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(-)

index 63b9a44..befbb5b 100644
@@ -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)

index dd56e7a..f5fdef5 100644
@@ -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;

index 6cdbd6b..9ac4324 100644
@@ -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);

index d254998..5423c30 100644
@@ -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);

index b218e7f..2f691ac 100644
@@ -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);

index dd24856..e2523c3 100644
@@ -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;

index 7c79397..5612de1 100644
@@ -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);

index 7c6a9d9..a4396a4 100644
@@ -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);

index 35d076a..c1b3ca9 100644
@@ -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);
 	}
 

index 5e49608..ddb3ede 100644
@@ -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;
 	}

index 42bc01e..25af281 100644
@@ -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,

index 0cbfef3..9f311bd 100644
@@ -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);
 			}

index 24d03d9..de1b2fd 100644
@@ -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;
 		}

index 3e2712a..1e84a62 100644
@@ -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;

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

index c269b0e..a1fb878 100644

	....

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

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

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

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.

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

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

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 <[email protected]>
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 <[email protected]>
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

// 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)
)

Semantic patches... a very interesting idea.

I am agnostic.

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

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

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

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

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

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

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

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