[PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[Qu Wenruo]

Again the buggy btrfs-convert, even David tried to ban mixed-bg features
for btrfs-convert, it will still put data and metadata extents into the
same chunk, without marking the chunk mixed.

So in the patchset, first add fsck support for such problem, and then
force btrfs-convert to use mixed block group.

Lastly change the test case as they won't convert with non pagesize
nodesize.

v2:
  Reword the title, as some patch uses wrong title 'btrfs' not
  'btrfs-progs'.
  No real code change, so no changelog for each patch.

Qu Wenruo (5):
  btrfs-progs: fsck: Add check for extent and parent chunk type
  btrfs-progs: utils: Check nodesize against features
  btrfs-progs: convert: force convert to used mixed block group
  btrfs-progs: util: add parameter for btrfs_list_all_fs_features
  btrfs-progs: convert-test: Disable different nodesize test

 btrfs-convert.c        | 59 ++++++++++++++++++++++++++-----------------
 cmds-check.c           | 68 +++++++++++++++++++++++++++++++++++++++++++++++++-
 ctree.h                |  2 +-
 extent-tree.c          | 15 ++++++++++-
 mkfs.c                 | 19 ++++++--------
 tests/convert-tests.sh | 36 +++++++++++++++-----------
 utils.c                | 12 ++++++---
 utils.h                | 20 ++++++++++++---
 8 files changed, 171 insertions(+), 60 deletions(-)

-- 
2.5.1

[PATCH v2 1/5] btrfs-progs: fsck: Add check for extent and parent chunk type

[Qu Wenruo]

There is a bug in btrfs-convert in 4.1.2, even we don't allow mixed
block group for converted image, btrfs-convert will still create image
with data and metadata inside one chunk.

And further more, the chunk type is still DATA or METADATA, not
DATA|METADATA (not mixed).

So add btrfsck check for it right now.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 cmds-check.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index 0694a3b..c818bec 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -55,6 +55,7 @@ static int repair = 0;
 static int no_holes = 0;
 static int init_extent_tree = 0;
 static int check_data_csum = 0;
+static struct btrfs_fs_info *global_info;
 
 struct extent_backref {
 	struct list_head list;
@@ -127,6 +128,7 @@ struct extent_record {
 	unsigned int metadata:1;
 	unsigned int bad_full_backref:1;
 	unsigned int crossing_stripes:1;
+	unsigned int wrong_chunk_type:1;
 };
 
 struct inode_backref {
@@ -3749,7 +3751,8 @@ static int maybe_free_extent_rec(struct cache_tree *extent_cache,
 	if (rec->content_checked && rec->owner_ref_checked &&
 	    rec->extent_item_refs == rec->refs && rec->refs > 0 &&
 	    rec->num_duplicates == 0 && !all_backpointers_checked(rec, 0) &&
-	    !rec->bad_full_backref && !rec->crossing_stripes) {
+	    !rec->bad_full_backref && !rec->crossing_stripes &&
+	    !rec->wrong_chunk_type) {
 		remove_cache_extent(extent_cache, &rec->cache);
 		free_all_extent_backrefs(rec);
 		list_del_init(&rec->list);
@@ -4313,6 +4316,56 @@ static struct data_backref *alloc_data_backref(struct extent_record *rec,
 	return ref;
 }
 
+/* Check if the type of extent matches with its chunk */
+static void check_extent_type(struct extent_record *rec)
+{
+	struct btrfs_block_group_cache *bg_cache;
+
+	bg_cache = btrfs_lookup_first_block_group(global_info, rec->start);
+	if (!bg_cache)
+		return;
+
+	/* data extent, check chunk directly*/
+	if (!rec->metadata) {
+		if (!(bg_cache->flags & BTRFS_BLOCK_GROUP_DATA))
+			rec->wrong_chunk_type = 1;
+		return;
+	}
+
+	/* metadata extent, check the obvious case first */
+	if (!(bg_cache->flags & (BTRFS_BLOCK_GROUP_SYSTEM |
+				 BTRFS_BLOCK_GROUP_METADATA))) {
+		rec->wrong_chunk_type = 1;
+		return;
+	}
+
+	/*
+	 * Check SYSTEM extent, as it's also marked as metadata, we can only
+	 * make sure it's a SYSTEM extent by its backref
+	 */
+	if (!list_empty(&rec->backrefs)) {
+		struct extent_backref *node;
+		struct tree_backref *tback;
+		u64 bg_type;
+
+		node = list_entry(rec->backrefs.next, struct extent_backref,
+				  list);
+		if (node->is_data) {
+			/* tree block shouldn't have data backref */
+			rec->wrong_chunk_type = 1;
+			return;
+		}
+		tback = container_of(node, struct tree_backref, node);
+
+		if (tback->root == BTRFS_CHUNK_TREE_OBJECTID)
+			bg_type = BTRFS_BLOCK_GROUP_SYSTEM;
+		else
+			bg_type = BTRFS_BLOCK_GROUP_METADATA;
+		if (!(bg_cache->flags & bg_type))
+			rec->wrong_chunk_type = 1;
+	}
+}
+
 static int add_extent_rec(struct cache_tree *extent_cache,
 			  struct btrfs_key *parent_key, u64 parent_gen,
 			  u64 start, u64 nr, u64 extent_item_refs,
@@ -4405,6 +4458,7 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 		if (metadata && check_crossing_stripes(rec->start,
 						       rec->max_size))
 				rec->crossing_stripes = 1;
+		check_extent_type(rec);
 		maybe_free_extent_rec(extent_cache, rec);
 		return ret;
 	}
@@ -4420,6 +4474,7 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 	rec->flag_block_full_backref = -1;
 	rec->bad_full_backref = 0;
 	rec->crossing_stripes = 0;
+	rec->wrong_chunk_type = 0;
 	INIT_LIST_HEAD(&rec->backrefs);
 	INIT_LIST_HEAD(&rec->dups);
 	INIT_LIST_HEAD(&rec->list);
@@ -4462,6 +4517,7 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 	if (metadata)
 		if (check_crossing_stripes(rec->start, rec->max_size))
 			rec->crossing_stripes = 1;
+	check_extent_type(rec);
 	return ret;
 }
 
@@ -4509,6 +4565,7 @@ static int add_tree_backref(struct cache_tree *extent_cache, u64 bytenr,
 		}
 		back->node.found_extent_tree = 1;
 	}
+	check_extent_type(rec);
 	maybe_free_extent_rec(extent_cache, rec);
 	return 0;
 }
@@ -7520,6 +7577,14 @@ static int check_extent_refs(struct btrfs_root *root,
 			cur_err = 1;
 		}
 
+		if (rec->wrong_chunk_type) {
+			fprintf(stderr,
+				"bad extent [%llu, %llu), type dismatch with chunk\n",
+				rec->start, rec->start + rec->max_size);
+			err = 1;
+			cur_err = 1;
+		}
+
 		remove_cache_extent(extent_cache, cache);
 		free_all_extent_backrefs(rec);
 		if (!init_extent_tree && repair && (!cur_err || fixed))
@@ -9378,6 +9443,7 @@ int cmd_check(int argc, char **argv)
 		goto err_out;
 	}
 
+	global_info = info;
 	root = info->fs_root;
 
 	/*
-- 
2.5.1

[PATCH v2 2/5] btrfs-progs: utils: Check nodesize against features

[Qu Wenruo]

Check nodesize against features, not only sectorsize.
In fact, one of the btrfs-convert and mkfs differs in the nodesize
check.

This patch also provides the basis for later btrfs-convert fix.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-convert.c |  2 +-
 mkfs.c          | 16 +++++-----------
 utils.c         |  8 +++++++-
 utils.h         |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index f4fc650..459b89a 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2314,7 +2314,7 @@ static int do_convert(const char *devname, int datacsum, int packing, int noxatt
 		fprintf(stderr, "filetype feature is missing\n");
 		goto fail;
 	}
-	if (btrfs_check_nodesize(nodesize, blocksize))
+	if (btrfs_check_nodesize(nodesize, blocksize, features))
 		goto fail;
 	blocks_per_node = nodesize / blocksize;
 	ret = -blocks_per_node;
diff --git a/mkfs.c b/mkfs.c
index 14e7eb4..b8879fc 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1466,9 +1466,8 @@ int main(int ac, char **av)
 				print_usage(c != GETOPT_VAL_HELP);
 		}
 	}
+
 	sectorsize = max(sectorsize, (u32)sysconf(_SC_PAGESIZE));
-	if (btrfs_check_nodesize(nodesize, sectorsize))
-		exit(1);
 	saved_optind = optind;
 	dev_cnt = ac - optind;
 	if (dev_cnt == 0)
@@ -1542,17 +1541,12 @@ int main(int ac, char **av)
 			}
 		}
 
-		if (!nodesize_forced) {
+		if (!nodesize_forced)
 			nodesize = best_nodesize;
-			if (btrfs_check_nodesize(nodesize, sectorsize))
-				exit(1);
-		}
-		if (nodesize != sectorsize) {
-			fprintf(stderr, "Error: mixed metadata/data block groups "
-				"require metadata blocksizes equal to the sectorsize\n");
-			exit(1);
-		}
 	}
+	if (btrfs_check_nodesize(nodesize, sectorsize,
+				 features))
+		exit(1);
 
 	/* Check device/block_count after the nodesize is determined */
 	if (block_count && block_count < btrfs_min_dev_size(nodesize)) {
diff --git a/utils.c b/utils.c
index 52791b5..c0d1afa 100644
--- a/utils.c
+++ b/utils.c
@@ -2928,7 +2928,7 @@ int btrfs_tree_search2_ioctl_supported(int fd)
 	return v2_supported;
 }
 
-int btrfs_check_nodesize(u32 nodesize, u32 sectorsize)
+int btrfs_check_nodesize(u32 nodesize, u32 sectorsize, u64 features)
 {
 	if (nodesize < sectorsize) {
 		fprintf(stderr,
@@ -2945,6 +2945,12 @@ int btrfs_check_nodesize(u32 nodesize, u32 sectorsize)
 			"ERROR: Illegal nodesize %u (not aligned to %u)\n",
 			nodesize, sectorsize);
 		return -1;
+	} else if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS &&
+		   nodesize != sectorsize) {
+		fprintf(stderr,
+			"ERROR: Illegal nodesize %u (not equal to %u for mixed block group)\n",
+			nodesize, sectorsize);
+		return -1;
 	}
 	return 0;
 }
