[PATCH 0/5] btrfs-progs: image: Fix error when restoring multi-disk image to single disk

[PATCH 0/5] btrfs-progs: image: Fix error when restoring multi-disk image to single disk

[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.

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: image: Remove all existing dev extents for later rebuild
  btrfs-progs: image: Rebuild dev extents using chunk tree
  btrfs-progs: misc-tests/021: Do extra btrfs check before mounting

 image/main.c                                  | 201 ++++++++++++++++--
 .../021-image-multi-devices/test.sh           |   3 +
 volumes.c                                     |  10 +-
 volumes.h                                     |   4 +
 4 files changed, 197 insertions(+), 21 deletions(-)

-- 
2.19.2

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

[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 <wqu@suse.com>
---
 image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/image/main.c b/image/main.c
index c680ab19de6c..36b5c95ea146 100644
--- a/image/main.c
+++ b/image/main.c
@@ -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 btrfs_fs_info *fs_info,
+			     struct mdrestore_struct *mdres,
+			     off_t dev_size)
 {
-	struct btrfs_trans_handle *trans;
 	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, fs_info, 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;
-- 
2.19.2

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

[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 <wqu@suse.com>
---
 image/main.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/image/main.c b/image/main.c
index 36b5c95ea146..9060f6b1b665 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2164,6 +2164,52 @@ again:
 	return 0;
 }
 
+static void fixup_block_groups(struct btrfs_trans_handle *trans,
+			      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 +2226,7 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 		return PTR_ERR(trans);
 	}
 
+	fixup_block_groups(trans, fs_info);
 	ret = fixup_device_size(trans, fs_info, mdres, dev_size);
 	if (ret < 0)
 		goto error;
-- 
2.19.2

[PATCH 3/5] btrfs-progs: image: Remove all existing dev extents for later rebuild

[Qu Wenruo]

This patch will remove all existing dev extents for later rebuild.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 image/main.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/image/main.c b/image/main.c
index 9060f6b1b665..707568f22e01 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2210,6 +2210,70 @@ static void fixup_block_groups(struct btrfs_trans_handle *trans,
 	}
 }
 
+static int remove_all_dev_extents(struct btrfs_trans_handle *trans,
+				  struct btrfs_fs_info *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)
+{
+	int ret;
+
+	ret = remove_all_dev_extents(trans, fs_info);
+	if (ret < 0)
+		error("failed to remove all existing dev extents: %s",
+			strerror(-ret));
+	return ret;
+}
+
 static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 			 struct mdrestore_struct *mdres, off_t dev_size)
 {
@@ -2227,6 +2291,10 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
 	}
 
 	fixup_block_groups(trans, fs_info);
+	ret = fixup_dev_extents(trans, fs_info);
+	if (ret < 0)
+		goto error;
+
 	ret = fixup_device_size(trans, fs_info, mdres, dev_size);
 	if (ret < 0)
 		goto error;
-- 
2.19.2

[PATCH 4/5] btrfs-progs: image: Rebuild dev extents using chunk tree

[Qu Wenruo]

With existing dev extents cleaned up, now we can rebuild dev extents
using the correct chunk tree.

Since new dev extents are all rebuild from scratch, even we're restoring
image from multi-device fs to single disk, we won't have any problem
reported by btrfs check.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 image/main.c | 34 ++++++++++++++++++++++++++++++++++
 volumes.c    | 10 +++++-----
 volumes.h    |  4 ++++
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/image/main.c b/image/main.c
index 707568f22e01..626eb933d5cc 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2265,12 +2265,46 @@ out:
 static int fixup_dev_extents(struct btrfs_trans_handle *trans,
 			     struct btrfs_fs_info *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, fs_info);
 	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_alloc_dev_extent(trans, dev, ce->start,
+				stripe_len, &map->stripes[i].physical, 1);
+			if (ret < 0) {
+				error(
+				"failed to insert dev extent %llu %llu: %s",
+					devid, map->stripes[i].physical,
+					strerror(-ret));
+				goto out;
+			}
+		}
+	}
+out:
 	return ret;
 }
 
