Skip to contentSkip to conversation

mkfs: Fix minimal device check so that reported

PATCH Applied Qu Wenruo 3 participants 11 messages Nov 1, 2017 – Nov 28, 2017 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

v4 cover mkfs: Fix minimal device check so that reported

Original reported by Wesley AC:
https://github.com/kdave/btrfs-progs/issues/70

Fix it by taking data/meta profile and minimal chunk size into consideration.

Also introduce a test case for that.

Changelog:
v2:
Refactor test_minimum_size() to take @min_dev_size directly. Refactor patch already in devel branch.
v3:
Split test/common modification into separate patch.
v4:
Further split run_mustfail_stdout() and prepare_test_dev() modification into separate patch.
Include the real mkfs fix into patchset, to prevent possible test failure.
Use suggestion from David to use run_check_mount_test_dev() and run_check_umount_test_dev().
Add "-b $good_size" to mkfs command line, so we can see the size in test result, to help further debugging.

Qu Wenruo (4):
btrfs-progs: mkfs: Enhance minimal device size calculation to fix mkfs
failure on small file
btrfs-progs: test/common: Introduce run_mustfail_stdout
btrfs-progs: test/common: Enhance prepare_test_dev to reset device
size
btrfs-progs: test/mkfs: Test if the minimal device size is valid

 mkfs/common.c                            | 68 ++++++++++++++++++++++++++++----
 mkfs/common.h                            |  4 +-
 mkfs/main.c                              |  3 +-
 tests/common                             | 58 ++++++++++++++++++++++++++-
 tests/mkfs-tests/010-small-image/test.sh | 49 +++++++++++++++++++++++
 5 files changed, 171 insertions(+), 11 deletions(-)
 create mode 100755 tests/mkfs-tests/010-small-image/test.sh
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH v4 1/4 btrfs-progs: mkfs: Enhance minimal device size calculation to fix mkfs failure on small file

Since commit c11e36a29e84 ("Btrfs-progs: Do not force mixed block group creation unless '-M' option is specified"), mkfs no longer use mixed block group unless specified manually.

This breaks the minimal device size calculation, which only considered mixed block group use case.

This patch enhances minimal device size calculation for mkfs, by using different minimal stripe length (calculated from code) for different profiles, and use them to calculate minimal device size.

Reported-by Wesley Aptekar-Cassels <[email protected]>
Fixes c11e36a29e84 ("Btrfs-progs: Do not force mixed block group creation unless '-M' option is specified")
Signed-off-by Qu Wenruo <[email protected]>
Reviewed-by Lu Fengqi <[email protected]>
---
 mkfs/common.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
 mkfs/common.h |  4 ++--
 mkfs/main.c   |  3 ++-
 3 files changed, 65 insertions(+), 10 deletions(-)
mkfs/common.c +61 -7
index 71318b10fa51..2c400f44dbf3 100644
@@ -438,12 +438,6 @@ out:
 	return ret;
 }
 
-u64 btrfs_min_dev_size(u32 nodesize)
-{
-	return 2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE +
-		    btrfs_min_global_blk_rsv_size(nodesize));
-}
-
 /*
  * Btrfs minimum size calculation is complicated, it should include at least:
  * 1. system group size

@@ -454,11 +448,71 @@ u64 btrfs_min_dev_size(u32 nodesize)
  * To avoid the overkill calculation, (system group + global block rsv) * 2
  * for *EACH* device should be good enough.
  */
-u64 btrfs_min_global_blk_rsv_size(u32 nodesize)
+static u64 btrfs_min_global_blk_rsv_size(u32 nodesize)
 {
 	return (u64)nodesize << 10;
 }
 
