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

[David Sterba <dsterba@suse.cz>]

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.