[PATCH 0/3] btrfs-progs: allow tree-checker to be triggered more frequently for btrfs-convert

[PATCH 0/3] btrfs-progs: allow tree-checker to be triggered more frequently for btrfs-convert

[Qu Wenruo]

We have issue #731, which is a large ext4 fs, and triggered tree-checker
very reliably.

The root cause is the bad write behavior of btrfs-convert, which always
insert inode refs/file extents/xattrs first, then the inode item.

Such bad behavior would normally trigger tree-checker, but for our
regular convert tests, it doesn't get trigger because:

- We have metadata cache
  The default size is system memory / 4, which can be very very large.
  And written tree blocks would still be cached thus no tree-checker
  triggered for those cached tree blocks.

- We won't commit transaction for every copied inodes
  This means for a lot of cases, we may just commit a large transaction
  for all inodes, further reduce the chance to trigger tree checker.

This patchset would introduce two debug environment variables:

- BTRFS_PROGS_DEBUG_METADATA_CACHE_SIZE
  Global metadata cache size, affecting all btrfs-progs tools that opens
  an unmounted btrfs.

- BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD
  This only affects ext2 conversion, which determine how many bytes of
  dirty tree blocks we need before commit a transaction.

Those two variables would affect the speed of btrfs-convert greatly, and
we mostly use them for the selftests, thus they won't be documented
anyway but the source code.

With those new debug environment variables, we can reduce those values
to minimal, so that the existing convert tests are enough to trigger the
bug of issue #731.

Although the cost is, we make btrfs-convert test case 001 and 003 much
slower than usual (due to much frequent commit transaction commitment
and more IO to read tree blocks).

But I'd say, it's still worthy, as long as it can detect bad
btrfs-convert behaviors.

Qu Wenruo (3):
  btrfs-progs: convert/ext2: new debug environment variable to finetune
    transaction size
  btrfs-progs: new debug environment variable to finetune metadata cache
    size
  btrfs-progs: convert-tests: trigger tree-checker more frequently

 common/utils.c                             | 62 ++++++++++++++++++++++
 common/utils.h                             |  1 +
 convert/source-ext2.c                      |  9 +++-
 kernel-shared/extent_io.c                  |  5 +-
 tests/convert-tests/001-ext2-basic/test.sh |  7 +++
 tests/convert-tests/003-ext4-basic/test.sh |  7 +++
 6 files changed, 89 insertions(+), 2 deletions(-)

--
2.43.0

[PATCH 1/3] btrfs-progs: convert/ext2: new debug environment variable to finetune transaction size

[Qu Wenruo]

Since we got a recent bug report about tree-checker triggered for large
fs conversion, we need a properly way to trigger the problem for test
case purpose.

To trigger that bug, we need to meet several conditions:

- We need to read some tree blocks which has half-backed inodes
- We need a large enough enough fs to generate more tree blocks than
  our cache.

  For our existing test cases, firstly the fs is not that large, thus
  we may even go just one transaction to generate all the inodes.

  Secondly we have a global cache for tree blocks, which means a lot of
  written tree blocks are still in the cache, thus won't trigger
  tree-checker.

To make the problem much easier for our existing test case to expose,
this patch would introduce a debug environment variable:
BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD.

This would affects the threshold for the transaction size, setting it to
a much smaller value would make the bug much easier to trigger.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/utils.c        | 62 +++++++++++++++++++++++++++++++++++++++++++
 common/utils.h        |  1 +
 convert/source-ext2.c |  9 ++++++-
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/common/utils.c b/common/utils.c
index 62f0e3f48b39..e6070791f5cc 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -956,6 +956,68 @@ u8 rand_u8(void)
 	return (u8)(rand_u32());
 }
 
