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

[Qu Wenruo <wqu@suse.com>]

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