diff --git a/utils.h b/utils.h
index dce0a47..0eadaf1 100644
--- a/utils.h
+++ b/utils.h
@@ -241,7 +241,7 @@ static inline u64 div_factor(u64 num, int factor)
 }
 
 int btrfs_tree_search2_ioctl_supported(int fd);
-int btrfs_check_nodesize(u32 nodesize, u32 sectorsize);
+int btrfs_check_nodesize(u32 nodesize, u32 sectorsize, u64 features);
 
 const char *get_argv0_buf(void);
 
-- 
2.5.1

[PATCH v2 3/5] btrfs-progs: convert: force convert to used mixed block group

[Qu Wenruo]

Convert has a bug that even we disable it from using mixed block, it
will still put data and metadata extent into the same chunk.

The bug can be reported by previous fsck patch.

Even without the bug, it's still high recommeneded to use mixed block
group, as unlike btrfs, ext* is not extent based from designed, so even
if we find a method to fix above bug, chunks will be quite scattered.

So force convert to use mixed block group right now.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-convert.c | 55 +++++++++++++++++++++++++++++++++----------------------
 ctree.h         |  2 +-
 extent-tree.c   | 15 ++++++++++++++-
 utils.h         | 16 ++++++++++++++--
 4 files changed, 62 insertions(+), 26 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 459b89a..a60f380 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -1683,7 +1683,7 @@ static int create_subvol(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
-static int init_btrfs(struct btrfs_root *root)
+static int init_btrfs(struct btrfs_root *root, u64 features)
 {
 	int ret;
 	struct btrfs_key location;
@@ -1693,7 +1693,7 @@ static int init_btrfs(struct btrfs_root *root)
 
 	trans = btrfs_start_transaction(root, 1);
 	BUG_ON(!trans);
-	ret = btrfs_make_block_groups(trans, root);
+	ret = btrfs_make_block_groups(trans, root, features);
 	if (ret)
 		goto err;
 	ret = btrfs_fix_block_accounting(trans, root);
@@ -2387,7 +2387,7 @@ static int do_convert(const char *devname, int datacsum, int packing, int noxatt
 			ext2_free_block_range(ext2_fs, blocks[i],
 					blocks_per_node);
 	}
-	ret = init_btrfs(root);
+	ret = init_btrfs(root, features);
 	if (ret) {
 		fprintf(stderr, "unable to setup the root tree\n");
 		goto fail;
@@ -2861,15 +2861,15 @@ int main(int argc, char *argv[])
 	int packing = 1;
 	int noxattr = 0;
 	int datacsum = 1;
-	u32 nodesize = max_t(u32, sysconf(_SC_PAGESIZE),
-			BTRFS_MKFS_DEFAULT_NODE_SIZE);
+	u32 nodesize = sysconf(_SC_PAGESIZE);
 	int rollback = 0;
 	int copylabel = 0;
 	int usage_error = 0;
 	int progress = 1;
 	char *file;
 	char fslabel[BTRFS_LABEL_SIZE];
-	u64 features = BTRFS_MKFS_DEFAULT_FEATURES;
+	u64 features = BTRFS_MKFS_DEFAULT_FEATURES |
+		       BTRFS_CONVERT_FORCE_FEATURES;
 
 	while(1) {
 		enum { GETOPT_VAL_NO_PROGRESS = 256 };
@@ -2937,22 +2937,6 @@ int main(int argc, char *argv[])
 					exit(1);
 				}
 				free(orig);
-				if (features & BTRFS_FEATURE_LIST_ALL) {
-					btrfs_list_all_fs_features(
-						~BTRFS_CONVERT_ALLOWED_FEATURES);
-					exit(0);
-				}
-				if (features & ~BTRFS_CONVERT_ALLOWED_FEATURES) {
-					char buf[64];
-
-					btrfs_parse_features_to_string(buf,
-						features & ~BTRFS_CONVERT_ALLOWED_FEATURES);
-					fprintf(stderr,
-						"ERROR: features not allowed for convert: %s\n",
-						buf);
-					exit(1);
-				}
-
 				break;
 				}
 			case GETOPT_VAL_NO_PROGRESS:
@@ -2964,6 +2948,12 @@ int main(int argc, char *argv[])
 				return c != GETOPT_VAL_HELP;
 		}
 	}
