Skip to contentSkip to conversation

btrfs-progs: image: Fix error when restoring multi-disk image to single disk

PATCH Applied Qu Wenruo 5 participants 16 messages Nov 27, 2018 – Dec 4, 2018 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

v2 cover btrfs-progs: image: Fix error when restoring multi-disk image to single disk

This patchset can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/image_recover

The base commit is as usual, the latest stable tag, v4.19.

Test case misc/021 will fail if using latest upstream kernel.

This is due to the enhanced code in kernel to check block group <-> chunk <-> dev extent mapping.

This means from the very beginning, btrfs-image can't really restore a multi-disk image to single-disk one correctly.

The problem is, although we modified chunk item, we didn't modify block group item's flags or dev extents.

This patch will reset block group flags, then rebuild the whole dev extents by removing existing ones first, then re-add the correct dev extents calculated from chunk map.

Now it could pass all misc tests and fsck tests.

Changelog:
v2:
- Parameter list cleanup
* Use trans->fs_info to remove fs_info parameter
* Remove trans parameter for function who doesn't need
- Merge dev extents removal code with rebuild code
- Refactor btrfs_alloc_dev_extent() into 2 functions
* btrfs_insert_dev_extent() for convert and dev extent rebuild
* btrfs_alloc_dev_extent() for old use case

Qu Wenruo (5):
btrfs-progs: image: Refactor fixup_devices() to
fixup_chunks_and_devices()
btrfs-progs: image: Fix block group item flags when restoring
multi-device image to single device
btrfs-progs: volumes: Refactor btrfs_alloc_dev_extent() into two
functions
btrfs-progs: image: Remove all existing dev extents for later rebuild
btrfs-progs: misc-tests/021: Do extra btrfs check before mounting

 image/main.c                                  | 200 ++++++++++++++++--
 .../021-image-multi-devices/test.sh           |   3 +
 volumes.c                                     |  48 +++--
 volumes.h                                     |   3 +
 4 files changed, 220 insertions(+), 34 deletions(-)
Signature · 1 line
2.19.2
author
#1 lore raw

PATCH v2 1/5 btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM size.
Later we need to do more fixup works, so change the name to fixup_chunks_and_devices() and refactor the original device size fixup to fixup_device_size().

Signed-off-by Qu Wenruo <[email protected]>
---
 image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)
image/main.c +36 -16
index c680ab19de6c..bbfcf8f19298 100644
@@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
 	}
 }
 
-static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+			     struct mdrestore_struct *mdres,
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_dev_item *dev_item;
 	struct btrfs_path path;
-	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
+	struct extent_buffer *leaf;
 	u64 devid, cur_devid;
 	int ret;
 
-	if (btrfs_super_log_root(fs_info->super_copy)) {
-		warning(
-		"log tree detected, its generation will not match superblock");
-	}
-	trans = btrfs_start_transaction(fs_info->tree_root, 1);
-	if (IS_ERR(trans)) {
-		error("cannot starting transaction %ld", PTR_ERR(trans));
-		return PTR_ERR(trans);
-	}
-
 	dev_item = &fs_info->super_copy->dev_item;
 
 	devid = btrfs_stack_device_id(dev_item);

@@ -2123,7 +2114,7 @@ again:
 	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
 	if (ret < 0) {
 		error("search failed: %d", ret);
-		exit(1);
+		return ret;
 	}
 
 	while (1) {

@@ -2170,12 +2161,41 @@ again:
 	}
 
 	btrfs_release_path(&path);
+	return 0;
+}
+
+static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
+			 struct mdrestore_struct *mdres, off_t dev_size)
+{
+	struct btrfs_trans_handle *trans;
+	int ret;
+
+	if (btrfs_super_log_root(fs_info->super_copy)) {
+		warning(
+		"log tree detected, its generation will not match superblock");
+	}
+	trans = btrfs_start_transaction(fs_info->tree_root, 1);
+	if (IS_ERR(trans)) {
+		error("cannot starting transaction %ld", PTR_ERR(trans));
+		return PTR_ERR(trans);
+	}
+
+	ret = fixup_device_size(trans, mdres, dev_size);
+	if (ret < 0)
+		goto error;
+
 	ret = btrfs_commit_transaction(trans, fs_info->tree_root);
 	if (ret) {
 		error("unable to commit transaction: %d", ret);
 		return ret;
 	}
 	return 0;
+error:
+	error(
+"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
+		strerror(-ret));
+	btrfs_abort_transaction(trans, ret);
+	return ret;
 }
 
 static int restore_metadump(const char *input, FILE *out, int old_restore,

@@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 			return 1;
 		}
 
-		ret = fixup_devices(info, &mdrestore, st.st_size);
+		ret = fixup_chunks_and_devices(info, &mdrestore, st.st_size);
 		close_ctree(info->chunk_root);
 		if (ret)
 			goto out;
Signature · 1 line
2.19.2
Reviewed-by on v2 1/5
#2 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

Quoted 7 lines · Qu Wenruo

Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM size.
Later we need to do more fixup works, so change the name to fixup_chunks_and_devices() and refactor the original device size fixup to fixup_device_size().

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

However, one minor nit below.

Quoted 16 lines
---
 image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)
