2008-03-03 20:40:18

by Eric Sandeen

[permalink] [raw]
Subject: [PATCH] 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]>
---

Index: e2fsprogs-1.40.7/resize/resize2fs.c
===================================================================
--- e2fsprogs-1.40.7.orig/resize/resize2fs.c
+++ 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,



2008-03-03 22:06:07

by Andreas Dilger

[permalink] [raw]
Subject: Re: [PATCH] resize2fs vs. large inodes, take 2

On Mar 03, 2008 14:40 -0600, Eric Sandeen wrote:
> 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
> ===================================================================
> --- e2fsprogs-1.40.7.orig/resize/resize2fs.c
> +++ 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));

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.


2008-03-03 22:18:54

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH] resize2fs vs. large inodes, take 2

Andreas Dilger wrote:

>> ===================================================================
>> --- e2fsprogs-1.40.7.orig/resize/resize2fs.c
>> +++ 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));
>
> 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

2008-03-04 03:19:25

by Eric Sandeen

[permalink] [raw]
Subject: [PATCH] resize2fs vs. large inodes, take 3

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

Index: e2fsprogs-1.40.7/resize/resize2fs.c
===================================================================
--- e2fsprogs-1.40.7.orig/resize/resize2fs.c
+++ 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;
}