[PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[An Long]

Function _scratch_mkfs_sized cannot recognize the size descriptor.

For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
ext4, will fail with "mkfs.ext4: invalid block size - 4".

The [PATCH v2 2/2] based on [PATCH v2 1/2].

An Long (2):
  common/rc: add _parse_size_string
  common/rc: fix input value to _scratch_mkfs_sized

 common/rc | 66 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 12 deletions(-)


base-commit: 568ac9fffeb6afec03e5d6c9936617232fd7fc6d
-- 
2.35.3

[PATCH v2 1/2] common/rc: add _parse_size_string

[An Long]

Add a helper to convert size value to bytes. This is used to handle
value as bytes, such as 4k to 4096.

Signed-off-by: An Long <lan@suse.com>
---
V1 -> V2:
 - Rename _parse_size_from_string to _parse_size_string
 - Remove unnecessary '$' sign
---
 common/rc | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/common/rc b/common/rc
index 3c072c16..09ffafa4 100644
--- a/common/rc
+++ b/common/rc
@@ -1028,6 +1028,54 @@ _check_minimal_fs_size()
 	fi
 }
 
+# Convert size value to bytes
+# _parse_size_string <size>
+_parse_size_string()
+{
+        local str=$1
+        local mult=1
+        local size
+        local endchar
+
+        if [[ $str =~ ^[0-9]+[a-zA-Z]$ ]] ; then
+                size=${str:: -1}
+                endchar=${str: -1}
+                case $endchar in
+                e|E)
+                                mult=$((mult * 1024))
+                                ;&
+                p|P)
+                                mult=$((mult * 1024))
+                                ;&
+                t|T)
+                                mult=$((mult * 1024))
+                                ;&
+                g|G)
+                                mult=$((mult * 1024))
+                                ;&
+                m|M)
+                                mult=$((mult * 1024))
+                                ;&
+                k|K)
+                                mult=$((mult * 1024))
+                                ;&
+                b|B)
+                                ;;
+                *)
+                                echo "unknown size descriptor $endchar"
+                                exit 1
+                esac
+        elif [[ $str =~ ^[0-9]+$ ]] ; then
+                size=$str
+        else
+                echo "size value $str is invalid"
+                exit 1
+        fi
+
+        size=$((size * mult))
+        echo $size
+}
+
 # Create fs of certain size on scratch device
 # _scratch_mkfs_sized <size in bytes> [optional blocksize]
 _scratch_mkfs_sized()
-- 
2.35.3

[PATCH v2 2/2] common/rc: fix input value to _scratch_mkfs_sized

[An Long]

_scratch_mkfs_sized only receive integer number of bytes as a valid
input. But if the MKFS_OPTIONS variable exists, it will use the value of
block size in MKFS_OPTIONS to override input. In case of
MKFS_OPTIONS="-b 4k", would result in blocksize=4 but not 4096. This
will give errors to ext2/3/4 etc, and brings potential bugs to xfs or
btrfs.

In addition, since we can receive various strings, so remove integer
number check.

This patch depends on patch ("common/rc: add _parse_size_from_string").

Signed-off-by: An Long <lan@suse.com>
---
V1 -> V2:
 - Rename _parse_size_from_string to _parse_size_string
 - Add dependency patch info to commit message
---
 common/rc | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/common/rc b/common/rc
index 09ffafa4..9b5d2f72 100644
--- a/common/rc
+++ b/common/rc
@@ -1077,7 +1077,7 @@ _parse_size_string()
 }
 
 # Create fs of certain size on scratch device
-# _scratch_mkfs_sized <size in bytes> [optional blocksize]
+# _scratch_mkfs_sized <size> [optional blocksize]
 _scratch_mkfs_sized()
 {
 	local fssize=$1
@@ -1086,13 +1086,13 @@ _scratch_mkfs_sized()
 
 	case $FSTYP in
 	xfs)
-		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'`
+		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+[a-zA-Z]?).*/\1/p'`
 		;;
 	btrfs)
-		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+).*/\1/p'`
+		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+[a-zA-Z]?).*/\1/p'`
 		;;
 	ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs)
-		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
+		def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+[a-zA-Z]?).*/\1/p'`
 		;;
 	jfs)
 		def_blksz=4096
@@ -1101,14 +1101,8 @@ _scratch_mkfs_sized()
 
 	[ -n "$def_blksz" ] && blocksize=$def_blksz
 	[ -z "$blocksize" ] && blocksize=4096