+	if (features & BTRFS_FEATURE_LIST_ALL) {
+		btrfs_list_all_fs_features(
+			~BTRFS_CONVERT_ALLOWED_FEATURES);
+		exit(0);
+	}
+
 	argc = argc - optind;
 	set_argv0(argv);
 	if (check_argc_exact(argc, 1)) {
@@ -2971,6 +2961,27 @@ int main(int argc, char *argv[])
 		return 1;
 	}
 
+	if (features & ~BTRFS_CONVERT_ALLOWED_FEATURES) {
+		char buf[64];
+
+		btrfs_parse_features_to_string(buf,
+			features & ~BTRFS_CONVERT_ALLOWED_FEATURES);
+		fprintf(stderr,
+			"ERROR: features not allowed for convert: %s\n",
+			buf);
+		exit(1);
+	}
+	if (!(features & BTRFS_CONVERT_FORCE_FEATURES)) {
+		char buf[64];
+
+		btrfs_parse_features_to_string(buf,
+				BTRFS_CONVERT_FORCE_FEATURES);
+		fprintf(stderr,
+			"ERROR: features %s must be enabled for convert\n",
+			buf);
+		exit(1);
+	}
+
 	if (rollback && (!datacsum || noxattr || !packing)) {
 		fprintf(stderr,
 			"Usage error: -d, -i, -n options do not apply to rollback\n");
diff --git a/ctree.h b/ctree.h
index bcad2b9..db39da7 100644
--- a/ctree.h
+++ b/ctree.h
@@ -2269,7 +2269,7 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
 			   u64 type, u64 chunk_objectid, u64 chunk_offset,
 			   u64 size);
 int btrfs_make_block_groups(struct btrfs_trans_handle *trans,
-			    struct btrfs_root *root);
+			    struct btrfs_root *root, u64 features);
 int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *root, u64 bytenr, u64 num,
 			     int alloc, int mark_free);