diff --git a/volumes.c b/volumes.c
index 30090ce5f8e8..73c9204fa7d1 100644
--- a/volumes.c
+++ b/volumes.c
@@ -530,10 +530,10 @@ 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)
+int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
+			   struct btrfs_device *device,
+			   u64 chunk_offset, u64 num_bytes, u64 *start,
+			   int insert_asis)
 {
 	int ret;
 	struct btrfs_path *path;
@@ -550,7 +550,7 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	 * For convert case, just skip search free dev_extent, as caller
 	 * is responsible to make sure it's free.
 	 */
-	if (!convert) {
+	if (!insert_asis) {
 		ret = find_free_dev_extent(device, num_bytes, start, NULL);
 		if (ret)
 			goto err;
diff --git a/volumes.h b/volumes.h
index b4ea93f0bec3..5ca2779ebd45 100644
--- a/volumes.h
+++ b/volumes.h
@@ -271,6 +271,10 @@ void btrfs_close_all_devices(void);
 int btrfs_add_device(struct btrfs_trans_handle *trans,
 		     struct btrfs_fs_info *fs_info,
 		     struct btrfs_device *device);
+int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
+			   struct btrfs_device *device,
+			   u64 chunk_offset, u64 num_bytes, u64 *start,
+			   int insert_asis);
 int btrfs_update_device(struct btrfs_trans_handle *trans,
 			struct btrfs_device *device);
 int btrfs_scan_one_device(int fd, const char *path,
-- 
2.19.2

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

[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 <wqu@suse.com>
---
 tests/misc-tests/021-image-multi-devices/test.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/misc-tests/021-image-multi-devices/test.sh b/tests/misc-tests/021-image-multi-devices/test.sh
index 5430847f4e2f..26beae6e4b85 100755
--- a/tests/misc-tests/021-image-multi-devices/test.sh
+++ b/tests/misc-tests/021-image-multi-devices/test.sh
@@ -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"
-- 
2.19.2

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

[Nikolay Borisov]

On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
> 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 <wqu@suse.com>
> ---
>  image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/image/main.c b/image/main.c
> index c680ab19de6c..36b5c95ea146 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -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 btrfs_fs_info *fs_info,

trans already has a handle to the fs_info so you can drop it from the
param list.

> +			     struct mdrestore_struct *mdres,
> +			     off_t dev_size)
>  {
> -	struct btrfs_trans_handle *trans;
>  	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, fs_info, 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;
> 

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

[Qu Wenruo]

On 2018/11/27 下午3:13, Nikolay Borisov wrote:
> 
> 
> On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
>> 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 <wqu@suse.com>
>> ---
>>  image/main.c | 52 ++++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 36 insertions(+), 16 deletions(-)
>>
>> diff --git a/image/main.c b/image/main.c
>> index c680ab19de6c..36b5c95ea146 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -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 btrfs_fs_info *fs_info,
> 
> trans already has a handle to the fs_info so you can drop it from the
> param list.

Indeed! My bad habbit of trans then fs_info definitely needs to be
corrected.

Thanks,
Qu

> 
>> +			     struct mdrestore_struct *mdres,
>> +			     off_t dev_size)
>>  {
>> -	struct btrfs_trans_handle *trans;
>>  	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, fs_info, 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;
>>

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

[Nikolay Borisov]

On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
> 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 <wqu@suse.com>
> ---
>  image/main.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/image/main.c b/image/main.c
> index 36b5c95ea146..9060f6b1b665 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2164,6 +2164,52 @@ again:
>  	return 0;
>  }
>  
> +static void fixup_block_groups(struct btrfs_trans_handle *trans,
> +			      struct btrfs_fs_info *fs_info)

You are not even using the trans handle in this function, why pass it?

> +{
> +	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 +2226,7 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
>  		return PTR_ERR(trans);
>  	}
>  
> +	fixup_block_groups(trans, fs_info);
>  	ret = fixup_device_size(trans, fs_info, mdres, dev_size);
>  	if (ret < 0)
>  		goto error;
> 

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

[Qu Wenruo]

On 2018/11/27 下午3:15, Nikolay Borisov wrote:
> 
> 
> On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
>> 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 <wqu@suse.com>
>> ---
>>  image/main.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> diff --git a/image/main.c b/image/main.c
>> index 36b5c95ea146..9060f6b1b665 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2164,6 +2164,52 @@ again:
>>  	return 0;
>>  }
>>  
>> +static void fixup_block_groups(struct btrfs_trans_handle *trans,
>> +			      struct btrfs_fs_info *fs_info)
> 
> You are not even using the trans handle in this function, why pass it?

Bad habit again.

Will definitely do something with that.

Thanks,
Qu

> 
>> +{
>> +	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 +2226,7 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
>>  		return PTR_ERR(trans);
>>  	}
>>  
>> +	fixup_block_groups(trans, fs_info);
>>  	ret = fixup_device_size(trans, fs_info, mdres, dev_size);
>>  	if (ret < 0)
>>  		goto error;
>>

Re: [PATCH 3/5] btrfs-progs: image: Remove all existing dev extents for later rebuild

[Nikolay Borisov]

On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
> This patch will remove all existing dev extents for later rebuild.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  image/main.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/image/main.c b/image/main.c
> index 9060f6b1b665..707568f22e01 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2210,6 +2210,70 @@ static void fixup_block_groups(struct btrfs_trans_handle *trans,
>  	}
>  }
>  
> +static int remove_all_dev_extents(struct btrfs_trans_handle *trans,
> +				  struct btrfs_fs_info *fs_info)

remove fs_info arg.

<snip>
> +
> +static int fixup_dev_extents(struct btrfs_trans_handle *trans,
> +			     struct btrfs_fs_info *fs_info)

Ditto

> +{
> +	int ret;
> +
> +	ret = remove_all_dev_extents(trans, fs_info);
> +	if (ret < 0)
> +		error("failed to remove all existing dev extents: %s",
> +			strerror(-ret));
> +	return ret;
> +}
> +
>  static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
>  			 struct mdrestore_struct *mdres, off_t dev_size)
>  {
> @@ -2227,6 +2291,10 @@ static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
>  	}
>  
>  	fixup_block_groups(trans, fs_info);
> +	ret = fixup_dev_extents(trans, fs_info);
> +	if (ret < 0)
> +		goto error;
> +
>  	ret = fixup_device_size(trans, fs_info, mdres, dev_size);
>  	if (ret < 0)
>  		goto error;
> 