-
-	local re='^[0-9]+$'
-	if ! [[ $fssize =~ $re ]] ; then
-		_notrun "error: _scratch_mkfs_sized: fs size \"$fssize\" not an integer."
-	fi
-	if ! [[ $blocksize =~ $re ]] ; then
-		_notrun "error: _scratch_mkfs_sized: block size \"$blocksize\" not an integer."
-	fi
+	blocksize=$(_parse_size_string $blocksize)
+	fssize=$(_parse_size_string $fssize)
 
 	local blocks=`expr $fssize / $blocksize`
 
-- 
2.35.3

Re: [PATCH v2 1/2] common/rc: add _parse_size_string

[Gabriel Krisman Bertazi]

An Long <lan@suse.com> writes:

> +        if [[ $str =~ ^[0-9]+[a-zA-Z]$ ]] ; then
> +                size=${str:: -1}
> +                endchar=${str: -1}
> +                case $endchar in
> +                e|E)
> +                                mult=$((mult * 1024))
> +                                ;&
> +                p|P)
> +                                mult=$((mult * 1024))
> +                                ;&
> +                t|T)
> +                                mult=$((mult * 1024))
> +                                ;&
> +                g|G)
> +                                mult=$((mult * 1024))
> +                                ;&
> +                m|M)
> +                                mult=$((mult * 1024))
> +                                ;&
> +                k|K)
> +                                mult=$((mult * 1024))
> +                                ;&
> +                b|B)
> +                                ;;
> +                *)
> +                                echo "unknown size descriptor $endchar"
> +                                exit 1
> +                esac
> +        elif [[ $str =~ ^[0-9]+$ ]] ; then
> +                size=$str
> +        else
> +                echo "size value $str is invalid"
> +                exit 1
> +        fi
> +
> +        size=$((size * mult))
> +        echo $size
> +}

Hi An,

Coreutils has numfmt(1) to do this kind of conversion.  I wonder if we
could use it here, unless it is not available all the platforms that
matters for xfstests, though:

$ echo 1K | numfmt --from=iec
1024

> +
>  # Create fs of certain size on scratch device
>  # _scratch_mkfs_sized <size in bytes> [optional blocksize]
>  _scratch_mkfs_sized()

-- 
Gabriel Krisman Bertazi

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Dave Chinner]

On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> Function _scratch_mkfs_sized cannot recognize the size descriptor.
> 
> For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
> ext4, will fail with "mkfs.ext4: invalid block size - 4".

So isn't the correct fix for this to use the correct format in
MKFS_OPTIONS? ie. "-b 4096"?

i.e. why do we need ito add code to fix something that the user must
specify themselves and could easily just use an integer to begin
with?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/2] common/rc: add _parse_size_string

[Long An]

On Thu, 2022-06-16 at 11:25 -0400, Gabriel Krisman Bertazi wrote:
> An Long <lan@suse.com> writes:
> 
> > +        if [[ $str =~ ^[0-9]+[a-zA-Z]$ ]] ; then
> > +                size=${str:: -1}
> > +                endchar=${str: -1}
> > +                case $endchar in
> > +                e|E)
> > +                                mult=$((mult * 1024))
> > +                                ;&
> > +                p|P)
> > +                                mult=$((mult * 1024))
> > +                                ;&
> > +                t|T)
> > +                                mult=$((mult * 1024))
> > +                                ;&
> > +                g|G)
> > +                                mult=$((mult * 1024))
> > +                                ;&
> > +                m|M)
> > +                                mult=$((mult * 1024))
> > +                                ;&
> > +                k|K)
> > +                                mult=$((mult * 1024))
> > +                                ;&
> > +                b|B)
> > +                                ;;
> > +                *)
> > +                                echo "unknown size descriptor
> > $endchar"
> > +                                exit 1
> > +                esac
> > +        elif [[ $str =~ ^[0-9]+$ ]] ; then
> > +                size=$str
> > +        else
> > +                echo "size value $str is invalid"
> > +                exit 1
> > +        fi
> > +
> > +        size=$((size * mult))
> > +        echo $size
> > +}
> 
> Hi An,
> 
> Coreutils has numfmt(1) to do this kind of conversion.  I wonder if
> we
> could use it here, unless it is not available all the platforms that
> matters for xfstests, though:
> 
> $ echo 1K | numfmt --from=iec
> 1024
> 

Hi Gabriel,

Using numfmt should reduce the code. But it brings new problems：

1) numfmt doesn't support lowercase
echo 4k | numfmt --from=iec
numfmt: invalid suffix in input: '4k'