diff --git a/extent-tree.c b/extent-tree.c
index 0c8152a..6c0960d 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -3373,7 +3373,7 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
  * before doing any block allocation.
  */
 int btrfs_make_block_groups(struct btrfs_trans_handle *trans,
-			    struct btrfs_root *root)
+			    struct btrfs_root *root, u64 features)
 {
 	u64 total_bytes;
 	u64 cur_start;
@@ -3406,7 +3406,20 @@ int btrfs_make_block_groups(struct btrfs_trans_handle *trans,
 			group_size &= ~(group_align - 1);
 			group_size = max_t(u64, group_size, 8 * 1024 * 1024);
 			group_size = min_t(u64, group_size, 32 * 1024 * 1024);
+		} else if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS) {
+			/* mixed block group case */
+
+			if ((total_bytes - cur_start) * 4 < group_size * 5)
+				group_size = total_bytes - cur_start;
+			group_size = round_down(group_size, group_align);
+			group_type = BTRFS_BLOCK_GROUP_METADATA |
+				     BTRFS_BLOCK_GROUP_DATA;
+			group_size = min_t(u64, group_size,
+					   1ULL * 1024 * 1024 * 1024);
+			total_metadata += group_size;
+			total_data += group_size;
 		} else {
+			/* no mixed block group case, not used yet */
 			group_size &= ~(group_align - 1);
 			if (total_data >= total_metadata * 2) {
 				group_type = BTRFS_BLOCK_GROUP_METADATA;
diff --git a/utils.h b/utils.h
index 0eadaf1..7f1a128 100644
--- a/utils.h
+++ b/utils.h
@@ -31,7 +31,7 @@
 		| BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA)
 
 /*
- * Avoid multi-device features (RAID56) and mixed block groups
+ * Avoid multi-device features (RAID56)
  */
 #define BTRFS_CONVERT_ALLOWED_FEATURES				\
 	(BTRFS_FEATURE_INCOMPAT_MIXED_BACKREF			\
@@ -41,7 +41,19 @@
 	| BTRFS_FEATURE_INCOMPAT_BIG_METADATA			\
 	| BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF			\
 	| BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA		\
-	| BTRFS_FEATURE_INCOMPAT_NO_HOLES)
+	| BTRFS_FEATURE_INCOMPAT_NO_HOLES			\
+	| BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
+
+/*
+ * Due to bugs in btrfs-convert, it will put data and metadata
+ * extents into the same chunk, even we disallow mixed block group.
+ *
+ * Also, ext* is not as extent based as btrfs, even if we find a method
+ * to fix above bug, it will cause chunks to be very scattered.
+ * So force mixed block group for now.
+ */
+#define BTRFS_CONVERT_FORCE_FEATURES				\
+	(BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
 
 #define BTRFS_FEATURE_LIST_ALL		(1ULL << 63)
 
-- 
2.5.1

[PATCH v2 4/5] btrfs-progs: util: add parameter for btrfs_list_all_fs_features

[Qu Wenruo]

Even we tried to make mkfs and btrfs-convert use the same features, but
previous patch forced btrfs-convert to use mix-bg feature.

So the default fs features is different for mkfs and btrfs-convert, add
mask_default parameter for btrfs_list_all_fs_features() to handle the
difference.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-convert.c | 4 +++-
 mkfs.c          | 3 ++-
 utils.c         | 4 ++--
 utils.h         | 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index a60f380..e730e4b 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2950,7 +2950,9 @@ int main(int argc, char *argv[])
 	}
 	if (features & BTRFS_FEATURE_LIST_ALL) {
 		btrfs_list_all_fs_features(
-			~BTRFS_CONVERT_ALLOWED_FEATURES);
+			~BTRFS_CONVERT_ALLOWED_FEATURES,
+			BTRFS_MKFS_DEFAULT_FEATURES |
+			BTRFS_CONVERT_FORCE_FEATURES);
 		exit(0);
 	}
 