Re: [PATCH 4/5] btrfs-progs: image: Rebuild dev extents using chunk tree

[Nikolay Borisov]

On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
> With existing dev extents cleaned up, now we can rebuild dev extents
> using the correct chunk tree.
> 
> Since new dev extents are all rebuild from scratch, even we're restoring
> image from multi-device fs to single disk, we won't have any problem
> reported by btrfs check.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  image/main.c | 34 ++++++++++++++++++++++++++++++++++
>  volumes.c    | 10 +++++-----
>  volumes.h    |  4 ++++
>  3 files changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/image/main.c b/image/main.c
> index 707568f22e01..626eb933d5cc 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2265,12 +2265,46 @@ out:
>  static int fixup_dev_extents(struct btrfs_trans_handle *trans,
>  			     struct btrfs_fs_info *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, fs_info);
>  	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_alloc_dev_extent(trans, dev, ce->start,
> +				stripe_len, &map->stripes[i].physical, 1);
> +			if (ret < 0) {
> +				error(
> +				"failed to insert dev extent %llu %llu: %s",
> +					devid, map->stripes[i].physical,
> +					strerror(-ret));
> +				goto out;
> +			}
> +		}
> +	}
> +out:
>  	return ret;
>  }
>  
> diff --git a/volumes.c b/volumes.c
> index 30090ce5f8e8..73c9204fa7d1 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -530,10 +530,10 @@ 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)
> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
> +			   struct btrfs_device *device,
> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
> +			   int insert_asis)

Make that parameter a bool. Also why do you rename it ?

