LinuxLists.cc - Re: [PATCH 13/23] ext4: Add new feature to make block group checksums use metadata

2012-02-13 22:28:48

Subject: Re: [PATCH 13/23] ext4: Add new feature to make block group checksums use metadata_csum algorithm

On Mon, Feb 13, 2012 at 02:40:26PM -0700, Andreas Dilger wrote:
>
> If a kernel understands METADATA_CSUM, it will check this first and
> ignore whether GDT_CSUM is set or not (though it shouldn't ever be
> set at the same time). Either of these features will cause an older
> kernel to mount the filesystem read-only, which is all that is needed.

... note that when we check for the uninit bits, this will have to be
done if GDT_CSUM || METADATA_CSUM are set.

- Ted

2012-02-29 01:28:13

by djwong

[permalink] [raw]

Subject: [RFC] e2fsprogs: Rework metadata_csum/gdt_csum flag handling

Ok, I've reworked the block group descriptor checksum handling code per this
email thread. INCOMPAT_BG_USE_META_CSUM is gone. METADATA_CSUM implies (and
in fact overrides) GDT_CSUM. When both are set, the group descriptor checksum
uses the same function as all other metadata blocks' checksums (by default
crc32c). I created a helper function to determine if group descriptor
checksums are enabled, and the actual gdt checksum verify/set functions are
smart enough to use the correct function.

Below are the changes that I intend to make to e2fsprogs. I'll integrate these
changes into the (huge) e2fsprogs patchset, but wanted to aggregate the changes
here first to avoid overwhelming reviewers. I'll send a kernel patch shortly.

Question: What will happen to old kernels when METADATA_CSUM and GDT_CSUM are
set? Should tune2fs/e2fsck change METADATA_CSUM|GDT_CSUM to only METADATA_CSUM
if they encounter it? I'm a little concerned that a pre-METADATA_CSUM kernel
will see the GDT_CSUM flag and assume it's ok to proceed in ro mode and get
confused.

Signed-off-by: Darrick J. Wong <[email protected]>
---

debugfs/debugfs.c | 3 +--
e2fsck/pass5.c | 18 ++++++-----------
e2fsck/super.c | 3 +--
e2fsck/unix.c | 2 +-
lib/e2p/feature.c | 2 --
lib/ext2fs/alloc.c | 6 ++----
lib/ext2fs/alloc_stats.c | 3 +--
lib/ext2fs/csum.c | 13 ++++--------
lib/ext2fs/ext2_fs.h | 6 +++++-
lib/ext2fs/ext2fs.h | 12 ++++++++----
lib/ext2fs/initialize.c | 3 +--
lib/ext2fs/inode.c | 9 +++------
lib/ext2fs/openfs.c | 3 +--
lib/ext2fs/rw_bitmaps.c | 12 ++++--------
misc/dumpe2fs.c | 4 ++--
misc/mke2fs.c | 23 +++++-----------------
misc/tune2fs.c | 48 +++-------------------------------------------
resize/resize2fs.c | 12 ++++--------
18 files changed, 52 insertions(+), 130 deletions(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index c1cbf06..9c8e48e 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -357,8 +357,7 @@ void do_show_super_stats(int argc, char *argv[])
return;
}