image/main.c +2 -2
index c680ab19de6c..bbfcf8f19298 100644
@@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
 	}
 }
 
-static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+			     struct mdrestore_struct *mdres,

image/main.c static void remap_overlapping_chunks(struct mdrestore_struct *mdres) · line 2096

Quoted 10 lines
image/main.c +3 -2
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_dev_item *dev_item;
 	struct btrfs_path path;
-	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
+	struct extent_buffer *leaf;

nit: Unnecessary change

Quoted 77 lines
image/main.c +31 -12
 	u64 devid, cur_devid;
 	int ret;
 
-	if (btrfs_super_log_root(fs_info->super_copy)) {
-		warning(
-		"log tree detected, its generation will not match superblock");
-	}
-	trans = btrfs_start_transaction(fs_info->tree_root, 1);
-	if (IS_ERR(trans)) {
-		error("cannot starting transaction %ld", PTR_ERR(trans));
-		return PTR_ERR(trans);
-	}
-
 	dev_item = &fs_info->super_copy->dev_item;
 
 	devid = btrfs_stack_device_id(dev_item);

@@ -2123,7 +2114,7 @@ again:
 	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
 	if (ret < 0) {
 		error("search failed: %d", ret);
-		exit(1);
+		return ret;
 	}
 
 	while (1) {

@@ -2170,12 +2161,41 @@ again:
 	}
 
 	btrfs_release_path(&path);
+	return 0;
+}
+
+static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
+			 struct mdrestore_struct *mdres, off_t dev_size)
+{
+	struct btrfs_trans_handle *trans;
+	int ret;
+
+	if (btrfs_super_log_root(fs_info->super_copy)) {
+		warning(
+		"log tree detected, its generation will not match superblock");
+	}
+	trans = btrfs_start_transaction(fs_info->tree_root, 1);
+	if (IS_ERR(trans)) {
+		error("cannot starting transaction %ld", PTR_ERR(trans));
+		return PTR_ERR(trans);
+	}
+
+	ret = fixup_device_size(trans, mdres, dev_size);
+	if (ret < 0)
+		goto error;
+
 	ret = btrfs_commit_transaction(trans, fs_info->tree_root);
 	if (ret) {
 		error("unable to commit transaction: %d", ret);
 		return ret;
 	}
 	return 0;
+error:
+	error(
+"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
+		strerror(-ret));
+	btrfs_abort_transaction(trans, ret);
+	return ret;
 }
 
 static int restore_metadump(const char *input, FILE *out, int old_restore,

@@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 			return 1;
 		}
 
-		ret = fixup_devices(info, &mdrestore, st.st_size);
+		ret = fixup_chunks_and_devices(info, &mdrestore, st.st_size);
 		close_ctree(info->chunk_root);
 		if (ret)
 			goto out;
#3 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

Quoted 27 lines · Nikolay Borisov
Quoted 7 lines · Qu Wenruo

Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM size.
Later we need to do more fixup works, so change the name to fixup_chunks_and_devices() and refactor the original device size fixup to fixup_device_size().

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

However, one minor nit below.

Quoted 12 lines
---
 image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)
image/main.c +0 -0
index c680ab19de6c..bbfcf8f19298 100644
@@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
 	}
 }
Quoted 16 lines
Quoted 4 lines
image/main.c +2 -2
-static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+			     struct mdrestore_struct *mdres,

image/main.c static void remap_overlapping_chunks(struct mdrestore_struct *mdres) · line 2096

Quoted 10 lines
image/main.c +3 -2
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_dev_item *dev_item;
 	struct btrfs_path path;
-	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
+	struct extent_buffer *leaf;

nit: Unnecessary change

Doesn't it look better when all btrfs_ prefix get batched together? :)

Thanks,
Qu

Quoted 78 lines
Quoted 78 lines
image/main.c +31 -12
 	u64 devid, cur_devid;
 	int ret;
 
-	if (btrfs_super_log_root(fs_info->super_copy)) {
-		warning(
-		"log tree detected, its generation will not match superblock");
-	}
-	trans = btrfs_start_transaction(fs_info->tree_root, 1);
-	if (IS_ERR(trans)) {
-		error("cannot starting transaction %ld", PTR_ERR(trans));
-		return PTR_ERR(trans);
-	}
-
 	dev_item = &fs_info->super_copy->dev_item;
 
 	devid = btrfs_stack_device_id(dev_item);

@@ -2123,7 +2114,7 @@ again:
 	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
 	if (ret < 0) {
 		error("search failed: %d", ret);
-		exit(1);
+		return ret;
 	}
 
 	while (1) {

@@ -2170,12 +2161,41 @@ again:
 	}
 
 	btrfs_release_path(&path);
