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

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

[Qu Wenruo]

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 <W.Aptekar@gmail.com>
Fixes: c11e36a29e84 ("Btrfs-progs: Do not force mixed block group creation unless '-M' option is specified")
Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 mkfs/common.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 mkfs/common.h |  7 +++---
 mkfs/main.c   | 12 ++++++----
 3 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/mkfs/common.c b/mkfs/common.c
index c9ce10d46a9d..64461daf3917 100644
--- a/mkfs/common.c
+++ b/mkfs/common.c
@@ -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)
@@ -698,7 +752,8 @@ int is_vol_small(const char *file)
 	}
 }
 
-int test_minimum_size(const char *file, u32 nodesize)
+int test_minimum_size(const char *file, u32 nodesize, int mixed,
+		      u64 meta_profile, u64 data_profile)
 {
 	int fd;
 	struct stat statbuf;
@@ -710,7 +765,8 @@ int test_minimum_size(const char *file, u32 nodesize)
 		close(fd);
 		return -errno;
 	}
-	if (btrfs_device_size(fd, &statbuf) < btrfs_min_dev_size(nodesize)) {
+	if (btrfs_device_size(fd, &statbuf) <
+	    btrfs_min_dev_size(nodesize, mixed, meta_profile, data_profile)) {
 		close(fd);
 		return 1;
 	}
diff --git a/mkfs/common.h b/mkfs/common.h
index dee0ea9740e0..0d1aa5c17c03 100644
--- a/mkfs/common.h
+++ b/mkfs/common.h
@@ -66,9 +66,10 @@ 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);
-int test_minimum_size(const char *file, u32 nodesize);
+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
+		       u64 data_profile);
+int test_minimum_size(const char *file, u32 nodesize, int mixed,
+		      u64 meta_profile, u64 data_profile);
 int is_vol_small(const char *file);
 int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
 	u64 dev_cnt, int mixed, int ssd);
diff --git a/mkfs/main.c b/mkfs/main.c
index 1b4cabc1ef90..527c508f64a5 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1436,6 +1436,7 @@ int main(int argc, char **argv)
 	u64 num_of_meta_chunks = 0;
 	u64 size_of_data = 0;
 	u64 source_dir_size = 0;
+	u64 min_dev_size;
 	int dev_cnt = 0;
 	int saved_optind;
 	char fs_uuid[BTRFS_UUID_UNPARSED_SIZE] = { 0 };
@@ -1646,19 +1647,22 @@ int main(int argc, char **argv)
 		goto error;
 	}
 
+	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 < btrfs_min_dev_size(nodesize)) {
+	if (block_count && block_count < min_dev_size) {
 		error("size %llu is too small to make a usable filesystem",
 			block_count);
 		error("minimum size for btrfs filesystem is %llu",
-			btrfs_min_dev_size(nodesize));
+			min_dev_size);
 		goto error;
 	}
 	for (i = saved_optind; i < saved_optind + dev_cnt; i++) {
 		char *path;
 
 		path = argv[i];
-		ret = test_minimum_size(path, nodesize);
+		ret = test_minimum_size(path, nodesize, mixed, metadata_profile,
+					data_profile);
 		if (ret < 0) {
 			error("failed to check size for %s: %s",
 				path, strerror(-ret));
@@ -1668,7 +1672,7 @@ int main(int argc, char **argv)
 			error("'%s' is too small to make a usable filesystem",
 				path);
 			error("minimum size for each btrfs device is %llu",
-				btrfs_min_dev_size(nodesize));
+				min_dev_size);
 			goto error;
 		}
 	}
-- 
2.14.2

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

[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 <quwenruo.btrfs@gmx.com>
---
 tests/common                             | 57 +++++++++++++++++++++++++++++++-
 tests/mkfs-tests/010-small-image/test.sh | 51 ++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100755 tests/mkfs-tests/010-small-image/test.sh

diff --git a/tests/common b/tests/common
index eb525a4d02c5..e026cc2f4d30 100644
--- a/tests/common
+++ b/tests/common
@@ -236,6 +236,57 @@ run_mustfail()
 	fi
 }
 
