[PATCH e2fsprogs] ignore "safe" flag differences when fsck compares superblocks

[PATCH e2fsprogs] ignore "safe" flag differences when fsck compares superblocks

[Eric Sandeen]

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 <sandeen@redhat.com>

---

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,

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

[Eric Sandeen]

(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 <sandeen@redhat.com>

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,

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

[Theodore Tso]

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 <tytso@mit.edu>
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 <sandeen@redhat.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

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,

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

[Eric Sandeen]

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 <tytso@mit.edu>
> Date:   Sat Jan 26 23:17:50 2008 -0500
> 
>     Ignore "safe" flag differences when e2fsck compares superblocks

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

[Theodore Tso]

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