resize2fs vs. large inodes, take 2

PATCH In review Eric Sandeen 2 participants 4 messages Mar 3, 2008 – Mar 4, 2008

lore

Share & tools

lore

Eric Sandeen

author

2008-03-03 20:40 UTC #0 lore raw

PATCH 1/1 resize2fs vs. large inodes, take 2

large inodes with both fast symlinks and in-inode selinux attrs were not surviving moves via resize2fs.

Somehow I missed that there was an ext2fs_get_next_inode_full... I think the below fix is correct, on top of my previous patch (and what is in e2fsprogs-1.40.7...)

But I don't mind a review ;)

Thanks,
-Eric

------------------- changelog ----------------------

Use ext2fs_get_next_inode_full() in resize2fs; previous attempt was not properly handling all cases, and was incorrectly setting i_extra_isize.

Signed-off-by Eric Sandeen <[email protected]>

---

resize/resize2fs.c +2 -4

Index: e2fsprogs-1.40.7/resize/resize2fs.c
===================================================================
@@ -1168,11 +1168,12 @@ static errcode_t inode_scan_and_fix(ext2
 	 * elsewhere in the inode table
 	 */
 	while (1) {
-		retval = ext2fs_get_next_inode(scan, &ino, &inode);
+		retval = ext2fs_get_next_inode_full(scan, &ino, buf, inode_size);
 		if (retval) goto errout;
 		if (!ino)
 			break;
 
+		memcpy(&inode, buf, sizeof(struct ext2_inode));
 		if (inode.i_links_count == 0 && ino != EXT2_RESIZE_INO)
 			continue; /* inode not in use */

@@ -1221,10 +1222,7 @@ static errcode_t inode_scan_and_fix(ext2
 			}
 		}
 		ext2fs_mark_inode_bitmap(rfs->new_fs->inode_map, new_inode);
-		memcpy(buf, &inode, sizeof(struct ext2_inode));
 		large_inode = (struct ext2_inode_large *)buf;
-		large_inode->i_extra_isize = sizeof(struct ext2_inode_large) -
-			EXT2_GOOD_OLD_INODE_SIZE;
 		if (pb.changed) {
 			/* Get the new version of the inode */
 			retval = ext2fs_read_inode_full(rfs->old_fs, ino,

Andreas Dilger

2008-03-03 22:06 UTC #1 lore raw

Quoted 21 lines · Eric Sandeen

large inodes with both fast symlinks and in-inode selinux attrs were not surviving moves via resize2fs.
Somehow I missed that there was an ext2fs_get_next_inode_full... I think the below fix is correct, on top of my previous patch (and what is in e2fsprogs-1.40.7...)
------------------- changelog ----------------------
Use ext2fs_get_next_inode_full() in resize2fs; previous attempt was not properly handling all cases, and was incorrectly setting i_extra_isize.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: e2fsprogs-1.40.7/resize/resize2fs.c
===================================================================

resize/resize2fs.c static errcode_t inode_scan_and_fix(ext2 · line 1176

Quoted 10 lines

resize/resize2fs.c +2 -1

@@ -1168,11 +1168,12 @@ static errcode_t inode_scan_and_fix(ext2
 	 * elsewhere in the inode table
 	 */
 	while (1) {
-		retval = ext2fs_get_next_inode(scan, &ino, &inode);
+		retval = ext2fs_get_next_inode_full(scan, &ino, buf, inode_size);
 		if (retval) goto errout;
 		if (!ino)
 			break;
 
+		memcpy(&inode, buf, sizeof(struct ext2_inode));

Should this be using "sizeof(struct ext2_inode)" or should it be using "sb->s_inode_size" instead (extracted from the right struct of course)?

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

Eric Sandeen

author

2008-03-03 22:18 UTC #2 lore raw

Quoted 13 lines · Andreas Dilger

Quoted 13 lines

===================================================================

resize/resize2fs.c +1 -1

@@ -1168,11 +1168,12 @@ static errcode_t inode_scan_and_fix(ext2
 	 * elsewhere in the inode table
 	 */
 	while (1) {
-		retval = ext2fs_get_next_inode(scan, &ino, &inode);
+		retval = ext2fs_get_next_inode_full(scan, &ino, buf, inode_size);
 		if (retval) goto errout;
 		if (!ino)
 			break;

Quoted 4 lines

resize/resize2fs.c static errcode_t inode_scan_and_fix(ext2 · line 1176
Quoted 1 line
resize/resize2fs.c +1 -0
+		memcpy(&inode, buf, sizeof(struct ext2_inode));
Should this be using "sizeof(struct ext2_inode)" or should it be using "sb->s_inode_size" instead (extracted from the right struct of course)?

well, let's see... I think we read "inode_size" in get_next_inode_full, which is s_inode_size, into buf, which was allocated to size inode_size/s_inode_size.

But "inode" is just a plain ol' little inode. I think really this "inode" is just for convenience for accessing the normal inode fields....

But I now that I try livecd-creator with this patch, even on 128-byte inodes, the fscks it runs is finding trouble post-resize (this despite all the regression test passing...) *sigh* I think I'd better sit on this problem for a while longer before I send the next patch :)

Ted, pls ignore this for now...

-Eric

Eric Sandeen

author

2008-03-04 03:19 UTC #3 lore raw

PATCH 1/1 resize2fs vs. large inodes, take 3

In reply to Eric Sandeen

(further livecd-creation testing found more problems; this patch seems cleaner, survives livecd creations w/o any corruption, and passes regression tests. Comments welcome of course. Should apply on top of e2fsprogs-1.40.7, still).

-----------------

Use ext2fs_get_next_inode_full() in resize2fs and clean up large inode handling; previous attempt was not properly handling all cases, and was incorrectly setting i_extra_isize.

Signed-off-by Eric Sandeen <[email protected]>

---

resize/resize2fs.c +19 ~~-24~~

Index: e2fsprogs-1.40.7/resize/resize2fs.c
===================================================================
@@ -1109,8 +1109,7 @@ static errcode_t inode_scan_and_fix(ext2
 {
 	struct process_block_struct	pb;
 	ext2_ino_t		ino, new_inode;
-	struct ext2_inode 	inode, *buf = NULL;
-	struct ext2_inode_large	*large_inode;
+	struct ext2_inode 	*inode = NULL;
 	ext2_inode_scan 	scan = NULL;
 	errcode_t		retval;
 	int			group;

@@ -1154,12 +1153,12 @@ static errcode_t inode_scan_and_fix(ext2
 	}
 	ext2fs_set_inode_callback(scan, progress_callback, (void *) rfs);
 	pb.rfs = rfs;
-	pb.inode = &inode;
+	pb.inode = inode;
 	pb.error = 0;
 	new_inode = EXT2_FIRST_INODE(rfs->new_fs->super);
 	inode_size = EXT2_INODE_SIZE(rfs->new_fs->super);
-	buf = malloc(inode_size);
-	if (!buf) {
+	inode = malloc(inode_size);
+	if (!inode) {
 		retval = ENOMEM;
 		goto errout;
 	}

@@ -1168,29 +1167,29 @@ static errcode_t inode_scan_and_fix(ext2
 	 * elsewhere in the inode table
 	 */
 	while (1) {
-		retval = ext2fs_get_next_inode(scan, &ino, &inode);
+		retval = ext2fs_get_next_inode_full(scan, &ino, inode, inode_size);
 		if (retval) goto errout;
 		if (!ino)
 			break;
 
-		if (inode.i_links_count == 0 && ino != EXT2_RESIZE_INO)
+		if (inode->i_links_count == 0 && ino != EXT2_RESIZE_INO)
 			continue; /* inode not in use */
 
-		pb.is_dir = LINUX_S_ISDIR(inode.i_mode);
+		pb.is_dir = LINUX_S_ISDIR(inode->i_mode);
 		pb.changed = 0;
 
-		if (inode.i_file_acl && rfs->bmap) {
+		if (inode->i_file_acl && rfs->bmap) {
 			new_block = ext2fs_extent_translate(rfs->bmap, 
-							    inode.i_file_acl);
+							    inode->i_file_acl);
 			if (new_block) {
-				inode.i_file_acl = new_block;
-				retval = ext2fs_write_inode(rfs->old_fs, 
-							    ino, &inode);
+				inode->i_file_acl = new_block;
+				retval = ext2fs_write_inode_full(rfs->old_fs, 
+							    ino, inode, inode_size);
 				if (retval) goto errout;
 			}
 		}
 		
-		if (ext2fs_inode_has_valid_blocks(&inode) &&
+		if (ext2fs_inode_has_valid_blocks(inode) &&
 		    (rfs->bmap || pb.is_dir)) {
 			pb.ino = ino;
 			retval = ext2fs_block_iterate2(rfs->old_fs,

@@ -1221,23 +1220,19 @@ static errcode_t inode_scan_and_fix(ext2
 			}
 		}
 		ext2fs_mark_inode_bitmap(rfs->new_fs->inode_map, new_inode);
-		memcpy(buf, &inode, sizeof(struct ext2_inode));
-		large_inode = (struct ext2_inode_large *)buf;
-		large_inode->i_extra_isize = sizeof(struct ext2_inode_large) -
-			EXT2_GOOD_OLD_INODE_SIZE;
 		if (pb.changed) {
 			/* Get the new version of the inode */
 			retval = ext2fs_read_inode_full(rfs->old_fs, ino,
-						buf, inode_size);
+						inode, inode_size);
 			if (retval) goto errout;
 		}
-		inode.i_ctime = time(0);
+		inode->i_ctime = time(0);
 		retval = ext2fs_write_inode_full(rfs->old_fs, new_inode,
-						buf, inode_size);
+						inode, inode_size);
 		if (retval) goto errout;
 
 		group = (new_inode-1) / EXT2_INODES_PER_GROUP(rfs->new_fs->super);
-		if (LINUX_S_ISDIR(inode.i_mode))
+		if (LINUX_S_ISDIR(inode->i_mode))
 			rfs->new_fs->group_desc[group].bg_used_dirs_count++;
 		
 #ifdef RESIZE2FS_DEBUG

@@ -1263,8 +1258,8 @@ errout:
 		ext2fs_close_inode_scan(scan);
 	if (block_buf)
 		ext2fs_free_mem(&block_buf);
-	if (buf)
-		free(buf);
+	if (inode)
+		free(inode);
 	return retval;
 }