+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
+		       u64 data_profile)
+{
+	u64 reserved = 0;
+	u64 meta_size;
+	u64 data_size;
+
+	if (mixed)
+		return 2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE +
+			    btrfs_min_global_blk_rsv_size(nodesize));
+
+	/*
+	 * minimal size calculation is complex due to several factors:
+	 * 1) Temporary chunk resue
+	 *    If specified chunk profile is SINGLE, we can resue
+	 *    temporary chunks, no need to alloc new chunks.
+	 *
+	 * 2) Different minimal chunk size for different profile
+	 *    For initial sys chunk, chunk size is fixed to 4M.
+	 *    For single profile, minimal chunk size is 8M for all.
+	 *    For other profiles, minimal chunk and stripe size
+	 *    differs from 8M to 64M.
+	 *
+	 * To calculate it a little easier, here we assume we don't
+	 * reuse any temporary chunk, and calculate the size all
+	 * by ourselves.
+	 *
+	 * Temporary chunks sizes are always fixed:
+	 * One initial sys chunk, one SINGLE meta, and one SINGLE data.
+	 * The latter two are all 8M, accroding to @calc_size of
+	 * btrfs_alloc_chunk().
+	 */
+	reserved += BTRFS_MKFS_SYSTEM_GROUP_SIZE + SZ_8M * 2;
+
+	/*
+	 * For real chunks, we need to refer different sizes:
+	 * For SINGLE, it's still fixed to 8M (@calc_size).
+	 * For other profiles, refer to max(@min_stripe_size, @calc_size).
+	 *
+	 * And use the stripe size to calculate its physical used space.
+	 */
+	if (meta_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		meta_size = SZ_8M + SZ_32M;
+	else
+		meta_size = SZ_8M + SZ_8M;
+	/* Only metadata put 2 stripes into one disk */
+	if (meta_profile & BTRFS_BLOCK_GROUP_DUP)
+		meta_size *= 2;
+	reserved += meta_size;
+
+	if (data_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		data_size = SZ_64M;
+	else
+		data_size = SZ_8M;
+	if (data_profile & BTRFS_BLOCK_GROUP_DUP)
+		data_size *= 2;
+	reserved += data_size;
+	return reserved;
+}
+
 #define isoctal(c)	(((c) & ~7) == '0')
 
 static inline void translate(char *f, char *t)
mkfs/common.h +2 -2
index 3757e9e7fd0a..e9e3b3eb43a6 100644
@@ -66,8 +66,8 @@ struct btrfs_mkfs_config {
 };
 
 int make_btrfs(int fd, struct btrfs_mkfs_config *cfg);
-u64 btrfs_min_dev_size(u32 nodesize);
-u64 btrfs_min_global_blk_rsv_size(u32 nodesize);
+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
+		       u64 data_profile);
 int test_minimum_size(const char *file, u64 min_dev_size);
 int is_vol_small(const char *file);
 int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
mkfs/main.c +2 -1
index 6669a4b901af..982cafc07b84 100644
@@ -979,7 +979,8 @@ int main(int argc, char **argv)
 		goto error;
 	}
 