2) This cannot clearly point out the error

3) Value range is limited
echo 16E | numfmt --from=iec
numfmt: value too large to be printed: '1.84467e+19' (consider using --
to)

4) Added system dependencies

More code is required to solve above problems, and not elegant.

> > +
> >  # Create fs of certain size on scratch device
> >  # _scratch_mkfs_sized <size in bytes> [optional blocksize]
> >  _scratch_mkfs_sized()
-- 
An Long <lan@suse.com>
SUSE QE LSG, QE 2, Beijing

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Long An]

On Fri, 2022-06-17 at 13:58 +1000, Dave Chinner wrote:
> On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > Function _scratch_mkfs_sized cannot recognize the size descriptor.
> > 
> > For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416
> > on
> > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> 
> So isn't the correct fix for this to use the correct format in
> MKFS_OPTIONS? ie. "-b 4096"?
> 
MKFS_OPTIONS="-b 4096" will get the correct result 4096.

> i.e. why do we need ito add code to fix something that the user must
> specify themselves and could easily just use an integer to begin
> with?
> 
This function must input an integer for now. But this function also
overwrites user input with MKFS_OPTIONS for compatibility. And for the
same reason MKFS_OPTIONS needs to receive such parameters as 4k.

> Cheers,
> 
> Dave.

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Zorro Lang]

On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > Function _scratch_mkfs_sized cannot recognize the size descriptor.
> > 
> > For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
> > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> 
> So isn't the correct fix for this to use the correct format in
> MKFS_OPTIONS? ie. "-b 4096"?
> 
> i.e. why do we need ito add code to fix something that the user must
> specify themselves and could easily just use an integer to begin
> with?

The fstests doesn't notice users that they *must* use pure number in
MKFS_OPTIONS, especially the block size.
As this issue, we have to accept the size unit(k/m/g ...) or have to
tell the users that they must use pure integer, any of "k/m/g" is
unacceptable. I prefer the former, so I'd like to have this change,
except anyone has a better idea :)

Thanks,
Zorro

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Dave Chinner]

On Sat, Jun 18, 2022 at 01:52:12AM +0800, Zorro Lang wrote:
> On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> > On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > > Function _scratch_mkfs_sized cannot recognize the size descriptor.
> > > 
> > > For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
> > > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> > 
> > So isn't the correct fix for this to use the correct format in
> > MKFS_OPTIONS? ie. "-b 4096"?
> > 
> > i.e. why do we need ito add code to fix something that the user must
> > specify themselves and could easily just use an integer to begin
> > with?
> 
> The fstests doesn't notice users that they *must* use pure number in
> MKFS_OPTIONS, especially the block size.

So why not just document the requirement? I mean,
_mkfs_scratch_sized is documented to take the size in bytes primarly
because some mkfs binaries only suport bytes and not shortform human
numbers. We did that because it was seen that consistency in all the
tests of using byte counts was much preferable to having a random
smattering of different units. It's much easier to programatically
calculate the size if it is in bytes, and that results in simpler
and easier to understand code.

The main issue I have here is that we need to reduce the overhead of
setting up every test - adding more complex parameter parsing to
_scratch_mkfs_sized means every test that calls it now takes just a
little bit longer to run. That's about a 100 tests that now take
just a little longer to run, meaning fstests takes a few seconds
more to run.

Every "lets make this pretty parsing" or "lets do complex parsing"
change that replaces a simple, straight forward integer or construct
adds bloat, overhead and runtime. It's a death-by-a-thousand-cuts
scenario - each individual change can be justified, but suddenly
we've adding another 15 minutes of runtime to fstests even for
people who require/use none of that functionality.

Bloat, overhead and runtime are the three main things we
need to remove from fstests, so I feel that the right thing to do is
document that sizes for filesystems, block sizes, etc must always be
in integer units rather than adding code to "be flexible".

> As this issue, we have to accept the size unit(k/m/g ...) or have to
> tell the users that they must use pure integer, any of "k/m/g" is
> unacceptable. I prefer the former, so I'd like to have this change,
> except anyone has a better idea :)

I'd much prefer we document the existing behaviour rather than add
more complexity and overhead....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Zorro Lang]