+/*
+ * Parse a u64 value from an environment variable.
+ *
+ * Supports unit suffixes "KMGTP", the suffix is always 2 ** 10 based.
+ * With proper overflow detection.
+ *
+ * The string must end with '\0', anything unexpected non-suffix string,
+ * including space, would lead to -EINVAL and no value updated.
+ */
+int get_env_u64(const char *env_name, u64 *value_ret)
+{
+	char *env_value_str;
+	char *retptr = NULL;
+	int shift = 0;
+	u64 value;
+
+	env_value_str = getenv(env_name);
+	if (!env_value_str)
+		return -ENOENT;
+	value = strtoull(env_value_str, &retptr, 0);
+	/* No numeric string found. */
+	if (retptr == env_value_str)
+		return -EINVAL;
+	if (value == ULLONG_MAX && errno == ERANGE)
+		return -ERANGE;
+	/* memparse_safe() like suffix detection. */
+	switch (*retptr) {
+		case 'K':
+		case 'k':
+			shift = 10;
+			break;
+		case 'M':
+		case 'm':
+			shift = 20;
+			break;
+		case 'G':
+		case 'g':
+			shift = 30;
+			break;
+		case 'T':
+		case 't':
+			shift = 40;
+			break;
+		case 'P':
+		case 'p':
+			shift = 50;
+			break;
+	}
+	if (shift) {
+		retptr++;
+		if (value >> (64 - shift))
+			return -ERANGE;
+		value <<= shift;
+	}
+	if (*retptr != '\0')
+		return -EINVAL;
+	pr_verbose(LOG_VERBOSE, "received env \"%s\" value %llu\n",
+		   env_name, value);
+	*value_ret = value;
+	return 0;
+}
+
 void btrfs_config_init(void)
 {
 	bconf.output_format = CMD_FORMAT_TEXT;
diff --git a/common/utils.h b/common/utils.h
index dcd817144f9c..30c75339b05b 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -132,6 +132,7 @@ u32 rand_u32(void);
 u64 rand_u64(void);
 unsigned int rand_range(unsigned int upper);
 void init_rand_seed(u64 seed);
+int get_env_u64(const char *env_name, u64 *value_ret);
 
 char *btrfs_test_for_multiple_profiles(int fd);
 int btrfs_warn_multiple_profiles(int fd);
diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index f56d79734715..e5f85111a711 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -30,6 +30,7 @@
 #include "kernel-shared/file-item.h"
 #include "common/extent-cache.h"
 #include "common/messages.h"
+#include "common/utils.h"
 #include "convert/common.h"
 #include "convert/source-fs.h"
 #include "convert/source-ext2.h"
@@ -974,6 +975,11 @@ static int ext2_copy_inodes(struct btrfs_convert_context *cctx,
 	ext2_ino_t ext2_ino;
 	u64 objectid;
 	struct btrfs_trans_handle *trans;
+	/* The hint on when to commit the transaction. */
+	u64 blocks_used_threshold = SZ_2M;
+
+	get_env_u64("BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD",
+		    &blocks_used_threshold);
 
 	trans = btrfs_start_transaction(root, 1);
 	if (IS_ERR(trans))
@@ -1014,7 +1020,8 @@ static int ext2_copy_inodes(struct btrfs_convert_context *cctx,
 		 * large enough to contain over 300 inlined files or
 		 * around 26k file extents. Which should be good enough.
 		 */
-		if (trans->blocks_used >= SZ_2M / root->fs_info->nodesize) {
+		if (trans->blocks_used >=
+		    (blocks_used_threshold / root->fs_info->nodesize)) {
 			ret = btrfs_commit_transaction(trans, root);
 			if (ret < 0) {
 				errno = -ret;
-- 
2.43.0

[PATCH 2/3] btrfs-progs: new debug environment variable to finetune metadata cache size

[Qu Wenruo]

Since we got a recent bug report about tree-checker triggered for large
fs conversion, we need a properly way to trigger the problem for test
case purpose.

To trigger that bug, we need to meet several conditions:

- We need to read some tree blocks which has half-backed inodes
- We need a large enough enough fs to generate more tree blocks than
  our cache.

  For our existing test cases, firstly the fs is not that large, thus
  we may even go just one transaction to generate all the inodes.

  Secondly we have a global cache for tree blocks, which means a lot of
  written tree blocks are still in the cache, thus won't trigger
  tree-checker.

To make the problem much easier for our existing test case to expose,
this patch would introduce a debug environment variable:
BTRFS_PROGS_DEBUG_METADATA_CACHE_SIZE.

This variable allows us to finetune the cache size, so that we can
reduce the metadata cache size and trigger more read thus more
tree-checker checks to shake out more bugs.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/extent_io.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/extent_io.c b/kernel-shared/extent_io.c
index ee19430daa12..ce5e4ffb7a59 100644
--- a/kernel-shared/extent_io.c
+++ b/kernel-shared/extent_io.c
@@ -43,7 +43,10 @@ static void free_extent_buffer_final(struct extent_buffer *eb);
 
 void extent_buffer_init_cache(struct btrfs_fs_info *fs_info)
 {
-	fs_info->max_cache_size = total_memory() / 4;
+	u64 max_cache_size = total_memory() / 4;
+
+	get_env_u64("BTRFS_PROGS_DEBUG_METADATA_CACHE_SIZE", &max_cache_size);
+	fs_info->max_cache_size = max_cache_size;
 	fs_info->cache_size = 0;
 	INIT_LIST_HEAD(&fs_info->lru);
 }
-- 
2.43.0

[PATCH 3/3] btrfs-progs: convert-tests: trigger tree-checker more frequently

[Qu Wenruo]

Inspired by a recent bug report where a large ext4 fs can lead to
tree-checker warning on bad previous key objectid, we need a way to
trigger tree-checker more frequently to shake out hidden bugs.

This patch would set blocks_used threshold of btrfs-convert to 16K, and
global metadata cache size to 16K (instead of 1/4 of total memory) for
convert-tests/001 and convert-tests/003.

With those two small values, it's already enough to trigger the bad
previous key objectid bugs (without the proper fixes):

    [TEST/conv]   001-ext2-basic
    [TEST/conv]     ext2 4k nodesize, btrfs defaults
failed: /home/adam/btrfs-progs/btrfs-convert -N 4096 /home/adam/btrfs-progs/tests/test.img
test failed for case 001-ext2-basic

And the failure is exactly the reported one:

 Create ext2 image file
 Create btrfs metadata
 Copy inodes [o] [         0/       529]
 corrupt leaf: root=5 block=147079168 slot=25 ino=267, invalid previous key objectid, have 266 expect 267

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/convert-tests/001-ext2-basic/test.sh | 7 +++++++
 tests/convert-tests/003-ext4-basic/test.sh | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/tests/convert-tests/001-ext2-basic/test.sh b/tests/convert-tests/001-ext2-basic/test.sh
index 6dc105b55e27..35bc39cee243 100755
--- a/tests/convert-tests/001-ext2-basic/test.sh
+++ b/tests/convert-tests/001-ext2-basic/test.sh
@@ -10,6 +10,13 @@ setup_root_helper
 prepare_test_dev
 check_kernel_support_acl
 
+# Extra finetunes to make btrfs-convert to commit transaction for each copied
+# inode, and keep cached tree blocks to minimal.
+# This is to ensure tree-checker is triggered for as many tree blocks as
+# possible, and expose any bad convert metadata behavior.
+export BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD=16K
+export BTRFS_PROGS_DEBUG_METADATA_CACHE_SIZE=16K
+
 # Iterate over defaults and options that are not tied to hardware capabilities
 # or number of devices
 for feature in '' 'block-group-tree' ; do
diff --git a/tests/convert-tests/003-ext4-basic/test.sh b/tests/convert-tests/003-ext4-basic/test.sh
index 8ae5264cb340..64041d91fcb8 100755
--- a/tests/convert-tests/003-ext4-basic/test.sh
+++ b/tests/convert-tests/003-ext4-basic/test.sh
@@ -10,6 +10,13 @@ setup_root_helper
 prepare_test_dev
 check_kernel_support_acl
 
+# Extra finetunes to make btrfs-convert to commit transaction for each copied
+# inode, and keep cached tree blocks to minimal.
+# This is to ensure tree-checker is triggered for as many tree blocks as
+# possible, and expose any bad convert metadata behavior.
+export BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD=16K
+export BTRFS_PROGS_DEBUG_METADATA_CACHE_SIZE=16K
+
 # Iterate over defaults and options that are not tied to hardware capabilities
 # or number of devices
 for feature in '' 'block-group-tree' ; do
-- 
2.43.0

Re: [PATCH 1/3] btrfs-progs: convert/ext2: new debug environment variable to finetune transaction size

[David Sterba]

On Sat, Jan 13, 2024 at 07:15:29PM +1030, Qu Wenruo wrote:
> Since we got a recent bug report about tree-checker triggered for large
> fs conversion, we need a properly way to trigger the problem for test
> case purpose.
> 
> To trigger that bug, we need to meet several conditions:
> 
> - We need to read some tree blocks which has half-backed inodes
> - We need a large enough enough fs to generate more tree blocks than
>   our cache.
> 
>   For our existing test cases, firstly the fs is not that large, thus
>   we may even go just one transaction to generate all the inodes.
> 
>   Secondly we have a global cache for tree blocks, which means a lot of
>   written tree blocks are still in the cache, thus won't trigger
>   tree-checker.
> 
> To make the problem much easier for our existing test case to expose,
> this patch would introduce a debug environment variable:
> BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD.
> 
> This would affects the threshold for the transaction size, setting it to
> a much smaller value would make the bug much easier to trigger.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  common/utils.c        | 62 +++++++++++++++++++++++++++++++++++++++++++
>  common/utils.h        |  1 +
>  convert/source-ext2.c |  9 ++++++-
>  3 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/common/utils.c b/common/utils.c
> index 62f0e3f48b39..e6070791f5cc 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -956,6 +956,68 @@ u8 rand_u8(void)
>  	return (u8)(rand_u32());
>  }
>  
> +/*
> + * Parse a u64 value from an environment variable.
> + *
> + * Supports unit suffixes "KMGTP", the suffix is always 2 ** 10 based.
> + * With proper overflow detection.
> + *
> + * The string must end with '\0', anything unexpected non-suffix string,
> + * including space, would lead to -EINVAL and no value updated.
> + */
> +int get_env_u64(const char *env_name, u64 *value_ret)

There already is a function for parsing sizes parse_size_from_string()
in common/parse-utils.c.

Re: [PATCH 0/3] btrfs-progs: allow tree-checker to be triggered more frequently for btrfs-convert

[David Sterba]

On Sat, Jan 13, 2024 at 07:15:28PM +1030, Qu Wenruo wrote:
> We have issue #731, which is a large ext4 fs, and triggered tree-checker
> very reliably.
> 
> The root cause is the bad write behavior of btrfs-convert, which always
> insert inode refs/file extents/xattrs first, then the inode item.
> 
> Such bad behavior would normally trigger tree-checker, but for our
> regular convert tests, it doesn't get trigger because:
> 
> - We have metadata cache
>   The default size is system memory / 4, which can be very very large.
>   And written tree blocks would still be cached thus no tree-checker
>   triggered for those cached tree blocks.
> 
> - We won't commit transaction for every copied inodes
>   This means for a lot of cases, we may just commit a large transaction
>   for all inodes, further reduce the chance to trigger tree checker.
> 
> This patchset would introduce two debug environment variables:
> 
> - BTRFS_PROGS_DEBUG_METADATA_CACHE_SIZE
>   Global metadata cache size, affecting all btrfs-progs tools that opens
>   an unmounted btrfs.
> 
> - BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD
>   This only affects ext2 conversion, which determine how many bytes of
>   dirty tree blocks we need before commit a transaction.
> 
> Those two variables would affect the speed of btrfs-convert greatly, and
> we mostly use them for the selftests, thus they won't be documented
> anyway but the source code.

Please add some documentation for them to tests/README.md a new '###'
section after 'Verbosity, test tuning'

> Although the cost is, we make btrfs-convert test case 001 and 003 much
> slower than usual (due to much frequent commit transaction commitment
> and more IO to read tree blocks).
> 
> But I'd say, it's still worthy, as long as it can detect bad
> btrfs-convert behaviors.

Yes it's worth, the tests are run manually and before release but it
does not matter if they take more time. We can keep only the 4k and 64k
nodesize cases if the speed is really a big issue.

Re: [PATCH 1/3] btrfs-progs: convert/ext2: new debug environment variable to finetune transaction size

[Qu Wenruo]

[-- Attachment #1.1.1: Type: text/plain, Size: 2550 bytes --]



On 2024/1/17 05:01, David Sterba wrote:
> On Sat, Jan 13, 2024 at 07:15:29PM +1030, Qu Wenruo wrote:
>> Since we got a recent bug report about tree-checker triggered for large
>> fs conversion, we need a properly way to trigger the problem for test
>> case purpose.
>>
>> To trigger that bug, we need to meet several conditions:
>>
>> - We need to read some tree blocks which has half-backed inodes
>> - We need a large enough enough fs to generate more tree blocks than
>>    our cache.
>>
>>    For our existing test cases, firstly the fs is not that large, thus
>>    we may even go just one transaction to generate all the inodes.
>>
>>    Secondly we have a global cache for tree blocks, which means a lot of
>>    written tree blocks are still in the cache, thus won't trigger
>>    tree-checker.
>>
>> To make the problem much easier for our existing test case to expose,
>> this patch would introduce a debug environment variable:
>> BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD.
>>
>> This would affects the threshold for the transaction size, setting it to
>> a much smaller value would make the bug much easier to trigger.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   common/utils.c        | 62 +++++++++++++++++++++++++++++++++++++++++++
>>   common/utils.h        |  1 +
>>   convert/source-ext2.c |  9 ++++++-
>>   3 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/utils.c b/common/utils.c
>> index 62f0e3f48b39..e6070791f5cc 100644
>> --- a/common/utils.c
>> +++ b/common/utils.c
>> @@ -956,6 +956,68 @@ u8 rand_u8(void)
>>   	return (u8)(rand_u32());
>>   }
>>   
>> +/*
>> + * Parse a u64 value from an environment variable.
>> + *
>> + * Supports unit suffixes "KMGTP", the suffix is always 2 ** 10 based.
>> + * With proper overflow detection.
>> + *
>> + * The string must end with '\0', anything unexpected non-suffix string,
>> + * including space, would lead to -EINVAL and no value updated.
>> + */
>> +int get_env_u64(const char *env_name, u64 *value_ret)
> 
> There already is a function for parsing sizes parse_size_from_string()
> in common/parse-utils.c.

Unfortunately that's not suitable.

We don't want a invalid string to fully blow up the program, as the 
existing parser would call exit(1), especially we only need it for a 
debug environmental variable.

Should I change the existing one to provide better error handling?
(Which means around 16 call sites update), or is there some better solution?

Thanks,
Qu

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7027 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/3] btrfs-progs: convert/ext2: new debug environment variable to finetune transaction size

[David Sterba]

On Wed, Jan 17, 2024 at 06:50:11AM +1030, Qu Wenruo wrote:
> On 2024/1/17 05:01, David Sterba wrote:
> > On Sat, Jan 13, 2024 at 07:15:29PM +1030, Qu Wenruo wrote:
> >> Since we got a recent bug report about tree-checker triggered for large
> >> fs conversion, we need a properly way to trigger the problem for test
> >> case purpose.
> >>
> >> To trigger that bug, we need to meet several conditions:
> >>
> >> - We need to read some tree blocks which has half-backed inodes
> >> - We need a large enough enough fs to generate more tree blocks than
> >>    our cache.
> >>
> >>    For our existing test cases, firstly the fs is not that large, thus
> >>    we may even go just one transaction to generate all the inodes.
> >>
> >>    Secondly we have a global cache for tree blocks, which means a lot of
> >>    written tree blocks are still in the cache, thus won't trigger
> >>    tree-checker.
> >>
> >> To make the problem much easier for our existing test case to expose,
> >> this patch would introduce a debug environment variable:
> >> BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD.
> >>
> >> This would affects the threshold for the transaction size, setting it to
> >> a much smaller value would make the bug much easier to trigger.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>   common/utils.c        | 62 +++++++++++++++++++++++++++++++++++++++++++
> >>   common/utils.h        |  1 +
> >>   convert/source-ext2.c |  9 ++++++-
> >>   3 files changed, 71 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/common/utils.c b/common/utils.c
> >> index 62f0e3f48b39..e6070791f5cc 100644
> >> --- a/common/utils.c
> >> +++ b/common/utils.c
> >> @@ -956,6 +956,68 @@ u8 rand_u8(void)
> >>   	return (u8)(rand_u32());
> >>   }
> >>   
> >> +/*
> >> + * Parse a u64 value from an environment variable.
> >> + *
> >> + * Supports unit suffixes "KMGTP", the suffix is always 2 ** 10 based.
> >> + * With proper overflow detection.
> >> + *
> >> + * The string must end with '\0', anything unexpected non-suffix string,
> >> + * including space, would lead to -EINVAL and no value updated.
> >> + */
> >> +int get_env_u64(const char *env_name, u64 *value_ret)
> > 
> > There already is a function for parsing sizes parse_size_from_string()
> > in common/parse-utils.c.
> 
> Unfortunately that's not suitable.
> 
> We don't want a invalid string to fully blow up the program, as the 
> existing parser would call exit(1), especially we only need it for a 
> debug environmental variable.

I see.

> Should I change the existing one to provide better error handling?
> (Which means around 16 call sites update), or is there some better solution?

Yeah, the parsers used for command line arguments are fine to exit as
error handling but generic helper should not do that. There's
arg_strotu64, so I'd keep the arg_ prefix for helpers that will exit on
error.

Looking at parse_size_from_string, it should return int or bool and the
value will be in parameter. This is cleaner than using errno for that.

Re: [PATCH 1/3] btrfs-progs: convert/ext2: new debug environment variable to finetune transaction size

[Qu Wenruo]

On 2024/1/17 11:38, David Sterba wrote:
> On Wed, Jan 17, 2024 at 06:50:11AM +1030, Qu Wenruo wrote:
>> On 2024/1/17 05:01, David Sterba wrote:
>>> On Sat, Jan 13, 2024 at 07:15:29PM +1030, Qu Wenruo wrote:
>>>> Since we got a recent bug report about tree-checker triggered for large
>>>> fs conversion, we need a properly way to trigger the problem for test
>>>> case purpose.
>>>>
>>>> To trigger that bug, we need to meet several conditions:
>>>>
>>>> - We need to read some tree blocks which has half-backed inodes
>>>> - We need a large enough enough fs to generate more tree blocks than
>>>>     our cache.
>>>>
>>>>     For our existing test cases, firstly the fs is not that large, thus
>>>>     we may even go just one transaction to generate all the inodes.
>>>>
>>>>     Secondly we have a global cache for tree blocks, which means a lot of
>>>>     written tree blocks are still in the cache, thus won't trigger
>>>>     tree-checker.
>>>>
>>>> To make the problem much easier for our existing test case to expose,
>>>> this patch would introduce a debug environment variable:
>>>> BTRFS_PROGS_DEBUG_BLOCKS_USED_THRESHOLD.
>>>>
>>>> This would affects the threshold for the transaction size, setting it to
>>>> a much smaller value would make the bug much easier to trigger.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    common/utils.c        | 62 +++++++++++++++++++++++++++++++++++++++++++
>>>>    common/utils.h        |  1 +
>>>>    convert/source-ext2.c |  9 ++++++-
>>>>    3 files changed, 71 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/common/utils.c b/common/utils.c
>>>> index 62f0e3f48b39..e6070791f5cc 100644
>>>> --- a/common/utils.c
>>>> +++ b/common/utils.c
>>>> @@ -956,6 +956,68 @@ u8 rand_u8(void)
>>>>    	return (u8)(rand_u32());
>>>>    }
>>>>
>>>> +/*
>>>> + * Parse a u64 value from an environment variable.
>>>> + *
>>>> + * Supports unit suffixes "KMGTP", the suffix is always 2 ** 10 based.
>>>> + * With proper overflow detection.
>>>> + *
>>>> + * The string must end with '\0', anything unexpected non-suffix string,
>>>> + * including space, would lead to -EINVAL and no value updated.
>>>> + */
>>>> +int get_env_u64(const char *env_name, u64 *value_ret)
>>>
>>> There already is a function for parsing sizes parse_size_from_string()
>>> in common/parse-utils.c.
>>
>> Unfortunately that's not suitable.
>>
>> We don't want a invalid string to fully blow up the program, as the
>> existing parser would call exit(1), especially we only need it for a
>> debug environmental variable.
>
> I see.
>
>> Should I change the existing one to provide better error handling?
>> (Which means around 16 call sites update), or is there some better solution?
>
> Yeah, the parsers used for command line arguments are fine to exit as
> error handling but generic helper should not do that. There's
> arg_strotu64, so I'd keep the arg_ prefix for helpers that will exit on
> error.
>
> Looking at parse_size_from_string, it should return int or bool and the
> value will be in parameter. This is cleaner than using errno for that.
>
And I can take it one step further, make arg_* helper to utilize the
proper parser version (which would return an int to indicate error), and
call exit(1) to error out.

By this, we can share the code and still keep the behavior.

Although it would mean I have to change the call sites.

I'll craft a proper series for the conversion/cleanup.

Thanks,
Qu