btrfs: Add incompat flags check for btrfs_check_super_valid()

Just like incompat flags check, although we have already done super csum type check before calling btrfs_check_super_valid(), we can still add such check for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/disk-io.c | 9 +++++++++
 1 file changed, 9 insertions(+)

fs/btrfs/disk-io.c +0 -0

index ec123158f051..b189ec086bc2 100644
@@ -3981,6 +3981,15 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 	u64 sectorsize = btrfs_super_sectorsize(sb);
 	int ret = 0;
 

Quoted 17 lines · Qu Wenruo

Just like incompat flags check, although we have already done super csum type check before calling btrfs_check_super_valid(), we can still add such check for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/disk-io.c | 9 +++++++++
 1 file changed, 9 insertions(+)

fs/btrfs/disk-io.c +0 -0

index ec123158f051..b189ec086bc2 100644
@@ -3981,6 +3981,15 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 	u64 sectorsize = btrfs_super_sectorsize(sb);
 	int ret = 0;
 

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info) · line 3993

Quoted 10 lines

fs/btrfs/disk-io.c +9 -0

+	/*
+	 * For write time check, as for mount time we have checked csum before
+	 * calling btrfs_check_super_valid(), so it must be a corruption
+	 */
+	if (btrfs_super_csum_type(sb) >= ARRAY_SIZE(btrfs_csum_sizes)) {
+		btrfs_err(fs_info, "corrupted csum type %u",
+			  btrfs_super_csum_type(sb));
+		ret = -EINVAL;
+	}
 	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {

The test for magic should be IMO the first one, that's how the filesystem type is identified before anything else. Granted that the magic must be correct before it can be even mounted, but the code should follow that logic too. Otherwise ok.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

fs/btrfs/disk-io.c +14 -2

+		return -EUCLEAN;
+	}
+
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	head = &fs_info->fs_devices->devices;
 	max_errors = btrfs_super_num_devices(fs_info->super_copy) - 1;

@@ -3974,9 +3982,18 @@ int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid, int level,
 					      level, first_key);
 }
 
-static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
+/*
+ * Check the validation of btrfs super block.
+ *
+ * @sb:			super block to check
+ * @super_mirror:	the super block number to check its bytenr.
+ * 			0 means the primary (1st) sb, 1 and 2 means 2nd and
+ * 			3rd backup sb, while -1 means to skip bytenr check.
+ */
+static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
+				   struct btrfs_super_block *sb,
+				   int super_mirror)
 {
-	struct btrfs_super_block *sb = fs_info->super_copy;
 	u64 nodesize = btrfs_super_nodesize(sb);
 	u64 sectorsize = btrfs_super_sectorsize(sb);
 	int ret = 0;

fs/btrfs/disk-io.c +4 -3

 	    sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
 		btrfs_err(fs_info, "invalid sectorsize %llu", sectorsize);
 		ret = -EINVAL;

@@ -4088,9 +4105,10 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 		ret = -EINVAL;
 	}
 
-	if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
-		btrfs_err(fs_info, "super offset mismatch %llu != %u",
-			  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
+	if (super_mirror >= 0 && btrfs_super_bytenr(sb) !=
+	    btrfs_sb_offset(super_mirror)) {
+		btrfs_err(fs_info, "super offset mismatch %llu != %llu",
+			btrfs_super_bytenr(sb), btrfs_sb_offset(super_mirror));
 		ret = -EINVAL;
 	}
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Looks good, some minor comments below. I'm wondering how to test that.

We'd have to inject either the corruption or to provide a way to forcibly fail the test. For the latter a debugfs should do, I'll send something for comments.

fs/btrfs/disk-io.c int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors) · line 3568

Quoted 5 lines

fs/btrfs/disk-io.c +1 -0

@@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	sb = fs_info->super_for_commit;
 	dev_item = &sb->dev_item;
 
+	if (btrfs_check_super_valid(fs_info, sb, -1)) {

A comment that this is skipping the bytenr check would be good.

fs/btrfs/disk-io.c int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors) · line 3570

Quoted 2 lines

fs/btrfs/disk-io.c +2 -0