+	return 0;
+}
+
+static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
+			 struct mdrestore_struct *mdres, off_t dev_size)
+{
+	struct btrfs_trans_handle *trans;
+	int ret;
+
+	if (btrfs_super_log_root(fs_info->super_copy)) {
+		warning(
+		"log tree detected, its generation will not match superblock");
+	}
+	trans = btrfs_start_transaction(fs_info->tree_root, 1);
+	if (IS_ERR(trans)) {
+		error("cannot starting transaction %ld", PTR_ERR(trans));
+		return PTR_ERR(trans);
+	}
+
+	ret = fixup_device_size(trans, mdres, dev_size);
+	if (ret < 0)
+		goto error;
+
 	ret = btrfs_commit_transaction(trans, fs_info->tree_root);
 	if (ret) {
 		error("unable to commit transaction: %d", ret);
 		return ret;
 	}
 	return 0;
+error:
+	error(
+"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
+		strerror(-ret));
+	btrfs_abort_transaction(trans, ret);
+	return ret;
 }
 
 static int restore_metadump(const char *input, FILE *out, int old_restore,

@@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 			return 1;
 		}
 
-		ret = fixup_devices(info, &mdrestore, st.st_size);
+		ret = fixup_chunks_and_devices(info, &mdrestore, st.st_size);
 		close_ctree(info->chunk_root);
 		if (ret)
 			goto out;
#4 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

Quoted 47 lines · Qu Wenruo
Quoted 27 lines · Nikolay Borisov
Quoted 7 lines · Qu Wenruo

Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM size.
Later we need to do more fixup works, so change the name to fixup_chunks_and_devices() and refactor the original device size fixup to fixup_device_size().

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

However, one minor nit below.

Quoted 12 lines
---
 image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)
image/main.c +0 -0
index c680ab19de6c..bbfcf8f19298 100644
@@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct mdrestore_struct *mdres)
 	}
 }
Quoted 16 lines
Quoted 4 lines
image/main.c +2 -2
-static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+			     struct mdrestore_struct *mdres,

image/main.c static void remap_overlapping_chunks(struct mdrestore_struct *mdres) · line 2096

Quoted 10 lines
image/main.c +3 -2
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_dev_item *dev_item;
 	struct btrfs_path path;
-	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
+	struct extent_buffer *leaf;

nit: Unnecessary change

Quoted 1 line

Doesn't it look better when all btrfs_ prefix get batched together? :)

Didn't even realize this was the intended effect. IMO doesn't make much difference, what does, though, is probably reverse christmas tree, ie

longer variable names
come before shorter
ones

But I guess this is a matter of taste, no need to resend.

Quoted 83 lines

Thanks,
Qu

Quoted 78 lines
Quoted 78 lines
image/main.c +31 -12
 	u64 devid, cur_devid;
 	int ret;
 
-	if (btrfs_super_log_root(fs_info->super_copy)) {
-		warning(
-		"log tree detected, its generation will not match superblock");
-	}
-	trans = btrfs_start_transaction(fs_info->tree_root, 1);
-	if (IS_ERR(trans)) {
-		error("cannot starting transaction %ld", PTR_ERR(trans));
-		return PTR_ERR(trans);
-	}
-
 	dev_item = &fs_info->super_copy->dev_item;
 
 	devid = btrfs_stack_device_id(dev_item);

@@ -2123,7 +2114,7 @@ again:
 	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
 	if (ret < 0) {
 		error("search failed: %d", ret);
-		exit(1);
+		return ret;
 	}
 
 	while (1) {

@@ -2170,12 +2161,41 @@ again:
 	}
 
 	btrfs_release_path(&path);
+	return 0;
+}
+
+static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
+			 struct mdrestore_struct *mdres, off_t dev_size)
+{
+	struct btrfs_trans_handle *trans;
+	int ret;
+
+	if (btrfs_super_log_root(fs_info->super_copy)) {
+		warning(
+		"log tree detected, its generation will not match superblock");
+	}
+	trans = btrfs_start_transaction(fs_info->tree_root, 1);
+	if (IS_ERR(trans)) {
+		error("cannot starting transaction %ld", PTR_ERR(trans));
+		return PTR_ERR(trans);
+	}
+
+	ret = fixup_device_size(trans, mdres, dev_size);
+	if (ret < 0)
+		goto error;
+
 	ret = btrfs_commit_transaction(trans, fs_info->tree_root);
 	if (ret) {
 		error("unable to commit transaction: %d", ret);
 		return ret;
 	}
 	return 0;
+error:
+	error(
+"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
+		strerror(-ret));
+	btrfs_abort_transaction(trans, ret);
+	return ret;
 }
 
 static int restore_metadump(const char *input, FILE *out, int old_restore,

@@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 			return 1;
 		}
 
-		ret = fixup_devices(info, &mdrestore, st.st_size);
+		ret = fixup_chunks_and_devices(info, &mdrestore, st.st_size);
 		close_ctree(info->chunk_root);
 		if (ret)
 			goto out;
#5 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

In reply to Qu Wenruo
Quoted 17 lines · Qu Wenruo
Quoted 8 lines
Quoted 8 lines
image/main.c +4 -3
-static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+			     struct mdrestore_struct *mdres,
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
Quoted 8 lines

image/main.c static void remap_overlapping_chunks(struct mdrestore_struct *mdres) · line 2096

Quoted 6 lines
image/main.c +1 -1
 	struct btrfs_dev_item *dev_item;
 	struct btrfs_path path;
-	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
+	struct extent_buffer *leaf;

nit: Unnecessary change

Quoted 1 line

Doesn't it look better when all btrfs_ prefix get batched together? :)

