[PATCH 0/2] btrfs-progs: only allow certain commands to ignore transid errors

[PATCH 0/2] btrfs-progs: only allow certain commands to ignore transid errors

[Qu Wenruo]

There is a bug in reddit (well, really the last place I expect to see
bug reports), that btrfstune -u fails due to transid error, but it also
leaves CHANGING_FSID flag to the super block, prevent btrfs-check to
properly check the fs.

The problem is, all commands in btrfs-progs can ignore transid error,
but there are only very limited usage of such ability.

Btrfstune definitely should not utilize this feature.

This patchset will introduce a new open ctree flag to explicitly
indicate we want to ignore transid errors.

Currently only there are only 3 tools using this feature:

- btrfs-check
  It may fix transid error (at least for the specific test case)

- btrfs-restore
  It wants to ignore all errors.

- btrfs-image
  To make fsck/002 happy.

Also add a test case for btrfstune, to make sure btrfstune can rejects
the fs when an obvious transid mismatch is detected during open_ctree().

Qu Wenruo (2):
  btrfs-progs: introduce OPEN_CTREE_ALLOW_TRANSID_MISMATCH flag
  btrfs-progs: misc-tests: add new test case to make sure btrfstune
    rejects corrupted fs

 check/main.c                                     |  3 ++-
 cmds/restore.c                                   |  3 ++-
 image/main.c                                     | 11 +++++++----
 kernel-shared/ctree.h                            |  1 +
 kernel-shared/disk-io.c                          | 11 +++++++++--
 kernel-shared/disk-io.h                          |  6 ++++++
 .../default_case.img                             |  1 +
 .../049-btrfstune-transid-mismatch/test.sh       | 16 ++++++++++++++++
 8 files changed, 44 insertions(+), 8 deletions(-)
 create mode 120000 tests/misc-tests/049-btrfstune-transid-mismatch/default_case.img
 create mode 100755 tests/misc-tests/049-btrfstune-transid-mismatch/test.sh

-- 
2.33.0

[PATCH 1/2] btrfs-progs: introduce OPEN_CTREE_ALLOW_TRANSID_MISMATCH flag

[Qu Wenruo]

[BUG]
There is a report that, btrfstune can even work while the fs has transid
mismatch problems.

  $ btrfstune -f -u /dev/sdb1
  Current fsid: b2b5ae8d-4c49-45f0-b42e-46fe7dcfcb07
  New fsid: b2b5ae8d-4c49-45f0-b42e-46fe7dcfcb07
  Set superblock flag CHANGING_FSID
  Change fsid in extents
  parent transid verify failed on 792854528 wanted 20103 found 20091
  parent transid verify failed on 792854528 wanted 20103 found 20091
  parent transid verify failed on 792854528 wanted 20103 found 20091
  Ignoring transid failure
  parent transid verify failed on 792870912 wanted 20103 found 20091
  parent transid verify failed on 792870912 wanted 20103 found 20091
  parent transid verify failed on 792870912 wanted 20103 found 20091
  Ignoring transid failure
  parent transid verify failed on 792887296 wanted 20103 found 20091
  parent transid verify failed on 792887296 wanted 20103 found 20091
  parent transid verify failed on 792887296 wanted 20103 found 20091
  Ignoring transid failure
  ERROR: child eb corrupted: parent bytenr=38010880 item=69 parent level=1 child level=1
  ERROR: failed to change UUID of metadata: -5
  ERROR: btrfstune failed

This leaves a corrupted fs even more corrupted, and due to the extra
CHANGING_FSID flag, btrfs check will not even try to run on it:

  Opening filesystem to check...
  ERROR: Filesystem UUID change in progress
  ERROR: cannot open file system

[CAUSE]
Unlike kernel, btrfs-progs has a less strict check on transid mismatch.

In read_tree_block() we will fall back to use the tree block even its
transid mismatch if we can't find any better copy.

However not all commands in btrfs-progs needs this feature, only
btrfs-check (which may fix the problem) and btrfs-restore (it just tries
to ignore any problems) really utilize this feature.

