btrfs: btrfs_super_block::log_root_transid related enhancement

we
already expect log tree root generation always to be super block genration + 1.

But it could be later used to detect log tree corruption early.

Quoted 4 lines

we
already expect log tree root generation always to be super block genration + 1.

 This is close to what I was looking for by reading the cover-letter, basically  what is the impact/bug by not populating it?

but can you
 explain more, I wonder how was check working so long then?

Quoted 2 lines

But it could be later used to detect log tree corruption early.

 If there is no impact/bug its rather a good idea to change this when the early log tree corruption detection part is ready. So that there is absolute clarity.

 The reason why I am grossly wary is - we have incomplete business about how do we handle the write-hole, say raid1 to begin with.

Thanks, Anand

We have btrfs_super_block::log_root_transid to record the transid of log tree root.

However it's never populated and it's always 0, just as the following dump-super:

 log_root                30572544
 log_root_transid        0
 log_root_level          0

This patch will populate it with log tree root correctly, so the result will be:

 log_root                30572544
 log_root_transid        6
 log_root_level          0

This won't affect current kernel behavior or btrfs check result as we already expect log tree root generation always to be super block genration + 1.

But it could be later used to detect log tree corruption early.

Quoted 15 lines · Qu Wenruo

We have btrfs_super_block::log_root_transid to record the transid of log tree root.

However it's never populated and it's always 0, just as the following dump-super:

 log_root                30572544
 log_root_transid        0
 log_root_level          0

This patch will populate it with log tree root correctly, so the result will be:

 log_root                30572544
 log_root_transid        6
 log_root_level          0

Quoted 5 lines

This won't affect current kernel behavior or btrfs check result as we already expect log tree root generation always to be super block genration + 1.

But it could be later used to detect log tree corruption early.

The backward compatibility seems to be ok in general, I found one scenario where the check will fail:

* mount with unpatched kernel, log_root_transid = 0
* mount with patched kernel, log_root_transid 100, generation 101
* mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
* mount with patched kernel -> check fails

So the problem is when log_root_transid is not 0 and changes out of sync with the generation. The above sequence of kernels can simply happen when switching between old stable and current releases.

Quoted 22 lines · David Sterba

Quoted 15 lines · Qu Wenruo

We have btrfs_super_block::log_root_transid to record the transid of log tree root.

However it's never populated and it's always 0, just as the following dump-super:

 log_root                30572544
 log_root_transid        0
 log_root_level          0

This patch will populate it with log tree root correctly, so the result will be:

 log_root                30572544
 log_root_transid        6
 log_root_level          0

Quoted 5 lines

This won't affect current kernel behavior or btrfs check result as we already expect log tree root generation always to be super block genration + 1.

But it could be later used to detect log tree corruption early.

Quoted 7 lines

The backward compatibility seems to be ok in general, I found one scenario where the check will fail:

* mount with unpatched kernel, log_root_transid = 0
* mount with patched kernel, log_root_transid 100, generation 101
* mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
* mount with patched kernel -> check fails

Indeed, this is a problem I missed.

It provides the old principle, if we're going to change how kernel use/interprete a on-disk memeber, we have to introduce a new incompatible flag.

Please just drop the series of patch.

Thanks,
Qu

Quoted 5 lines

So the problem is when log_root_transid is not 0 and changes out of sync with the generation. The above sequence of kernels can simply happen when switching between old stable and current releases.

Quoted 39 lines · Qu Wenruo

Quoted 22 lines · David Sterba

Quoted 15 lines · Qu Wenruo

We have btrfs_super_block::log_root_transid to record the transid of log tree root.

However it's never populated and it's always 0, just as the following dump-super:

 log_root                30572544
 log_root_transid        0
 log_root_level          0

This patch will populate it with log tree root correctly, so the result will be:

 log_root                30572544
 log_root_transid        6
 log_root_level          0

Quoted 5 lines

This won't affect current kernel behavior or btrfs check result as we already expect log tree root generation always to be super block genration + 1.

But it could be later used to detect log tree corruption early.

Quoted 7 lines

The backward compatibility seems to be ok in general, I found one scenario where the check will fail:

* mount with unpatched kernel, log_root_transid = 0
* mount with patched kernel, log_root_transid 100, generation 101
* mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
* mount with patched kernel -> check fails

Indeed, this is a problem I missed.

It provides the old principle, if we're going to change how kernel use/interprete a on-disk memeber, we have to introduce a new incompatible flag.

Quoted 1 line

Please just drop the series of patch.

Since this member really doesn't have any purpose now I think the prudent thing to do is to rename it to "reserved1" or "padding" or whatever but basically signal that it's no longer in used. In the future when a new incompaat bit will be required for some feature it can be re-used.

Quoted 10 lines

Thanks,
Qu

Quoted 5 lines

So the problem is when log_root_transid is not 0 and changes out of sync with the generation. The above sequence of kernels can simply happen when switching between old stable and current releases.

So we can detect log root related problem early and report the error more meaningfully other than hitting it log root read time.

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

---
 fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

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