diff --git a/mkfs.c b/mkfs.c
index b8879fc..56ce381 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1430,7 +1430,8 @@ int main(int ac, char **av)
 				}
 				free(orig);
 				if (features & BTRFS_FEATURE_LIST_ALL) {
-					btrfs_list_all_fs_features(0);
+					btrfs_list_all_fs_features(0,
+						BTRFS_MKFS_DEFAULT_FEATURES);
 					exit(0);
 				}
 				break;
diff --git a/utils.c b/utils.c
index c0d1afa..78ba67b 100644
--- a/utils.c
+++ b/utils.c
@@ -633,7 +633,7 @@ void btrfs_process_fs_features(u64 flags)
 	}
 }
 
-void btrfs_list_all_fs_features(u64 mask_disallowed)
+void btrfs_list_all_fs_features(u64 mask_disallowed, u64 mask_default)
 {
 	int i;
 
@@ -643,7 +643,7 @@ void btrfs_list_all_fs_features(u64 mask_disallowed)
 
 		if (mkfs_features[i].flag & mask_disallowed)
 			continue;
-		if (mkfs_features[i].flag & BTRFS_MKFS_DEFAULT_FEATURES)
+		if (mkfs_features[i].flag & mask_default)
 			is_default = ", default";
 		fprintf(stderr, "%-20s- %s (0x%llx%s)\n",
 				mkfs_features[i].name,
diff --git a/utils.h b/utils.h
index 7f1a128..f8fd745 100644
--- a/utils.h
+++ b/utils.h
@@ -112,7 +112,7 @@ void set_argv0(char **argv);
 void units_set_mode(unsigned *units, unsigned mode);
 void units_set_base(unsigned *units, unsigned base);
 
-void btrfs_list_all_fs_features(u64 mask_disallowed);
+void btrfs_list_all_fs_features(u64 mask_disallowed, u64 mask_default);
 char* btrfs_parse_fs_features(char *namelist, u64 *flags);
 void btrfs_process_fs_features(u64 flags);
 void btrfs_parse_features_to_string(char *buf, u64 flags);
-- 
2.5.1

[PATCH v2 5/5] btrfs-progs: convert-test: Disable different nodesize test

[Qu Wenruo]

As now btrfs-convert is forced to used mixed block group, only test
pagesize as nodesize for convert.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/convert-tests.sh | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/tests/convert-tests.sh b/tests/convert-tests.sh
index 68c03dc..f6f4679 100755
--- a/tests/convert-tests.sh
+++ b/tests/convert-tests.sh
@@ -57,19 +57,25 @@ if ! [ -z "$TEST" ]; then
 fi
 
 for feature in '' 'extref' 'skinny-metadata' 'no-holes'; do
-	convert_test "$feature" "ext2 4k nodesize" 4096 mke2fs -b 4096
-	convert_test "$feature" "ext3 4k nodesize" 4096 mke2fs -j -b 4096
-	convert_test "$feature" "ext4 4k nodesize" 4096 mke2fs -t ext4 -b 4096
-	convert_test "$feature" "ext2 8k nodesize" 8192 mke2fs -b 4096
-	convert_test "$feature" "ext3 8k nodesize" 8192 mke2fs -j -b 4096
-	convert_test "$feature" "ext4 8k nodesize" 8192 mke2fs -t ext4 -b 4096
-	convert_test "$feature" "ext2 16k nodesize" 16384 mke2fs -b 4096
-	convert_test "$feature" "ext3 16k nodesize" 16384 mke2fs -j -b 4096
-	convert_test "$feature" "ext4 16k nodesize" 16384 mke2fs -t ext4 -b 4096
-	convert_test "$feature" "ext2 32k nodesize" 32768 mke2fs -b 4096
-	convert_test "$feature" "ext3 32k nodesize" 32768 mke2fs -j -b 4096
-	convert_test "$feature" "ext4 32k nodesize" 32768 mke2fs -t ext4 -b 4096
-	convert_test "$feature" "ext2 64k nodesize" 65536 mke2fs -b 4096
-	convert_test "$feature" "ext3 64k nodesize" 65536 mke2fs -j -b 4096
-	convert_test "$feature" "ext4 64k nodesize" 65536 mke2fs -t ext4 -b 4096
+	# Btrfs-convert is forced to use mixed-bg, so must use pagesize
+	# for nodesize right now
+	nodesize=`getconf PAGESIZE`
+	convert_test "$feature" "ext2 4k nodesize" $nodesize mke2fs -b 4096
+	convert_test "$feature" "ext3 4k nodesize" $nodesize mke2fs -b 4096
+	convert_test "$feature" "ext4 4k nodesize" $nodesize mke2fs -b 4096
+#	convert_test "$feature" "ext2 4k nodesize" 4096 mke2fs -b 4096
+#	convert_test "$feature" "ext3 4k nodesize" 4096 mke2fs -j -b 4096
+#	convert_test "$feature" "ext4 4k nodesize" 4096 mke2fs -t ext4 -b 4096
+#	convert_test "$feature" "ext2 8k nodesize" 8192 mke2fs -b 4096
+#	convert_test "$feature" "ext3 8k nodesize" 8192 mke2fs -j -b 4096
+#	convert_test "$feature" "ext4 8k nodesize" 8192 mke2fs -t ext4 -b 4096
+#	convert_test "$feature" "ext2 16k nodesize" 16384 mke2fs -b 4096
+#	convert_test "$feature" "ext3 16k nodesize" 16384 mke2fs -j -b 4096
+#	convert_test "$feature" "ext4 16k nodesize" 16384 mke2fs -t ext4 -b 4096
+#	convert_test "$feature" "ext2 32k nodesize" 32768 mke2fs -b 4096
+#	convert_test "$feature" "ext3 32k nodesize" 32768 mke2fs -j -b 4096
+#	convert_test "$feature" "ext4 32k nodesize" 32768 mke2fs -t ext4 -b 4096
+#	convert_test "$feature" "ext2 64k nodesize" 65536 mke2fs -b 4096
+#	convert_test "$feature" "ext3 64k nodesize" 65536 mke2fs -j -b 4096
+#	convert_test "$feature" "ext4 64k nodesize" 65536 mke2fs -t ext4 -b 4096
 done
-- 
2.5.1

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[David Sterba]

On Thu, Sep 10, 2015 at 10:34:13AM +0800, Qu Wenruo wrote:
> Again the buggy btrfs-convert, even David tried to ban mixed-bg features
> for btrfs-convert, it will still put data and metadata extents into the
> same chunk, without marking the chunk mixed.
> 
> So in the patchset, first add fsck support for such problem, and then
> force btrfs-convert to use mixed block group.

I don't think this is a good option for now. People convert
many-terabytes filesystems. Unless there's a way how to convert such
filesystem to the split data/metadata type I don't want to force mixed
bg to convert. The bug you describe is there, but I wonder why didn't
we notice problems that arise from it.

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[Qu Wenruo]

在 2015年09月11日 22:56, David Sterba 写道:
> On Thu, Sep 10, 2015 at 10:34:13AM +0800, Qu Wenruo wrote:
>> Again the buggy btrfs-convert, even David tried to ban mixed-bg features
>> for btrfs-convert, it will still put data and metadata extents into the
>> same chunk, without marking the chunk mixed.
>>
>> So in the patchset, first add fsck support for such problem, and then
>> force btrfs-convert to use mixed block group.
>
> I don't think this is a good option for now.

I'm OK with the decision not to force mixed bg right now.
As you mentioned, it should be better to do it until we fix the whole 
problem.

> People convert
> many-terabytes filesystems.Unless there's a way how to convert such
> filesystem to the split data/metadata type I don't want to force mixed
> bg to convert.

But even for case of TB convert, I'm not sure if that will be a good 
idea to use sperate data/metadata chunk.
Even for ext4, I'm not sure if it does store extents/metadata in a good 
manaer.
IIRC, ext* will allocate space from the middle of free space to avoid 
fragment. (I'm not familiar with ext* anyway, so I can be totally wrong)
So ext* may cause a lot of small holes in its free space.

And for convert, all ext* data and metadata must be covered by btrfs 
DATA chunk, and then restore btrfs metadata into the resting space.
Either causing tons of small metadata chunks between scatterd data 
chunks, or almost no space left for metadata.

So IMHO, for converted case, mixed bg would be a quite good and generic 
choice.

> The bug you describe is there, but I wonder why didn't
> we notice problems that arise from it.

Personally speaking, COW nature of btrfs is doing a quite good job to 
hide the bug, and even self healing.

For metadata in non-mixed DATA chunk, for read case, kernel won't detect 
anything wrong as long as it can pass the generation/csum/backref check.

For writting metadata in non-mixed DATA chunk, COW will alloc new tree 
block from METADATA chunk.
And if we have enough metadata operation, the problem will just 
disappear after all metadata is COWed.

Only some corner case will trigger a WARNING or BUG.

We can add some extra check in check_tree_block() to check such case, 
but I think it will bring a bad impact on performance.

Thanks,
Qu

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

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[Qu Wenruo]

Hi David,

After some ext* lecture given by my teammate, Wang Xiaoguang, I'm more 
convinced that, at least for convert from ext*, separate chunk type will 
not be a good idea.

[[Ext* file system struct]]

Unlike pure extent based file, ext* is block group based.
0		128M		256M		384M	...
| Group 0       | Group 1	| Group 2	|       ...

And in each group, the beginning part is always taken by its metadata.

So even for a empty group, it will have some used space:
For Group 1:
128M   128+4M				256M
|\\\\\\|				|

Fro Group 0, it has journal taken more space at beginning, for about 30+M.

So even for newly created ext4 fs, its available space map will be like 
the fowlloing:
0		128M		256M		384M	...
|\\\\|		|\\|		|\\|		|\\|	...

For separate chunk type, we must ensure all above used space be covered 
by DATA chunk.
On the other hand, we must alloc other METADATA/DATA chunk for btrfs 
metadata/data.

So the bad news is, if we follow that behavior, we will end up 
allocating at most 100+M metadata chunk.

For above almost empty ext4 case, it will cause less problem, as
we can batch several groups and put a large data chunk to cover them,
then allocate a 100+M metadata chunk.

0		128M		256M		384M	...
|\\\\|		|\\|		|\\|		|\\|	...
|<-----------DATA------------------->|<--META-->|
     (256 + 16)			       (128-16)

But if the filesystem is used and most group only has scattered 
available space, we may ended up unable to alloc any metadata chunk.

This will make the usage of limited block group quite limited.
Although I'll continue add such support for btrfs-convert, I'm quite 
concerned about the usage...

Thanks,
Qu

David Sterba wrote on 2015/09/11 16:56 +0200:
> On Thu, Sep 10, 2015 at 10:34:13AM +0800, Qu Wenruo wrote:
>> Again the buggy btrfs-convert, even David tried to ban mixed-bg features
>> for btrfs-convert, it will still put data and metadata extents into the
>> same chunk, without marking the chunk mixed.
>>
>> So in the patchset, first add fsck support for such problem, and then
>> force btrfs-convert to use mixed block group.
>
> I don't think this is a good option for now. People convert
> many-terabytes filesystems. Unless there's a way how to convert such
> filesystem to the split data/metadata type I don't want to force mixed
> bg to convert. The bug you describe is there, but I wonder why didn't
> we notice problems that arise from it.
>

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[David Sterba]

On Thu, Sep 24, 2015 at 09:18:51AM +0800, Qu Wenruo wrote:
> After some ext* lecture given by my teammate, Wang Xiaoguang, I'm more 
> convinced that, at least for convert from ext*, separate chunk type will 
> not be a good idea.

Thanks for the additional research.

> For above almost empty ext4 case, it will cause less problem, as
> we can batch several groups and put a large data chunk to cover them,
> then allocate a 100+M metadata chunk.

> But if the filesystem is used and most group only has scattered 
> available space, we may ended up unable to alloc any metadata chunk.

I think this is the price we pay for the in-place conversion, we can
safely use only the free space and respect the constraints it gives.
As long as the ext2_subvol exists, ie. the original data and metadata
are pinned, we don't have much choice. Rebalancing is possible but given
the remaining space may fail to allocate block groups of desired size.

> This will make the usage of limited block group quite limited.
> Although I'll continue add such support for btrfs-convert, I'm quite 
> concerned about the usage...

So can we do it like this:

1) enable support for mixed bg in convert
2) implement mixed -> split conversion in balance
3) force convert to do mixed bgs by default