+run_mustfail_stdout()
+{
+	local spec
+	local ins
+	local cmd
+	local msg
+	local ret
+
+	# 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
+	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
@@ -389,7 +440,11 @@ prepare_test_dev()
 	# num[K/M/G/T...]
 	local size="$1"
 
-	[[ "$TEST_DEV" ]] && return
+	# Still truncate it to new size
+	if [[ "$TEST_DEV" ]]; then
+		truncate -s "$size" "$TEST_DEV"
+		return;
+	fi
 	[[ "$size" ]] || size='2G'
 
 	echo "\$TEST_DEV not given, use $TOP/test/test.img as fallback" >> \
diff --git a/tests/mkfs-tests/010-small-image/test.sh b/tests/mkfs-tests/010-small-image/test.sh
new file mode 100755
index 000000000000..f12c9c836bd6
--- /dev/null
+++ b/tests/mkfs-tests/010-small-image/test.sh
@@ -0,0 +1,51 @@
+#!/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
+
+pagesize=$(getconf PAGESIZE)
+
+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 -o "[[:digit:]]\{3,\}")
+
+	prepare_test_dev "$good_size"
+	run_check $TOP/mkfs.btrfs -f $@ "$TEST_DEV"
+	run_check $SUDO_HELPER mount $TEST_DEV $TEST_MNT
+	run_check $SUDO_HELPER umount $TEST_MNT
+}
+
+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 
-- 
2.14.2

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

[Lu Fengqi]

On Thu, Oct 12, 2017 at 12:28:58AM -0700, Qu Wenruo wrote:
>New test case to test if the minimal device size given by "mkfs.btrfs"
>failure case is valid.
>
>Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>---
> tests/common                             | 57 +++++++++++++++++++++++++++++++-
> tests/mkfs-tests/010-small-image/test.sh | 51 ++++++++++++++++++++++++++++
> 2 files changed, 107 insertions(+), 1 deletion(-)
> create mode 100755 tests/mkfs-tests/010-small-image/test.sh
>
>diff --git a/tests/common b/tests/common
>index eb525a4d02c5..e026cc2f4d30 100644
>--- a/tests/common
>+++ b/tests/common
>@@ -236,6 +236,57 @@ run_mustfail()
> 	fi
> }
> 
>+run_mustfail_stdout()
>+{
>+	local spec
>+	local ins
>+	local cmd
>+	local msg
>+	local ret
>+
>+	# 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
>+	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
>@@ -389,7 +440,11 @@ prepare_test_dev()
> 	# num[K/M/G/T...]
> 	local size="$1"
> 
>-	[[ "$TEST_DEV" ]] && return
>+	# Still truncate it to new size
>+	if [[ "$TEST_DEV" ]]; then
>+		truncate -s "$size" "$TEST_DEV"
>+		return;
>+	fi
> 	[[ "$size" ]] || size='2G'
> 
> 	echo "\$TEST_DEV not given, use $TOP/test/test.img as fallback" >> \
>diff --git a/tests/mkfs-tests/010-small-image/test.sh b/tests/mkfs-tests/010-small-image/test.sh
>new file mode 100755
>index 000000000000..f12c9c836bd6
>--- /dev/null
>+++ b/tests/mkfs-tests/010-small-image/test.sh
>@@ -0,0 +1,51 @@
>+#!/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
>+
>+pagesize=$(getconf PAGESIZE)
>+
>+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 -o "[[:digit:]]\{3,\}")

I applied these patches on kdave/devel, and found a small problem when run
mkfs-tests/010-small-image.

# fallocate -l 1M a.img
# ./mkfs.btrfs a.img
btrfs-progs v4.13.2-34-ga8dc557a
See http://btrfs.wiki.kernel.org for more information.

ERROR: 'a.img' is too small to make a usable filesystem
ERROR: minimum size for each btrfs device is 113246208

"[[:digit:]]\{3,\}" will not only match 113246208, but will match 557.

So I suggest using the following regex here. It will only match the
number after "is ".

good_size=$(echo $output | grep -oP "(?<=is )\d+")

>+
>+	prepare_test_dev "$good_size"
>+	run_check $TOP/mkfs.btrfs -f $@ "$TEST_DEV"
>+	run_check $SUDO_HELPER mount $TEST_DEV $TEST_MNT
>+	run_check $SUDO_HELPER umount $TEST_MNT
>+}
>+
>+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 
>-- 
>2.14.2
>
>--
>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
>
>

-- 
Thanks,
Lu

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

[Qu Wenruo]