>  {
>  	int ret;
>  	struct btrfs_path *path;
> @@ -550,7 +550,7 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>  	 * For convert case, just skip search free dev_extent, as caller
>  	 * is responsible to make sure it's free.
>  	 */
> -	if (!convert) {
> +	if (!insert_asis) {
>  		ret = find_free_dev_extent(device, num_bytes, start, NULL);
>  		if (ret)
>  			goto err;
> diff --git a/volumes.h b/volumes.h
> index b4ea93f0bec3..5ca2779ebd45 100644
> --- a/volumes.h
> +++ b/volumes.h
> @@ -271,6 +271,10 @@ void btrfs_close_all_devices(void);
>  int btrfs_add_device(struct btrfs_trans_handle *trans,
>  		     struct btrfs_fs_info *fs_info,
>  		     struct btrfs_device *device);
> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
> +			   struct btrfs_device *device,
> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
> +			   int insert_asis);
>  int btrfs_update_device(struct btrfs_trans_handle *trans,
>  			struct btrfs_device *device);
>  int btrfs_scan_one_device(int fd, const char *path,
> 

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

[Nikolay Borisov]

On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
> 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 <wqu@suse.com>
> ---
>  tests/misc-tests/021-image-multi-devices/test.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/misc-tests/021-image-multi-devices/test.sh b/tests/misc-tests/021-image-multi-devices/test.sh
> index 5430847f4e2f..26beae6e4b85 100755
> --- a/tests/misc-tests/021-image-multi-devices/test.sh
> +++ b/tests/misc-tests/021-image-multi-devices/test.sh
> @@ -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"

I think this needs to be run_check $SUDO_HELPER "$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"
> 

Re: [PATCH 4/5] btrfs-progs: image: Rebuild dev extents using chunk tree

[Qu Wenruo]

On 2018/11/27 下午3:28, Nikolay Borisov wrote:
> 
> 
> On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
>> With existing dev extents cleaned up, now we can rebuild dev extents
>> using the correct chunk tree.
>>
>> Since new dev extents are all rebuild from scratch, even we're restoring
>> image from multi-device fs to single disk, we won't have any problem
>> reported by btrfs check.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  image/main.c | 34 ++++++++++++++++++++++++++++++++++
>>  volumes.c    | 10 +++++-----
>>  volumes.h    |  4 ++++
>>  3 files changed, 43 insertions(+), 5 deletions(-)
>>
>> diff --git a/image/main.c b/image/main.c
>> index 707568f22e01..626eb933d5cc 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2265,12 +2265,46 @@ out:
>>  static int fixup_dev_extents(struct btrfs_trans_handle *trans,
>>  			     struct btrfs_fs_info *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, fs_info);
>>  	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_alloc_dev_extent(trans, dev, ce->start,
>> +				stripe_len, &map->stripes[i].physical, 1);
>> +			if (ret < 0) {
>> +				error(
>> +				"failed to insert dev extent %llu %llu: %s",
>> +					devid, map->stripes[i].physical,
>> +					strerror(-ret));
>> +				goto out;
>> +			}
>> +		}
>> +	}
>> +out:
>>  	return ret;
>>  }
>>  
>> diff --git a/volumes.c b/volumes.c
>> index 30090ce5f8e8..73c9204fa7d1 100644
>> --- a/volumes.c
>> +++ b/volumes.c
>> @@ -530,10 +530,10 @@ 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)
>> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>> +			   struct btrfs_device *device,
>> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
>> +			   int insert_asis)
> 
> Make that parameter a bool. Also why do you rename it ?

Since it's no longer only used by convert.

The best naming may be two function, one called
btrfs_insert_device_extent(), and then btrfs_alloc_device_extent().

As for convert and this use case, we are not allocating, but just
inserting one.

What about above naming change?

Thanks,
Qu