Please don't do unrelated changes like that.

#6 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

Quoted 18 lines · David Sterba
Quoted 17 lines · Qu Wenruo
Quoted 16 lines
Quoted 4 lines
image/main.c +2 -2
-static int fixup_devices(struct btrfs_fs_info *fs_info,
-			 struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+			     struct mdrestore_struct *mdres,

image/main.c static void remap_overlapping_chunks(struct mdrestore_struct *mdres) · line 2096

Quoted 10 lines
image/main.c +3 -2
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_dev_item *dev_item;
 	struct btrfs_path path;
-	struct extent_buffer *leaf;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_key key;
+	struct extent_buffer *leaf;

nit: Unnecessary change

Quoted 3 lines
Quoted 1 line

Doesn't it look better when all btrfs_ prefix get batched together? :)

Please don't do unrelated changes like that.

Understood, the github version has that @leaf reverted to the original location.

Thanks,
Qu

#7 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

In reply to Qu Wenruo

image/main.c again: · line 2196

Quoted 4 lines · Qu Wenruo
image/main.c +4 -0
+error:
+	error(
+"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
+		strerror(-ret));

Recently the sterror(error code) have been converted to %m, so I changed this to

	errno = -ret
	error("... %m");
#8 lore raw

Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()

Quoted 9 lines · David Sterba

image/main.c again: · line 2196

Quoted 4 lines · Qu Wenruo
image/main.c +4 -0
+error:
+	error(
+"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
+		strerror(-ret));

Recently the sterror(error code) have been converted to %m, so I changed this to

Quoted 3 lines
	errno = -ret
	error("... %m");

Thanks for mentioning this.

I'll use this format for later patches.

Thanks,
Qu

author
#9 lore raw

PATCH v2 2/5 btrfs-progs: image: Fix block group item flags when restoring multi-device image to single device

In reply to Qu Wenruo

Since btrfs-image is just restoring tree blocks without really check if that tree block contents makes sense, for multi-device image, block group items will keep that incorrect block group flags.

For example, for a metadata RAID1 data RAID0 btrfs recovered to a single disk, its chunk tree will look like:

	item 1 key (FIRST_CHUNK_TREE CHUNK_ITEM 22020096)
		length 8388608 owner 2 stripe_len 65536 type SYSTEM
	item 2 key (FIRST_CHUNK_TREE CHUNK_ITEM 30408704)
		length 1073741824 owner 2 stripe_len 65536 type METADATA
	item 3 key (FIRST_CHUNK_TREE CHUNK_ITEM 1104150528)
		length 1073741824 owner 2 stripe_len 65536 type DATA

All chunks have correct type (SINGLE).

While its block group items will look like:

	item 1 key (22020096 BLOCK_GROUP_ITEM 8388608)
		block group used 16384 chunk_objectid 256 flags SYSTEM|RAID1
	item 3 key (30408704 BLOCK_GROUP_ITEM 1073741824)
		block group used 114688 chunk_objectid 256 flags METADATA|RAID1
	item 11 key (1104150528 BLOCK_GROUP_ITEM 1073741824)
		block group used 1572864 chunk_objectid 256 flags DATA|RAID0

All block group items still have the wrong profiles.

And btrfs check (lowmem mode for better output) will report error for such image:

ERROR: chunk[22020096 30408704) related block group item flags mismatch, wanted: 2, have: 18
ERROR: chunk[30408704 1104150528) related block group item flags mismatch, wanted: 4, have: 20
ERROR: chunk[1104150528 2177892352) related block group item flags mismatch, wanted: 1, have: 9

This patch will do an extra repair for block group items to fix the profile of them.

Signed-off-by Qu Wenruo <[email protected]>
---
 image/main.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
image/main.c +46 -0
index bbfcf8f19298..9187de34f34a 100644
@@ -2164,6 +2164,51 @@ again:
 	return 0;
 }
 
+static void fixup_block_groups(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_block_group_cache *bg;
+	struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
+	struct cache_extent *ce;
+	struct map_lookup *map;
+	u64 extra_flags;
+
+	for (ce = search_cache_extent(&map_tree->cache_tree, 0); ce;
+	     ce = next_cache_extent(ce)) {
+		map = container_of(ce, struct map_lookup, ce);
+
+		bg = btrfs_lookup_block_group(fs_info, ce->start);
+		if (!bg) {
+			warning(
+		"can't find block group %llu, result fs may not be mountable",
+				ce->start);
+			continue;
+		}
+		extra_flags = map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK;
+
+		if (bg->flags == map->type)
+			continue;
+
+		/* Update the block group item and mark the bg dirty */
+		bg->flags = map->type;
+		btrfs_set_block_group_flags(&bg->item, bg->flags);
+		set_extent_bits(&fs_info->block_group_cache, ce->start,
+				ce->start + ce->size - 1, BLOCK_GROUP_DIRTY);
+
+		/*
+		 * Chunk and bg flags can be different, changing bg flags
+		 * without update avail_data/meta_alloc_bits will lead to
+		 * ENOSPC.
+		 * So here we set avail_*_alloc_bits to match chunk types.
+		 */
+		if (map->type & BTRFS_BLOCK_GROUP_DATA)
+			fs_info->avail_data_alloc_bits = extra_flags;
+		if (map->type & BTRFS_BLOCK_GROUP_METADATA)
+			fs_info->avail_metadata_alloc_bits = extra_flags;
+		if (map->type & BTRFS_BLOCK_GROUP_SYSTEM)
+			fs_info->avail_system_alloc_bits = extra_flags;
+	}
+}
+
 static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 			 struct mdrestore_struct *mdres, off_t dev_size)
 {

@@ -2180,6 +2225,7 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 		return PTR_ERR(trans);
 	}
 