On Sat, Jun 18, 2022 at 08:24:05AM +1000, Dave Chinner wrote:
> On Sat, Jun 18, 2022 at 01:52:12AM +0800, Zorro Lang wrote:
> > On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> > > On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > > > Function _scratch_mkfs_sized cannot recognize the size descriptor.
> > > > 
> > > > For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
> > > > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> > > 
> > > So isn't the correct fix for this to use the correct format in
> > > MKFS_OPTIONS? ie. "-b 4096"?
> > > 
> > > i.e. why do we need ito add code to fix something that the user must
> > > specify themselves and could easily just use an integer to begin
> > > with?
> > 
> > The fstests doesn't notice users that they *must* use pure number in
> > MKFS_OPTIONS, especially the block size.
> 
> So why not just document the requirement? I mean,
> _mkfs_scratch_sized is documented to take the size in bytes primarly
> because some mkfs binaries only suport bytes and not shortform human
> numbers. We did that because it was seen that consistency in all the
> tests of using byte counts was much preferable to having a random
> smattering of different units. It's much easier to programatically
> calculate the size if it is in bytes, and that results in simpler
> and easier to understand code.
> 
> The main issue I have here is that we need to reduce the overhead of
> setting up every test - adding more complex parameter parsing to
> _scratch_mkfs_sized means every test that calls it now takes just a
> little bit longer to run. That's about a 100 tests that now take
> just a little longer to run, meaning fstests takes a few seconds
> more to run.

Oh, so that's what's your really worry about. Understand. But will this
change takes that long time? If the user still use pure integer as usual,
it'll through:

    elif [[ $str =~ ^[0-9]+$ ]] ; then
            size=$str

then return directly, won't through those complex calculation. I didn't
test, but I think it's fast enough. We even can put this judgement to be
the first judgement, let the pure integer won't be affected much.

Then we document that "recommend pure integer in MKFS_OPTIONS". How about
that?

Due to if we only tell the users to use pure integer in document, if they
don't follow that, and cause some tests actually didn't run as expected,
but we don't report any failures. That looks I'm all talk, no clear
"bad sequent" if no one follow.

So if we don't merge this patchset. I'd like to make something wrong to
remind that "must use pure integer in MKFS_OPTIONS". What do you think?

Thanks,
Zorro

> 
> Every "lets make this pretty parsing" or "lets do complex parsing"
> change that replaces a simple, straight forward integer or construct
> adds bloat, overhead and runtime. It's a death-by-a-thousand-cuts
> scenario - each individual change can be justified, but suddenly
> we've adding another 15 minutes of runtime to fstests even for
> people who require/use none of that functionality.
> 
> Bloat, overhead and runtime are the three main things we
> need to remove from fstests, so I feel that the right thing to do is
> document that sizes for filesystems, block sizes, etc must always be
> in integer units rather than adding code to "be flexible".
> 
> > As this issue, we have to accept the size unit(k/m/g ...) or have to
> > tell the users that they must use pure integer, any of "k/m/g" is
> > unacceptable. I prefer the former, so I'd like to have this change,
> > except anyone has a better idea :)
> 
> I'd much prefer we document the existing behaviour rather than add
> more complexity and overhead....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Dave Chinner]

On Sat, Jun 18, 2022 at 11:14:13AM +0800, Zorro Lang wrote:
> On Sat, Jun 18, 2022 at 08:24:05AM +1000, Dave Chinner wrote:
> > On Sat, Jun 18, 2022 at 01:52:12AM +0800, Zorro Lang wrote:
> > > On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> > > > On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > > > > Function _scratch_mkfs_sized cannot recognize the size descriptor.
> > > > > 
> > > > > For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
> > > > > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> > > > 
> > > > So isn't the correct fix for this to use the correct format in
> > > > MKFS_OPTIONS? ie. "-b 4096"?
> > > > 
> > > > i.e. why do we need ito add code to fix something that the user must
> > > > specify themselves and could easily just use an integer to begin
> > > > with?
> > > 
> > > The fstests doesn't notice users that they *must* use pure number in
> > > MKFS_OPTIONS, especially the block size.
> > 
> > So why not just document the requirement? I mean,
> > _mkfs_scratch_sized is documented to take the size in bytes primarly
> > because some mkfs binaries only suport bytes and not shortform human
> > numbers. We did that because it was seen that consistency in all the
> > tests of using byte counts was much preferable to having a random
> > smattering of different units. It's much easier to programatically
> > calculate the size if it is in bytes, and that results in simpler
> > and easier to understand code.
> > 
> > The main issue I have here is that we need to reduce the overhead of
> > setting up every test - adding more complex parameter parsing to
> > _scratch_mkfs_sized means every test that calls it now takes just a
> > little bit longer to run. That's about a 100 tests that now take
> > just a little longer to run, meaning fstests takes a few seconds
> > more to run.
> 
> Oh, so that's what's your really worry about. Understand. But will this
> change takes that long time? If the user still use pure integer as usual,
> it'll through:
> 
>     elif [[ $str =~ ^[0-9]+$ ]] ; then
>             size=$str
> 
> then return directly, won't through those complex calculation.