index 5124c15705ce..34c68401c6bc 100644
@@ -2408,6 +2408,7 @@ static int validate_super(struct btrfs_fs_info *fs_info,
 {
 	u64 nodesize = btrfs_super_nodesize(sb);
 	u64 sectorsize = btrfs_super_sectorsize(sb);
+	u64 generation;
 	int ret = 0;
 
 	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {

@@ -2534,21 +2535,36 @@ static int validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
+	generation = btrfs_super_generation(sb);
 	/*
 	 * 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.
 	 */
-	if (btrfs_super_generation(sb) < btrfs_super_chunk_root_generation(sb))
+	if (generation < btrfs_super_chunk_root_generation(sb))
 		btrfs_warn(fs_info,
 			"suspicious: generation < chunk_root_generation: %llu < %llu",
-			btrfs_super_generation(sb),
-			btrfs_super_chunk_root_generation(sb));
-	if (btrfs_super_generation(sb) < btrfs_super_cache_generation(sb)
+			generation, btrfs_super_chunk_root_generation(sb));
+	if (generation < btrfs_super_cache_generation(sb)
 	    && btrfs_super_cache_generation(sb) != (u64)-1)
 		btrfs_warn(fs_info,
 			"suspicious: generation < cache_generation: %llu < %llu",
-			btrfs_super_generation(sb),
-			btrfs_super_cache_generation(sb));
+			generation, btrfs_super_cache_generation(sb));
+

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

+		btrfs_err(fs_info,
+	"bad log tree, log_root_transid != generation + 1: %llu != %llu + 1",
+			  btrfs_super_log_root_transid(sb), generation);
+		ret = -EINVAL;
+	}
 
 	return ret;
 }

Quoted 51 lines · Qu Wenruo

So we can detect log root related problem early and report the error more meaningfully other than hitting it log root read time.

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

---
 fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

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

index 5124c15705ce..34c68401c6bc 100644
@@ -2408,6 +2408,7 @@ static int validate_super(struct btrfs_fs_info *fs_info,
 {
 	u64 nodesize = btrfs_super_nodesize(sb);
 	u64 sectorsize = btrfs_super_sectorsize(sb);
+	u64 generation;
 	int ret = 0;
 
 	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {

@@ -2534,21 +2535,36 @@ static int validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
+	generation = btrfs_super_generation(sb);
 	/*
 	 * 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.
 	 */
-	if (btrfs_super_generation(sb) < btrfs_super_chunk_root_generation(sb))
+	if (generation < btrfs_super_chunk_root_generation(sb))
 		btrfs_warn(fs_info,
 			"suspicious: generation < chunk_root_generation: %llu < %llu",
-			btrfs_super_generation(sb),
-			btrfs_super_chunk_root_generation(sb));
-	if (btrfs_super_generation(sb) < btrfs_super_cache_generation(sb)
+			generation, btrfs_super_chunk_root_generation(sb));
+	if (generation < btrfs_super_cache_generation(sb)
 	    && btrfs_super_cache_generation(sb) != (u64)-1)
 		btrfs_warn(fs_info,
 			"suspicious: generation < cache_generation: %llu < %llu",
-			btrfs_super_generation(sb),
-			btrfs_super_cache_generation(sb));
+			generation, btrfs_super_cache_generation(sb));
+
+	/*
+	 * Check log root transid against super block generation.
+	 *
+	 * Older kernel doesn't populate log_root_transid, only need to check
+	 * it when it's not zero.
+	 * Since replaying suspicious log tree can cause serious problem, refuse

fs/btrfs/disk-io.c static int validate_super(struct btrfs_fs_info *fs_info, · line 2562

Quoted 4 lines

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

+	 * to mount if it doesn't match.
+	 */
+	if (btrfs_super_log_root_transid(sb) &&
+	    btrfs_super_log_root_transid(sb) != generation + 1) {

nit: in the same vein as you do for btrfs_super_generation just assign super_log_root_Transid to a local var and use that. IMO it's more consistent with what's happening in this function.

Quoted 10 lines

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

+		btrfs_err(fs_info,
+	"bad log tree, log_root_transid != generation + 1: %llu != %llu + 1",
+			  btrfs_super_log_root_transid(sb), generation);
+		ret = -EINVAL;
+	}
 
 	return ret;
 }

Quoted 6 lines

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

+	/*
+	 * Check log root transid against super block generation.
+	 *
+	 * Older kernel doesn't populate log_root_transid, only need to check
+	 * it when it's not zero.
+	 * Since replaying suspicious log tree can cause serious problem, refuse

fs/btrfs/disk-io.c static int validate_super(struct btrfs_fs_info *fs_info, · line 2562

Quoted 4 lines

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

+	 * to mount if it doesn't match.
+	 */
+	if (btrfs_super_log_root_transid(sb) &&
+	    btrfs_super_log_root_transid(sb) != generation + 1) {

nit: in the same vein as you do for btrfs_super_generation just assign super_log_root_Transid to a local var and use that. IMO it's more consistent with what's happening in this function.