+	fixup_block_groups(fs_info);
 	ret = fixup_device_size(trans, mdres, dev_size);
 	if (ret < 0)
 		goto error;
Signature · 1 line
2.19.2
author
#10 lore raw

PATCH v2 3/5 btrfs-progs: volumes: Refactor btrfs_alloc_dev_extent() into two functions

In reply to Qu Wenruo

We have btrfs_alloc_dev_extent() accepting @convert flag to toggle special handling for convert.

However that @convert flag only determine whether we call find_free_dev_extent(), and we may later need to insert dev extents without searching dev tree.

So refactor btrfs_alloc_dev_extent() into 2 functions, btrfs_alloc_dev_extent(), which will try to find free dev extent, and btrfs_insert_dev_extent(), which will just insert a dev extent.

For implementation, btrfs_alloc_dev_extent() will call btrfs_insert_dev_extent() anyway, so no duplicated code.

This removes the need of @convert parameter, and make btrfs_insert_dev_extent() public for later usage.

Signed-off-by Qu Wenruo <[email protected]>
---
 volumes.c | 48 ++++++++++++++++++++++++++++++------------------
 volumes.h |  3 +++
 2 files changed, 33 insertions(+), 18 deletions(-)
volumes.c +30 -18
index 30090ce5f8e8..0dd082cd1718 100644
@@ -530,10 +530,12 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes,
 	return find_free_dev_extent_start(device, num_bytes, 0, start, len);
 }
 
-static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
-				  struct btrfs_device *device,
-				  u64 chunk_offset, u64 num_bytes, u64 *start,
-				  int convert)
+/*
+ * Insert one device extent into the fs.
+ */
+int btrfs_insert_dev_extent(struct btrfs_trans_handle *trans,
+			    struct btrfs_device *device,
+			    u64 chunk_offset, u64 num_bytes, u64 start)
 {
 	int ret;
 	struct btrfs_path *path;

@@ -546,18 +548,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	if (!path)
 		return -ENOMEM;
 
-	/*
-	 * For convert case, just skip search free dev_extent, as caller
-	 * is responsible to make sure it's free.
-	 */
-	if (!convert) {
-		ret = find_free_dev_extent(device, num_bytes, start, NULL);
-		if (ret)
-			goto err;
-	}
-
 	key.objectid = device->devid;
-	key.offset = *start;
+	key.offset = start;
 	key.type = BTRFS_DEV_EXTENT_KEY;
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      sizeof(*extent));

@@ -583,6 +575,22 @@ err:
 	return ret;
 }
 
+/*
+ * Allocate one free dev extent and insert it into the fs.
+ */
+static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
+				  struct btrfs_device *device,
+				  u64 chunk_offset, u64 num_bytes, u64 *start)
+{
+	int ret;
+
+	ret = find_free_dev_extent(device, num_bytes, start, NULL);
+	if (ret)
+		return ret;
+	return btrfs_insert_dev_extent(trans, device, chunk_offset, num_bytes,
+					*start);
+}
+
 static int find_next_chunk(struct btrfs_fs_info *fs_info, u64 *offset)
 {
 	struct btrfs_root *root = fs_info->chunk_root;

@@ -1107,7 +1115,7 @@ again:
 			list_move_tail(&device->dev_list, dev_list);
 
 		ret = btrfs_alloc_dev_extent(trans, device, key.offset,
-			     calc_size, &dev_offset, 0);
+			     calc_size, &dev_offset);
 		if (ret < 0)
 			goto out_chunk_map;
 