+		btrfs_err(fs_info,
+		"superblock corruption detected before transaction commitment");

commit

Quoted 28 lines

fs/btrfs/disk-io.c +14 -2

+		return -EUCLEAN;
+	}
+
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	head = &fs_info->fs_devices->devices;
 	max_errors = btrfs_super_num_devices(fs_info->super_copy) - 1;

@@ -3974,9 +3982,18 @@ int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid, int level,
 					      level, first_key);
 }
 
-static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
+/*
+ * Check the validation of btrfs super block.
+ *
+ * @sb:			super block to check
+ * @super_mirror:	the super block number to check its bytenr.
+ * 			0 means the primary (1st) sb, 1 and 2 means 2nd and
+ * 			3rd backup sb, while -1 means to skip bytenr check.
+ */
+static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
+				   struct btrfs_super_block *sb,
+				   int super_mirror)
 {
-	struct btrfs_super_block *sb = fs_info->super_copy;
 	u64 nodesize = btrfs_super_nodesize(sb);
 	u64 sectorsize = btrfs_super_sectorsize(sb);
 	int ret = 0;

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info) · line 4039

Quoted 5 lines

fs/btrfs/disk-io.c +1 -1

@@ -4019,7 +4036,7 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 	 * Check sectorsize and nodesize first, other check will need it.
 	 * Check all possible sectorsize(4K, 8K, 16K, 32K, 64K) here.
 	 */
-	if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
+	if (!is_power_of_2(sectorsize) || sectorsize < SZ_4K ||

No unrelated changes please. There are some remaining raw values, send a separate patch if you want to convert them.

Quoted 24 lines

fs/btrfs/disk-io.c +4 -3

 	    sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
 		btrfs_err(fs_info, "invalid sectorsize %llu", sectorsize);
 		ret = -EINVAL;

@@ -4088,9 +4105,10 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 		ret = -EINVAL;
 	}
 
-	if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
-		btrfs_err(fs_info, "super offset mismatch %llu != %u",
-			  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
+	if (super_mirror >= 0 && btrfs_super_bytenr(sb) !=
+	    btrfs_sb_offset(super_mirror)) {
+		btrfs_err(fs_info, "super offset mismatch %llu != %llu",
+			btrfs_super_bytenr(sb), btrfs_sb_offset(super_mirror));
 		ret = -EINVAL;
 	}
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

fs/btrfs/disk-io.c +0 -0

@@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
  	sb = fs_info->super_for_commit;
  	dev_item = &sb->dev_item;

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

fs/btrfs/disk-io.c +10 -0

+	 * some corruption.
+	 */
+	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
+		btrfs_err(fs_info,
+		"corrupted incompat flags detected 0x%llx, supported 0x%llx",
+			  btrfs_super_incompat_flags(sb),
+			  BTRFS_FEATURE_INCOMPAT_SUPP);
+		ret = -EINVAL;
+	}
+
 	/*
 	 * The generation is a global counter, we'll trust it more than the others
 	 * but it's still possible that it's the one that's wrong.

Quoted 9 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 4 lines

---
 fs/btrfs/disk-io.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

Quoted 5 lines

fs/btrfs/disk-io.c +0 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

nit: Thinking out loud here - wouldn't it make more sense to name the function btrfs_validate_super. check_super_valid sounds a bit cumbersome to me. What do you think ?

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info) · line 4109

Quoted 6 lines

fs/btrfs/disk-io.c +3 -0

 		ret = -EINVAL;
 	}
 
+	/*
+	 * Before calling btrfs_check_super_valid() we have already checked
+	 * incompat flags. So if we developr new incompat flags, it's must be

s/developr/detect ?

Quoted 4 lines

fs/btrfs/disk-io.c +3 -0

+	 * some corruption.
+	 */
+	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info) · line 4122

Quoted 10 lines

fs/btrfs/disk-io.c +7 -0