On 2017年10月12日 16:24, Lu Fengqi wrote:
> On Thu, Oct 12, 2017 at 12:28:58AM -0700, Qu Wenruo wrote:
>> New test case to test if the minimal device size given by "mkfs.btrfs"
>> failure case is valid.
>>
>> Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>> ---
>> tests/common                             | 57 +++++++++++++++++++++++++++++++-
>> tests/mkfs-tests/010-small-image/test.sh | 51 ++++++++++++++++++++++++++++
>> 2 files changed, 107 insertions(+), 1 deletion(-)
>> create mode 100755 tests/mkfs-tests/010-small-image/test.sh
>>
>> diff --git a/tests/common b/tests/common
>> index eb525a4d02c5..e026cc2f4d30 100644
>> --- a/tests/common
>> +++ b/tests/common
>> @@ -236,6 +236,57 @@ run_mustfail()
>> 	fi
>> }
>>
>> +run_mustfail_stdout()
>> +{
>> +	local spec
>> +	local ins
>> +	local cmd
>> +	local msg
>> +	local ret
>> +
>> +	# 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
>> +	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
>> @@ -389,7 +440,11 @@ prepare_test_dev()
>> 	# num[K/M/G/T...]
>> 	local size="$1"
>>
>> -	[[ "$TEST_DEV" ]] && return
>> +	# Still truncate it to new size
>> +	if [[ "$TEST_DEV" ]]; then
>> +		truncate -s "$size" "$TEST_DEV"
>> +		return;
>> +	fi
>> 	[[ "$size" ]] || size='2G'
>>
>> 	echo "\$TEST_DEV not given, use $TOP/test/test.img as fallback" >> \
>> diff --git a/tests/mkfs-tests/010-small-image/test.sh b/tests/mkfs-tests/010-small-image/test.sh
>> new file mode 100755
>> index 000000000000..f12c9c836bd6
>> --- /dev/null
>> +++ b/tests/mkfs-tests/010-small-image/test.sh
>> @@ -0,0 +1,51 @@
>> +#!/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
>> +
>> +pagesize=$(getconf PAGESIZE)
>> +
>> +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 -o "[[:digit:]]\{3,\}")
> 
> I applied these patches on kdave/devel, and found a small problem when run
> mkfs-tests/010-small-image.
> 
> # fallocate -l 1M a.img
> # ./mkfs.btrfs a.img
> btrfs-progs v4.13.2-34-ga8dc557a
> See http://btrfs.wiki.kernel.org for more information.
> 
> ERROR: 'a.img' is too small to make a usable filesystem
> ERROR: minimum size for each btrfs device is 113246208
> 
> "[[:digit:]]\{3,\}" will not only match 113246208, but will match 557.
> 
> So I suggest using the following regex here. It will only match the
> number after "is ".
> 
> good_size=$(echo $output | grep -oP "(?<=is )\d+")

Yes, this is much better.
I just forget that in version string we can have digits.

I'll update the test case.

Thanks,
Qu
> 
>> +
>> +	prepare_test_dev "$good_size"
>> +	run_check $TOP/mkfs.btrfs -f $@ "$TEST_DEV"
>> +	run_check $SUDO_HELPER mount $TEST_DEV $TEST_MNT
>> +	run_check $SUDO_HELPER umount $TEST_MNT
>> +}
>> +
>> +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
>> -- 
>> 2.14.2
>>
>> --
>> 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
>>
>>
> 

[PATCH v2 2/2] btrfs-progs: test/mkfs: Test if the minimal device size is valid

[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 <quwenruo.btrfs@gmx.com>
---
changelog:
v2:
  Remove tailing whitespace.
  Use more accurate regex to match minimal device size, suggested by Lu
  Fengqi.
---
 tests/common                             | 57 +++++++++++++++++++++++++++++++-
 tests/mkfs-tests/010-small-image/test.sh | 51 ++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100755 tests/mkfs-tests/010-small-image/test.sh

diff --git a/tests/common b/tests/common
index eb525a4d02c5..e026cc2f4d30 100644
--- a/tests/common
+++ b/tests/common
@@ -236,6 +236,57 @@ run_mustfail()
 	fi
 }
 
+run_mustfail_stdout()
+{
+	local spec
+	local ins
+	local cmd
+	local msg
+	local ret
+
+	# 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
+	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
@@ -389,7 +440,11 @@ prepare_test_dev()
 	# num[K/M/G/T...]
 	local size="$1"
 
-	[[ "$TEST_DEV" ]] && return
+	# Still truncate it to new size
+	if [[ "$TEST_DEV" ]]; then
+		truncate -s "$size" "$TEST_DEV"
+		return;
+	fi
 	[[ "$size" ]] || size='2G'
 
 	echo "\$TEST_DEV not given, use $TOP/test/test.img as fallback" >> \
diff --git a/tests/mkfs-tests/010-small-image/test.sh b/tests/mkfs-tests/010-small-image/test.sh
new file mode 100755
index 000000000000..2ed379f9b66f
--- /dev/null
+++ b/tests/mkfs-tests/010-small-image/test.sh
@@ -0,0 +1,51 @@
+#!/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
+
+pagesize=$(getconf PAGESIZE)
+
+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 -f $@ "$TEST_DEV"
+	run_check $SUDO_HELPER mount $TEST_DEV $TEST_MNT
+	run_check $SUDO_HELPER umount $TEST_MNT
+}
+
+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
-- 
2.14.2

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