@@ -1241,8 +1249,12 @@ int btrfs_alloc_data_chunk(struct btrfs_trans_handle *trans,
 	while (index < num_stripes) {
 		struct btrfs_stripe *stripe;
 
-		ret = btrfs_alloc_dev_extent(trans, device, key.offset,
-			     calc_size, &dev_offset, convert);
+		if (convert)
+			ret = btrfs_insert_dev_extent(trans, device, key.offset,
+					calc_size, dev_offset);
+		else
+			ret = btrfs_alloc_dev_extent(trans, device, key.offset,
+					calc_size, &dev_offset);
 		BUG_ON(ret);
 
 		device->bytes_used += calc_size;
volumes.h +3 -0
index b4ea93f0bec3..44284ee75adb 100644
@@ -268,6 +268,9 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       int flags);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_close_all_devices(void);
+int btrfs_insert_dev_extent(struct btrfs_trans_handle *trans,
+			    struct btrfs_device *device,
+			    u64 chunk_offset, u64 num_bytes, u64 start);
 int btrfs_add_device(struct btrfs_trans_handle *trans,
 		     struct btrfs_fs_info *fs_info,
 		     struct btrfs_device *device);
Signature · 1 line
2.19.2
Reviewed-by on v2 3/5
#11 lore raw

Re: [PATCH v2 3/5] btrfs-progs: volumes: Refactor btrfs_alloc_dev_extent() into two functions

Quoted 11 lines · Qu Wenruo

We have btrfs_alloc_dev_extent() accepting @convert flag to toggle special handling for convert.

However that @convert flag only determine whether we call find_free_dev_extent(), and we may later need to insert dev extents without searching dev tree.

So refactor btrfs_alloc_dev_extent() into 2 functions, btrfs_alloc_dev_extent(), which will try to find free dev extent, and btrfs_insert_dev_extent(), which will just insert a dev extent.

Quoted 7 lines

For implementation, btrfs_alloc_dev_extent() will call btrfs_insert_dev_extent() anyway, so no duplicated code.

This removes the need of @convert parameter, and make btrfs_insert_dev_extent() public for later usage.

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

This looks much better:

Reviewed-by Nikolay Borisov <[email protected]>
Quoted 108 lines
---
 volumes.c | 48 ++++++++++++++++++++++++++++++------------------
 volumes.h |  3 +++
 2 files changed, 33 insertions(+), 18 deletions(-)
volumes.c +30 -18
index 30090ce5f8e8..0dd082cd1718 100644
@@ -530,10 +530,12 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes,
 	return find_free_dev_extent_start(device, num_bytes, 0, start, len);
 }
 
-static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
-				  struct btrfs_device *device,
-				  u64 chunk_offset, u64 num_bytes, u64 *start,
-				  int convert)
+/*
+ * Insert one device extent into the fs.
+ */
+int btrfs_insert_dev_extent(struct btrfs_trans_handle *trans,
+			    struct btrfs_device *device,
+			    u64 chunk_offset, u64 num_bytes, u64 start)
 {
 	int ret;
 	struct btrfs_path *path;

@@ -546,18 +548,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	if (!path)
 		return -ENOMEM;
 
-	/*
-	 * For convert case, just skip search free dev_extent, as caller
-	 * is responsible to make sure it's free.
-	 */
-	if (!convert) {
-		ret = find_free_dev_extent(device, num_bytes, start, NULL);
-		if (ret)
-			goto err;
-	}
-
 	key.objectid = device->devid;
-	key.offset = *start;
+	key.offset = start;
 	key.type = BTRFS_DEV_EXTENT_KEY;
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      sizeof(*extent));

@@ -583,6 +575,22 @@ err:
 	return ret;
 }
 
+/*
+ * Allocate one free dev extent and insert it into the fs.
+ */
+static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
+				  struct btrfs_device *device,
+				  u64 chunk_offset, u64 num_bytes, u64 *start)
+{
+	int ret;
+
+	ret = find_free_dev_extent(device, num_bytes, start, NULL);
+	if (ret)
+		return ret;
+	return btrfs_insert_dev_extent(trans, device, chunk_offset, num_bytes,
+					*start);
+}
+
 static int find_next_chunk(struct btrfs_fs_info *fs_info, u64 *offset)
 {
 	struct btrfs_root *root = fs_info->chunk_root;

@@ -1107,7 +1115,7 @@ again:
 			list_move_tail(&device->dev_list, dev_list);
 
 		ret = btrfs_alloc_dev_extent(trans, device, key.offset,
-			     calc_size, &dev_offset, 0);
+			     calc_size, &dev_offset);
 		if (ret < 0)
 			goto out_chunk_map;
 