It's still additional unnecessary overhead to be adding to
_mkfs_scratch_sized as user supplied MKFS_OPTIONS do not change from
test to test.  So why do we even want to do this verification every
time _mkfs_scratch_sized is run?  Look at the *big picture*, not the
individual test context . Validate the user supplied MKFS_OPTIONS
once at startup, not every time _mkfs_scratch_sized is run!

And looking at the big picture, we have a bunch of scratch_mkfs
operations that take byte counts.  _scratch_mkfs_geom that takes
stripe aligment parameters in bytes, _scratch_mkfs_blocksized that
takes block size in bytes, _mkfs_scratch_sized that takes the fs
size (and block size) in bytes, etc.

Bytes as an integer count  is the common unit across all tests,
filesystems and APIs. We can do math directly on them, we don't need
to care if different filesystems support some form of human readable
or not (e.g. some filesystems will recognise "k" but not "K" for
kilobytes), etc.

So if you've going to actually support human readable units for byte
values, think through the consequences of doing that. Think about
the difficultly that then poses for tests that are written as

_mkfs_scratch_sized 1G

and now someone else comes along, needs to modify the test and do
calculations based on the size of the filesystem. Do we expect the
test to now have string parsing in it to convert the filesystem size
to an integer for this new functionality? Or do we then have to
convert every part of the test to use byte units instead of human
units before making the modification? Either way, it adds more work
to future changes than the amount of tiem and work it might save
now.

Hence to me, the big picture implications of allowing human readable
units in fstests code and configs just does not add up to be a net
positive.

> So if we don't merge this patchset. I'd like to make something wrong to
> remind that "must use pure integer in MKFS_OPTIONS". What do you think?

IMO, a single validation check after section config loading in check
is all that is necessary....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Zorro Lang]

On Tue, Jun 21, 2022 at 09:12:29AM +1000, Dave Chinner wrote:
> On Sat, Jun 18, 2022 at 11:14:13AM +0800, Zorro Lang wrote:
> > On Sat, Jun 18, 2022 at 08:24:05AM +1000, Dave Chinner wrote:
> > > On Sat, Jun 18, 2022 at 01:52:12AM +0800, Zorro Lang wrote:
> > > > On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> > > > > On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > > > > > Function _scratch_mkfs_sized cannot recognize the size descriptor.
> > > > > > 
> > > > > > For example, we set MKFS_OPTIONS="-b 4k" and then run generic/416 on
> > > > > > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> > > > > 
> > > > > So isn't the correct fix for this to use the correct format in
> > > > > MKFS_OPTIONS? ie. "-b 4096"?
> > > > > 
> > > > > i.e. why do we need ito add code to fix something that the user must
> > > > > specify themselves and could easily just use an integer to begin
> > > > > with?
> > > > 
> > > > The fstests doesn't notice users that they *must* use pure number in
> > > > MKFS_OPTIONS, especially the block size.
> > > 
> > > So why not just document the requirement? I mean,
> > > _mkfs_scratch_sized is documented to take the size in bytes primarly
> > > because some mkfs binaries only suport bytes and not shortform human
> > > numbers. We did that because it was seen that consistency in all the
> > > tests of using byte counts was much preferable to having a random
> > > smattering of different units. It's much easier to programatically
> > > calculate the size if it is in bytes, and that results in simpler
> > > and easier to understand code.
> > > 
> > > The main issue I have here is that we need to reduce the overhead of
> > > setting up every test - adding more complex parameter parsing to
> > > _scratch_mkfs_sized means every test that calls it now takes just a
> > > little bit longer to run. That's about a 100 tests that now take
> > > just a little longer to run, meaning fstests takes a few seconds
> > > more to run.
> > 
> > Oh, so that's what's your really worry about. Understand. But will this
> > change takes that long time? If the user still use pure integer as usual,
> > it'll through:
> > 
> >     elif [[ $str =~ ^[0-9]+$ ]] ; then
> >             size=$str
> > 
> > then return directly, won't through those complex calculation.
> 
> It's still additional unnecessary overhead to be adding to
> _mkfs_scratch_sized as user supplied MKFS_OPTIONS do not change from
> test to test.  So why do we even want to do this verification every
> time _mkfs_scratch_sized is run?  Look at the *big picture*, not the
> individual test context . Validate the user supplied MKFS_OPTIONS
> once at startup, not every time _mkfs_scratch_sized is run!
> 
> And looking at the big picture, we have a bunch of scratch_mkfs
> operations that take byte counts.  _scratch_mkfs_geom that takes
> stripe aligment parameters in bytes, _scratch_mkfs_blocksized that
> takes block size in bytes, _mkfs_scratch_sized that takes the fs
> size (and block size) in bytes, etc.
> 
> Bytes as an integer count  is the common unit across all tests,
> filesystems and APIs. We can do math directly on them, we don't need
> to care if different filesystems support some form of human readable
> or not (e.g. some filesystems will recognise "k" but not "K" for
> kilobytes), etc.
> 
> So if you've going to actually support human readable units for byte
> values, think through the consequences of doing that. Think about
> the difficultly that then poses for tests that are written as
> 
> _mkfs_scratch_sized 1G
> 
> and now someone else comes along, needs to modify the test and do
> calculations based on the size of the filesystem. Do we expect the
> test to now have string parsing in it to convert the filesystem size
> to an integer for this new functionality? Or do we then have to
> convert every part of the test to use byte units instead of human
> units before making the modification? Either way, it adds more work
> to future changes than the amount of tiem and work it might save
> now.
> 
> Hence to me, the big picture implications of allowing human readable
> units in fstests code and configs just does not add up to be a net
> positive.
> 
> > So if we don't merge this patchset. I'd like to make something wrong to
> > remind that "must use pure integer in MKFS_OPTIONS". What do you think?
> 
> IMO, a single validation check after section config loading in check
> is all that is necessary....