[Lu Fengqi]

On Thu, Oct 12, 2017 at 12:28:57AM -0700, Qu Wenruo wrote:
>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 <W.Aptekar@gmail.com>
>Fixes: c11e36a29e84 ("Btrfs-progs: Do not force mixed block group creation unless '-M' option is specified")
>Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>---
> mkfs/common.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
> mkfs/common.h |  7 +++---
> mkfs/main.c   | 12 ++++++----
> 3 files changed, 77 insertions(+), 16 deletions(-)
>
>diff --git a/mkfs/common.c b/mkfs/common.c
>index c9ce10d46a9d..64461daf3917 100644
>--- a/mkfs/common.c
>+++ b/mkfs/common.c
>@@ -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)
>@@ -698,7 +752,8 @@ int is_vol_small(const char *file)
> 	}
> }
> 
>-int test_minimum_size(const char *file, u32 nodesize)
>+int test_minimum_size(const char *file, u32 nodesize, int mixed,
>+		      u64 meta_profile, u64 data_profile)
> {
> 	int fd;
> 	struct stat statbuf;
>@@ -710,7 +765,8 @@ int test_minimum_size(const char *file, u32 nodesize)
> 		close(fd);
> 		return -errno;
> 	}
>-	if (btrfs_device_size(fd, &statbuf) < btrfs_min_dev_size(nodesize)) {
>+	if (btrfs_device_size(fd, &statbuf) <
>+	    btrfs_min_dev_size(nodesize, mixed, meta_profile, data_profile)) {
> 		close(fd);
> 		return 1;
> 	}
>diff --git a/mkfs/common.h b/mkfs/common.h
>index dee0ea9740e0..0d1aa5c17c03 100644
>--- a/mkfs/common.h
>+++ b/mkfs/common.h
>@@ -66,9 +66,10 @@ 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);
>-int test_minimum_size(const char *file, u32 nodesize);
>+u64 btrfs_min_dev_size(u32 nodesize, int mixed, u64 meta_profile,
>+		       u64 data_profile);
>+int test_minimum_size(const char *file, u32 nodesize, int mixed,
>+		      u64 meta_profile, u64 data_profile);
> int is_vol_small(const char *file);
> int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
> 	u64 dev_cnt, int mixed, int ssd);
>diff --git a/mkfs/main.c b/mkfs/main.c
>index 1b4cabc1ef90..527c508f64a5 100644
>--- a/mkfs/main.c
>+++ b/mkfs/main.c
>@@ -1436,6 +1436,7 @@ int main(int argc, char **argv)
> 	u64 num_of_meta_chunks = 0;
> 	u64 size_of_data = 0;
> 	u64 source_dir_size = 0;
>+	u64 min_dev_size;
> 	int dev_cnt = 0;
> 	int saved_optind;
> 	char fs_uuid[BTRFS_UUID_UNPARSED_SIZE] = { 0 };
>@@ -1646,19 +1647,22 @@ int main(int argc, char **argv)
> 		goto error;
> 	}
> 
>+	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 < btrfs_min_dev_size(nodesize)) {
>+	if (block_count && block_count < min_dev_size) {
> 		error("size %llu is too small to make a usable filesystem",
> 			block_count);
> 		error("minimum size for btrfs filesystem is %llu",
>-			btrfs_min_dev_size(nodesize));
>+			min_dev_size);
> 		goto error;
> 	}
> 	for (i = saved_optind; i < saved_optind + dev_cnt; i++) {
> 		char *path;
> 
> 		path = argv[i];
>-		ret = test_minimum_size(path, nodesize);
>+		ret = test_minimum_size(path, nodesize, mixed, metadata_profile,
>+					data_profile);

As mentioned in the offline discussion, I think test_minimum_size don't
need so many parameters, min_dev_size is enough.

The rest looks good for me.

Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

-- 
Thanks,
Lu

> 		if (ret < 0) {
> 			error("failed to check size for %s: %s",
> 				path, strerror(-ret));
>@@ -1668,7 +1672,7 @@ int main(int argc, char **argv)
> 			error("'%s' is too small to make a usable filesystem",
> 				path);
> 			error("minimum size for each btrfs device is %llu",
>-				btrfs_min_dev_size(nodesize));
>+				min_dev_size);
> 			goto error;
> 		}
> 	}
>-- 
>2.14.2
>
>--
>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
>
>