After the fix for resize2fs's inode mover losing in-inode
extended attributes, the regression test I wrote caught
that the attrs were still getting lost on powerpc.
Looks like the problem is that ext2fs_swap_inode_full()
isn't paying attention to whether or not the EA magic is
in hostorder, so it's not recognized (and not swapped)
on BE machines. Patch below seems to fix it.
Yay for regression tests. ;)
Signed-off-by: Eric Sandeen <[email protected]>
---
Index: e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
===================================================================
--- e2fsprogs-1.40.7.orig/lib/ext2fs/swapfs.c
+++ e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys
struct ext2_inode_large *f, int hostorder,
int bufsize)
{
- unsigned i, has_data_blocks, extra_isize;
+ unsigned i, has_data_blocks, extra_isize, attr_magic;
int islnk = 0;
__u32 *eaf, *eat;
@@ -231,13 +231,17 @@ void ext2fs_swap_inode_full(ext2_filsys
eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
extra_isize);
-
- if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
- return; /* it seems no magic here */
Eric Sandeen <[email protected]> writes:
> After the fix for resize2fs's inode mover losing in-inode
> extended attributes, the regression test I wrote caught
> that the attrs were still getting lost on powerpc.
>
> Looks like the problem is that ext2fs_swap_inode_full()
> isn't paying attention to whether or not the EA magic is
> in hostorder, so it's not recognized (and not swapped)
> on BE machines. Patch below seems to fix it.
>
> Yay for regression tests. ;)
>
> Signed-off-by: Eric Sandeen <[email protected]>
> ---
>
> Index: e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
> ===================================================================
> --- e2fsprogs-1.40.7.orig/lib/ext2fs/swapfs.c
> +++ e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
> @@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys
> struct ext2_inode_large *f, int hostorder,
> int bufsize)
> {
> - unsigned i, has_data_blocks, extra_isize;
> + unsigned i, has_data_blocks, extra_isize, attr_magic;
> int islnk = 0;
> __u32 *eaf, *eat;
>
> @@ -231,13 +231,17 @@ void ext2fs_swap_inode_full(ext2_filsys
>
> eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
> extra_isize);
> -
> - if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
> - return; /* it seems no magic here */
> -
> eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
> extra_isize);
> +
> + if (hostorder)
> + attr_magic = *eaf;
> *eat = ext2fs_swab32(*eaf);
> + if (!hostorder)
> + attr_magic = *eat;
> +
> + if (attr_magic != EXT2_EXT_ATTR_MAGIC)
> + return; /* it seems no magic here */
>
> /* convert EA(s) */
> ext2fs_swap_ext_attr((char *) (eat + 1), (char *) (eaf + 1),
>
This patch seems to be missing in the current e2fsprogs 1.40.8 release
(though announced in the release notes that this issue should be fixed
on BE machines).
Check r_inline_xattr still fails on ppc and s390 architectures without
this patch. It builds fine with this patch applied.
Could you please check (also the git tag for the 1.40.8 release seems to
be missing).
Thanks,
Matthias
On Fri, Mar 14, 2008 at 02:09:06PM +0100, Matthias Koenig wrote:
> (also the git tag for the 1.40.8 release seems to be missing).
Oops, forgot to do a "git push --tags" again. Fixed.
> Eric Sandeen <[email protected]> writes:
>
> > After the fix for resize2fs's inode mover losing in-inode
> > extended attributes, the regression test I wrote caught
> > that the attrs were still getting lost on powerpc.
> >
> > Looks like the problem is that ext2fs_swap_inode_full()
> > isn't paying attention to whether or not the EA magic is
> > in hostorder, so it's not recognized (and not swapped)
> > on BE machines. Patch below seems to fix it.
>
> This patch seems to be missing in the current e2fsprogs 1.40.8 release
> (though announced in the release notes that this issue should be fixed
> on BE machines).
> Check r_inline_xattr still fails on ppc and s390 architectures without
> this patch. It builds fine with this patch applied.
This patch is in the repo, which I thought superceded the one which
Eric sent. (The authorship is wrong; and that was my fault for not
including '--author="Eric Sandeen <[email protected]>"' on the commit
command-line. I've already apologized to Eric privately; now I'm
apologizing publically. :-)
But looking at it again, I believe I misunderstood, and the two
patches are attacking quite separate problems. OK, i'll get this one
in as well in the next go-around.
- Ted
commit edfd9b0a9fe7b90f56da981ca26d5233cc3749d6
Author: Theodore Ts'o <[email protected]>
Date: Sat Mar 8 20:20:40 2008 -0500
resize2fs: Fix resizing filesystems with large inodes
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. This caused some extended
attributes to get removed or randomly assigned to other inodes as a
result of the resize, which can be unfortunate on systems using
SELinux.
The previous commit didn't fix things completely on big-endian systems
like PowerPC.
Addresses-Red-Hat-Bugzilla: #434893
Signed-off-by: Eric Sandeen <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index dd26089..6dec3eb 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -1109,8 +1109,7 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
{
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_resize_t rfs)
}
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_resize_t rfs)
* 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_resize_t rfs)
}
}
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;
}
Theodore Tso wrote:
> This patch is in the repo, which I thought superceded the one which
> Eric sent. (The authorship is wrong; and that was my fault for not
> including '--author="Eric Sandeen <[email protected]>"' on the commit
> command-line. I've already apologized to Eric privately; now I'm
> apologizing publically. :-)
And I've realized that I really should start using git to make it easier
on Ted. :)
> But looking at it again, I believe I misunderstood, and the two
> patches are attacking quite separate problems. OK, i'll get this one
> in as well in the next go-around.
Yep, there were 2 problems.
One was that inode_scan_and_fix() wasn't moving all of large inodes.
The other was that ext2fs_swap_inode_full was doing bad things for
in-inode attrs on big endian boxes, that was the patch in this thread
which isn't in the repo yet, but is in fact needed.
I suppose that the iterative nature of my fix for this problem didn't
help Ted keep things straight, sorry.
-Eric
On Fri, Mar 14, 2008 at 09:13:03AM -0500, Eric Sandeen wrote:
> Yep, there were 2 problems.
>
> One was that inode_scan_and_fix() wasn't moving all of large inodes.
>
> The other was that ext2fs_swap_inode_full was doing bad things for
> in-inode attrs on big endian boxes, that was the patch in this thread
> which isn't in the repo yet, but is in fact needed.
>
> I suppose that the iterative nature of my fix for this problem didn't
> help Ted keep things straight, sorry.
I actually *prefer* multiple patches, actually, but what works best
when there is a series of patches is to resend the whole patch stack
using git-format-patch or git-send-email --- or set up your own git
repository on the network (or use repo.or.cz) and send me a pull
request. But that requires much more use of git. :-)
In any case, I'll pull in the last change, and this time I promise
I'll be careful about remembering to use --author. That *is*
something I should be doing, but I certainly appreciate people using
git's facilities to make my life easier, thanks.
- Ted
On Fri, Feb 29, 2008 at 04:10:21PM -0600, Eric Sandeen wrote:
> +
> + if (hostorder)
> + attr_magic = *eaf;
> *eat = ext2fs_swab32(*eaf);
> + if (!hostorder)
> + attr_magic = *eat;
> +
> + if (attr_magic != EXT2_EXT_ATTR_MAGIC)
> + return; /* it seems no magic here */
The problem with this is that if the magic isn't there, we still end
up writing 4 bytes into the destination. So I think this is a better
patch....
- Ted
>From 91a2fbe36ff5578c46f637687a56d7a0d169a807 Mon Sep 17 00:00:00 2001
From: Eric Sandeen <[email protected]>
Date: Fri, 29 Feb 2008 16:10:21 -0600
Subject: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes
After the fix for resize2fs's inode mover losing in-inode
extended attributes, the regression test I wrote caught
that the attrs were still getting lost on powerpc.
Looks like the problem is that ext2fs_swap_inode_full()
isn't paying attention to whether or not the EA magic is
in hostorder, so it's not recognized (and not swapped)
on BE machines. Patch below seems to fix it.
Yay for regression tests. ;)
Signed-off-by: Eric Sandeen <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
lib/ext2fs/swapfs.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/lib/ext2fs/swapfs.c b/lib/ext2fs/swapfs.c
index 6576c59..e07e87c 100644
--- a/lib/ext2fs/swapfs.c
+++ b/lib/ext2fs/swapfs.c
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
struct ext2_inode_large *f, int hostorder,
int bufsize)
{
- unsigned i, has_data_blocks, extra_isize;
+ unsigned i, has_data_blocks, extra_isize, attr_magic;
int islnk = 0;
__u32 *eaf, *eat;
@@ -232,7 +232,11 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
extra_isize);
- if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
+ attr_magic = *eaf;
+ if (!hostorder)
+ attr_magic = ext2fs_swab32(attr_magic);
+
+ if (attr_magic != EXT2_EXT_ATTR_MAGIC)
return; /* it seems no magic here */
eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
--
1.5.4.1.144.gdfee-dirty
Theodore Tso wrote:
> On Fri, Feb 29, 2008 at 04:10:21PM -0600, Eric Sandeen wrote:
>> +
>> + if (hostorder)
>> + attr_magic = *eaf;
>> *eat = ext2fs_swab32(*eaf);
>> + if (!hostorder)
>> + attr_magic = *eat;
>> +
>> + if (attr_magic != EXT2_EXT_ATTR_MAGIC)
>> + return; /* it seems no magic here */
>
> The problem with this is that if the magic isn't there, we still end
> up writing 4 bytes into the destination. So I think this is a better
> patch....
>
Seems good to me.
Thanks,
-Eric