The conversion from/to mixed would be good on it's own but may not be
trivial to implement. If the main concern about result of conversion is
bad bg layout, I'd say that we rely on balance to reshuffle the bgs to
make the filesystem more up to the desired layout.

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[Qu Wenruo]

在 2015年09月25日 23:19, David Sterba 写道:
> On Thu, Sep 24, 2015 at 09:18:51AM +0800, Qu Wenruo wrote:
>> After some ext* lecture given by my teammate, Wang Xiaoguang, I'm more
>> convinced that, at least for convert from ext*, separate chunk type will
>> not be a good idea.
>
> Thanks for the additional research.
>
>> For above almost empty ext4 case, it will cause less problem, as
>> we can batch several groups and put a large data chunk to cover them,
>> then allocate a 100+M metadata chunk.
>
>> But if the filesystem is used and most group only has scattered
>> available space, we may ended up unable to alloc any metadata chunk.
>
> I think this is the price we pay for the in-place conversion, we can
> safely use only the free space and respect the constraints it gives.
> As long as the ext2_subvol exists, ie. the original data and metadata
> are pinned, we don't have much choice. Rebalancing is possible but given
> the remaining space may fail to allocate block groups of desired size.
>
>> This will make the usage of limited block group quite limited.
>> Although I'll continue add such support for btrfs-convert, I'm quite
>> concerned about the usage...
>
> So can we do it like this:
>
> 1) enable support for mixed bg in convert
> 2) implement mixed -> split conversion in balance
> 3) force convert to do mixed bgs by default
>
> The conversion from/to mixed would be good on it's own but may not be
> trivial to implement.

