btrfs-progs: mkfs: Prevent temporary system chunk to use space in reserved 1M range

PATCH In review Qu Wenruo 4 participants 8 messages Jan 10, 2018 – Jan 24, 2018 Btrfs file system

Share & tools

Qu Wenruo

author

2018-01-10 04:57 UTC #0 lore raw

PATCH 1/2 btrfs-progs: mkfs: Prevent temporary system chunk to use space in reserved 1M range

When creating btrfs, mkfs.btrfs will firstly create a temporary system chunk as basis, and then created needed trees or new devices.

However the layout temporary system chunk is hard-coded and uses reserved [0, 1M) range of devid 1.

Change the temporary chunk layout from old:

0 1M 4M 5M
|<----------- temp chunk -------------->|
And it's 1:1 mapped, which means it's a SINGLE chunk, and stripe offset is also 0.

to new layout:

0 1M 4M 5M

	|<----------- temp chunk -------------->|

And still keeps the 1:1 mapping.

The problem can only be exposed by "-m single" or "-M" where we reuse the temporary chunk.

With other meta profiles, system and meta chunks are allocated by later btrfs_alloc_chunk() call, and old SINGLE chunks are removed, so it will be no such problem for other meta profiles.

Reported-by Nikolay Borisov <[email protected]>

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

---
 mkfs/common.c | 29 +++++++++++++++++++++++------
 mkfs/main.c   |  7 ++++++-
 2 files changed, 29 insertions(+), 7 deletions(-)

mkfs/common.c +23 -6