[FIX]
Introduce a new open ctree flag, OPEN_CTREE_ALLOW_TRANSID_MISMATCH, to
be explicit about whether we really want to ignore transid error.

Currently only btrfs-check and btrfs-restore will utilize this new flag.

Also add btrfs-image to allow opening such fs with transid error.

Link: https://www.reddit.com/r/btrfs/comments/pivpqk/failure_during_btrfstune_u/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c            |  3 ++-
 cmds/restore.c          |  3 ++-
 image/main.c            | 11 +++++++----
 kernel-shared/ctree.h   |  1 +
 kernel-shared/disk-io.c | 11 +++++++++--
 kernel-shared/disk-io.h |  6 ++++++
 6 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/check/main.c b/check/main.c
index 661c996a2cb1..970ad6a259b8 100644
--- a/check/main.c
+++ b/check/main.c
@@ -10384,7 +10384,8 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 	int qgroup_report = 0;
 	int qgroups_repaired = 0;
 	int qgroup_verify_ret;
-	unsigned ctree_flags = OPEN_CTREE_EXCLUSIVE;
+	unsigned ctree_flags = OPEN_CTREE_EXCLUSIVE |
+			       OPEN_CTREE_ALLOW_TRANSID_MISMATCH;
 	int force = 0;
 
 	while(1) {
diff --git a/cmds/restore.c b/cmds/restore.c
index dd75508aabd6..8f71d6f8e11d 100644
--- a/cmds/restore.c
+++ b/cmds/restore.c
@@ -1213,7 +1213,8 @@ static struct btrfs_root *open_fs(const char *dev, u64 root_location,
 		ocf.filename = dev;
 		ocf.sb_bytenr = bytenr;
 		ocf.root_tree_bytenr = root_location;
-		ocf.flags = OPEN_CTREE_PARTIAL | OPEN_CTREE_NO_BLOCK_GROUPS;
+		ocf.flags = OPEN_CTREE_PARTIAL | OPEN_CTREE_NO_BLOCK_GROUPS |
+			    OPEN_CTREE_ALLOW_TRANSID_MISMATCH;
 		fs_info = open_ctree_fs_info(&ocf);
 		if (fs_info)
 			break;
diff --git a/image/main.c b/image/main.c
index 6595ca93cd69..b40e0e5550f8 100644
--- a/image/main.c
+++ b/image/main.c
@@ -1004,7 +1004,7 @@ static int create_metadump(const char *input, FILE *out, int num_threads,
 	int ret;
 	int err = 0;
 
-	root = open_ctree(input, 0, 0);
+	root = open_ctree(input, 0, OPEN_CTREE_ALLOW_TRANSID_MISMATCH);
 	if (!root) {
 		error("open ctree failed");
 		return -EIO;
@@ -2781,7 +2781,8 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		struct open_ctree_flags ocf = { 0 };
 
 		ocf.filename = target;
-		ocf.flags = OPEN_CTREE_WRITES | OPEN_CTREE_RESTORE | OPEN_CTREE_PARTIAL;
+		ocf.flags = OPEN_CTREE_WRITES | OPEN_CTREE_RESTORE |
+			    OPEN_CTREE_PARTIAL;
 		info = open_ctree_fs_info(&ocf);
 		if (!info) {
 			error("open ctree failed");
@@ -2846,7 +2847,8 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		root = open_ctree_fd(fileno(out), target, 0,
 					  OPEN_CTREE_PARTIAL |
 					  OPEN_CTREE_WRITES |
-					  OPEN_CTREE_NO_DEVICES);
+					  OPEN_CTREE_NO_DEVICES |
+					  OPEN_CTREE_ALLOW_TRANSID_MISMATCH);
 		if (!root) {
 			error("open ctree failed in %s", target);
 			ret = -EIO;
@@ -2864,7 +2866,8 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		u64 dev_size;
 
 		if (!info) {
-			root = open_ctree_fd(fileno(out), target, 0, 0);
+			root = open_ctree_fd(fileno(out), target, 0,
+					     OPEN_CTREE_ALLOW_TRANSID_MISMATCH);
 			if (!root) {
 				error("open ctree failed in %s", target);
 				ret = -EIO;
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
index e79b1e4ced60..c0e86b708fe2 100644
--- a/kernel-shared/ctree.h
+++ b/kernel-shared/ctree.h
@@ -1216,6 +1216,7 @@ struct btrfs_fs_info {
 	unsigned int avoid_sys_chunk_alloc:1;
 	unsigned int finalize_on_close:1;
 	unsigned int hide_names:1;
+	unsigned int allow_transid_mismatch:1;
 
 	int transaction_aborted;
 
diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 0dc31c364317..1cda6f3a98af 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -421,7 +421,7 @@ struct extent_buffer* read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
 			ret = -EIO;
 			break;
 		}
-		if (num_copies == 1) {
+		if (num_copies == 1 && fs_info->allow_transid_mismatch) {
 			ignore = 1;
 			continue;
 		}
@@ -431,6 +431,10 @@ struct extent_buffer* read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
 		}
 		mirror_num++;
 		if (mirror_num > num_copies) {
+			if (!fs_info->allow_transid_mismatch) {
+				ret = -EIO;
+				break;
+			}
 			if (candidate_mirror > 0)
 				mirror_num = candidate_mirror;
 			else
@@ -1231,6 +1235,8 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, struct open_ctree_flags *oc
 		fs_info->ignore_chunk_tree_error = 1;
 	if (flags & OPEN_CTREE_HIDE_NAMES)
 		fs_info->hide_names = 1;
+	if (flags & OPEN_CTREE_ALLOW_TRANSID_MISMATCH)
+		fs_info->allow_transid_mismatch = 1;
 
 	if ((flags & OPEN_CTREE_RECOVER_SUPER)
 	     && (flags & OPEN_CTREE_TEMPORARY_SUPER)) {
@@ -1988,7 +1994,8 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid)
 		return ret;
 
 	ret = verify_parent_transid(&buf->fs_info->extent_cache, buf,
-				    parent_transid, 1);
+				    parent_transid,
+				    buf->fs_info->allow_transid_mismatch);
 	return !ret;
 }
 
diff --git a/kernel-shared/disk-io.h b/kernel-shared/disk-io.h
index 603ff8a3f945..265b3e01297a 100644
--- a/kernel-shared/disk-io.h
+++ b/kernel-shared/disk-io.h
@@ -88,6 +88,12 @@ enum btrfs_open_ctree_flags {
 
 	/* For print-tree, print HIDDEN instead of filenames/xattrs/refs */
 	OPEN_CTREE_HIDE_NAMES = (1U << 14),
+
+	/*
+	 * Allow certain command like btrfs-check/btrfs-restore to ignore
+	 * transid mismatch.
+	 */
+	OPEN_CTREE_ALLOW_TRANSID_MISMATCH = (1U << 15),
 };
 
 /*
-- 
2.33.0

[PATCH 2/2] btrfs-progs: misc-tests: add new test case to make sure btrfstune rejects corrupted fs

[Qu Wenruo]

Although btrfstune will already warn users to make sure the fs is not
corrupted, we can never trust end users.

If the target fs has transid error, btrfstune can cause further damage,
thus we need to make sure btrfstune can safely reject fs with transid
error, other than ignoring the problem.

The image is copied from fsck-tests/002, just override check_image() to
run "btrfstune -u" instead.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../default_case.img                             |  1 +
 .../049-btrfstune-transid-mismatch/test.sh       | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 120000 tests/misc-tests/049-btrfstune-transid-mismatch/default_case.img
 create mode 100755 tests/misc-tests/049-btrfstune-transid-mismatch/test.sh

diff --git a/tests/misc-tests/049-btrfstune-transid-mismatch/default_case.img b/tests/misc-tests/049-btrfstune-transid-mismatch/default_case.img
new file mode 120000
index 000000000000..eb54ddcbb402
--- /dev/null
+++ b/tests/misc-tests/049-btrfstune-transid-mismatch/default_case.img
@@ -0,0 +1 @@
+../../fsck-tests/002-bad-transid/default_case.img
\ No newline at end of file
diff --git a/tests/misc-tests/049-btrfstune-transid-mismatch/test.sh b/tests/misc-tests/049-btrfstune-transid-mismatch/test.sh
new file mode 100755
index 000000000000..c6d721eaea65
--- /dev/null
+++ b/tests/misc-tests/049-btrfstune-transid-mismatch/test.sh
@@ -0,0 +1,16 @@
+#!/bin/bash
+# Verify that btrfstune would reject fs with transid mismatch problems
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs-image
+check_prereq btrfs
+check_prereq btrfstune
+
+# Although we're not checking the image, here we just reuse the infrastructure
+check_image() {
+	run_mustfail "btrfstune should fail when the image has transid error" \
+		  "$TOP/btrfstune" -u "$1"
+}
+
+check_all_images
-- 
2.33.0

Re: [PATCH 0/2] btrfs-progs: only allow certain commands to ignore transid errors

[David Sterba]

On Wed, Sep 08, 2021 at 10:05:41AM +0800, Qu Wenruo wrote:
> There is a bug in reddit (well, really the last place I expect to see
> bug reports)

Why? People discuss things where they're used to, reddit is one of them
and from what I've seen there are good questions or usage recommendations.
Some of that goes to documentation or helps to tune usability.

> that btrfstune -u fails due to transid error, but it also
> leaves CHANGING_FSID flag to the super block, prevent btrfs-check to
> properly check the fs.
> 
> The problem is, all commands in btrfs-progs can ignore transid error,
> but there are only very limited usage of such ability.
> 
> Btrfstune definitely should not utilize this feature.
> 
> This patchset will introduce a new open ctree flag to explicitly
> indicate we want to ignore transid errors.
> 
> Currently only there are only 3 tools using this feature:
> 
> - btrfs-check
>   It may fix transid error (at least for the specific test case)
> 
> - btrfs-restore
>   It wants to ignore all errors.
> 
> - btrfs-image
>   To make fsck/002 happy.
> 
> Also add a test case for btrfstune, to make sure btrfstune can rejects
> the fs when an obvious transid mismatch is detected during open_ctree().
> 
> Qu Wenruo (2):
>   btrfs-progs: introduce OPEN_CTREE_ALLOW_TRANSID_MISMATCH flag
>   btrfs-progs: misc-tests: add new test case to make sure btrfstune
>     rejects corrupted fs

Added to devel, thanks.

Re: [PATCH 0/2] btrfs-progs: only allow certain commands to ignore transid errors

[Qu Wenruo]

On 2021/9/20 18:43, David Sterba wrote:
> On Wed, Sep 08, 2021 at 10:05:41AM +0800, Qu Wenruo wrote:
>> There is a bug in reddit (well, really the last place I expect to see
>> bug reports)
>
> Why? People discuss things where they're used to, reddit is one of them
> and from what I've seen there are good questions or usage recommendations.
> Some of that goes to documentation or helps to tune usability.

Well, I'm more used to see memes/trolls there, maybe it's per-subreddit.

Anyway next time I would drop the prejudice.

Thanks,
Qu
>
>> that btrfstune -u fails due to transid error, but it also
>> leaves CHANGING_FSID flag to the super block, prevent btrfs-check to
>> properly check the fs.
>>
>> The problem is, all commands in btrfs-progs can ignore transid error,
>> but there are only very limited usage of such ability.
>>
>> Btrfstune definitely should not utilize this feature.
>>
>> This patchset will introduce a new open ctree flag to explicitly
>> indicate we want to ignore transid errors.
>>
>> Currently only there are only 3 tools using this feature:
>>
>> - btrfs-check
>>    It may fix transid error (at least for the specific test case)
>>
>> - btrfs-restore
>>    It wants to ignore all errors.
>>
>> - btrfs-image
>>    To make fsck/002 happy.
>>
>> Also add a test case for btrfstune, to make sure btrfstune can rejects
>> the fs when an obvious transid mismatch is detected during open_ctree().
>>
>> Qu Wenruo (2):
>>    btrfs-progs: introduce OPEN_CTREE_ALLOW_TRANSID_MISMATCH flag
>>    btrfs-progs: misc-tests: add new test case to make sure btrfstune
>>      rejects corrupted fs
>
> Added to devel, thanks.
>