@@ -1241,8 +1249,12 @@ int btrfs_alloc_data_chunk(struct btrfs_trans_handle *trans,
 	while (index < num_stripes) {
 		struct btrfs_stripe *stripe;
 
-		ret = btrfs_alloc_dev_extent(trans, device, key.offset,
-			     calc_size, &dev_offset, convert);
+		if (convert)
+			ret = btrfs_insert_dev_extent(trans, device, key.offset,
+					calc_size, dev_offset);
+		else
+			ret = btrfs_alloc_dev_extent(trans, device, key.offset,
+					calc_size, &dev_offset);
 		BUG_ON(ret);
 
 		device->bytes_used += calc_size;
volumes.h +3 -0
index b4ea93f0bec3..44284ee75adb 100644
@@ -268,6 +268,9 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       int flags);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_close_all_devices(void);
+int btrfs_insert_dev_extent(struct btrfs_trans_handle *trans,
+			    struct btrfs_device *device,
+			    u64 chunk_offset, u64 num_bytes, u64 start);
 int btrfs_add_device(struct btrfs_trans_handle *trans,
 		     struct btrfs_fs_info *fs_info,
 		     struct btrfs_device *device);
author
#12 lore raw

PATCH v2 4/5 btrfs-progs: image: Remove all existing dev extents for later rebuild

In reply to Qu Wenruo

For multi-disk btrfs image recovered to single disk, the dev tree would look like:

        item 2 key (1 DEV_EXTENT 22020096)
                dev extent chunk_tree 3
                chunk_objectid 256 chunk_offset 22020096 length 8388608
        item 3 key (1 DEV_EXTENT 30408704)
                dev extent chunk_tree 3
                chunk_objectid 256 chunk_offset 30408704 length 1073741824
        item 4 key (1 DEV_EXTENT 1104150528)
                dev extent chunk_tree 3
                chunk_objectid 256 chunk_offset 1104150528 length 536870912
        item 5 key (2 DEV_EXTENT 1048576)
                dev extent chunk_tree 3
                chunk_objectid 256 chunk_offset 22020096 length 8388608
        item 6 key (2 DEV_EXTENT 9437184)
                dev extent chunk_tree 3
                chunk_objectid 256 chunk_offset 30408704 length 1073741824
        item 7 key (2 DEV_EXTENT 1083179008)
                dev extent chunk_tree 3
                chunk_objectid 256 chunk_offset 1104150528 length 536870912

However in chunk tree, we only use devid 2, thus devid 1 is completely garbage:

        item 0 key (DEV_ITEMS DEV_ITEM 2)
        item 1 key (FIRST_CHUNK_TREE CHUNK_ITEM 22020096) itemoff 16105 itemsize 80
                length 8388608 owner 2 stripe_len 65536 type SYSTEM
                num_stripes 1 sub_stripes 0
                        stripe 0 devid 2 offset 1048576
        item 2 key (FIRST_CHUNK_TREE CHUNK_ITEM 30408704) itemoff 16025 itemsize 80
                length 1073741824 owner 2 stripe_len 65536 type METADATA
                num_stripes 1 sub_stripes 0
                        stripe 0 devid 2 offset 9437184
        item 3 key (FIRST_CHUNK_TREE CHUNK_ITEM 1104150528) itemoff 15945 itemsize 80
                length 1073741824 owner 2 stripe_len 65536 type DATA
                num_stripes 1 sub_stripes 0
                        stripe 0 devid 2 offset 1083179008

To fix the problem, the most straight-forward way is to remove all existing dev extents, and then re-fill correct dev extents from chunk.

So this patch just follow the straight-forward way to fix it, causing the final dev extents layout to match with chunk tree, and make btrfs check happy.

Signed-off-by Qu Wenruo <[email protected]>
---
 image/main.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)
image/main.c +102 -0
index 9187de34f34a..8f756689a1fa 100644
@@ -2209,6 +2209,104 @@ static void fixup_block_groups(struct btrfs_fs_info *fs_info)
 	}
 }
 
+static int remove_all_dev_extents(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_root *root = fs_info->dev_root;
+	struct btrfs_path path;
+	struct btrfs_key key;
+	struct extent_buffer *leaf;
+	int slot;
+	int ret;
+
+	key.objectid = 1;
+	key.type = BTRFS_DEV_EXTENT_KEY;
+	key.offset = 0;
+	btrfs_init_path(&path);
+
+	ret = btrfs_search_slot(trans, root, &key, &path, -1, 1);
+	if (ret < 0) {
+		error("failed to search dev tree: %s", strerror(-ret));
+		return ret;
+	}
+
+	while (1) {
+		slot = path.slots[0];
+		leaf = path.nodes[0];
+		if (slot >= btrfs_header_nritems(leaf)) {
+			ret = btrfs_next_leaf(root, &path);
+			if (ret < 0) {
+				error("failed to search dev tree: %s",
+					strerror(-ret));
+				goto out;
+			}
+			if (ret > 0) {
+				ret = 0;
+				goto out;
+			}
+		}
+
+		btrfs_item_key_to_cpu(leaf, &key, slot);
+		if (key.type != BTRFS_DEV_EXTENT_KEY)
+			break;
+		ret = btrfs_del_item(trans, root, &path);
+		if (ret < 0) {
+			error("failed to delete dev extent %llu, %llu: %s",
+				key.objectid, key.offset, strerror(-ret));
+			goto out;
+		}
+	}
+out:
+	btrfs_release_path(&path);
+	return ret;
+}
+
+static int fixup_dev_extents(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
+	struct btrfs_device *dev;
+	struct cache_extent *ce;
+	struct map_lookup *map;
+	u64 devid = btrfs_stack_device_id(&fs_info->super_copy->dev_item);
+	int i;
+	int ret;
+
+	ret = remove_all_dev_extents(trans);
+	if (ret < 0)
+		error("failed to remove all existing dev extents: %s",
+			strerror(-ret));
+
+	dev = btrfs_find_device(fs_info, devid, NULL, NULL);
+	if (!dev) {
+		error("faild to find devid %llu", devid);
+		return -ENODEV;
+	}
+
+	/* Rebuild all dev extents using chunk maps */
+	for (ce = search_cache_extent(&map_tree->cache_tree, 0); ce;
+	     ce = next_cache_extent(ce)) {
+		u64 stripe_len;
+
+		map = container_of(ce, struct map_lookup, ce);
+		stripe_len = calc_stripe_length(map->type, ce->size,
+						map->num_stripes);
+		for (i = 0; i < map->num_stripes; i++) {
+			ret = btrfs_insert_dev_extent(trans, dev, ce->start,
+					stripe_len, map->stripes[i].physical);
+			if (ret < 0) {
+				error(
+				"failed to insert dev extent %llu %llu: %s",
+					devid, map->stripes[i].physical,
+					strerror(-ret));
+				goto out;
+			}
+		}
+	}
+out:
+	return ret;
+}
+
 static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 			 struct mdrestore_struct *mdres, off_t dev_size)
 {

@@ -2226,6 +2324,10 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 	}
 
 	fixup_block_groups(fs_info);