+		btrfs_err(fs_info,
+		"corrupted incompat flags detected 0x%llx, supported 0x%llx",
+			  btrfs_super_incompat_flags(sb),
+			  BTRFS_FEATURE_INCOMPAT_SUPP);
+		ret = -EINVAL;
+	}
+
 	/*
 	 * The generation is a global counter, we'll trust it more than the others
 	 * but it's still possible that it's the one that's wrong.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 19 lines · Nikolay Borisov

Quoted 9 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 4 lines

---
 fs/btrfs/disk-io.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Quoted 9 lines

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

Quoted 5 lines

fs/btrfs/disk-io.c +0 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

nit: Thinking out loud here - wouldn't it make more sense to name the function btrfs_validate_super. check_super_valid sounds a bit cumbersome to me. What do you think ?

Indeed, I also like to remove the btrfs_ prefix since it's a static function.
validate_super() looks much better.

Quoted 32 lines · Qu Wenruo

Quoted 28 lines · Nikolay Borisov

Quoted 9 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

Quoted 9 lines

---
 fs/btrfs/disk-io.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c +0 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

nit: Thinking out loud here - wouldn't it make more sense to name the function btrfs_validate_super. check_super_valid sounds a bit cumbersome to me. What do you think ?

Quoted 3 lines

Indeed, I also like to remove the btrfs_ prefix since it's a static function.
validate_super() looks much better.

It's not necessary to remove the btrfs_ prefix from all static functions, sometimes the functions appear on stacks or mixed with other subystem helpers that have generic names. The prefix makes it clear that it's our function.

I agree with David, just make it btrfs_validate_super

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
  fs/btrfs/disk-io.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c +6 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
  		ret = -EINVAL;
  	}
 
+	/*
+	 * Before calling btrfs_check_super_valid() we have already checked
+	 * incompat flags. So if we developr new incompat flags, it's must be
+	 * some corruption.
+	 */
+	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {

Quoted 19 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
  fs/btrfs/disk-io.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c +0 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
  		ret = -EINVAL;

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info) · line 4114

Quoted 10 lines

fs/btrfs/disk-io.c +8 -0

  	}
 
+	/*
+	 * Before calling btrfs_check_super_valid() we have already checked
+	 * incompat flags. So if we developr new incompat flags, it's must be
+	 * some corruption.
+	 */
+	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
+		btrfs_err(fs_info,
+		"corrupted incompat flags detected 0x%llx, supported 0x%llx",

2707 features = btrfs_super_incompat_flags(disk_super) &

2708 ~BTRFS_FEATURE_INCOMPAT_SUPP;
2709 if (features) {
2710 btrfs_err(fs_info,
2711 "cannot mount because of unsupported optional features (%llx)",
2712 features);
2713 err = -EINVAL;
2714 goto fail_alloc;
2715 }

Quoted 34 lines · Anand Jain

Quoted 19 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(), currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
   fs/btrfs/disk-io.c | 13 +++++++++++++
   1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c +0 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
   		ret = -EINVAL;

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info) · line 4114

Quoted 10 lines

fs/btrfs/disk-io.c +8 -0

   	}
 