Agreed, and consider it is kernel code, it will be much harder to 
code/debug.

> If the main concern about result of conversion is
> bad bg layout, I'd say that we rely on balance to reshuffle the bgs to
> make the filesystem more up to the desired layout.

That's OK.
At least we don't need to implement kernel convert support for mixed bg.

I'll implement both mixed bg and separate one and allow user to choose.

And for extreme case, if we can't allocate any metadata chunk larger 
than 16M, either info user and exit or do automatic mixed-bg conversion 
if no conflicting options.

Thanks,
Qu

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

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[David Sterba]

On Sat, Sep 26, 2015 at 03:56:42PM +0800, Qu Wenruo wrote:
> > So can we do it like this:
> >
> > 1) enable support for mixed bg in convert
> > 2) implement mixed -> split conversion in balance
> > 3) force convert to do mixed bgs by default
> >
> > The conversion from/to mixed would be good on it's own but may not be
> > trivial to implement.
> 
> Agreed, and consider it is kernel code, it will be much harder to 
> code/debug.

I've looked into that some time ago. The relocation code seems ready for
that, the data and metadata blocks are filtered separately from the
given chunk and moved to the new chunk. We need to propagate the request
to convert from/to mixed-bg through the balance filters.

> > If the main concern about result of conversion is
> > bad bg layout, I'd say that we rely on balance to reshuffle the bgs to
> > make the filesystem more up to the desired layout.
> 
> That's OK.
> At least we don't need to implement kernel convert support for mixed bg.
> 
> I'll implement both mixed bg and separate one and allow user to choose.
> 
> And for extreme case, if we can't allocate any metadata chunk larger 
> than 16M, either info user and exit or do automatic mixed-bg conversion 
> if no conflicting options.