index dd5e7ecff479..5c5e9c3b9e01 100644
@@ -100,6 +100,21 @@ static int btrfs_create_tree_root(int fd, struct btrfs_mkfs_config *cfg,
  *
  * The superblock signature is not valid, denotes a partially created
  * filesystem, needs to be finalized.
+ *
+ * The temporary fs will have the following chunk layout:
+ * Device extent:
+ * 0		1M				5M	......
+ * | Reserved	| dev extent for SYS chunk      |
+ *
+ * And chunk mapping will be:
+ * Chunk mapping:
+ * 0		1M				5M
+ * |		| System chunk, 1:1 mapped	|
+ *
+ * That's to say, there will only be *ONE* system chunk, mapped to
+ * [1M, 5M) physical offset.
+ * And the only chunk is also in logical address [1M, 5M), containing
+ * all essential tree blocks.
  */
 int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 {

@@ -154,8 +169,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	cfg->blocks[MKFS_SUPER_BLOCK] = BTRFS_SUPER_INFO_OFFSET;
 	for (i = 1; i < MKFS_BLOCK_COUNT; i++) {
-		cfg->blocks[i] = BTRFS_SUPER_INFO_OFFSET + SZ_1M +
-			cfg->nodesize * i;
+		cfg->blocks[i] = BTRFS_BLOCK_RESERVED_1M_FOR_SUPER +
+			cfg->nodesize * (i - 1);
 	}
 
 	btrfs_set_super_bytenr(&super, cfg->blocks[MKFS_SUPER_BLOCK]);

@@ -309,7 +324,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	/* then we have chunk 0 */
 	btrfs_set_disk_key_objectid(&disk_key, BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_CHUNK_ITEM_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -325,7 +340,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 	btrfs_set_chunk_sector_size(buf, chunk, cfg->sectorsize);
 	btrfs_set_chunk_num_stripes(buf, chunk, 1);
 	btrfs_set_stripe_devid_nr(buf, chunk, 0, 1);
-	btrfs_set_stripe_offset_nr(buf, chunk, 0, 0);
+	btrfs_set_stripe_offset_nr(buf, chunk, 0,
+				   BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	nritems++;
 
 	write_extent_buffer(buf, super.dev_item.uuid,

@@ -363,7 +379,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 		sizeof(struct btrfs_dev_extent);
 
 	btrfs_set_disk_key_objectid(&disk_key, 1);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_DEV_EXTENT_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -374,7 +390,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 					BTRFS_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_objectid(buf, dev_extent,
 					BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_dev_extent_chunk_offset(buf, dev_extent, 0);
+	btrfs_set_dev_extent_chunk_offset(buf, dev_extent,
+					  BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 
 	write_extent_buffer(buf, chunk_tree_uuid,
 		    (unsigned long)btrfs_dev_extent_chunk_tree_uuid(dev_extent),

mkfs/main.c +6 -1

index d817ad8dfd1a..8e3d19acb6f2 100644
@@ -81,10 +81,15 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
 	bytes_used = btrfs_super_bytes_used(fs_info->super_copy);
 
 	root->fs_info->system_allocs = 1;
+	/*
+	 * First temporary system chunk must match the chunk layout
+	 * created in make_btrfs().
+	 */
 	ret = btrfs_make_block_group(trans, fs_info, bytes_used,
 				     BTRFS_BLOCK_GROUP_SYSTEM,
 				     BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-				     0, BTRFS_MKFS_SYSTEM_GROUP_SIZE);
+				     BTRFS_BLOCK_RESERVED_1M_FOR_SUPER,
+				     BTRFS_MKFS_SYSTEM_GROUP_SIZE);
 	allocation->system += BTRFS_MKFS_SYSTEM_GROUP_SIZE;
 	if (ret)
 		return ret;

Signature · 1 line

2.15.1

Qu Wenruo

author

2018-01-10 04:57 UTC #1 lore raw

PATCH 2/2 btrfs-progs: mkfs-tests: Add test case to check if the first device extent is occupying reserved 0~1M range

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

---
 .../mkfs-tests/010-reserved-1M-for-single/test.sh  | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100755 tests/mkfs-tests/010-reserved-1M-for-single/test.sh

tests/mkfs-tests/010-reserved-1M-for-single/test.sh new file +37 -0

new file mode 100755
index 000000000000..1a4936eaf72e
@@ -0,0 +1,37 @@
+#!/bin/bash
+# Test if "-m single" or "-M" can cause dev extent to use reserved 1M range
+#
+# Other profiles will cause mkfs.btrfs to allocate new meta/sys chunks
+# using btrfs_alloc_chunk() which won't use 0~1M range, so other profiles
+# are safe.
+
+source "$TOP/tests/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+prepare_test_dev
+
+do_one_test ()
+{
+	run_check "$TOP/mkfs.btrfs" -f $@ "$TEST_DEV"
+
+	# Use dev-extent tree to find first device extent
+	first_dev_extent=$(run_check_stdout "$TOP/btrfs" inspect-internal \
+		dump-tree -t device "$TEST_DEV" | \
+		grep -oP '(?<=DEV_EXTENT )[[:digit:]]*' | head -n1)
+
+	if [ -z $first_dev_extent ]; then
+		_fail "Failed to get first device extent"
+	fi
+
+	echo "First dev extent starts at $first_dev_extent" >> "$RESULTS"
+	echo "Reserved range is [0, $(( 1024 * 1024)))" >> "$RESULTS"
+	# First device extent should not start below 1M
+	if [ $first_dev_extent -lt $(( 1024 * 1024 )) ]; then
+		_fail "First device extent occupies reserved 0~1M range"
+	fi
+}
+
+do_one_test "-M"
+do_one_test "-m single"

Signature · 1 line

2.15.1

Nikolay Borisov

Reviewed-by on v1 2/2

2018-01-10 08:57 UTC #2 lore raw

Re: [PATCH 2/2] btrfs-progs: mkfs-tests: Add test case to check if the first device extent is occupying reserved 0~1M range

Quoted 1 line · Qu Wenruo

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

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 49 lines

---
 .../mkfs-tests/010-reserved-1M-for-single/test.sh  | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100755 tests/mkfs-tests/010-reserved-1M-for-single/test.sh

tests/mkfs-tests/010-reserved-1M-for-single/test.sh new file +37 -0

new file mode 100755
index 000000000000..1a4936eaf72e
@@ -0,0 +1,37 @@
+#!/bin/bash
+# Test if "-m single" or "-M" can cause dev extent to use reserved 1M range
+#
+# Other profiles will cause mkfs.btrfs to allocate new meta/sys chunks
+# using btrfs_alloc_chunk() which won't use 0~1M range, so other profiles
+# are safe.
+
+source "$TOP/tests/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+prepare_test_dev
+
+do_one_test ()
+{
+	run_check "$TOP/mkfs.btrfs" -f $@ "$TEST_DEV"
+
+	# Use dev-extent tree to find first device extent
+	first_dev_extent=$(run_check_stdout "$TOP/btrfs" inspect-internal \
+		dump-tree -t device "$TEST_DEV" | \
+		grep -oP '(?<=DEV_EXTENT )[[:digit:]]*' | head -n1)
+
+	if [ -z $first_dev_extent ]; then
+		_fail "Failed to get first device extent"
+	fi
+
+	echo "First dev extent starts at $first_dev_extent" >> "$RESULTS"
+	echo "Reserved range is [0, $(( 1024 * 1024)))" >> "$RESULTS"
+	# First device extent should not start below 1M
+	if [ $first_dev_extent -lt $(( 1024 * 1024 )) ]; then
+		_fail "First device extent occupies reserved 0~1M range"
+	fi
+}
+
+do_one_test "-M"
+do_one_test "-m single"

Nikolay Borisov

Tested-by Reviewed-by on v1 1/2

2018-01-10 08:37 UTC #3 lore raw

In reply to Qu Wenruo

Quoted 22 lines · Qu Wenruo

When creating btrfs, mkfs.btrfs will firstly create a temporary system chunk as basis, and then created needed trees or new devices.
However the layout temporary system chunk is hard-coded and uses reserved [0, 1M) range of devid 1.
Change the temporary chunk layout from old:
0 1M 4M 5M
|<----------- temp chunk -------------->|
And it's 1:1 mapped, which means it's a SINGLE chunk, and stripe offset is also 0.
to new layout:
0 1M 4M 5M
	|<----------- temp chunk -------------->|
And still keeps the 1:1 mapping.
The problem can only be exposed by "-m single" or "-M" where we reuse the temporary chunk.

Quoted 6 lines

With other meta profiles, system and meta chunks are allocated by later btrfs_alloc_chunk() call, and old SINGLE chunks are removed, so it will be no such problem for other meta profiles.
Reported-by Nikolay Borisov <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

Tested-by Nikolay Borisov <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

After creating in -M or in -m single I can see:

item 0 key (1 DEV_EXTENT 1048576) itemoff 3947 itemsize 48 and
item 1 key (1048576 BLOCK_GROUP_ITEM 4194304) itemoff 3938 itemsize 24
block group used 4096 chunk_objectid 256 flags SYSTEM

Quoted 103 lines

---

 mkfs/common.c | 29 +++++++++++++++++++++++------
 mkfs/main.c   |  7 ++++++-
 2 files changed, 29 insertions(+), 7 deletions(-)

mkfs/common.c +23 -6

index dd5e7ecff479..5c5e9c3b9e01 100644
@@ -100,6 +100,21 @@ static int btrfs_create_tree_root(int fd, struct btrfs_mkfs_config *cfg,
  *
  * The superblock signature is not valid, denotes a partially created
  * filesystem, needs to be finalized.
+ *
+ * The temporary fs will have the following chunk layout:
+ * Device extent:
+ * 0		1M				5M	......
+ * | Reserved	| dev extent for SYS chunk      |
+ *
+ * And chunk mapping will be:
+ * Chunk mapping:
+ * 0		1M				5M
+ * |		| System chunk, 1:1 mapped	|
+ *
+ * That's to say, there will only be *ONE* system chunk, mapped to
+ * [1M, 5M) physical offset.
+ * And the only chunk is also in logical address [1M, 5M), containing
+ * all essential tree blocks.
  */
 int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 {

@@ -154,8 +169,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	cfg->blocks[MKFS_SUPER_BLOCK] = BTRFS_SUPER_INFO_OFFSET;
 	for (i = 1; i < MKFS_BLOCK_COUNT; i++) {
-		cfg->blocks[i] = BTRFS_SUPER_INFO_OFFSET + SZ_1M +
-			cfg->nodesize * i;
+		cfg->blocks[i] = BTRFS_BLOCK_RESERVED_1M_FOR_SUPER +
+			cfg->nodesize * (i - 1);
 	}
 
 	btrfs_set_super_bytenr(&super, cfg->blocks[MKFS_SUPER_BLOCK]);

@@ -309,7 +324,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	/* then we have chunk 0 */
 	btrfs_set_disk_key_objectid(&disk_key, BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_CHUNK_ITEM_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -325,7 +340,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 	btrfs_set_chunk_sector_size(buf, chunk, cfg->sectorsize);
 	btrfs_set_chunk_num_stripes(buf, chunk, 1);
 	btrfs_set_stripe_devid_nr(buf, chunk, 0, 1);
-	btrfs_set_stripe_offset_nr(buf, chunk, 0, 0);
+	btrfs_set_stripe_offset_nr(buf, chunk, 0,
+				   BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	nritems++;
 
 	write_extent_buffer(buf, super.dev_item.uuid,

@@ -363,7 +379,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 		sizeof(struct btrfs_dev_extent);
 
 	btrfs_set_disk_key_objectid(&disk_key, 1);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_DEV_EXTENT_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -374,7 +390,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 					BTRFS_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_objectid(buf, dev_extent,
 					BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_dev_extent_chunk_offset(buf, dev_extent, 0);
+	btrfs_set_dev_extent_chunk_offset(buf, dev_extent,
+					  BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 
 	write_extent_buffer(buf, chunk_tree_uuid,
 		    (unsigned long)btrfs_dev_extent_chunk_tree_uuid(dev_extent),

mkfs/main.c +6 -1

index d817ad8dfd1a..8e3d19acb6f2 100644
@@ -81,10 +81,15 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
 	bytes_used = btrfs_super_bytes_used(fs_info->super_copy);
 
 	root->fs_info->system_allocs = 1;
+	/*
+	 * First temporary system chunk must match the chunk layout
+	 * created in make_btrfs().
+	 */
 	ret = btrfs_make_block_group(trans, fs_info, bytes_used,
 				     BTRFS_BLOCK_GROUP_SYSTEM,
 				     BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-				     0, BTRFS_MKFS_SYSTEM_GROUP_SIZE);
+				     BTRFS_BLOCK_RESERVED_1M_FOR_SUPER,
+				     BTRFS_MKFS_SYSTEM_GROUP_SIZE);
 	allocation->system += BTRFS_MKFS_SYSTEM_GROUP_SIZE;
 	if (ret)
 		return ret;

Nikolay Borisov

2018-01-10 14:14 UTC #4 lore raw

In reply to Qu Wenruo

Quoted 22 lines · Qu Wenruo

When creating btrfs, mkfs.btrfs will firstly create a temporary system chunk as basis, and then created needed trees or new devices.
However the layout temporary system chunk is hard-coded and uses reserved [0, 1M) range of devid 1.
Change the temporary chunk layout from old:
0 1M 4M 5M
|<----------- temp chunk -------------->|
And it's 1:1 mapped, which means it's a SINGLE chunk, and stripe offset is also 0.
to new layout:
0 1M 4M 5M
	|<----------- temp chunk -------------->|
And still keeps the 1:1 mapping.
The problem can only be exposed by "-m single" or "-M" where we reuse the temporary chunk.

Quoted 6 lines

With other meta profiles, system and meta chunks are allocated by later btrfs_alloc_chunk() call, and old SINGLE chunks are removed, so it will be no such problem for other meta profiles.
Reported-by Nikolay Borisov <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

Those changes break existing xfs tests:

btrfs/140 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/140.out.bad)

    --- tests/btrfs/140.out	2017-05-22 13:24:36.116301772 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/140.out.bad	2018-01-10 14:12:09.919034975 +0000
    @@ -1,39 +1,39 @@
     QA output created by 140
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/140.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/140.out.bad'  to see the entire diff)
btrfs/141	 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/141.out.bad)
    --- tests/btrfs/141.out	2017-05-22 13:24:36.117301793 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/141.out.bad	2018-01-10 14:12:10.661035432 +0000
    @@ -1,39 +1,39 @@
     QA output created by 141
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/141.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/141.out.bad'  to see the entire diff)
btrfs/142	 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/142.out.bad)
    --- tests/btrfs/142.out	2017-05-22 13:24:36.117301793 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/142.out.bad	2018-01-10 14:12:11.411035894 +0000
    @@ -1,39 +1,39 @@
     QA output created by 142
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/142.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/142.out.bad'  to see the entire diff)
btrfs/143	 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/143.out.bad)
    --- tests/btrfs/143.out	2017-05-22 13:24:36.117301793 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/143.out.bad	2018-01-10 14:12:12.305036444 +0000
    @@ -1,39 +1,39 @@
     QA output created by 143
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/143.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/143.out.bad'  to see the entire diff)

All of these are offset by a single megabyte

Quoted 102 lines

---
 mkfs/common.c | 29 +++++++++++++++++++++++------
 mkfs/main.c   |  7 ++++++-
 2 files changed, 29 insertions(+), 7 deletions(-)

mkfs/common.c +23 -6

index dd5e7ecff479..5c5e9c3b9e01 100644
@@ -100,6 +100,21 @@ static int btrfs_create_tree_root(int fd, struct btrfs_mkfs_config *cfg,
  *
  * The superblock signature is not valid, denotes a partially created
  * filesystem, needs to be finalized.
+ *
+ * The temporary fs will have the following chunk layout:
+ * Device extent:
+ * 0		1M				5M	......
+ * | Reserved	| dev extent for SYS chunk      |
+ *
+ * And chunk mapping will be:
+ * Chunk mapping:
+ * 0		1M				5M
+ * |		| System chunk, 1:1 mapped	|
+ *
+ * That's to say, there will only be *ONE* system chunk, mapped to
+ * [1M, 5M) physical offset.
+ * And the only chunk is also in logical address [1M, 5M), containing
+ * all essential tree blocks.
  */
 int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 {

@@ -154,8 +169,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	cfg->blocks[MKFS_SUPER_BLOCK] = BTRFS_SUPER_INFO_OFFSET;
 	for (i = 1; i < MKFS_BLOCK_COUNT; i++) {
-		cfg->blocks[i] = BTRFS_SUPER_INFO_OFFSET + SZ_1M +
-			cfg->nodesize * i;
+		cfg->blocks[i] = BTRFS_BLOCK_RESERVED_1M_FOR_SUPER +
+			cfg->nodesize * (i - 1);
 	}
 
 	btrfs_set_super_bytenr(&super, cfg->blocks[MKFS_SUPER_BLOCK]);

@@ -309,7 +324,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	/* then we have chunk 0 */
 	btrfs_set_disk_key_objectid(&disk_key, BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_CHUNK_ITEM_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -325,7 +340,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 	btrfs_set_chunk_sector_size(buf, chunk, cfg->sectorsize);
 	btrfs_set_chunk_num_stripes(buf, chunk, 1);
 	btrfs_set_stripe_devid_nr(buf, chunk, 0, 1);
-	btrfs_set_stripe_offset_nr(buf, chunk, 0, 0);
+	btrfs_set_stripe_offset_nr(buf, chunk, 0,
+				   BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	nritems++;
 
 	write_extent_buffer(buf, super.dev_item.uuid,

@@ -363,7 +379,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 		sizeof(struct btrfs_dev_extent);
 
 	btrfs_set_disk_key_objectid(&disk_key, 1);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_DEV_EXTENT_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -374,7 +390,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 					BTRFS_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_objectid(buf, dev_extent,
 					BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_dev_extent_chunk_offset(buf, dev_extent, 0);
+	btrfs_set_dev_extent_chunk_offset(buf, dev_extent,
+					  BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 
 	write_extent_buffer(buf, chunk_tree_uuid,
 		    (unsigned long)btrfs_dev_extent_chunk_tree_uuid(dev_extent),

mkfs/main.c +6 -1

index d817ad8dfd1a..8e3d19acb6f2 100644
@@ -81,10 +81,15 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
 	bytes_used = btrfs_super_bytes_used(fs_info->super_copy);
 
 	root->fs_info->system_allocs = 1;
+	/*
+	 * First temporary system chunk must match the chunk layout
+	 * created in make_btrfs().
+	 */
 	ret = btrfs_make_block_group(trans, fs_info, bytes_used,
 				     BTRFS_BLOCK_GROUP_SYSTEM,
 				     BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-				     0, BTRFS_MKFS_SYSTEM_GROUP_SIZE);
+				     BTRFS_BLOCK_RESERVED_1M_FOR_SUPER,
+				     BTRFS_MKFS_SYSTEM_GROUP_SIZE);
 	allocation->system += BTRFS_MKFS_SYSTEM_GROUP_SIZE;
 	if (ret)
 		return ret;

Qu Wenruo

2018-01-10 14:28 UTC #5 lore raw

Quoted 29 lines · Nikolay Borisov

Quoted 26 lines · Qu Wenruo
When creating btrfs, mkfs.btrfs will firstly create a temporary system chunk as basis, and then created needed trees or new devices.
However the layout temporary system chunk is hard-coded and uses reserved [0, 1M) range of devid 1.
Change the temporary chunk layout from old:
0 1M 4M 5M
|<----------- temp chunk -------------->|
And it's 1:1 mapped, which means it's a SINGLE chunk, and stripe offset is also 0.
to new layout:
0 1M 4M 5M
	|<----------- temp chunk -------------->|
And still keeps the 1:1 mapping.
The problem can only be exposed by "-m single" or "-M" where we reuse the temporary chunk.
With other meta profiles, system and meta chunks are allocated by later btrfs_alloc_chunk() call, and old SINGLE chunks are removed, so it will be no such problem for other meta profiles.

Quoted 5 lines

Quoted 2 lines
Reported-by Nikolay Borisov <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
Those changes break existing xfs tests:

These tests are manually getting the on-disk data to verify if the repair is done correctly.

And if underlying chunk mapping changed, old hard-coded (or less flex) location will definitely fail.

I'll update these test cases to use a more flex way to get on-disk file extent offset.

Thanks for the report,
Qu

Quoted 164 lines

btrfs/140 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/140.out.bad)

    --- tests/btrfs/140.out	2017-05-22 13:24:36.116301772 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/140.out.bad	2018-01-10 14:12:09.919034975 +0000
    @@ -1,39 +1,39 @@
     QA output created by 140
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/140.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/140.out.bad'  to see the entire diff)
btrfs/141	 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/141.out.bad)
    --- tests/btrfs/141.out	2017-05-22 13:24:36.117301793 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/141.out.bad	2018-01-10 14:12:10.661035432 +0000
    @@ -1,39 +1,39 @@
     QA output created by 141
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/141.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/141.out.bad'  to see the entire diff)
btrfs/142	 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/142.out.bad)
    --- tests/btrfs/142.out	2017-05-22 13:24:36.117301793 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/142.out.bad	2018-01-10 14:12:11.411035894 +0000
    @@ -1,39 +1,39 @@
     QA output created by 142
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/142.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/142.out.bad'  to see the entire diff)
btrfs/143	 - output mismatch (see /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/143.out.bad)
    --- tests/btrfs/143.out	2017-05-22 13:24:36.117301793 +0000
    +++ /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/143.out.bad	2018-01-10 14:12:12.305036444 +0000
    @@ -1,39 +1,39 @@
     QA output created by 143
     wrote 131072/131072 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -wrote 65536/65536 bytes at offset 136708096
    +wrote 65536/65536 bytes at offset 137756672
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -08260000:  aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa  ................
    ...
    (Run 'diff -u tests/btrfs/143.out /root/xfstests-dev/results/ubuntu-virtual/4.15.0-rc5-nbor/2018-01-10-13-41-845986621//btrfs/btrfs/143.out.bad'  to see the entire diff)

All of these are offset by a single megabyte

Quoted 92 lines

---
 mkfs/common.c | 29 +++++++++++++++++++++++------
 mkfs/main.c   |  7 ++++++-
 2 files changed, 29 insertions(+), 7 deletions(-)

mkfs/common.c +23 -6

index dd5e7ecff479..5c5e9c3b9e01 100644
@@ -100,6 +100,21 @@ static int btrfs_create_tree_root(int fd, struct btrfs_mkfs_config *cfg,
  *
  * The superblock signature is not valid, denotes a partially created
  * filesystem, needs to be finalized.
+ *
+ * The temporary fs will have the following chunk layout:
+ * Device extent:
+ * 0		1M				5M	......
+ * | Reserved	| dev extent for SYS chunk      |
+ *
+ * And chunk mapping will be:
+ * Chunk mapping:
+ * 0		1M				5M
+ * |		| System chunk, 1:1 mapped	|
+ *
+ * That's to say, there will only be *ONE* system chunk, mapped to
+ * [1M, 5M) physical offset.
+ * And the only chunk is also in logical address [1M, 5M), containing
+ * all essential tree blocks.
  */
 int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 {

@@ -154,8 +169,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	cfg->blocks[MKFS_SUPER_BLOCK] = BTRFS_SUPER_INFO_OFFSET;
 	for (i = 1; i < MKFS_BLOCK_COUNT; i++) {
-		cfg->blocks[i] = BTRFS_SUPER_INFO_OFFSET + SZ_1M +
-			cfg->nodesize * i;
+		cfg->blocks[i] = BTRFS_BLOCK_RESERVED_1M_FOR_SUPER +
+			cfg->nodesize * (i - 1);
 	}
 
 	btrfs_set_super_bytenr(&super, cfg->blocks[MKFS_SUPER_BLOCK]);

@@ -309,7 +324,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 
 	/* then we have chunk 0 */
 	btrfs_set_disk_key_objectid(&disk_key, BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_CHUNK_ITEM_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -325,7 +340,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 	btrfs_set_chunk_sector_size(buf, chunk, cfg->sectorsize);
 	btrfs_set_chunk_num_stripes(buf, chunk, 1);
 	btrfs_set_stripe_devid_nr(buf, chunk, 0, 1);
-	btrfs_set_stripe_offset_nr(buf, chunk, 0, 0);
+	btrfs_set_stripe_offset_nr(buf, chunk, 0,
+				   BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	nritems++;
 
 	write_extent_buffer(buf, super.dev_item.uuid,

@@ -363,7 +379,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 		sizeof(struct btrfs_dev_extent);
 
 	btrfs_set_disk_key_objectid(&disk_key, 1);
-	btrfs_set_disk_key_offset(&disk_key, 0);
+	btrfs_set_disk_key_offset(&disk_key, BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 	btrfs_set_disk_key_type(&disk_key, BTRFS_DEV_EXTENT_KEY);
 	btrfs_set_item_key(buf, &disk_key, nritems);
 	btrfs_set_item_offset(buf, btrfs_item_nr(nritems), itemoff);

@@ -374,7 +390,8 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg)
 					BTRFS_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_objectid(buf, dev_extent,
 					BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_dev_extent_chunk_offset(buf, dev_extent, 0);
+	btrfs_set_dev_extent_chunk_offset(buf, dev_extent,
+					  BTRFS_BLOCK_RESERVED_1M_FOR_SUPER);
 
 	write_extent_buffer(buf, chunk_tree_uuid,
 		    (unsigned long)btrfs_dev_extent_chunk_tree_uuid(dev_extent),

mkfs/main.c +3 -0

index d817ad8dfd1a..8e3d19acb6f2 100644
@@ -81,10 +81,15 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
 	bytes_used = btrfs_super_bytes_used(fs_info->super_copy);
 
 	root->fs_info->system_allocs = 1;
+	/*
+	 * First temporary system chunk must match the chunk layout
+	 * created in make_btrfs().

mkfs/main.c static int create_metadata_block_groups(struct btrfs_root *root, int mixed, · line 95

Quoted 10 lines

mkfs/main.c +3 -1

+	 */
 	ret = btrfs_make_block_group(trans, fs_info, bytes_used,
 				     BTRFS_BLOCK_GROUP_SYSTEM,
 				     BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-				     0, BTRFS_MKFS_SYSTEM_GROUP_SIZE);
+				     BTRFS_BLOCK_RESERVED_1M_FOR_SUPER,
+				     BTRFS_MKFS_SYSTEM_GROUP_SIZE);
 	allocation->system += BTRFS_MKFS_SYSTEM_GROUP_SIZE;
 	if (ret)
 		return ret;

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

David Sterba

2018-01-23 16:45 UTC #6 lore raw

In reply to Qu Wenruo

Quoted 22 lines · Qu Wenruo

When creating btrfs, mkfs.btrfs will firstly create a temporary system chunk as basis, and then created needed trees or new devices.
However the layout temporary system chunk is hard-coded and uses reserved [0, 1M) range of devid 1.
Change the temporary chunk layout from old:
0 1M 4M 5M
|<----------- temp chunk -------------->|
And it's 1:1 mapped, which means it's a SINGLE chunk, and stripe offset is also 0.
to new layout:
0 1M 4M 5M
	|<----------- temp chunk -------------->|
And still keeps the 1:1 mapping.
The problem can only be exposed by "-m single" or "-M" where we reuse the temporary chunk.

Quoted 6 lines

With other meta profiles, system and meta chunks are allocated by later btrfs_alloc_chunk() call, and old SINGLE chunks are removed, so it will be no such problem for other meta profiles.
Reported-by Nikolay Borisov <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

The test mkfs-tests/010-minimal-size fails with this patch (devel branch). I've added some debugging and the requested minimal size by mkfs is not sufficient. I think it's because of the 1MB shift but haven't looked closely.

The math in "btrfs-progs: mkfs: move source dir size calculation to its own files" may need to be updated, I'm not sure how exactly to do that. It would be good if you find the fix and we'll then see where to put it (separate patch or fold it to some other one).

Otherwise I'm going to apply patches 1 and 2, thanks.

Qu Wenruo

2018-01-24 00:43 UTC #7 lore raw

Quoted 27 lines · David Sterba

Quoted 26 lines · Qu Wenruo
When creating btrfs, mkfs.btrfs will firstly create a temporary system chunk as basis, and then created needed trees or new devices.
However the layout temporary system chunk is hard-coded and uses reserved [0, 1M) range of devid 1.
Change the temporary chunk layout from old:
0 1M 4M 5M
|<----------- temp chunk -------------->|
And it's 1:1 mapped, which means it's a SINGLE chunk, and stripe offset is also 0.
to new layout:
0 1M 4M 5M
	|<----------- temp chunk -------------->|
And still keeps the 1:1 mapping.
The problem can only be exposed by "-m single" or "-M" where we reuse the temporary chunk.
With other meta profiles, system and meta chunks are allocated by later btrfs_alloc_chunk() call, and old SINGLE chunks are removed, so it will be no such problem for other meta profiles.

Quoted 7 lines

Quoted 2 lines
Reported-by Nikolay Borisov <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
The test mkfs-tests/010-minimal-size fails with this patch (devel branch). I've added some debugging and the requested minimal size by mkfs is not sufficient. I think it's because of the 1MB shift but haven't looked closely.

Patch under the way.

And this reminds me to refactor btrfs_alloc_chunk() to allow caller get minimal chunk size to use other than manually read the code.

Thanks,
Qu

Quoted 12 lines

The math in "btrfs-progs: mkfs: move source dir size calculation to its own files" may need to be updated, I'm not sure how exactly to do that. It would be good if you find the fix and we'll then see where to put it (separate patch or fold it to some other one).
Otherwise I'm going to apply patches 1 and 2, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html