-	min_dev_size = btrfs_min_dev_size(nodesize);
+	min_dev_size = btrfs_min_dev_size(nodesize, mixed, metadata_profile,
+					  data_profile);
 	/* Check device/block_count after the nodesize is determined */
 	if (block_count && block_count < min_dev_size) {
 		error("size %llu is too small to make a usable filesystem",
Signature · 1 line
2.14.3
#2 lore raw

Re: [PATCH v4 1/4] btrfs-progs: mkfs: Enhance minimal device size calculation to fix mkfs failure on small file

Quoted 36 lines · Qu Wenruo
mkfs/common.c +36 -0
+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
+		       u64 data_profile)
+{
+	u64 reserved = 0;
+	u64 meta_size;
+	u64 data_size;
+
+	if (mixed)
+		return 2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE +
+			    btrfs_min_global_blk_rsv_size(nodesize));
+
+	/*
+	 * minimal size calculation is complex due to several factors:
+	 * 1) Temporary chunk resue
+	 *    If specified chunk profile is SINGLE, we can resue
+	 *    temporary chunks, no need to alloc new chunks.
+	 *
+	 * 2) Different minimal chunk size for different profile
+	 *    For initial sys chunk, chunk size is fixed to 4M.
+	 *    For single profile, minimal chunk size is 8M for all.
+	 *    For other profiles, minimal chunk and stripe size
+	 *    differs from 8M to 64M.
+	 *
+	 * To calculate it a little easier, here we assume we don't
+	 * reuse any temporary chunk, and calculate the size all
+	 * by ourselves.
+	 *
+	 * Temporary chunks sizes are always fixed:
+	 * One initial sys chunk, one SINGLE meta, and one SINGLE data.
+	 * The latter two are all 8M, accroding to @calc_size of
+	 * btrfs_alloc_chunk().
+	 */
+	reserved += BTRFS_MKFS_SYSTEM_GROUP_SIZE + SZ_8M * 2;
+
+	/*
+	 * For real chunks, we need to refer different sizes:

mkfs/common.c u64 btrfs_min_dev_size(u32 nodesize) · line 501

Quoted 10 lines
mkfs/common.c +10 -0
+	 * For SINGLE, it's still fixed to 8M (@calc_size).
+	 * For other profiles, refer to max(@min_stripe_size, @calc_size).
+	 *
+	 * And use the stripe size to calculate its physical used space.
+	 */
+	if (meta_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		meta_size = SZ_8M + SZ_32M;
+	else
+		meta_size = SZ_8M + SZ_8M;
+	/* Only metadata put 2 stripes into one disk */

'mkfs.btrfs -d dup' also works, so I think this should be handled here the same way as metadata, right?

Quoted 13 lines
mkfs/common.c +13 -0
+	if (meta_profile & BTRFS_BLOCK_GROUP_DUP)
+		meta_size *= 2;
+	reserved += meta_size;
+
+	if (data_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		data_size = SZ_64M;
+	else
+		data_size = SZ_8M;
+	if (data_profile & BTRFS_BLOCK_GROUP_DUP)
+		data_size *= 2;
+	reserved += data_size;
+	return reserved;
+}
#3 lore raw

Re: [PATCH v4 1/4] btrfs-progs: mkfs: Enhance minimal device size calculation to fix mkfs failure on small file

Quoted 42 lines · David Sterba
Quoted 41 lines · Qu Wenruo
mkfs/common.c +41 -0
+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
+		       u64 data_profile)
+{
+	u64 reserved = 0;
+	u64 meta_size;
+	u64 data_size;
+
+	if (mixed)
+		return 2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE +
+			    btrfs_min_global_blk_rsv_size(nodesize));
+
+	/*
+	 * minimal size calculation is complex due to several factors:
+	 * 1) Temporary chunk resue
+	 *    If specified chunk profile is SINGLE, we can resue
+	 *    temporary chunks, no need to alloc new chunks.
+	 *
+	 * 2) Different minimal chunk size for different profile
+	 *    For initial sys chunk, chunk size is fixed to 4M.
+	 *    For single profile, minimal chunk size is 8M for all.
+	 *    For other profiles, minimal chunk and stripe size
+	 *    differs from 8M to 64M.
+	 *
+	 * To calculate it a little easier, here we assume we don't
+	 * reuse any temporary chunk, and calculate the size all
+	 * by ourselves.
+	 *
+	 * Temporary chunks sizes are always fixed:
+	 * One initial sys chunk, one SINGLE meta, and one SINGLE data.
+	 * The latter two are all 8M, accroding to @calc_size of
+	 * btrfs_alloc_chunk().
+	 */
+	reserved += BTRFS_MKFS_SYSTEM_GROUP_SIZE + SZ_8M * 2;
+
+	/*
+	 * For real chunks, we need to refer different sizes:
+	 * For SINGLE, it's still fixed to 8M (@calc_size).
+	 * For other profiles, refer to max(@min_stripe_size, @calc_size).
+	 *
+	 * And use the stripe size to calculate its physical used space.
+	 */
Quoted 8 lines

mkfs/common.c u64 btrfs_min_dev_size(u32 nodesize) · line 501

Quoted 5 lines
mkfs/common.c +5 -0
+	if (meta_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		meta_size = SZ_8M + SZ_32M;
+	else
+		meta_size = SZ_8M + SZ_8M;
+	/* Only metadata put 2 stripes into one disk */

'mkfs.btrfs -d dup' also works, so I think this should be handled here the same way as metadata, right?

Just a few lines under.

Quoted 11 lines

mkfs/common.c u64 btrfs_min_dev_size(u32 nodesize) · line 511

Quoted 11 lines
mkfs/common.c +10 -0
+	if (meta_profile & BTRFS_BLOCK_GROUP_DUP)
+		meta_size *= 2;
+	reserved += meta_size;
+
+	if (data_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		data_size = SZ_64M;
+	else
+		data_size = SZ_8M;
+	if (data_profile & BTRFS_BLOCK_GROUP_DUP)
+		data_size *= 2;

Here data DUP is also handled.

So the problem is about the word "Only".

Do I need to re-submit the whole patchset or just a separate patch to update the comment?

Thanks,
Qu

Quoted 8 lines

mkfs/common.c u64 btrfs_min_dev_size(u32 nodesize) · line 514

Quoted 3 lines
mkfs/common.c +3 -0
+	reserved += data_size;
+	return reserved;
+}

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

#4 lore raw

Re: [PATCH v4 1/4] btrfs-progs: mkfs: Enhance minimal device size calculation to fix mkfs failure on small file

Quoted 68 lines · Qu Wenruo
Quoted 42 lines · David Sterba
Quoted 41 lines · Qu Wenruo
mkfs/common.c +41 -0
+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
+		       u64 data_profile)
+{
+	u64 reserved = 0;
+	u64 meta_size;
+	u64 data_size;
+
+	if (mixed)
+		return 2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE +
+			    btrfs_min_global_blk_rsv_size(nodesize));
+
+	/*
+	 * minimal size calculation is complex due to several factors:
+	 * 1) Temporary chunk resue
+	 *    If specified chunk profile is SINGLE, we can resue
+	 *    temporary chunks, no need to alloc new chunks.
+	 *
+	 * 2) Different minimal chunk size for different profile
+	 *    For initial sys chunk, chunk size is fixed to 4M.
+	 *    For single profile, minimal chunk size is 8M for all.
+	 *    For other profiles, minimal chunk and stripe size
+	 *    differs from 8M to 64M.
+	 *
+	 * To calculate it a little easier, here we assume we don't
+	 * reuse any temporary chunk, and calculate the size all
+	 * by ourselves.
+	 *
+	 * Temporary chunks sizes are always fixed:
+	 * One initial sys chunk, one SINGLE meta, and one SINGLE data.
+	 * The latter two are all 8M, accroding to @calc_size of
+	 * btrfs_alloc_chunk().
+	 */
+	reserved += BTRFS_MKFS_SYSTEM_GROUP_SIZE + SZ_8M * 2;
+
+	/*
+	 * For real chunks, we need to refer different sizes:
+	 * For SINGLE, it's still fixed to 8M (@calc_size).
+	 * For other profiles, refer to max(@min_stripe_size, @calc_size).
+	 *
+	 * And use the stripe size to calculate its physical used space.
+	 */
Quoted 8 lines

mkfs/common.c u64 btrfs_min_dev_size(u32 nodesize) · line 501

Quoted 5 lines
mkfs/common.c +5 -0
+	if (meta_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		meta_size = SZ_8M + SZ_32M;
+	else
+		meta_size = SZ_8M + SZ_8M;
+	/* Only metadata put 2 stripes into one disk */

'mkfs.btrfs -d dup' also works, so I think this should be handled here the same way as metadata, right?

Just a few lines under.

Quoted 11 lines

mkfs/common.c u64 btrfs_min_dev_size(u32 nodesize) · line 511

Quoted 11 lines
mkfs/common.c +10 -0
+	if (meta_profile & BTRFS_BLOCK_GROUP_DUP)
+		meta_size *= 2;
+	reserved += meta_size;
+
+	if (data_profile & BTRFS_BLOCK_GROUP_PROFILE_MASK)
+		data_size = SZ_64M;
+	else
+		data_size = SZ_8M;
+	if (data_profile & BTRFS_BLOCK_GROUP_DUP)
+		data_size *= 2;

Here data DUP is also handled.

Quoted 1 line

So the problem is about the word "Only".

Yeah, it's confusing when comment does not match the code because it's not clear what's the actual intention.

Quoted 2 lines

Do I need to re-submit the whole patchset or just a separate patch to update the comment?

No, I'll update it myself as it is fairly trivial change.

author
#5 lore raw

PATCH v4 2/4 btrfs-progs: test/common: Introduce run_mustfail_stdout

In reply to Qu Wenruo

For later test case which needs info from stderr.

Signed-off-by Qu Wenruo <[email protected]>
---
 tests/common | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
tests/common +52 -0
index dec090fe5849..977bade201a6 100644
@@ -236,6 +236,58 @@ run_mustfail()
 	fi
 }
 
+# The first parameter is error message to print if it fails, just like
+# run_must_fail().
+# Also we don't use pipefail to avoid disturbing other script, so here we
+# use temporary output file.
+# So it doesn't support pipeline in the @cmd
+run_mustfail_stdout()
+{
+	local spec
+	local ins
+	local cmd
+	local msg
+	local ret
+	local tmp_output
+
+	tmp_output=$(mktemp --tmpdir btrfs-progs-test--mustfail-stdtout.XXXXXX)
+
+	msg="$1"
+	shift
+
+	if _is_file_or_command "$msg"; then
+		echo "ASSERTION FAIL: 1st argument of run_mustfail_stdout must be a message"
+		exit 1
+	fi
+
+	ins=$(_get_spec_ins "$@")
+	spec=$(($ins-1))
+	cmd=$(eval echo "\${$spec}")
+	spec=$(_cmd_spec "${@:$spec}")
+	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
+	echo "############### $@" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): $@" > /dev/tty; fi
+	if [ "$1" = 'root_helper' ]; then
+		"$@" 2>&1 > "$tmp_output"
+	else
+		$INSTRUMENT "$@" 2>&1 > "$tmp_output"
+	fi
+	ret=$?
+
+	cat "$tmp_output" >> "$RESULTS"
+	cat "$tmp_output"
+	rm "$tmp_output"
+
+	if [ "$ret" != 0 ]; then
+		echo "failed (expected): $@" >> "$RESULTS"
+		return 0
+	else
+		echo "succeeded (unexpected!): $@" >> "$RESULTS"
+		_fail "unexpected success: $msg"
+		return 1
+	fi
+}
+
 check_prereq()
 {
 	if ! [ -f "$TOP/$1" ]; then
Signature · 1 line
2.14.3
Applied
#6 lore raw

Re: [PATCH v4 2/4] btrfs-progs: test/common: Introduce run_mustfail_stdout

Quoted 3 lines · Qu Wenruo

For later test case which needs info from stderr.

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

Applied, thanks.

author
#7 lore raw

PATCH v4 3/4 btrfs-progs: test/common: Enhance prepare_test_dev to reset device size

In reply to Qu Wenruo

So prepare_test_dev() can be called several times in one test case, to test different device sizes.

Signed-off-by Qu Wenruo <[email protected]>
---
 tests/common | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
tests/common +5 -1
index 977bade201a6..63750dbfeffe 100644
@@ -441,8 +441,12 @@ prepare_test_dev()
 	# num[K/M/G/T...]
 	local size="$1"
 
-	[[ "$TEST_DEV" ]] && return
 	[[ "$size" ]] || size='2G'
+	# Still truncate it to new size
+	if [[ "$TEST_DEV" ]]; then
+		truncate -s "$size" "$TEST_DEV"
+		return;
+	fi
 
 	echo "\$TEST_DEV not given, use $TOP/test/test.img as fallback" >> \
 		"$RESULTS"
Signature · 1 line
2.14.3
Applied
#8 lore raw

Re: [PATCH v4 3/4] btrfs-progs: test/common: Enhance prepare_test_dev to reset device size

Quoted 4 lines · Qu Wenruo

So prepare_test_dev() can be called several times in one test case, to test different device sizes.

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

Applied, thanks.

author
#9 lore raw

PATCH v4 4/4 btrfs-progs: test/mkfs: Test if the minimal device size is valid

In reply to Qu Wenruo

New test case to test if the minimal device size given by "mkfs.btrfs" failure case is valid.

Signed-off-by Qu Wenruo <[email protected]>
---
 tests/mkfs-tests/010-small-image/test.sh | 49 ++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100755 tests/mkfs-tests/010-small-image/test.sh
tests/mkfs-tests/010-small-image/test.sh new file +49 -0
new file mode 100755
index 000000000000..304c87057304
@@ -0,0 +1,49 @@
+#!/bin/bash
+# test if the reported minimal size of mkfs.btrfs is valid
+
+source "$TOP"/tests/common
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+
+do_test()
+{
+	# Well, 1M small enough to fail, we just use the output
+	# to get the minimal device size
+	prepare_test_dev 1M
+	output=$(run_mustfail_stdout "mkfs.btrfs for small image" \
+		 "$TOP"/mkfs.btrfs -f $@ "$TEST_DEV")
+	good_size=$(echo "$output" | grep -oP "(?<=is )\d+")
+
+	prepare_test_dev "$good_size"
+	run_check "$TOP"/mkfs.btrfs -b $good_size -f $@ "$TEST_DEV"
+	run_check_mount_test_dev
+	run_check_umount_test_dev
+}
+
+do_test -n 4k	-m single	-d single
+do_test -n 4k	-m single	-d dup
+do_test -n 4k	-m dup		-d single
+do_test -n 4k	-m dup		-d dup
+
+do_test -n 8k	-m single	-d single
+do_test -n 8k	-m single	-d dup
+do_test -n 8k	-m dup		-d single
+do_test -n 8k	-m dup		-d dup
+
+do_test -n 16k	-m single	-d single
+do_test -n 16k	-m single	-d dup
+do_test -n 16k	-m dup		-d single
+do_test -n 16k	-m dup		-d dup
+
+do_test -n 32k	-m single	-d single
+do_test -n 32k	-m single	-d dup
+do_test -n 32k	-m dup		-d single
+do_test -n 32k	-m dup		-d dup
+
+do_test -n 64k	-m single	-d single
+do_test -n 64k	-m single	-d dup
+do_test -n 64k	-m dup		-d single
+do_test -n 64k	-m dup		-d dup
Signature · 1 line
2.14.3
Applied
#10 lore raw

Re: [PATCH v4 4/4] btrfs-progs: test/mkfs: Test if the minimal device size is valid

Quoted 4 lines · Qu Wenruo

New test case to test if the minimal device size given by "mkfs.btrfs" failure case is valid.

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

Applied, thanks.