OK, so we agree on this.

Hi Long, if you're still interested in fixing this issue, please change it as:
1) Check MKFS_OPTIONS (and other options if need) at local.config loading time,
   make sure it follow the rules (pure integer)
2) Add this rule into doc (README)

Or I can help to do that, and mark you as "Reported-by", if you don't have
time to do that.

Thanks,
Zorro

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Long An]

On Tue, 2022-06-21 at 12:05 +0800, Zorro Lang wrote:
> On Tue, Jun 21, 2022 at 09:12:29AM +1000, Dave Chinner wrote:
> > On Sat, Jun 18, 2022 at 11:14:13AM +0800, Zorro Lang wrote:
> > > On Sat, Jun 18, 2022 at 08:24:05AM +1000, Dave Chinner wrote:
> > > > On Sat, Jun 18, 2022 at 01:52:12AM +0800, Zorro Lang wrote:
> > > > > On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> > > > > > On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > > > > > > Function _scratch_mkfs_sized cannot recognize the size
> > > > > > > descriptor.
> > > > > > > 
> > > > > > > For example, we set MKFS_OPTIONS="-b 4k" and then run
> > > > > > > generic/416 on
> > > > > > > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> > > > > > 
> > > > > > So isn't the correct fix for this to use the correct format
> > > > > > in
> > > > > > MKFS_OPTIONS? ie. "-b 4096"?
> > > > > > 
> > > > > > i.e. why do we need ito add code to fix something that the
> > > > > > user must
> > > > > > specify themselves and could easily just use an integer to
> > > > > > begin
> > > > > > with?
> > > > > 
> > > > > The fstests doesn't notice users that they *must* use pure
> > > > > number in
> > > > > MKFS_OPTIONS, especially the block size.
> > > > 
> > > > So why not just document the requirement? I mean,
> > > > _mkfs_scratch_sized is documented to take the size in bytes
> > > > primarly
> > > > because some mkfs binaries only suport bytes and not shortform
> > > > human
> > > > numbers. We did that because it was seen that consistency in
> > > > all the
> > > > tests of using byte counts was much preferable to having a
> > > > random
> > > > smattering of different units. It's much easier to
> > > > programatically
> > > > calculate the size if it is in bytes, and that results in
> > > > simpler
> > > > and easier to understand code.
> > > > 
> > > > The main issue I have here is that we need to reduce the
> > > > overhead of
> > > > setting up every test - adding more complex parameter parsing
> > > > to
> > > > _scratch_mkfs_sized means every test that calls it now takes
> > > > just a
> > > > little bit longer to run. That's about a 100 tests that now
> > > > take
> > > > just a little longer to run, meaning fstests takes a few
> > > > seconds
> > > > more to run.
> > > 
> > > Oh, so that's what's your really worry about. Understand. But
> > > will this
> > > change takes that long time? If the user still use pure integer
> > > as usual,
> > > it'll through:
> > > 
> > >     elif [[ $str =~ ^[0-9]+$ ]] ; then
> > >             size=$str
> > > 
> > > then return directly, won't through those complex calculation.
> > 
> > It's still additional unnecessary overhead to be adding to
> > _mkfs_scratch_sized as user supplied MKFS_OPTIONS do not change
> > from
> > test to test.  So why do we even want to do this verification every
> > time _mkfs_scratch_sized is run?  Look at the *big picture*, not
> > the
> > individual test context . Validate the user supplied MKFS_OPTIONS
> > once at startup, not every time _mkfs_scratch_sized is run!
> > 
> > And looking at the big picture, we have a bunch of scratch_mkfs
> > operations that take byte counts.  _scratch_mkfs_geom that takes
> > stripe aligment parameters in bytes, _scratch_mkfs_blocksized that
> > takes block size in bytes, _mkfs_scratch_sized that takes the fs
> > size (and block size) in bytes, etc.
> > 
> > Bytes as an integer count  is the common unit across all tests,
> > filesystems and APIs. We can do math directly on them, we don't
> > need
> > to care if different filesystems support some form of human
> > readable
> > or not (e.g. some filesystems will recognise "k" but not "K" for
> > kilobytes), etc.
> > 
> > So if you've going to actually support human readable units for
> > byte
> > values, think through the consequences of doing that. Think about
> > the difficultly that then poses for tests that are written as
> > 
> > _mkfs_scratch_sized 1G
> > 
> > and now someone else comes along, needs to modify the test and do
> > calculations based on the size of the filesystem. Do we expect the
> > test to now have string parsing in it to convert the filesystem
> > size
> > to an integer for this new functionality? Or do we then have to
> > convert every part of the test to use byte units instead of human
> > units before making the modification? Either way, it adds more work
> > to future changes than the amount of tiem and work it might save
> > now.
> > 
> > Hence to me, the big picture implications of allowing human
> > readable
> > units in fstests code and configs just does not add up to be a net
> > positive.
> > 
> > > So if we don't merge this patchset. I'd like to make something
> > > wrong to
> > > remind that "must use pure integer in MKFS_OPTIONS". What do you
> > > think?
> > 
> > IMO, a single validation check after section config loading in
> > check
> > is all that is necessary....
> 
> OK, so we agree on this.
> 
> Hi Long, if you're still interested in fixing this issue, please
> change it as:
> 1) Check MKFS_OPTIONS (and other options if need) at local.config
> loading time,
>    make sure it follow the rules (pure integer)
> 2) Add this rule into doc (README)
> 
> Or I can help to do that, and mark you as "Reported-by", if you don't
> have
> time to do that.