+	/*
+	 * Before calling btrfs_check_super_valid() we have already checked
+	 * incompat flags. So if we developr new incompat flags, it's must be
+	 * some corruption.
+	 */
+	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
+		btrfs_err(fs_info,
+		"corrupted incompat flags detected 0x%llx, supported 0x%llx",

2707 features = btrfs_super_incompat_flags(disk_super) &

Quoted 8 lines

2708 ~BTRFS_FEATURE_INCOMPAT_SUPP;
2709 if (features) {
2710 btrfs_err(fs_info,
2711 "cannot mount because of unsupported optional features (%llx)",
2712 features);
2713 err = -EINVAL;
2714 goto fail_alloc;
2715 }

So there's a "user experience" change, now that you pointed out the other check.

If a filesystem with incompat bits set will be mounted,
this will say 'you have corrupted filesystem', which is not IMO what we want to tell.

Tough the extended btrfs_check_super_valid could catch the corrupted incompat bits, what we need at mount time is wording from the 2nd message ("cannot mount unsuppported features").

I think that there should be a separate function that does the pre-commit checks, calls btrfs_check_super_valid and also validates the incompat bit.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 49 lines · David Sterba

Quoted 39 lines · Anand Jain

Quoted 25 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(),
currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
   fs/btrfs/disk-io.c | 13 +++++++++++++

   1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c +0 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct
 btrfs_fs_info *fs_info)
            ret = -EINVAL;
        }

   +    /*
+     * Before calling btrfs_check_super_valid() we have already checked

Quoted 8 lines

+     * incompat flags. So if we developr new incompat flags, it's must be
+     * some corruption.
+     */
+    if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
+        btrfs_err(fs_info,
+        "corrupted incompat flags detected 0x%llx, supported 0x%llx",

2707         features = btrfs_super_incompat_flags(disk_super) & 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;

Quoted 8 lines

2709         if (features) {
2710                 btrfs_err(fs_info,
2711                     "cannot mount because of unsupported optional features (%llx)",
2712                     features);
2713                 err = -EINVAL;
2714                 goto fail_alloc;
2715         }

Quoted 2 lines

So there's a "user experience" change, now that you pointed out the other check.

 Its a regression.

Quoted 8 lines

If a filesystem with incompat bits set will be mounted,
this will say 'you have corrupted filesystem', which is not IMO what we want to tell.

Tough the extended btrfs_check_super_valid could catch the corrupted incompat bits, what we need at mount time is wording from the 2nd message ("cannot mount unsuppported features").

Quoted 3 lines

I think that there should be a separate function that does the pre-commit checks, calls btrfs_check_super_valid and also validates the incompat bit.

 We can still print it as 'unsupported optional features', which would imply corruption in the non-mount context.

Thanks, Anand

Quoted 5 lines

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 69 lines · Anand Jain

Quoted 49 lines · David Sterba

Quoted 39 lines · Anand Jain

Quoted 25 lines · Qu Wenruo

Although we have already checked incompat flags manually before really mounting it, we could still enhance btrfs_check_super_valid() to check incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(),
currently it won't be triggered, but provides the basis for later write time check.

Signed-off-by Qu Wenruo <[email protected]>

---
    fs/btrfs/disk-io.c | 13 +++++++++++++
    1 file changed, 13 insertions(+)

fs/btrfs/disk-io.c +1 -0

index 60caa68c3618..ec123158f051 100644
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct
 btrfs_fs_info *fs_info)
            ret = -EINVAL;
        }
    +    /*
+     * Before calling btrfs_check_super_valid() we have already

checked

Quoted 8 lines

+     * incompat flags. So if we developr new incompat flags, it's must be
+     * some corruption.
+     */
+    if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
+        btrfs_err(fs_info,
+        "corrupted incompat flags detected 0x%llx, supported 0x%llx",

2707         features = btrfs_super_incompat_flags(disk_super) & 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;

Quoted 8 lines

2709         if (features) {
2710                 btrfs_err(fs_info,
2711                     "cannot mount because of unsupported optional features (%llx)",
2712                     features);
2713                 err = -EINVAL;
2714                 goto fail_alloc;
2715         }

Quoted 2 lines

So there's a "user experience" change, now that you pointed out the other check.

 Its a regression.

Quoted 11 lines

If a filesystem with incompat bits set will be mounted,
this will say 'you have corrupted filesystem', which is not IMO what we want to tell.

Tough the extended btrfs_check_super_valid could catch the corrupted incompat bits, what we need at mount time is wording from the 2nd message ("cannot mount unsuppported features").

I think that there should be a separate function that does the pre-commit checks, calls btrfs_check_super_valid and also validates the incompat bit.

Quoted 2 lines

 We can still print it as 'unsupported optional features', which would imply corruption in the non-mount context.

In that case such wording is not obvious enough to info it's super block corruption.

So I still prefer current way of checking incompact flags and csum types manually at mount time before btrfs_validate_super().

David's idea of a separate function to do mount time check and gives better prompt is pretty good.
Especially for incompat features, as incompat features will increase in the future and for older kernel info such feature as corruption is not acceptable.

But currently, there are only 2 members we need to check at mount time (csum_type and incompat features), and only one caller, current code looks good enough for its purpose.

Thanks,
Qu

Quoted 12 lines

Thanks, Anand

Quoted 5 lines

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html