> 
>>  {
>>  	int ret;
>>  	struct btrfs_path *path;
>> @@ -550,7 +550,7 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>  	 * For convert case, just skip search free dev_extent, as caller
>>  	 * is responsible to make sure it's free.
>>  	 */
>> -	if (!convert) {
>> +	if (!insert_asis) {
>>  		ret = find_free_dev_extent(device, num_bytes, start, NULL);
>>  		if (ret)
>>  			goto err;
>> diff --git a/volumes.h b/volumes.h
>> index b4ea93f0bec3..5ca2779ebd45 100644
>> --- a/volumes.h
>> +++ b/volumes.h
>> @@ -271,6 +271,10 @@ void btrfs_close_all_devices(void);
>>  int btrfs_add_device(struct btrfs_trans_handle *trans,
>>  		     struct btrfs_fs_info *fs_info,
>>  		     struct btrfs_device *device);
>> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>> +			   struct btrfs_device *device,
>> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
>> +			   int insert_asis);
>>  int btrfs_update_device(struct btrfs_trans_handle *trans,
>>  			struct btrfs_device *device);
>>  int btrfs_scan_one_device(int fd, const char *path,
>>

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

[Qu Wenruo]

On 2018/11/27 下午3:29, Nikolay Borisov wrote:
> 
> 
> On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
>> 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 <wqu@suse.com>
>> ---
>>  tests/misc-tests/021-image-multi-devices/test.sh | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/misc-tests/021-image-multi-devices/test.sh b/tests/misc-tests/021-image-multi-devices/test.sh
>> index 5430847f4e2f..26beae6e4b85 100755
>> --- a/tests/misc-tests/021-image-multi-devices/test.sh
>> +++ b/tests/misc-tests/021-image-multi-devices/test.sh
>> @@ -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"
> 
> I think this needs to be run_check $SUDO_HELPER "$TOP/btrfs" check "$loop1"

For read-only check, it's OK and I always run the tests using normal
user, no privilege problem.

Thanks,
Qu

>> +
>>  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"
>>

Re: [PATCH 4/5] btrfs-progs: image: Rebuild dev extents using chunk tree

[Nikolay Borisov]

On 27.11.18 г. 9:30 ч., Qu Wenruo wrote:
> 
> 
> On 2018/11/27 下午3:28, Nikolay Borisov wrote:
>>
>>
>> On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
>>> With existing dev extents cleaned up, now we can rebuild dev extents
>>> using the correct chunk tree.
>>>
>>> Since new dev extents are all rebuild from scratch, even we're restoring
>>> image from multi-device fs to single disk, we won't have any problem
>>> reported by btrfs check.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  image/main.c | 34 ++++++++++++++++++++++++++++++++++
>>>  volumes.c    | 10 +++++-----
>>>  volumes.h    |  4 ++++
>>>  3 files changed, 43 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/image/main.c b/image/main.c
>>> index 707568f22e01..626eb933d5cc 100644
>>> --- a/image/main.c
>>> +++ b/image/main.c
>>> @@ -2265,12 +2265,46 @@ out:
>>>  static int fixup_dev_extents(struct btrfs_trans_handle *trans,
>>>  			     struct btrfs_fs_info *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, fs_info);
>>>  	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_alloc_dev_extent(trans, dev, ce->start,
>>> +				stripe_len, &map->stripes[i].physical, 1);
>>> +			if (ret < 0) {
>>> +				error(
>>> +				"failed to insert dev extent %llu %llu: %s",
>>> +					devid, map->stripes[i].physical,
>>> +					strerror(-ret));
>>> +				goto out;
>>> +			}
>>> +		}
>>> +	}
>>> +out:
>>>  	return ret;
>>>  }
>>>  
>>> diff --git a/volumes.c b/volumes.c
>>> index 30090ce5f8e8..73c9204fa7d1 100644
>>> --- a/volumes.c
>>> +++ b/volumes.c
>>> @@ -530,10 +530,10 @@ 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)
>>> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>> +			   struct btrfs_device *device,
>>> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
>>> +			   int insert_asis)
>>
>> Make that parameter a bool. Also why do you rename it ?
> 
> Since it's no longer only used by convert.
> 
> The best naming may be two function, one called
> btrfs_insert_device_extent(), and then btrfs_alloc_device_extent().
> 
> As for convert and this use case, we are not allocating, but just
> inserting one.
> 
> What about above naming change?

Indeed  two functions seem better, btrfs_alloc_device_extent will be a
wrapper of btrfs_insert_device_extent + the added find_free_dev_extent,
whereas btrfs_insert_device_extent will be the function doing the actual
insert.

> 
> Thanks,
> Qu
> 
>>
>>>  {
>>>  	int ret;
>>>  	struct btrfs_path *path;
>>> @@ -550,7 +550,7 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>>  	 * For convert case, just skip search free dev_extent, as caller
>>>  	 * is responsible to make sure it's free.
>>>  	 */
>>> -	if (!convert) {
>>> +	if (!insert_asis) {
>>>  		ret = find_free_dev_extent(device, num_bytes, start, NULL);
>>>  		if (ret)
>>>  			goto err;
>>> diff --git a/volumes.h b/volumes.h
>>> index b4ea93f0bec3..5ca2779ebd45 100644
>>> --- a/volumes.h
>>> +++ b/volumes.h
>>> @@ -271,6 +271,10 @@ void btrfs_close_all_devices(void);
>>>  int btrfs_add_device(struct btrfs_trans_handle *trans,
>>>  		     struct btrfs_fs_info *fs_info,
>>>  		     struct btrfs_device *device);
>>> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>> +			   struct btrfs_device *device,
>>> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
>>> +			   int insert_asis);
>>>  int btrfs_update_device(struct btrfs_trans_handle *trans,
>>>  			struct btrfs_device *device);
>>>  int btrfs_scan_one_device(int fd, const char *path,
>>>
>