OK， I'll fix it ASAP according to the comments

> 
> Thanks,
> Zorro
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 

Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

[Zorro Lang]

On Tue, Jun 21, 2022 at 04:25:07AM +0000, Long An wrote:
> On Tue, 2022-06-21 at 12:05 +0800, Zorro Lang wrote:
> > On Tue, Jun 21, 2022 at 09:12:29AM +1000, Dave Chinner wrote:
> > > On Sat, Jun 18, 2022 at 11:14:13AM +0800, Zorro Lang wrote:
> > > > On Sat, Jun 18, 2022 at 08:24:05AM +1000, Dave Chinner wrote:
> > > > > On Sat, Jun 18, 2022 at 01:52:12AM +0800, Zorro Lang wrote:
> > > > > > On Fri, Jun 17, 2022 at 01:58:36PM +1000, Dave Chinner wrote:
> > > > > > > On Thu, Jun 16, 2022 at 12:38:43PM +0800, An Long wrote:
> > > > > > > > Function _scratch_mkfs_sized cannot recognize the size
> > > > > > > > descriptor.
> > > > > > > > 
> > > > > > > > For example, we set MKFS_OPTIONS="-b 4k" and then run
> > > > > > > > generic/416 on
> > > > > > > > ext4, will fail with "mkfs.ext4: invalid block size - 4".
> > > > > > > 
> > > > > > > So isn't the correct fix for this to use the correct format
> > > > > > > in
> > > > > > > MKFS_OPTIONS? ie. "-b 4096"?
> > > > > > > 
> > > > > > > i.e. why do we need ito add code to fix something that the
> > > > > > > user must
> > > > > > > specify themselves and could easily just use an integer to
> > > > > > > begin
> > > > > > > with?
> > > > > > 
> > > > > > The fstests doesn't notice users that they *must* use pure
> > > > > > number in
> > > > > > MKFS_OPTIONS, especially the block size.
> > > > > 
> > > > > So why not just document the requirement? I mean,
> > > > > _mkfs_scratch_sized is documented to take the size in bytes
> > > > > primarly
> > > > > because some mkfs binaries only suport bytes and not shortform
> > > > > human
> > > > > numbers. We did that because it was seen that consistency in
> > > > > all the
> > > > > tests of using byte counts was much preferable to having a
> > > > > random
> > > > > smattering of different units. It's much easier to
> > > > > programatically
> > > > > calculate the size if it is in bytes, and that results in
> > > > > simpler
> > > > > and easier to understand code.
> > > > > 
> > > > > The main issue I have here is that we need to reduce the
> > > > > overhead of
> > > > > setting up every test - adding more complex parameter parsing
> > > > > to
> > > > > _scratch_mkfs_sized means every test that calls it now takes
> > > > > just a
> > > > > little bit longer to run. That's about a 100 tests that now
> > > > > take
> > > > > just a little longer to run, meaning fstests takes a few
> > > > > seconds
> > > > > more to run.
> > > > 
> > > > Oh, so that's what's your really worry about. Understand. But
> > > > will this
> > > > change takes that long time? If the user still use pure integer
> > > > as usual,
> > > > it'll through:
> > > > 
> > > >     elif [[ $str =~ ^[0-9]+$ ]] ; then
> > > >             size=$str
> > > > 
> > > > then return directly, won't through those complex calculation.
> > > 
> > > It's still additional unnecessary overhead to be adding to
> > > _mkfs_scratch_sized as user supplied MKFS_OPTIONS do not change
> > > from
> > > test to test.  So why do we even want to do this verification every
> > > time _mkfs_scratch_sized is run?  Look at the *big picture*, not
> > > the
> > > individual test context . Validate the user supplied MKFS_OPTIONS
> > > once at startup, not every time _mkfs_scratch_sized is run!
> > > 
> > > And looking at the big picture, we have a bunch of scratch_mkfs
> > > operations that take byte counts.  _scratch_mkfs_geom that takes
> > > stripe aligment parameters in bytes, _scratch_mkfs_blocksized that
> > > takes block size in bytes, _mkfs_scratch_sized that takes the fs
> > > size (and block size) in bytes, etc.
> > > 
> > > Bytes as an integer count  is the common unit across all tests,
> > > filesystems and APIs. We can do math directly on them, we don't
> > > need
> > > to care if different filesystems support some form of human
> > > readable
> > > or not (e.g. some filesystems will recognise "k" but not "K" for
> > > kilobytes), etc.
> > > 
> > > So if you've going to actually support human readable units for
> > > byte
> > > values, think through the consequences of doing that. Think about
> > > the difficultly that then poses for tests that are written as
> > > 
> > > _mkfs_scratch_sized 1G
> > > 
> > > and now someone else comes along, needs to modify the test and do
> > > calculations based on the size of the filesystem. Do we expect the
> > > test to now have string parsing in it to convert the filesystem
> > > size
> > > to an integer for this new functionality? Or do we then have to
> > > convert every part of the test to use byte units instead of human
> > > units before making the modification? Either way, it adds more work
> > > to future changes than the amount of tiem and work it might save
> > > now.
> > > 
> > > Hence to me, the big picture implications of allowing human
> > > readable
> > > units in fstests code and configs just does not add up to be a net
> > > positive.
> > > 
> > > > So if we don't merge this patchset. I'd like to make something
> > > > wrong to
> > > > remind that "must use pure integer in MKFS_OPTIONS". What do you
> > > > think?
> > > 
> > > IMO, a single validation check after section config loading in
> > > check
> > > is all that is necessary....
> > 
> > OK, so we agree on this.
> > 
> > Hi Long, if you're still interested in fixing this issue, please
> > change it as:
> > 1) Check MKFS_OPTIONS (and other options if need) at local.config
> > loading time,
> >    make sure it follow the rules (pure integer)
> > 2) Add this rule into doc (README)
> > 
> > Or I can help to do that, and mark you as "Reported-by", if you don't
> > have
> > time to do that.
> 
> OK， I'll fix it ASAP according to the comments

Thanks, no push :)

> 
> > 
> > Thanks,
> > Zorro
> > 
> > > Cheers,
> > > 
> > > Dave.
> > > -- 
> > > Dave Chinner
> > > david@fromorbit.com
> > >