- gdt_csum = EXT2_HAS_RO_COMPAT_FEATURE(current_fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ gdt_csum = ext2fs_has_group_desc_csum(current_fs);
for (i = 0; i < current_fs->group_desc_count; i++) {
fprintf(out, " Group %2d: block bitmap at %llu, "
"inode bitmap at %llu, "
diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index f1ce6d7..c5dba0b 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -88,7 +88,7 @@ static void check_inode_bitmap_checksum(e2fsck_t ctx)
int nbytes;
ext2_ino_t ino_itr;
errcode_t retval;
- int csum_flag = 0;
+ int csum_flag;

/* If bitmap is dirty from being fixed, checksum will be corrected */
if (ext2fs_test_ib_dirty(ctx->fs))
@@ -103,9 +103,7 @@ static void check_inode_bitmap_checksum(e2fsck_t ctx)
fatal_error(ctx, 0);
}

- if (EXT2_HAS_RO_COMPAT_FEATURE(ctx->fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
- csum_flag = 1;
+ csum_flag = ext2fs_has_group_desc_csum(ctx->fs);

clear_problem_context(&pctx);
for (i = 0; i < ctx->fs->group_desc_count; i++) {
@@ -149,7 +147,7 @@ static void check_block_bitmap_checksum(e2fsck_t ctx)
int nbytes;
blk64_t blk_itr;
errcode_t retval;
- int csum_flag = 0;
+ int csum_flag;

/* If bitmap is dirty from being fixed, checksum will be corrected */
if (ext2fs_test_bb_dirty(ctx->fs))
@@ -164,9 +162,7 @@ static void check_block_bitmap_checksum(e2fsck_t ctx)
fatal_error(ctx, 0);
}

- if (EXT2_HAS_RO_COMPAT_FEATURE(ctx->fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
- csum_flag = 1;
+ csum_flag = ext2fs_has_group_desc_csum(ctx->fs);

clear_problem_context(&pctx);
for (i = 0; i < ctx->fs->group_desc_count; i++) {
@@ -322,8 +318,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
goto errout;
}

- csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ csum_flag = ext2fs_has_group_desc_csum(fs);
redo_counts:
had_problem = 0;
save_problem = 0;
@@ -599,8 +594,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
goto errout;
}

- csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ csum_flag = ext2fs_has_group_desc_csum(fs);
redo_counts:
had_problem = 0;
save_problem = 0;
diff --git a/e2fsck/super.c b/e2fsck/super.c
index dbd337c..5f6fb08 100644
--- a/e2fsck/super.c
+++ b/e2fsck/super.c
@@ -583,8 +583,7 @@ void check_super_block(e2fsck_t ctx)
first_block = sb->s_first_data_block;
last_block = ext2fs_blocks_count(sb)-1;

- csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ csum_flag = ext2fs_has_group_desc_csum(fs);
for (i = 0; i < fs->group_desc_count; i++) {
pctx.group = i;

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 9319e40..d3fb8f8 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1658,7 +1658,7 @@ no_journal:
}

if ((run_result & E2F_FLAG_CANCEL) == 0 &&
- sb->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM &&
+ ext2fs_has_group_desc_csum(ctx->fs) &&
!(ctx->options & E2F_OPT_READONLY)) {
retval = ext2fs_set_gdt_csum(ctx->fs);
if (retval) {
diff --git a/lib/e2p/feature.c b/lib/e2p/feature.c
index 9f9c6dd..486f846 100644
--- a/lib/e2p/feature.c
+++ b/lib/e2p/feature.c
@@ -87,8 +87,6 @@ static struct feature feature_list[] = {
"mmp" },
{ E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_FLEX_BG,
"flex_bg"},
- { E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
- "bg_use_meta_csum"},
{ 0, 0, 0 },
};

diff --git a/lib/ext2fs/alloc.c b/lib/ext2fs/alloc.c
index 948a0ec..e62ed68 100644
--- a/lib/ext2fs/alloc.c
+++ b/lib/ext2fs/alloc.c
@@ -36,8 +36,7 @@ static void check_block_uninit(ext2_filsys fs, ext2fs_block_bitmap map,
blk64_t blk, super_blk, old_desc_blk, new_desc_blk;
int old_desc_blocks;

- if (!(EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) ||
+ if (!ext2fs_has_group_desc_csum(fs) ||
!(ext2fs_bg_flags_test(fs, group, EXT2_BG_BLOCK_UNINIT)))
return;

@@ -83,8 +82,7 @@ static void check_inode_uninit(ext2_filsys fs, ext2fs_inode_bitmap map,
{
ext2_ino_t i, ino;

- if (!(EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) ||
+ if (!ext2fs_has_group_desc_csum(fs) ||
!(ext2fs_bg_flags_test(fs, group, EXT2_BG_INODE_UNINIT)))
return;

diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
index adec363..4229084 100644
--- a/lib/ext2fs/alloc_stats.c
+++ b/lib/ext2fs/alloc_stats.c
@@ -38,8 +38,7 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
/* We don't strictly need to be clearing the uninit flag if inuse < 0
* (i.e. freeing inodes) but it also means something is bad. */
ext2fs_bg_flags_clear(fs, group, EXT2_BG_INODE_UNINIT);
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
+ if (ext2fs_has_group_desc_csum(fs)) {
ext2_ino_t first_unused_inode = fs->super->s_inodes_per_group -
ext2fs_bg_itable_unused(fs, group) +
group * fs->super->s_inodes_per_group + 1;
diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index 99ca652..425f736 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -743,9 +743,7 @@ STATIC __u16 ext2fs_group_desc_csum(ext2_filsys fs, dgrp_t group)
#endif

if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
- EXT2_HAS_INCOMPAT_FEATURE(fs->super,
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)) {
+ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
/* new metadata csum code */
__u16 old_crc;
__u32 crc32;
@@ -781,8 +779,7 @@ out:

int ext2fs_group_desc_csum_verify(ext2_filsys fs, dgrp_t group)
{
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
+ if (ext2fs_has_group_desc_csum(fs) &&
(ext2fs_bg_checksum(fs, group) !=
ext2fs_group_desc_csum(fs, group)))
return 0;
@@ -792,8 +789,7 @@ int ext2fs_group_desc_csum_verify(ext2_filsys fs, dgrp_t group)

void ext2fs_group_desc_csum_set(ext2_filsys fs, dgrp_t group)
{
- if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
+ if (!ext2fs_has_group_desc_csum(fs))
return;

/* ext2fs_bg_checksum_set() sets the actual checksum field but
@@ -827,8 +823,7 @@ errcode_t ext2fs_set_gdt_csum(ext2_filsys fs)
if (!fs->inode_map)
return EXT2_ET_NO_INODE_BITMAP;

- if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
+ if (!ext2fs_has_group_desc_csum(fs))
return 0;

for (i = 0; i < fs->group_desc_count; i++) {
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index c2e7fbe..89df977 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -729,6 +729,11 @@ struct ext2_super_block {
#define EXT4_FEATURE_RO_COMPAT_HAS_SNAPSHOT 0x0080
#define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100
#define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200
+/*
+ * METADATA_CSUM implies GDT_CSUM. When METADATA_CSUM is set, group
+ * descriptor checksums use the same algorithm as all other data
+ * structures' checksums.
+ */
#define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400
#define EXT4_FEATURE_RO_COMPAT_REPLICA 0x0800

@@ -743,7 +748,6 @@ struct ext2_super_block {
#define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200
#define EXT4_FEATURE_INCOMPAT_EA_INODE 0x0400
#define EXT4_FEATURE_INCOMPAT_DIRDATA 0x1000
-#define EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM 0x8000

#define EXT2_FEATURE_COMPAT_SUPP 0
#define EXT2_FEATURE_INCOMPAT_SUPP (EXT2_FEATURE_INCOMPAT_FILETYPE| \
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index ff2799a..28cb626 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -579,8 +579,7 @@ typedef struct ext2_icount *ext2_icount_t;
EXT3_FEATURE_INCOMPAT_EXTENTS|\
EXT4_FEATURE_INCOMPAT_FLEX_BG|\
EXT4_FEATURE_INCOMPAT_MMP|\
- EXT4_FEATURE_INCOMPAT_64BIT|\
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)
+ EXT4_FEATURE_INCOMPAT_64BIT)
#else
#define EXT2_LIB_FEATURE_INCOMPAT_SUPP (EXT2_FEATURE_INCOMPAT_FILETYPE|\
EXT3_FEATURE_INCOMPAT_JOURNAL_DEV|\
@@ -589,8 +588,7 @@ typedef struct ext2_icount *ext2_icount_t;
EXT3_FEATURE_INCOMPAT_EXTENTS|\
EXT4_FEATURE_INCOMPAT_FLEX_BG|\
EXT4_FEATURE_INCOMPAT_MMP|\
- EXT4_FEATURE_INCOMPAT_64BIT|\
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)
+ EXT4_FEATURE_INCOMPAT_64BIT)
#endif
#ifdef CONFIG_QUOTA
#define EXT2_LIB_FEATURE_RO_COMPAT_SUPP (EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER|\
@@ -646,6 +644,12 @@ typedef struct stat ext2fs_struct_stat;
/*
* function prototypes
*/
+static inline int ext2fs_has_group_desc_csum(ext2_filsys fs)
+{
+ return EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
+ EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
+ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM);
+}

/* alloc.c */
extern errcode_t ext2fs_new_inode(ext2_filsys fs, ext2_ino_t dir, int mode,
diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
index a63ea18..a22cab4 100644
--- a/lib/ext2fs/initialize.c
+++ b/lib/ext2fs/initialize.c
@@ -435,8 +435,7 @@ ipg_retry:
* bitmaps will be accounted for when allocated).
*/
free_blocks = 0;
- csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ csum_flag = ext2fs_has_group_desc_csum(fs);
for (i = 0; i < fs->group_desc_count; i++) {
/*
* Don't set the BLOCK_UNINIT group for the last group
diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index 74703c5..3e6d853 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -157,8 +157,7 @@ errcode_t ext2fs_open_inode_scan(ext2_filsys fs, int buffer_blocks,
scan->current_group);
scan->inodes_left = EXT2_INODES_PER_GROUP(scan->fs->super);
scan->blocks_left = scan->fs->inode_blocks_per_group;
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
+ if (ext2fs_has_group_desc_csum(fs)) {
scan->inodes_left -=
ext2fs_bg_itable_unused(fs, scan->current_group);
scan->blocks_left =
@@ -183,8 +182,7 @@ errcode_t ext2fs_open_inode_scan(ext2_filsys fs, int buffer_blocks,
}
if (scan->fs->badblocks && scan->fs->badblocks->num)
scan->scan_flags |= EXT2_SF_CHK_BADBLOCKS;
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
+ if (ext2fs_has_group_desc_csum(fs))
scan->scan_flags |= EXT2_SF_DO_LAZY;
*ret_scan = scan;
return 0;
@@ -250,8 +248,7 @@ static errcode_t get_next_blockgroup(ext2_inode_scan scan)
scan->bytes_left = 0;
scan->inodes_left = EXT2_INODES_PER_GROUP(fs->super);
scan->blocks_left = fs->inode_blocks_per_group;
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
+ if (ext2fs_has_group_desc_csum(fs)) {
scan->inodes_left -=
ext2fs_bg_itable_unused(fs, scan->current_group);
scan->blocks_left =
diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index d2b64f4..2dc9b94 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -382,8 +382,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
* If recovery is from backup superblock, Clear _UNININT flags &
* reset bg_itable_unused to zero
*/
- if (superblock > 1 && EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
+ if (superblock > 1 && ext2fs_has_group_desc_csum(fs)) {
dgrp_t group;

for (group = 0; group < fs->group_desc_count; group++) {
diff --git a/lib/ext2fs/rw_bitmaps.c b/lib/ext2fs/rw_bitmaps.c
index a5097c1..18e18aa 100644
--- a/lib/ext2fs/rw_bitmaps.c
+++ b/lib/ext2fs/rw_bitmaps.c
@@ -36,7 +36,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
unsigned int nbits;
errcode_t retval;
char *block_buf = NULL, *inode_buf = NULL;
- int csum_flag = 0;
+ int csum_flag;
blk64_t blk;
blk64_t blk_itr = EXT2FS_B2C(fs, fs->super->s_first_data_block);
ext2_ino_t ino_itr = 1;
@@ -46,9 +46,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
if (!(fs->flags & EXT2_FLAG_RW))
return EXT2_ET_RO_FILSYS;

- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
- csum_flag = 1;
+ csum_flag = ext2fs_has_group_desc_csum(fs);

inode_nbytes = block_nbytes = 0;
if (do_block) {
@@ -168,7 +166,7 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
errcode_t retval;
int block_nbytes = EXT2_CLUSTERS_PER_GROUP(fs->super) / 8;
int inode_nbytes = EXT2_INODES_PER_GROUP(fs->super) / 8;
- int csum_flag = 0;
+ int csum_flag;
int do_image = fs->flags & EXT2_FLAG_IMAGE_FILE;
unsigned int cnt;
blk64_t blk;
@@ -181,9 +179,7 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)

fs->write_bitmaps = ext2fs_write_bitmaps;

- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
- csum_flag = 1;
+ csum_flag = ext2fs_has_group_desc_csum(fs);

retval = ext2fs_get_mem(strlen(fs->device_name) + 80, &buf);
if (retval)
diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
index b8f386e..3ceb0f8 100644
--- a/misc/dumpe2fs.c
+++ b/misc/dumpe2fs.c
@@ -114,7 +114,7 @@ static void print_bg_opts(ext2_filsys fs, dgrp_t i)
{
int first = 1, bg_flags = 0;

- if (fs->super->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM)
+ if (ext2fs_has_group_desc_csum(fs))
bg_flags = ext2fs_bg_flags(fs, i);

print_bg_opt(bg_flags, EXT2_BG_INODE_UNINIT, "INODE_UNINIT",
@@ -190,7 +190,7 @@ static void list_desc (ext2_filsys fs)
print_range(first_block, last_block);
fputs(")", stdout);
print_bg_opts(fs, i);
- if (fs->super->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM)
+ if (ext2fs_has_group_desc_csum(fs))
printf(_(" Checksum 0x%04x, unused inodes %u\n"),
ext2fs_bg_checksum(fs, i),
ext2fs_bg_itable_unused(fs, i));
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 8852735..f5d3d3b 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -885,8 +885,7 @@ static __u32 ok_features[3] = {
EXT2_FEATURE_INCOMPAT_META_BG|
EXT4_FEATURE_INCOMPAT_FLEX_BG|
EXT4_FEATURE_INCOMPAT_MMP |
- EXT4_FEATURE_INCOMPAT_64BIT |
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
+ EXT4_FEATURE_INCOMPAT_64BIT,
/* R/O compat */
EXT2_FEATURE_RO_COMPAT_LARGE_FILE|
EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
@@ -2049,7 +2048,8 @@ static int should_do_undo(const char *name)
int csum_flag, force_undo;

csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(&fs_param,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
+ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM);
force_undo = get_int_from_profile(fs_types, "force_undo", 0);
if (!force_undo && (!csum_flag || !lazy_itable_init))
return 0;
@@ -2306,19 +2306,6 @@ int main (int argc, char *argv[])
if (!quiet &&
EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
- if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
- printf(_("Group descriptor checksums "
- "are not enabled. This reduces the "
- "coverage of metadata checksumming. "
- "Pass -O uninit_bg to rectify.\n"));
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
- !EXT2_HAS_INCOMPAT_FEATURE(fs->super,
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM))
- printf(_("Group descriptor checksums will not use "
- "the faster metadata_checksum algorithm. "
- "Pass -O bg_use_meta_csum to rectify.\n"));
if (!EXT2_HAS_INCOMPAT_FEATURE(fs->super,
EXT3_FEATURE_INCOMPAT_EXTENTS))
printf(_("Extents are not enabled. The file extent "
@@ -2358,6 +2345,7 @@ int main (int argc, char *argv[])
(fs_param.s_feature_ro_compat &
(EXT4_FEATURE_RO_COMPAT_HUGE_FILE|EXT4_FEATURE_RO_COMPAT_GDT_CSUM|
EXT4_FEATURE_RO_COMPAT_DIR_NLINK|
+ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM|
EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE)))
fs->super->s_kbytes_written = 1;

@@ -2505,8 +2493,7 @@ int main (int argc, char *argv[])
* inodes as unused; we want e2fsck to consider all
* inodes as potentially containing recoverable data.
*/
- if (fs->super->s_feature_ro_compat &
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
+ if (ext2fs_has_group_desc_csum(fs)) {
for (i = 1; i < fs->group_desc_count; i++)
ext2fs_bg_itable_unused_set(fs, i, 0);
}
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index cba4d4c..5a55412 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -92,7 +92,6 @@ static unsigned long new_inode_size;
static char *ext_mount_opts;
static int usrquota, grpquota;
static int rewrite_checksums;
-static int rewrite_bgs_for_checksum;

int journal_size, journal_flags;
char *journal_device;
@@ -138,8 +137,7 @@ static __u32 ok_features[3] = {
EXT2_FEATURE_INCOMPAT_FILETYPE |
EXT3_FEATURE_INCOMPAT_EXTENTS |
EXT4_FEATURE_INCOMPAT_FLEX_BG |
- EXT4_FEATURE_INCOMPAT_MMP |
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
+ EXT4_FEATURE_INCOMPAT_MMP,
/* R/O compat */
EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
@@ -159,8 +157,7 @@ static __u32 clear_ok_features[3] = {
/* Incompat */
EXT2_FEATURE_INCOMPAT_FILETYPE |
EXT4_FEATURE_INCOMPAT_FLEX_BG |
- EXT4_FEATURE_INCOMPAT_MMP |
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
+ EXT4_FEATURE_INCOMPAT_MMP,
/* R/O compat */
EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
@@ -718,29 +715,6 @@ static void rewrite_metadata_checksums(ext2_filsys fs)
}

/*
- * Rewrite just the block group checksums. Only call this function if
- * you're _not_ calling rewrite_metadata_checksums; this function exists
- * to handle the case that you're changing bg_use_meta_csum and NOT changing
- * either gdt_csum or metadata_csum.
- */
-static void rewrite_bg_checksums(ext2_filsys fs)
-{
- int i;
-
- if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM) ||
- !EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
- return;
-
- ext2fs_init_csum_seed(fs);
- for (i = 0; i < fs->group_desc_count; i++)
- ext2fs_group_desc_csum_set(fs, i);
- fs->flags &= ~EXT2_FLAG_SUPER_ONLY;
- ext2fs_mark_super_dirty(fs);
-}
-
-/*
* Update the feature set as provided by the user.
*/
static int update_feature_set(ext2_filsys fs, char *features)
@@ -912,20 +886,6 @@ mmp_error:
}
}

- if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)) {
- if (check_fsck_needed(fs))
- exit(1);
- rewrite_bgs_for_checksum = 1;
- }
-
- if (FEATURE_OFF(E2P_FEATURE_INCOMPAT,
- EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)) {
- if (check_fsck_needed(fs))
- exit(1);
- rewrite_bgs_for_checksum = 1;
- }
-
if (FEATURE_ON(E2P_FEATURE_RO_INCOMPAT,
EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
if (check_fsck_needed(fs))
@@ -965,7 +925,7 @@ mmp_error:
}
gd->bg_itable_unused = 0;
gd->bg_flags = 0;
- gd->bg_checksum = 0;
+ ext2fs_group_desc_csum_set(fs, i);
}
fs->flags &= ~EXT2_FLAG_SUPER_ONLY;
}
@@ -2588,8 +2548,6 @@ retry_open:
}
if (rewrite_checksums)
rewrite_metadata_checksums(fs);
- else if (rewrite_bgs_for_checksum)
- rewrite_bg_checksums(fs);
if (I_flag) {
if (mount_flags & EXT2_MF_MOUNTED) {
fputs(_("The inode size may only be "
diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index dc2805d..8a02ff4 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -191,8 +191,7 @@ static void fix_uninit_block_bitmaps(ext2_filsys fs)
int old_desc_blocks;
dgrp_t g;

- if (!(EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM)))
+ if (!ext2fs_has_group_desc_csum(fs))
return;

for (g=0; g < fs->group_desc_count; g++) {
@@ -482,8 +481,7 @@ retry:
group_block = fs->super->s_first_data_block +
old_fs->group_desc_count * fs->super->s_blocks_per_group;

- csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
+ csum_flag = ext2fs_has_group_desc_csum(fs);
adj = old_fs->group_desc_count;
max_group = fs->group_desc_count - adj;
if (fs->super->s_feature_incompat & EXT2_FEATURE_INCOMPAT_META_BG)
@@ -743,8 +741,7 @@ static void mark_fs_metablock(ext2_resize_t rfs,
} else if (IS_INODE_TB(fs, group, blk)) {
ext2fs_inode_table_loc_set(fs, group, 0);
rfs->needed_blocks++;
- } else if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
+ } else if (ext2fs_has_group_desc_csum(fs) &&
(ext2fs_bg_flags_test(fs, group, EXT2_BG_BLOCK_UNINIT))) {
/*
* If the block bitmap is uninitialized, which means
@@ -804,8 +801,7 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
for (blk = ext2fs_blocks_count(fs->super);
blk < ext2fs_blocks_count(old_fs->super); blk++) {
g = ext2fs_group_of_blk2(fs, blk);
- if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
- EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
+ if (ext2fs_has_group_desc_csum(fs) &&
ext2fs_bg_flags_test(old_fs, g, EXT2_BG_BLOCK_UNINIT)) {
/*
* The block bitmap is uninitialized, so skip

2012-02-29 01:32:52

by djwong

[permalink] [raw]

Subject: [RFC] ext4: Rework metadata_csum/gdt_csum flag handling in kernel

2012-02-29 05:41:06

by Andreas Dilger

[permalink] [raw]

Subject: Re: [RFC] e2fsprogs: Rework metadata_csum/gdt_csum flag handling

On 2012-02-28, at 6:27 PM, Darrick J. Wong wrote:
> Ok, I've reworked the block group descriptor checksum handling code per this
> email thread. INCOMPAT_BG_USE_META_CSUM is gone. METADATA_CSUM implies (and
> in fact overrides) GDT_CSUM. When both are set, the group descriptor checksum
> uses the same function as all other metadata blocks' checksums (by default
> crc32c). I created a helper function to determine if group descriptor
> checksums are enabled, and the actual gdt checksum verify/set functions are
> smart enough to use the correct function.
>
> Below are the changes that I intend to make to e2fsprogs. I'll integrate these
> changes into the (huge) e2fsprogs patchset, but wanted to aggregate the changes
> here first to avoid overwhelming reviewers. I'll send a kernel patch shortly.
>
> Question: What will happen to old kernels when METADATA_CSUM and GDT_CSUM are
> set?

This should never be allowed by the tools, and should be treated by e2fsck as an error, that is fixed by clearing GDT_CSUM and leaving METADATA_CSUM set.

> Should tune2fs/e2fsck change METADATA_CSUM|GDT_CSUM to only METADATA_CSUM
> if they encounter it?

Yes.

> I'm a little concerned that a pre-METADATA_CSUM kernel will see the GDT_CSUM
> flag and assume it's ok to proceed in ro mode and get confused.

Right, so if tune2fs/mke2fs set METADATA_CSUM and always disable GDT_CSUM at the same time there will be no problem. e2fsck will correct this in case it is seen in the wild. This should be rare, since it means the other feature flags are also corrupted, and that will probably force the use of a backup superblock, or make mincemeat of the filesystem for other reasons (bad checksums cannot themselves corrupt the filesystem).

> Signed-off-by: Darrick J. Wong <[email protected]>
> ---

Looks like a net win all around. One comment inline, but you can add my

Acked-by: Andreas Dilger <[email protected]>

> debugfs/debugfs.c | 3 +--
> e2fsck/pass5.c | 18 ++++++-----------
> e2fsck/super.c | 3 +--
> e2fsck/unix.c | 2 +-
> lib/e2p/feature.c | 2 --
> lib/ext2fs/alloc.c | 6 ++----
> lib/ext2fs/alloc_stats.c | 3 +--
> lib/ext2fs/csum.c | 13 ++++--------
> lib/ext2fs/ext2_fs.h | 6 +++++-
> lib/ext2fs/ext2fs.h | 12 ++++++++----
> lib/ext2fs/initialize.c | 3 +--
> lib/ext2fs/inode.c | 9 +++------
> lib/ext2fs/openfs.c | 3 +--
> lib/ext2fs/rw_bitmaps.c | 12 ++++--------
> misc/dumpe2fs.c | 4 ++--
> misc/mke2fs.c | 23 +++++-----------------
> misc/tune2fs.c | 48 +++-------------------------------------------
> resize/resize2fs.c | 12 ++++--------
> 18 files changed, 52 insertions(+), 130 deletions(-)
>
> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index c1cbf06..9c8e48e 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -357,8 +357,7 @@ void do_show_super_stats(int argc, char *argv[])
> return;
> }
>
> - gdt_csum = EXT2_HAS_RO_COMPAT_FEATURE(current_fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + gdt_csum = ext2fs_has_group_desc_csum(current_fs);
> for (i = 0; i < current_fs->group_desc_count; i++) {
> fprintf(out, " Group %2d: block bitmap at %llu, "
> "inode bitmap at %llu, "
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index f1ce6d7..c5dba0b 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -88,7 +88,7 @@ static void check_inode_bitmap_checksum(e2fsck_t ctx)
> int nbytes;
> ext2_ino_t ino_itr;
> errcode_t retval;
> - int csum_flag = 0;
> + int csum_flag;
>
> /* If bitmap is dirty from being fixed, checksum will be corrected */
> if (ext2fs_test_ib_dirty(ctx->fs))
> @@ -103,9 +103,7 @@ static void check_inode_bitmap_checksum(e2fsck_t ctx)
> fatal_error(ctx, 0);
> }
>
> - if (EXT2_HAS_RO_COMPAT_FEATURE(ctx->fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> - csum_flag = 1;
> + csum_flag = ext2fs_has_group_desc_csum(ctx->fs);
>
> clear_problem_context(&pctx);
> for (i = 0; i < ctx->fs->group_desc_count; i++) {
> @@ -149,7 +147,7 @@ static void check_block_bitmap_checksum(e2fsck_t ctx)
> int nbytes;
> blk64_t blk_itr;
> errcode_t retval;
> - int csum_flag = 0;
> + int csum_flag;
>
> /* If bitmap is dirty from being fixed, checksum will be corrected */
> if (ext2fs_test_bb_dirty(ctx->fs))
> @@ -164,9 +162,7 @@ static void check_block_bitmap_checksum(e2fsck_t ctx)
> fatal_error(ctx, 0);
> }
>
> - if (EXT2_HAS_RO_COMPAT_FEATURE(ctx->fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> - csum_flag = 1;
> + csum_flag = ext2fs_has_group_desc_csum(ctx->fs);
>
> clear_problem_context(&pctx);
> for (i = 0; i < ctx->fs->group_desc_count; i++) {
> @@ -322,8 +318,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
> goto errout;
> }
>
> - csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + csum_flag = ext2fs_has_group_desc_csum(fs);
> redo_counts:
> had_problem = 0;
> save_problem = 0;
> @@ -599,8 +594,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
> goto errout;
> }
>
> - csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + csum_flag = ext2fs_has_group_desc_csum(fs);
> redo_counts:
> had_problem = 0;
> save_problem = 0;
> diff --git a/e2fsck/super.c b/e2fsck/super.c
> index dbd337c..5f6fb08 100644
> --- a/e2fsck/super.c
> +++ b/e2fsck/super.c
> @@ -583,8 +583,7 @@ void check_super_block(e2fsck_t ctx)
> first_block = sb->s_first_data_block;
> last_block = ext2fs_blocks_count(sb)-1;
>
> - csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + csum_flag = ext2fs_has_group_desc_csum(fs);
> for (i = 0; i < fs->group_desc_count; i++) {
> pctx.group = i;
>
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 9319e40..d3fb8f8 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1658,7 +1658,7 @@ no_journal:
> }
>
> if ((run_result & E2F_FLAG_CANCEL) == 0 &&
> - sb->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM &&
> + ext2fs_has_group_desc_csum(ctx->fs) &&
> !(ctx->options & E2F_OPT_READONLY)) {
> retval = ext2fs_set_gdt_csum(ctx->fs);
> if (retval) {
> diff --git a/lib/e2p/feature.c b/lib/e2p/feature.c
> index 9f9c6dd..486f846 100644
> --- a/lib/e2p/feature.c
> +++ b/lib/e2p/feature.c
> @@ -87,8 +87,6 @@ static struct feature feature_list[] = {
> "mmp" },
> { E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_FLEX_BG,
> "flex_bg"},
> - { E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
> - "bg_use_meta_csum"},
> { 0, 0, 0 },
> };
>
> diff --git a/lib/ext2fs/alloc.c b/lib/ext2fs/alloc.c
> index 948a0ec..e62ed68 100644
> --- a/lib/ext2fs/alloc.c
> +++ b/lib/ext2fs/alloc.c
> @@ -36,8 +36,7 @@ static void check_block_uninit(ext2_filsys fs, ext2fs_block_bitmap map,
> blk64_t blk, super_blk, old_desc_blk, new_desc_blk;
> int old_desc_blocks;
>
> - if (!(EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) ||
> + if (!ext2fs_has_group_desc_csum(fs) ||
> !(ext2fs_bg_flags_test(fs, group, EXT2_BG_BLOCK_UNINIT)))
> return;
>
> @@ -83,8 +82,7 @@ static void check_inode_uninit(ext2_filsys fs, ext2fs_inode_bitmap map,
> {
> ext2_ino_t i, ino;
>
> - if (!(EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) ||
> + if (!ext2fs_has_group_desc_csum(fs) ||
> !(ext2fs_bg_flags_test(fs, group, EXT2_BG_INODE_UNINIT)))
> return;
>
> diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
> index adec363..4229084 100644
> --- a/lib/ext2fs/alloc_stats.c
> +++ b/lib/ext2fs/alloc_stats.c
> @@ -38,8 +38,7 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
> /* We don't strictly need to be clearing the uninit flag if inuse < 0
> * (i.e. freeing inodes) but it also means something is bad. */
> ext2fs_bg_flags_clear(fs, group, EXT2_BG_INODE_UNINIT);
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
> + if (ext2fs_has_group_desc_csum(fs)) {
> ext2_ino_t first_unused_inode = fs->super->s_inodes_per_group -
> ext2fs_bg_itable_unused(fs, group) +
> group * fs->super->s_inodes_per_group + 1;
> diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
> index 99ca652..425f736 100644
> --- a/lib/ext2fs/csum.c
> +++ b/lib/ext2fs/csum.c
> @@ -743,9 +743,7 @@ STATIC __u16 ext2fs_group_desc_csum(ext2_filsys fs, dgrp_t group)
> #endif
>
> if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
> - EXT2_HAS_INCOMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)) {
> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
> /* new metadata csum code */
> __u16 old_crc;
> __u32 crc32;
> @@ -781,8 +779,7 @@ out:
>
> int ext2fs_group_desc_csum_verify(ext2_filsys fs, dgrp_t group)
> {
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
> + if (ext2fs_has_group_desc_csum(fs) &&
> (ext2fs_bg_checksum(fs, group) !=
> ext2fs_group_desc_csum(fs, group)))
> return 0;
> @@ -792,8 +789,7 @@ int ext2fs_group_desc_csum_verify(ext2_filsys fs, dgrp_t group)
>
> void ext2fs_group_desc_csum_set(ext2_filsys fs, dgrp_t group)
> {
> - if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> + if (!ext2fs_has_group_desc_csum(fs))
> return;
>
> /* ext2fs_bg_checksum_set() sets the actual checksum field but
> @@ -827,8 +823,7 @@ errcode_t ext2fs_set_gdt_csum(ext2_filsys fs)
> if (!fs->inode_map)
> return EXT2_ET_NO_INODE_BITMAP;
>
> - if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> + if (!ext2fs_has_group_desc_csum(fs))
> return 0;
>
> for (i = 0; i < fs->group_desc_count; i++) {
> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
> index c2e7fbe..89df977 100644
> --- a/lib/ext2fs/ext2_fs.h
> +++ b/lib/ext2fs/ext2_fs.h
> @@ -729,6 +729,11 @@ struct ext2_super_block {
> #define EXT4_FEATURE_RO_COMPAT_HAS_SNAPSHOT 0x0080
> #define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100
> #define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200
> +/*
> + * METADATA_CSUM implies GDT_CSUM. When METADATA_CSUM is set, group

This comment should explicitly state that METADATA_CSUM is mutually
exclusive of GDT_CSUM.

> + * descriptor checksums use the same algorithm as all other data
> + * structures' checksums.
> + */
> #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400
> #define EXT4_FEATURE_RO_COMPAT_REPLICA 0x0800
>
> @@ -743,7 +748,6 @@ struct ext2_super_block {
> #define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200
> #define EXT4_FEATURE_INCOMPAT_EA_INODE 0x0400
> #define EXT4_FEATURE_INCOMPAT_DIRDATA 0x1000
> -#define EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM 0x8000
>
> #define EXT2_FEATURE_COMPAT_SUPP 0
> #define EXT2_FEATURE_INCOMPAT_SUPP (EXT2_FEATURE_INCOMPAT_FILETYPE| \
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index ff2799a..28cb626 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -579,8 +579,7 @@ typedef struct ext2_icount *ext2_icount_t;
> EXT3_FEATURE_INCOMPAT_EXTENTS|\
> EXT4_FEATURE_INCOMPAT_FLEX_BG|\
> EXT4_FEATURE_INCOMPAT_MMP|\
> - EXT4_FEATURE_INCOMPAT_64BIT|\
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)
> + EXT4_FEATURE_INCOMPAT_64BIT)
> #else
> #define EXT2_LIB_FEATURE_INCOMPAT_SUPP (EXT2_FEATURE_INCOMPAT_FILETYPE|\
> EXT3_FEATURE_INCOMPAT_JOURNAL_DEV|\
> @@ -589,8 +588,7 @@ typedef struct ext2_icount *ext2_icount_t;
> EXT3_FEATURE_INCOMPAT_EXTENTS|\
> EXT4_FEATURE_INCOMPAT_FLEX_BG|\
> EXT4_FEATURE_INCOMPAT_MMP|\
> - EXT4_FEATURE_INCOMPAT_64BIT|\
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)
> + EXT4_FEATURE_INCOMPAT_64BIT)
> #endif
> #ifdef CONFIG_QUOTA
> #define EXT2_LIB_FEATURE_RO_COMPAT_SUPP (EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER|\
> @@ -646,6 +644,12 @@ typedef struct stat ext2fs_struct_stat;
> /*
> * function prototypes
> */
> +static inline int ext2fs_has_group_desc_csum(ext2_filsys fs)
> +{
> + return EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> + EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM);
> +}
>
> /* alloc.c */
> extern errcode_t ext2fs_new_inode(ext2_filsys fs, ext2_ino_t dir, int mode,
> diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
> index a63ea18..a22cab4 100644
> --- a/lib/ext2fs/initialize.c
> +++ b/lib/ext2fs/initialize.c
> @@ -435,8 +435,7 @@ ipg_retry:
> * bitmaps will be accounted for when allocated).
> */
> free_blocks = 0;
> - csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + csum_flag = ext2fs_has_group_desc_csum(fs);
> for (i = 0; i < fs->group_desc_count; i++) {
> /*
> * Don't set the BLOCK_UNINIT group for the last group
> diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
> index 74703c5..3e6d853 100644
> --- a/lib/ext2fs/inode.c
> +++ b/lib/ext2fs/inode.c
> @@ -157,8 +157,7 @@ errcode_t ext2fs_open_inode_scan(ext2_filsys fs, int buffer_blocks,
> scan->current_group);
> scan->inodes_left = EXT2_INODES_PER_GROUP(scan->fs->super);
> scan->blocks_left = scan->fs->inode_blocks_per_group;
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
> + if (ext2fs_has_group_desc_csum(fs)) {
> scan->inodes_left -=
> ext2fs_bg_itable_unused(fs, scan->current_group);
> scan->blocks_left =
> @@ -183,8 +182,7 @@ errcode_t ext2fs_open_inode_scan(ext2_filsys fs, int buffer_blocks,
> }
> if (scan->fs->badblocks && scan->fs->badblocks->num)
> scan->scan_flags |= EXT2_SF_CHK_BADBLOCKS;
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> + if (ext2fs_has_group_desc_csum(fs))
> scan->scan_flags |= EXT2_SF_DO_LAZY;
> *ret_scan = scan;
> return 0;
> @@ -250,8 +248,7 @@ static errcode_t get_next_blockgroup(ext2_inode_scan scan)
> scan->bytes_left = 0;
> scan->inodes_left = EXT2_INODES_PER_GROUP(fs->super);
> scan->blocks_left = fs->inode_blocks_per_group;
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
> + if (ext2fs_has_group_desc_csum(fs)) {
> scan->inodes_left -=
> ext2fs_bg_itable_unused(fs, scan->current_group);
> scan->blocks_left =
> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
> index d2b64f4..2dc9b94 100644
> --- a/lib/ext2fs/openfs.c
> +++ b/lib/ext2fs/openfs.c
> @@ -382,8 +382,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
> * If recovery is from backup superblock, Clear _UNININT flags &
> * reset bg_itable_unused to zero
> */
> - if (superblock > 1 && EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
> + if (superblock > 1 && ext2fs_has_group_desc_csum(fs)) {
> dgrp_t group;
>
> for (group = 0; group < fs->group_desc_count; group++) {
> diff --git a/lib/ext2fs/rw_bitmaps.c b/lib/ext2fs/rw_bitmaps.c
> index a5097c1..18e18aa 100644
> --- a/lib/ext2fs/rw_bitmaps.c
> +++ b/lib/ext2fs/rw_bitmaps.c
> @@ -36,7 +36,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
> unsigned int nbits;
> errcode_t retval;
> char *block_buf = NULL, *inode_buf = NULL;
> - int csum_flag = 0;
> + int csum_flag;
> blk64_t blk;
> blk64_t blk_itr = EXT2FS_B2C(fs, fs->super->s_first_data_block);
> ext2_ino_t ino_itr = 1;
> @@ -46,9 +46,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
> if (!(fs->flags & EXT2_FLAG_RW))
> return EXT2_ET_RO_FILSYS;
>
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> - csum_flag = 1;
> + csum_flag = ext2fs_has_group_desc_csum(fs);
>
> inode_nbytes = block_nbytes = 0;
> if (do_block) {
> @@ -168,7 +166,7 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
> errcode_t retval;
> int block_nbytes = EXT2_CLUSTERS_PER_GROUP(fs->super) / 8;
> int inode_nbytes = EXT2_INODES_PER_GROUP(fs->super) / 8;
> - int csum_flag = 0;
> + int csum_flag;
> int do_image = fs->flags & EXT2_FLAG_IMAGE_FILE;
> unsigned int cnt;
> blk64_t blk;
> @@ -181,9 +179,7 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
>
> fs->write_bitmaps = ext2fs_write_bitmaps;
>
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> - csum_flag = 1;
> + csum_flag = ext2fs_has_group_desc_csum(fs);
>
> retval = ext2fs_get_mem(strlen(fs->device_name) + 80, &buf);
> if (retval)
> diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
> index b8f386e..3ceb0f8 100644
> --- a/misc/dumpe2fs.c
> +++ b/misc/dumpe2fs.c
> @@ -114,7 +114,7 @@ static void print_bg_opts(ext2_filsys fs, dgrp_t i)
> {
> int first = 1, bg_flags = 0;
>
> - if (fs->super->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM)
> + if (ext2fs_has_group_desc_csum(fs))
> bg_flags = ext2fs_bg_flags(fs, i);
>
> print_bg_opt(bg_flags, EXT2_BG_INODE_UNINIT, "INODE_UNINIT",
> @@ -190,7 +190,7 @@ static void list_desc (ext2_filsys fs)
> print_range(first_block, last_block);
> fputs(")", stdout);
> print_bg_opts(fs, i);
> - if (fs->super->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM)
> + if (ext2fs_has_group_desc_csum(fs))
> printf(_(" Checksum 0x%04x, unused inodes %u\n"),
> ext2fs_bg_checksum(fs, i),
> ext2fs_bg_itable_unused(fs, i));
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 8852735..f5d3d3b 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -885,8 +885,7 @@ static __u32 ok_features[3] = {
> EXT2_FEATURE_INCOMPAT_META_BG|
> EXT4_FEATURE_INCOMPAT_FLEX_BG|
> EXT4_FEATURE_INCOMPAT_MMP |
> - EXT4_FEATURE_INCOMPAT_64BIT |
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
> + EXT4_FEATURE_INCOMPAT_64BIT,
> /* R/O compat */
> EXT2_FEATURE_RO_COMPAT_LARGE_FILE|
> EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> @@ -2049,7 +2048,8 @@ static int should_do_undo(const char *name)
> int csum_flag, force_undo;
>
> csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(&fs_param,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM);
> force_undo = get_int_from_profile(fs_types, "force_undo", 0);
> if (!force_undo && (!csum_flag || !lazy_itable_init))
> return 0;
> @@ -2306,19 +2306,6 @@ int main (int argc, char *argv[])
> if (!quiet &&
> EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
> - if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> - printf(_("Group descriptor checksums "
> - "are not enabled. This reduces the "
> - "coverage of metadata checksumming. "
> - "Pass -O uninit_bg to rectify.\n"));
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
> - !EXT2_HAS_INCOMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM))
> - printf(_("Group descriptor checksums will not use "
> - "the faster metadata_checksum algorithm. "
> - "Pass -O bg_use_meta_csum to rectify.\n"));
> if (!EXT2_HAS_INCOMPAT_FEATURE(fs->super,
> EXT3_FEATURE_INCOMPAT_EXTENTS))
> printf(_("Extents are not enabled. The file extent "
> @@ -2358,6 +2345,7 @@ int main (int argc, char *argv[])
> (fs_param.s_feature_ro_compat &
> (EXT4_FEATURE_RO_COMPAT_HUGE_FILE|EXT4_FEATURE_RO_COMPAT_GDT_CSUM|
> EXT4_FEATURE_RO_COMPAT_DIR_NLINK|
> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM|
> EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE)))
> fs->super->s_kbytes_written = 1;
>
> @@ -2505,8 +2493,7 @@ int main (int argc, char *argv[])
> * inodes as unused; we want e2fsck to consider all
> * inodes as potentially containing recoverable data.
> */
> - if (fs->super->s_feature_ro_compat &
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
> + if (ext2fs_has_group_desc_csum(fs)) {
> for (i = 1; i < fs->group_desc_count; i++)
> ext2fs_bg_itable_unused_set(fs, i, 0);
> }
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index cba4d4c..5a55412 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -92,7 +92,6 @@ static unsigned long new_inode_size;
> static char *ext_mount_opts;
> static int usrquota, grpquota;
> static int rewrite_checksums;
> -static int rewrite_bgs_for_checksum;
>
> int journal_size, journal_flags;
> char *journal_device;
> @@ -138,8 +137,7 @@ static __u32 ok_features[3] = {
> EXT2_FEATURE_INCOMPAT_FILETYPE |
> EXT3_FEATURE_INCOMPAT_EXTENTS |
> EXT4_FEATURE_INCOMPAT_FLEX_BG |
> - EXT4_FEATURE_INCOMPAT_MMP |
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
> + EXT4_FEATURE_INCOMPAT_MMP,
> /* R/O compat */
> EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
> EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> @@ -159,8 +157,7 @@ static __u32 clear_ok_features[3] = {
> /* Incompat */
> EXT2_FEATURE_INCOMPAT_FILETYPE |
> EXT4_FEATURE_INCOMPAT_FLEX_BG |
> - EXT4_FEATURE_INCOMPAT_MMP |
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM,
> + EXT4_FEATURE_INCOMPAT_MMP,
> /* R/O compat */
> EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
> EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> @@ -718,29 +715,6 @@ static void rewrite_metadata_checksums(ext2_filsys fs)
> }
>
> /*
> - * Rewrite just the block group checksums. Only call this function if
> - * you're _not_ calling rewrite_metadata_checksums; this function exists
> - * to handle the case that you're changing bg_use_meta_csum and NOT changing
> - * either gdt_csum or metadata_csum.
> - */
> -static void rewrite_bg_checksums(ext2_filsys fs)
> -{
> - int i;
> -
> - if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM) ||
> - !EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> - return;
> -
> - ext2fs_init_csum_seed(fs);
> - for (i = 0; i < fs->group_desc_count; i++)
> - ext2fs_group_desc_csum_set(fs, i);
> - fs->flags &= ~EXT2_FLAG_SUPER_ONLY;
> - ext2fs_mark_super_dirty(fs);
> -}
> -
> -/*
> * Update the feature set as provided by the user.
> */
> static int update_feature_set(ext2_filsys fs, char *features)
> @@ -912,20 +886,6 @@ mmp_error:
> }
> }
>
> - if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)) {
> - if (check_fsck_needed(fs))
> - exit(1);
> - rewrite_bgs_for_checksum = 1;
> - }
> -
> - if (FEATURE_OFF(E2P_FEATURE_INCOMPAT,
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)) {
> - if (check_fsck_needed(fs))
> - exit(1);
> - rewrite_bgs_for_checksum = 1;
> - }
> -
> if (FEATURE_ON(E2P_FEATURE_RO_INCOMPAT,
> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
> if (check_fsck_needed(fs))
> @@ -965,7 +925,7 @@ mmp_error:
> }
> gd->bg_itable_unused = 0;
> gd->bg_flags = 0;
> - gd->bg_checksum = 0;
> + ext2fs_group_desc_csum_set(fs, i);
> }
> fs->flags &= ~EXT2_FLAG_SUPER_ONLY;
> }
> @@ -2588,8 +2548,6 @@ retry_open:
> }
> if (rewrite_checksums)
> rewrite_metadata_checksums(fs);
> - else if (rewrite_bgs_for_checksum)
> - rewrite_bg_checksums(fs);
> if (I_flag) {
> if (mount_flags & EXT2_MF_MOUNTED) {
> fputs(_("The inode size may only be "
> diff --git a/resize/resize2fs.c b/resize/resize2fs.c
> index dc2805d..8a02ff4 100644
> --- a/resize/resize2fs.c
> +++ b/resize/resize2fs.c
> @@ -191,8 +191,7 @@ static void fix_uninit_block_bitmaps(ext2_filsys fs)
> int old_desc_blocks;
> dgrp_t g;
>
> - if (!(EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM)))
> + if (!ext2fs_has_group_desc_csum(fs))
> return;
>
> for (g=0; g < fs->group_desc_count; g++) {
> @@ -482,8 +481,7 @@ retry:
> group_block = fs->super->s_first_data_block +
> old_fs->group_desc_count * fs->super->s_blocks_per_group;
>
> - csum_flag = EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> + csum_flag = ext2fs_has_group_desc_csum(fs);
> adj = old_fs->group_desc_count;
> max_group = fs->group_desc_count - adj;
> if (fs->super->s_feature_incompat & EXT2_FEATURE_INCOMPAT_META_BG)
> @@ -743,8 +741,7 @@ static void mark_fs_metablock(ext2_resize_t rfs,
> } else if (IS_INODE_TB(fs, group, blk)) {
> ext2fs_inode_table_loc_set(fs, group, 0);
> rfs->needed_blocks++;
> - } else if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
> + } else if (ext2fs_has_group_desc_csum(fs) &&
> (ext2fs_bg_flags_test(fs, group, EXT2_BG_BLOCK_UNINIT))) {
> /*
> * If the block bitmap is uninitialized, which means
> @@ -804,8 +801,7 @@ static errcode_t blocks_to_move(ext2_resize_t rfs)
> for (blk = ext2fs_blocks_count(fs->super);
> blk < ext2fs_blocks_count(old_fs->super); blk++) {
> g = ext2fs_group_of_blk2(fs, blk);
> - if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
> + if (ext2fs_has_group_desc_csum(fs) &&
> ext2fs_bg_flags_test(old_fs, g, EXT2_BG_BLOCK_UNINIT)) {
> /*
> * The block bitmap is uninitialized, so skip
>

Cheers, Andreas

2012-02-29 05:48:20

by Andreas Dilger

[permalink] [raw]

Subject: Re: [RFC] ext4: Rework metadata_csum/gdt_csum flag handling in kernel

On 2012-02-28, at 6:32 PM, Darrick J. Wong wrote:
> Ok, I've reworked the block group descriptor checksum handling code per this
> email thread. INCOMPAT_BG_USE_META_CSUM is gone. METADATA_CSUM implies (and
> in fact overrides) GDT_CSUM, though the group descriptor checksum uses the same
> function as all other metadata blocks' checksums (by default crc32c). I
> created a helper function to determine if group descriptor checksums are
> enabled, and the actual gdt checksum verify/set functions are smart enough to
> use the correct function.
>
> Below are the changes that I intend to make to the kernel. I'll integrate these
> changes into the (huge) kernel patchset, but wanted to aggregate the changes
> here first to avoid overwhelming reviewers.
>
> Question: What will happen to old kernels when METADATA_CSUM and GDT_CSUM are
> set? Should the kernel reject the combination and ask for fsck? I think it
> will be ok, but older kernels might not be...?

As with the e2fsprogs patch, I think METADATA_CSUM should override GDT_CSUM
completely. If both are set, then the kernel should ignore GDT_CSUM entirely
and just use the new checksum algorithm for the group descriptors. It is up
to the user tools not to allow this combination of features to be set, and
there is no value in adding an extra failure case if they are (though if the
superblock checksum is also incorrect, that means the superblock is broken
and a backup should be used and/or the mount failed).

> Signed-off-by: Darrick J. Wong <[email protected]>
> ---

One minor comment below, but I think this patch is the right approach. I was
also going to proffer my Acked-by: for this patch, but I now recall that this
patch is itself short lived and will be merged into the patch series.

> fs/ext4/balloc.c | 4 ++--
> fs/ext4/ext4.h | 20 +++++++++++++++-----
> fs/ext4/ialloc.c | 19 ++++++++-----------
> fs/ext4/inode.c | 3 +--
> fs/ext4/mballoc.c | 6 +++---
> fs/ext4/resize.c | 9 +++------
> fs/ext4/super.c | 23 ++++++++++-------------
> 7 files changed, 42 insertions(+), 42 deletions(-)
>
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 6eee0e6..b5a7951 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -168,7 +168,7 @@ void ext4_init_block_bitmap(struct super_block *sb, struct buffer_head *bh,
>
> /* If checksum is bad mark all blocks used to prevent allocation
> * essentially implementing a per-group read-only flag. */
> - if (!ext4_group_desc_csum_verify(sbi, block_group, gdp)) {
> + if (!ext4_group_desc_csum_verify(sb, block_group, gdp)) {
> ext4_error(sb, "Checksum bad for group %u", block_group);
> ext4_free_group_clusters_set(sb, gdp, 0);
> ext4_free_inodes_set(sb, gdp, 0);
> @@ -214,7 +214,7 @@ void ext4_init_block_bitmap(struct super_block *sb, struct buffer_head *bh,
> sb->s_blocksize * 8, bh->b_data);
> ext4_block_bitmap_csum_set(sb, block_group, gdp, bh,
> EXT4_BLOCKS_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, block_group, gdp);
> + ext4_group_desc_csum_set(sb, block_group, gdp);
> }
>
> /* Return the number of free blocks in a block group. It is used when
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 70bd236..a518930 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1434,6 +1434,11 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
> #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040
> #define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100
> #define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200
> +/*
> + * METADATA_CSUM implies GDT_CSUM. When METADATA_CSUM is set, group

This should also get an explicit comment that METADATA_CSUM overrides and
is mutually exclusive with GDT_CSUM.

> + * descriptor checksums use the same algorithm as all other data
> + * structures' checksums.
> + */
> #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400
>
> #define EXT4_FEATURE_INCOMPAT_COMPRESSION 0x0001
> @@ -1449,7 +1454,6 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
> #define EXT4_FEATURE_INCOMPAT_DIRDATA 0x1000 /* data in dirent */
> #define EXT4_FEATURE_INCOMPAT_INLINEDATA 0x2000 /* data in inode */
> #define EXT4_FEATURE_INCOMPAT_LARGEDIR 0x4000 /* >2GB or 3-lvl htree */
> -#define EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM 0x8000
>
> #define EXT2_FEATURE_COMPAT_SUPP EXT4_FEATURE_COMPAT_EXT_ATTR
> #define EXT2_FEATURE_INCOMPAT_SUPP (EXT4_FEATURE_INCOMPAT_FILETYPE| \
> @@ -1473,8 +1477,7 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
> EXT4_FEATURE_INCOMPAT_EXTENTS| \
> EXT4_FEATURE_INCOMPAT_64BIT| \
> EXT4_FEATURE_INCOMPAT_FLEX_BG| \
> - EXT4_FEATURE_INCOMPAT_MMP| \
> - EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM)
> + EXT4_FEATURE_INCOMPAT_MMP)
> #define EXT4_FEATURE_RO_COMPAT_SUPP (EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER| \
> EXT4_FEATURE_RO_COMPAT_LARGE_FILE| \
> EXT4_FEATURE_RO_COMPAT_GDT_CSUM| \
> @@ -2092,11 +2095,18 @@ extern void ext4_used_dirs_set(struct super_block *sb,
> struct ext4_group_desc *bg, __u32 count);
> extern void ext4_itable_unused_set(struct super_block *sb,
> struct ext4_group_desc *bg, __u32 count);
> -extern int ext4_group_desc_csum_verify(struct ext4_sb_info *sbi, __u32 group,
> +extern int ext4_group_desc_csum_verify(struct super_block *sb, __u32 group,
> struct ext4_group_desc *gdp);
> -extern void ext4_group_desc_csum_set(struct ext4_sb_info *sbi, __u32 group,
> +extern void ext4_group_desc_csum_set(struct super_block *sb, __u32 group,
> struct ext4_group_desc *gdp);
>
> +static inline int ext4_has_group_desc_csum(struct super_block *sb)
> +{
> + return EXT4_HAS_RO_COMPAT_FEATURE(sb,
> + EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM);
> +}
> +
> static inline ext4_fsblk_t ext4_blocks_count(struct ext4_super_block *es)
> {
> return ((ext4_fsblk_t)le32_to_cpu(es->s_blocks_count_hi) << 32) |
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index b9b6b27..1ade34d 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -70,13 +70,11 @@ static unsigned ext4_init_inode_bitmap(struct super_block *sb,
> ext4_group_t block_group,
> struct ext4_group_desc *gdp)
> {
> - struct ext4_sb_info *sbi = EXT4_SB(sb);
> -
> J_ASSERT_BH(bh, buffer_locked(bh));
>
> /* If checksum is bad mark all blocks and inodes use to prevent
> * allocation, essentially implementing a per-group read-only flag. */
> - if (!ext4_group_desc_csum_verify(sbi, block_group, gdp)) {
> + if (!ext4_group_desc_csum_verify(sb, block_group, gdp)) {
> ext4_error(sb, "Checksum bad for group %u", block_group);
> ext4_free_group_clusters_set(sb, gdp, 0);
> ext4_free_inodes_set(sb, gdp, 0);
> @@ -92,7 +90,7 @@ static unsigned ext4_init_inode_bitmap(struct super_block *sb,
> bh->b_data);
> ext4_inode_bitmap_csum_set(sb, block_group, gdp, bh,
> EXT4_INODES_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, block_group, gdp);
> + ext4_group_desc_csum_set(sb, block_group, gdp);
>
> return EXT4_INODES_PER_GROUP(sb);
> }
> @@ -287,7 +285,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode)
> }
> ext4_inode_bitmap_csum_set(sb, block_group, gdp, bitmap_bh,
> EXT4_INODES_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, block_group, gdp);
> + ext4_group_desc_csum_set(sb, block_group, gdp);
> ext4_unlock_group(sb, block_group);
>
> percpu_counter_inc(&sbi->s_freeinodes_counter);
> @@ -657,8 +655,7 @@ static int ext4_claim_inode(struct super_block *sb,
> }
> /* If we didn't allocate from within the initialized part of the inode
> * table then we need to initialize up to this inode. */
> - if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
> -
> + if (ext4_has_group_desc_csum(sb)) {
> if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) {
> gdp->bg_flags &= cpu_to_le16(~EXT4_BG_INODE_UNINIT);
> /* When marking the block group with
> @@ -697,7 +694,7 @@ static int ext4_claim_inode(struct super_block *sb,
> }
> ext4_inode_bitmap_csum_set(sb, group, gdp, inode_bitmap_bh,
> EXT4_INODES_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, group, gdp);
> + ext4_group_desc_csum_set(sb, group, gdp);
> err_ret:
> ext4_unlock_group(sb, group);
> up_read(&grp->alloc_sem);
> @@ -832,7 +829,7 @@ repeat_in_this_group:
>
> got:
> /* We may have to initialize the block bitmap if it isn't already */
> - if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_GDT_CSUM) &&
> + if (ext4_has_group_desc_csum(sb) &&
> gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
> struct buffer_head *block_bitmap_bh;
>
> @@ -858,7 +855,7 @@ got:
> block_bitmap_bh,
> EXT4_BLOCKS_PER_GROUP(sb) /
> 8);
> - ext4_group_desc_csum_set(sbi, group, gdp);
> + ext4_group_desc_csum_set(sb, group, gdp);
> }
> ext4_unlock_group(sb, group);
>
> @@ -1226,7 +1223,7 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
> skip_zeroout:
> ext4_lock_group(sb, group);
> gdp->bg_flags |= cpu_to_le16(EXT4_BG_INODE_ZEROED);
> - ext4_group_desc_csum_set(sbi, group, gdp);
> + ext4_group_desc_csum_set(sb, group, gdp);
> ext4_unlock_group(sb, group);
>
> BUFFER_TRACE(group_desc_bh,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c0200cf..e94ac91 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3573,8 +3573,7 @@ make_io:
> b = table;
> end = b + EXT4_SB(sb)->s_inode_readahead_blks;
> num = EXT4_INODES_PER_GROUP(sb);
> - if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> + if (ext4_has_group_desc_csum(sb))
> num -= ext4_itable_unused_count(sb, gdp);
> table += num / inodes_per_block;
> if (end > table)
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 5f2e2ed..d6062e7 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2919,7 +2919,7 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
> ext4_free_group_clusters_set(sb, gdp, len);
> ext4_block_bitmap_csum_set(sb, ac->ac_b_ex.fe_group, gdp, bitmap_bh,
> EXT4_BLOCKS_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, ac->ac_b_ex.fe_group, gdp);
> + ext4_group_desc_csum_set(sb, ac->ac_b_ex.fe_group, gdp);
>
> ext4_unlock_group(sb, ac->ac_b_ex.fe_group);
> percpu_counter_sub(&sbi->s_freeclusters_counter, ac->ac_b_ex.fe_len);
> @@ -4787,7 +4787,7 @@ do_more:
> ext4_free_group_clusters_set(sb, gdp, ret);
> ext4_block_bitmap_csum_set(sb, block_group, gdp, bitmap_bh,
> EXT4_BLOCKS_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, block_group, gdp);
> + ext4_group_desc_csum_set(sb, block_group, gdp);
> ext4_unlock_group(sb, block_group);
> percpu_counter_add(&sbi->s_freeclusters_counter, count_clusters);
>
> @@ -4933,7 +4933,7 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
> ext4_free_group_clusters_set(sb, desc, blk_free_count);
> ext4_block_bitmap_csum_set(sb, block_group, desc, bitmap_bh,
> EXT4_BLOCKS_PER_GROUP(sb) / 8);
> - ext4_group_desc_csum_set(sbi, block_group, desc);
> + ext4_group_desc_csum_set(sb, block_group, desc);
> ext4_unlock_group(sb, block_group);
> percpu_counter_add(&sbi->s_freeclusters_counter,
> EXT4_B2C(sbi, blocks_freed));
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 2363532..21ace95 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -1106,7 +1106,7 @@ static int ext4_setup_new_descs(handle_t *handle, struct super_block *sb,
> EXT4_B2C(sbi, group_data->free_blocks_count));
> ext4_free_inodes_set(sb, gdp, EXT4_INODES_PER_GROUP(sb));
> gdp->bg_flags = cpu_to_le16(*bg_flags);
> - ext4_group_desc_csum_set(sbi, group, gdp);
> + ext4_group_desc_csum_set(sb, group, gdp);
>
> err = ext4_handle_dirty_metadata(handle, NULL, gdb_bh);
> if (unlikely(err)) {
> @@ -1342,17 +1342,14 @@ static int ext4_setup_next_flex_gd(struct super_block *sb,
> (1 + ext4_bg_num_gdb(sb, group + i) +
> le16_to_cpu(es->s_reserved_gdt_blocks)) : 0;
> group_data[i].free_blocks_count = blocks_per_group - overhead;
> - if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> + if (ext4_has_group_desc_csum(sb))
> flex_gd->bg_flags[i] = EXT4_BG_BLOCK_UNINIT |
> EXT4_BG_INODE_UNINIT;
> else
> flex_gd->bg_flags[i] = EXT4_BG_INODE_ZEROED;
> }
>
> - if (last_group == n_group &&
> - EXT4_HAS_RO_COMPAT_FEATURE(sb,
> - EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
> + if (last_group == n_group && ext4_has_group_desc_csum(sb))
> /* We need to initialize block bitmap of last group. */
> flex_gd->bg_flags[i - 1] &= ~EXT4_BG_BLOCK_UNINIT;
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 2190044..6196bfa 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2097,9 +2097,7 @@ static __le16 ext4_group_desc_csum(struct ext4_sb_info *sbi, __u32 block_group,
> __le32 le_group = cpu_to_le32(block_group);
>
> if ((sbi->s_es->s_feature_ro_compat &
> - cpu_to_le32(EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) &&
> - (sbi->s_es->s_feature_incompat &
> - cpu_to_le32(EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM))) {
> + cpu_to_le32(EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))) {
> /* Use new metadata_csum algorithm */
> __u16 old_csum;
> __u32 csum32;
> @@ -2135,24 +2133,23 @@ out:
> return cpu_to_le16(crc);
> }
>
> -int ext4_group_desc_csum_verify(struct ext4_sb_info *sbi, __u32 block_group,
> +int ext4_group_desc_csum_verify(struct super_block *sb, __u32 block_group,
> struct ext4_group_desc *gdp)
> {
> - if ((sbi->s_es->s_feature_ro_compat &
> - cpu_to_le32(EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) &&
> - (gdp->bg_checksum != ext4_group_desc_csum(sbi, block_group, gdp)))
> + if (ext4_has_group_desc_csum(sb) &&
> + (gdp->bg_checksum != ext4_group_desc_csum(EXT4_SB(sb),
> + block_group, gdp)))
> return 0;
>
> return 1;
> }
>
> -void ext4_group_desc_csum_set(struct ext4_sb_info *sbi, __u32 block_group,
> +void ext4_group_desc_csum_set(struct super_block *sb, __u32 block_group,
> struct ext4_group_desc *gdp)
> {
> - if (!(sbi->s_es->s_feature_ro_compat &
> - cpu_to_le32(EXT4_FEATURE_RO_COMPAT_GDT_CSUM)))
> + if (!ext4_has_group_desc_csum(sb))
> return;
> - gdp->bg_checksum = ext4_group_desc_csum(sbi, block_group, gdp);
> + gdp->bg_checksum = ext4_group_desc_csum(EXT4_SB(sb), block_group, gdp);
> }
>
> /* Called at mount-time, super-block is locked */
> @@ -2209,7 +2206,7 @@ static int ext4_check_descriptors(struct super_block *sb,
> return 0;
> }
> ext4_lock_group(sb, i);
> - if (!ext4_group_desc_csum_verify(sbi, i, gdp)) {
> + if (!ext4_group_desc_csum_verify(sb, i, gdp)) {
> ext4_msg(sb, KERN_ERR, "ext4_check_descriptors: "
> "Checksum for group %u failed (%u!=%u)",
> i, le16_to_cpu(ext4_group_desc_csum(sbi, i,
> @@ -4620,7 +4617,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> struct ext4_group_desc *gdp =
> ext4_get_group_desc(sb, g, NULL);
>
> - if (!ext4_group_desc_csum_verify(sbi, g, gdp)) {
> + if (!ext4_group_desc_csum_verify(sb, g, gdp)) {
> ext4_msg(sb, KERN_ERR,
> "ext4_remount: Checksum for group %u failed (%u!=%u)",
> g, le16_to_cpu(ext4_group_desc_csum(sbi, g, gdp)),
>

Cheers, Andreas