+	ret = fixup_dev_extents(trans);
+	if (ret < 0)
+		goto error;
+
 	ret = fixup_device_size(trans, mdres, dev_size);
 	if (ret < 0)
 		goto error;
Signature · 1 line
2.19.2
author
#13 lore raw

PATCH v2 5/5 btrfs-progs: misc-tests/021: Do extra btrfs check before mounting

In reply to Qu Wenruo

Test case misc/021 is testing if we could mount a single disk btrfs image recovered from multi disk fs.

The problem is, current kernel has extra check for block group, chunk and dev extent.
This means any image can't pass btrfs check for chunk tree will not mount.

So do extra btrfs check before mount, this will also help us to locate the problem in btrfs-image easier.

Signed-off-by Qu Wenruo <[email protected]>
---
 tests/misc-tests/021-image-multi-devices/test.sh | 3 +++
 1 file changed, 3 insertions(+)
tests/misc-tests/021-image-multi-devices/test.sh +3 -0
index 5430847f4e2f..26beae6e4b85 100755
@@ -37,6 +37,9 @@ run_check $SUDO_HELPER wipefs -a "$loop2"
 
 run_check $SUDO_HELPER "$TOP/btrfs-image" -r "$IMAGE" "$loop1"
 
+# Run check to make sure there is nothing wrong for the recovered image
+run_check "$TOP/btrfs" check "$loop1"
+
 run_check $SUDO_HELPER mount "$loop1" "$TEST_MNT"
 new_md5=$(run_check_stdout md5sum "$TEST_MNT/foobar" | cut -d ' ' -f 1)
 run_check $SUDO_HELPER umount "$TEST_MNT"
Signature · 1 line
2.19.2
Reviewed-by on v2 5/5
#14 lore raw

Re: [PATCH v2 5/5] btrfs-progs: misc-tests/021: Do extra btrfs check before mounting

Quoted 8 lines · Qu Wenruo

Test case misc/021 is testing if we could mount a single disk btrfs image recovered from multi disk fs.

The problem is, current kernel has extra check for block group, chunk and dev extent.
This means any image can't pass btrfs check for chunk tree will not mount.

Quoted 4 lines

So do extra btrfs check before mount, this will also help us to locate the problem in btrfs-image easier.

Signed-off-by Qu Wenruo <[email protected]>
Reviewed-by Nikolay Borisov <[email protected]>
Quoted 19 lines
---
 tests/misc-tests/021-image-multi-devices/test.sh | 3 +++
 1 file changed, 3 insertions(+)
tests/misc-tests/021-image-multi-devices/test.sh +3 -0
index 5430847f4e2f..26beae6e4b85 100755
@@ -37,6 +37,9 @@ run_check $SUDO_HELPER wipefs -a "$loop2"
 
 run_check $SUDO_HELPER "$TOP/btrfs-image" -r "$IMAGE" "$loop1"
 
+# Run check to make sure there is nothing wrong for the recovered image
+run_check "$TOP/btrfs" check "$loop1"
+
 run_check $SUDO_HELPER mount "$loop1" "$TEST_MNT"
 new_md5=$(run_check_stdout md5sum "$TEST_MNT/foobar" | cut -d ' ' -f 1)
 run_check $SUDO_HELPER umount "$TEST_MNT"
Applied
#15 lore raw
In reply to Qu Wenruo
Quoted 33 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/image_recover

The base commit is as usual, the latest stable tag, v4.19.

Test case misc/021 will fail if using latest upstream kernel.

This is due to the enhanced code in kernel to check block group <-> chunk <-> dev extent mapping.

This means from the very beginning, btrfs-image can't really restore a multi-disk image to single-disk one correctly.

The problem is, although we modified chunk item, we didn't modify block group item's flags or dev extents.

This patch will reset block group flags, then rebuild the whole dev extents by removing existing ones first, then re-add the correct dev extents calculated from chunk map.

Now it could pass all misc tests and fsck tests.

Changelog:
v2:
- Parameter list cleanup
* Use trans->fs_info to remove fs_info parameter
* Remove trans parameter for function who doesn't need
- Merge dev extents removal code with rebuild code
- Refactor btrfs_alloc_dev_extent() into 2 functions
* btrfs_insert_dev_extent() for convert and dev extent rebuild
* btrfs_alloc_dev_extent() for old use case

Quoted 9 lines

Qu Wenruo (5):
btrfs-progs: image: Refactor fixup_devices() to
fixup_chunks_and_devices()
btrfs-progs: image: Fix block group item flags when restoring
multi-device image to single device
btrfs-progs: volumes: Refactor btrfs_alloc_dev_extent() into two
functions
btrfs-progs: image: Remove all existing dev extents for later rebuild
btrfs-progs: misc-tests/021: Do extra btrfs check before mounting

Merged to devel, thanks. I've switched strerror(-ret) to the errno pattern.