2008-01-22 17:37:40

by Eric Sandeen

[permalink] [raw]
Subject: [PATCH e2fsprogs] ignore "safe" flag differences when fsck compares superblocks

Recent e2fsprogs (1.40.3 and higher) fsck compares primary superblock to
backups, and if things differ, it forces a full check. However, the
kernel has a penchant for updating flags the first time a feature is
used - attributes, large files, etc.

However, it only updates these on the primary sb. This then causes
the new e2fsck behavior to trigger a full check. I think these flags
can be safely ignored on this check; having them set on the primary
but not the backups doesn't indicate corruption; if they're wrongly
set on the primary, really no damage is done, and if the backup is
used, but it doesn't have the flags set when it should, I'm pretty sure
e2fsck can cope with that.

I'll admit the patch below is not glamorous. Any comments, either
on the style(sic) or the intent of the patch?

Thanks,

-Eric

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

---

Index: e2fsprogs-1.40.4/e2fsck/super.c
===================================================================
--- e2fsprogs-1.40.4.orig/e2fsck/super.c
+++ e2fsprogs-1.40.4/e2fsck/super.c
@@ -814,10 +814,29 @@ int check_backup_super_block(e2fsck_t ct
continue;
}

-#define SUPER_DIFFERENT(x) (fs->super->x != tfs->super->x)
- if (SUPER_DIFFERENT(s_feature_compat) ||
- SUPER_DIFFERENT(s_feature_incompat) ||
- SUPER_DIFFERENT(s_feature_ro_compat) ||
+ /*
+ * A few flags are set on the fly by the kernel, but
+ * only in the primary superblock. They are safe
+ * to copy even if they differ.
+ */
+
+#define FEATURE_COMPAT_IGNORE (EXT2_FEATURE_COMPAT_EXT_ATTR)
+#define FEATURE_RO_COMPAT_IGNORE (EXT2_FEATURE_RO_COMPAT_LARGE_FILE | \
+ EXT4_FEATURE_RO_COMPAT_DIR_NLINK)
+#define FEATURE_INCOMPAT_IGNORE (EXT3_FEATURE_INCOMPAT_EXTENTS)
+
+#define SUPER_COMPAT_DIFFERENT(x) \
+ ((fs->super->x & ~FEATURE_COMPAT_IGNORE) != tfs->super->x)
+#define SUPER_INCOMPAT_DIFFERENT(x) \
+ ((fs->super->x & ~FEATURE_INCOMPAT_IGNORE) != tfs->super->x)
+#define SUPER_RO_COMPAT_DIFFERENT(x) \
+ ((fs->super->x & ~FEATURE_RO_COMPAT_IGNORE) != tfs->super->x)
+#define SUPER_DIFFERENT(x) \
+ (fs->super->x != tfs->super->x)
+
+ if (SUPER_COMPAT_DIFFERENT(s_feature_compat) ||
+ SUPER_INCOMPAT_DIFFERENT(s_feature_incompat) ||
+ SUPER_RO_COMPAT_DIFFERENT(s_feature_ro_compat) ||
SUPER_DIFFERENT(s_blocks_count) ||
SUPER_DIFFERENT(s_inodes_count) ||
memcmp(fs->super->s_uuid, tfs->super->s_uuid,


2008-01-25 18:30:01

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH e2fsprogs] UPDATED: ignore "safe" flag differences when fsck compares superblocks

(updated for thinko: when proper flag *is* set on both primary & backup)

Recent e2fsprogs (1.40.3 and higher) fsck compares primary superblock to
backups, and if things differ, it forces a full check. However, the
kernel has a penchant for updating flags the first time a feature is
used - attributes, large files, etc.

However, it only updates these on the primary sb. This then causes
the new e2fsck behavior to trigger a full check. I think these flags
can be safely ignored on this check; having them set on the primary
but not the backups doesn't indicate corruption; if they're wrongly
set on the primary, really no damage is done, and if the backup is
used, but it doesn't have the flags set when it should, I'm pretty sure
e2fsck can cope with that.

I'll admit the patch below is not glamorous. Any comments, either
on the style(sic) or the intent of the patch?

Thanks,

-Eric

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

Index: e2fsprogs-1.40.4/e2fsck/super.c
===================================================================
--- e2fsprogs-1.40.4.orig/e2fsck/super.c
+++ e2fsprogs-1.40.4/e2fsck/super.c
@@ -814,10 +814,32 @@ int check_backup_super_block(e2fsck_t ct
continue;
}

-#define SUPER_DIFFERENT(x) (fs->super->x != tfs->super->x)
- if (SUPER_DIFFERENT(s_feature_compat) ||
- SUPER_DIFFERENT(s_feature_incompat) ||
- SUPER_DIFFERENT(s_feature_ro_compat) ||
+ /*
+ * A few flags are set on the fly by the kernel, but
+ * only in the primary superblock. They are safe
+ * to copy even if they differ.
+ */
+
+#define FEATURE_COMPAT_IGNORE (EXT2_FEATURE_COMPAT_EXT_ATTR)
+#define FEATURE_RO_COMPAT_IGNORE (EXT2_FEATURE_RO_COMPAT_LARGE_FILE| \
+ EXT4_FEATURE_RO_COMPAT_DIR_NLINK)
+#define FEATURE_INCOMPAT_IGNORE (EXT3_FEATURE_INCOMPAT_EXTENTS)
+
+#define SUPER_COMPAT_DIFFERENT(x) \
+ (( fs->super->x & ~FEATURE_COMPAT_IGNORE) != \
+ (tfs->super->x & ~FEATURE_COMPAT_IGNORE))
+#define SUPER_INCOMPAT_DIFFERENT(x) \
+ (( fs->super->x & ~FEATURE_INCOMPAT_IGNORE) != \
+ (tfs->super->x & ~FEATURE_INCOMPAT_IGNORE))
+#define SUPER_RO_COMPAT_DIFFERENT(x) \
+ (( fs->super->x & ~FEATURE_RO_COMPAT_IGNORE) != \
+ (tfs->super->x & ~FEATURE_RO_COMPAT_IGNORE))
+#define SUPER_DIFFERENT(x) \
+ (fs->super->x != tfs->super->x)
+
+ if (SUPER_COMPAT_DIFFERENT(s_feature_compat) ||
+ SUPER_INCOMPAT_DIFFERENT(s_feature_incompat) ||
+ SUPER_RO_COMPAT_DIFFERENT(s_feature_ro_compat) ||
SUPER_DIFFERENT(s_blocks_count) ||
SUPER_DIFFERENT(s_inodes_count) ||
memcmp(fs->super->s_uuid, tfs->super->s_uuid,

2008-01-27 04:22:22

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH e2fsprogs] UPDATED: ignore "safe" flag differences when fsck compares superblocks

This is what I've checked in. See the comment about why we can't
ignore a difference for the extended attribute feature.

- Ted

commit a8cde73acbf6e0f9c0a3601e4f5fac2b01a27bd2
Author: Theodore Ts'o <[email protected]>
Date: Sat Jan 26 23:17:50 2008 -0500

Ignore "safe" flag differences when e2fsck compares superblocks

Recent e2fsprogs (1.40.3 and higher) fsck compares primary superblock to
backups, and if things differ, it forces a full check. However, the
kernel has a penchant for updating flags the first time a feature is
used - attributes, large files, etc.

This is a bad idea, and we should break the kernel of this habit,
especially for the ext4 feature flags. But for now, let's make e2fsck
avoid forcing a full check and backup except when absolutely
necessary.

Signed-off-by: Eric Sandeen <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>

diff --git a/e2fsck/super.c b/e2fsck/super.c
index a4835f7..954783e 100644
--- a/e2fsck/super.c
+++ b/e2fsck/super.c
@@ -773,8 +773,26 @@ void check_super_block(e2fsck_t ctx)

/*
* Check to see if we should backup the master sb to the backup super
- * blocks.
+ * blocks. Returns non-zero if the sb should be backed up.
*/
+
+/*
+ * A few flags are set on the fly by the kernel, but only in the
+ * primary superblock. This is actually a bad thing, and we should
+ * try to discourage it in the future. In particular, for the newer
+ * ext4 files, especially EXT4_FEATURE_RO_COMPAT_DIR_NLINK and
+ * EXT3_FEATURE_INCOMPAT_EXTENTS. So some of these may go away in the
+ * future.
+ *
+ * The kernel will set EXT2_FEATURE_COMPAT_EXT_ATTR, but
+ * unfortunately, we shouldn't ignore it since if it's not set in the
+ * backup, the extended attributes in the filesystem will be stripped
+ * away.
+ */
+#define FEATURE_RO_COMPAT_IGNORE (EXT2_FEATURE_RO_COMPAT_LARGE_FILE| \
+ EXT4_FEATURE_RO_COMPAT_DIR_NLINK)
+#define FEATURE_INCOMPAT_IGNORE (EXT3_FEATURE_INCOMPAT_EXTENTS)
+
int check_backup_super_block(e2fsck_t ctx)
{
ext2_filsys fs = ctx->fs;
@@ -814,10 +832,18 @@ int check_backup_super_block(e2fsck_t ctx)
continue;
}

-#define SUPER_DIFFERENT(x) (fs->super->x != tfs->super->x)
+#define SUPER_INCOMPAT_DIFFERENT(x) \
+ (( fs->super->x & ~FEATURE_INCOMPAT_IGNORE) != \
+ (tfs->super->x & ~FEATURE_INCOMPAT_IGNORE))
+#define SUPER_RO_COMPAT_DIFFERENT(x) \
+ (( fs->super->x & ~FEATURE_RO_COMPAT_IGNORE) != \
+ (tfs->super->x & ~FEATURE_RO_COMPAT_IGNORE))
+#define SUPER_DIFFERENT(x) \
+ (fs->super->x != tfs->super->x)
+
if (SUPER_DIFFERENT(s_feature_compat) ||
- SUPER_DIFFERENT(s_feature_incompat) ||
- SUPER_DIFFERENT(s_feature_ro_compat) ||
+ SUPER_INCOMPAT_DIFFERENT(s_feature_incompat) ||
+ SUPER_RO_COMPAT_DIFFERENT(s_feature_ro_compat) ||
SUPER_DIFFERENT(s_blocks_count) ||
SUPER_DIFFERENT(s_inodes_count) ||
memcmp(fs->super->s_uuid, tfs->super->s_uuid,

2008-01-27 04:51:29

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH e2fsprogs] UPDATED: ignore "safe" flag differences when fsck compares superblocks

Theodore Tso wrote:
> This is what I've checked in. See the comment about why we can't
> ignore a difference for the extended attribute feature.

Ok. Unfortunately, that one really hurts on fedora, or any distro which
writes xattrs during install.

The installer mkfs's, mounts, installs with selinux, reboots => force
recheck of the entire filesystem, because the kernel wrote extended
attributes for selinux, but only to the primary superblock.

Solely having this set in primary but not in the backups should hardly
be a trigger for a full filesystem check, should it?

-Eric

> - Ted
>
> commit a8cde73acbf6e0f9c0a3601e4f5fac2b01a27bd2
> Author: Theodore Ts'o <[email protected]>
> Date: Sat Jan 26 23:17:50 2008 -0500
>
> Ignore "safe" flag differences when e2fsck compares superblocks

2008-01-27 05:25:43

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH e2fsprogs] UPDATED: ignore "safe" flag differences when fsck compares superblocks

On Sat, Jan 26, 2008 at 10:51:10PM -0600, Eric Sandeen wrote:
> Theodore Tso wrote:
> > This is what I've checked in. See the comment about why we can't
> > ignore a difference for the extended attribute feature.
>
> Ok. Unfortunately, that one really hurts on fedora, or any distro which
> writes xattrs during install.

The right fix is to simply force the extended attribute features on in
mke2fs. We can do this in mke2fs.conf. I plan to change it so that
by default it's enabled for ext3 filesystems.

- Ted