We can possibly scan the image first for the available holes and then
decide if it's feasible to do the conversion with the requested fs features.

Re: [PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[David Sterba]

On Thu, Sep 10, 2015 at 10:34:13AM +0800, Qu Wenruo wrote:
>   btrfs-progs: fsck: Add check for extent and parent chunk type
>   btrfs-progs: utils: Check nodesize against features

Applied the two, thanks.

>   btrfs-progs: convert: force convert to used mixed block group
>   btrfs-progs: util: add parameter for btrfs_list_all_fs_features
>   btrfs-progs: convert-test: Disable different nodesize test

[PATCH 0/4] Fix for btrfs-convert chunk type and fsck support

[Qu Wenruo]

Again the buggy btrfs-convert, even David tried to ban mixed-bg features
for btrfs-convert, it will still put data and metadata extents into the
same chunk, without marking the chunk mixed.

So in the patchset, first add fsck support for such problem, and then
force btrfs-convert to use mixed block group.

Lastly change the test case as they won't convert with non pagesize
nodesize.

Qu Wenruo (5):
  btrfs-progs: fsck: Add check for extent and parent chunk type
  btrfs: utils: Check nodesize against features
  btrfs: convert: force convert to used mixed block group
  btrfs: util: add parameter for btrfs_list_all_fs_features
  btrfs-progs: convert-test: Disable different nodesize test

 btrfs-convert.c        | 59 ++++++++++++++++++++++++++-----------------
 cmds-check.c           | 68 +++++++++++++++++++++++++++++++++++++++++++++++++-
 ctree.h                |  2 +-
 extent-tree.c          | 15 ++++++++++-
 mkfs.c                 | 19 ++++++--------
 tests/convert-tests.sh | 36 +++++++++++++++-----------
 utils.c                | 12 ++++++---
 utils.h                | 20 ++++++++++++---
 8 files changed, 171 insertions(+), 60 deletions(-)

-- 
2.5.1