[PATCH] common/config: Fix use of MKFS_OPTIONS

[PATCH] common/config: Fix use of MKFS_OPTIONS

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

Therefore, add validation check to force MKFS_OPTIONS to use pure
integer in bytes for any size value.

Signed-off-by: An Long <lan@suse.com>
---
 README        | 3 ++-
 common/config | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/README b/README
index 80d148be..7c2d3334 100644
--- a/README
+++ b/README
@@ -241,7 +241,8 @@ Misc:
    this option is supported for all filesystems currently only -overlay is
    expected to run without issues. For other filesystems additional patches
    and fixes to the test suite might be needed.
-
+ - If MKFS_OPTIONS is used, the size value must be an integer number of bytes
+   without units. For example, set MKFS_OPTIONS="-b 4096" but not "-b 4k".
 ______________________
 USING THE FSQA SUITE
 ______________________
diff --git a/common/config b/common/config
index de3aba15..ec723ac4 100644
--- a/common/config
+++ b/common/config
@@ -896,5 +896,10 @@ else
 	fi
 fi
 
+# check the size value in $MKFS_OPTIONS is an integer
+if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
+        _fatal "error: \$MKFS_OPTIONS: Only number in bytes is allowed, no units."
+fi
+
 # make sure this script returns success
 /bin/true
-- 
2.35.3

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Darrick J. Wong]

On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> _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.
> 
> Therefore, add validation check to force MKFS_OPTIONS to use pure
> integer in bytes for any size value.
> 
> Signed-off-by: An Long <lan@suse.com>
> ---
>  README        | 3 ++-
>  common/config | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/README b/README
> index 80d148be..7c2d3334 100644
> --- a/README
> +++ b/README
> @@ -241,7 +241,8 @@ Misc:
>     this option is supported for all filesystems currently only -overlay is
>     expected to run without issues. For other filesystems additional patches
>     and fixes to the test suite might be needed.
> -
> + - If MKFS_OPTIONS is used, the size value must be an integer number of bytes
> +   without units. For example, set MKFS_OPTIONS="-b 4096" but not "-b 4k".

IDGI.  mkfs.$FSTYP does its own input validation, which means that fstest
runners are required to set MKFS_OPTIONS to something that mkfs will
accept.  It's not the job of fstests to add /another/ layer of
validation...

>  ______________________
>  USING THE FSQA SUITE
>  ______________________
> diff --git a/common/config b/common/config
> index de3aba15..ec723ac4 100644
> --- a/common/config
> +++ b/common/config
> @@ -896,5 +896,10 @@ else
>  	fi
>  fi
>  
> +# check the size value in $MKFS_OPTIONS is an integer
> +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then

...because this regex will break /any/ MKFS_OPTIONS with a number
followed by a letter.  mkfs.xfs handles units just fine, so I don't
understand why you're adding this blunt instrument.

MKFS_OPTIONS='-b size=1k' works just fine for XFS.

--D

> +        _fatal "error: \$MKFS_OPTIONS: Only number in bytes is allowed, no units."
> +fi
> +
>  # make sure this script returns success
>  /bin/true
> -- 
> 2.35.3
> 

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Zorro Lang]

On Thu, Jun 23, 2022 at 11:17:44AM -0700, Darrick J. Wong wrote:
> On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> > _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.
> > 
> > Therefore, add validation check to force MKFS_OPTIONS to use pure
> > integer in bytes for any size value.
> > 
> > Signed-off-by: An Long <lan@suse.com>
> > ---
> >  README        | 3 ++-
> >  common/config | 5 +++++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/README b/README
> > index 80d148be..7c2d3334 100644
> > --- a/README
> > +++ b/README
> > @@ -241,7 +241,8 @@ Misc:
> >     this option is supported for all filesystems currently only -overlay is
> >     expected to run without issues. For other filesystems additional patches
> >     and fixes to the test suite might be needed.
> > -
> > + - If MKFS_OPTIONS is used, the size value must be an integer number of bytes
> > +   without units. For example, set MKFS_OPTIONS="-b 4096" but not "-b 4k".
> 
> IDGI.  mkfs.$FSTYP does its own input validation, which means that fstest
> runners are required to set MKFS_OPTIONS to something that mkfs will
> accept.  It's not the job of fstests to add /another/ layer of
> validation...
> 
> >  ______________________
> >  USING THE FSQA SUITE
> >  ______________________
> > diff --git a/common/config b/common/config
> > index de3aba15..ec723ac4 100644
> > --- a/common/config
> > +++ b/common/config
> > @@ -896,5 +896,10 @@ else
> >  	fi
> >  fi
> >  
> > +# check the size value in $MKFS_OPTIONS is an integer
> > +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> 
> ...because this regex will break /any/ MKFS_OPTIONS with a number
> followed by a letter.  mkfs.xfs handles units just fine, so I don't
> understand why you're adding this blunt instrument.
> 
> MKFS_OPTIONS='-b size=1k' works just fine for XFS.

Hi Darrick,

That's another story from this patch review:
https://lore.kernel.org/fstests/20220616043845.14320-1-lan@suse.com/

Any more review points about that?

Thanks,
Zorro

> 
> --D
> 
> > +        _fatal "error: \$MKFS_OPTIONS: Only number in bytes is allowed, no units."
> > +fi
> > +
> >  # make sure this script returns success
> >  /bin/true
> > -- 
> > 2.35.3
> > 
> 

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Dave Chinner]

On Thu, Jun 23, 2022 at 11:17:44AM -0700, Darrick J. Wong wrote:
> On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> > _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.
> > 
> > Therefore, add validation check to force MKFS_OPTIONS to use pure
> > integer in bytes for any size value.
> > 
> > Signed-off-by: An Long <lan@suse.com>
> > ---
> >  README        | 3 ++-
> >  common/config | 5 +++++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/README b/README
> > index 80d148be..7c2d3334 100644
> > --- a/README
> > +++ b/README
> > @@ -241,7 +241,8 @@ Misc:
> >     this option is supported for all filesystems currently only -overlay is
> >     expected to run without issues. For other filesystems additional patches
> >     and fixes to the test suite might be needed.
> > -
> > + - If MKFS_OPTIONS is used, the size value must be an integer number of bytes
> > +   without units. For example, set MKFS_OPTIONS="-b 4096" but not "-b 4k".
> 
> IDGI.  mkfs.$FSTYP does its own input validation, which means that fstest
> runners are required to set MKFS_OPTIONS to something that mkfs will
> accept.  It's not the job of fstests to add /another/ layer of
> validation...

It's not validating it - it refelecting the fact that fstests parses
the block device size out of MKFS_OPTIONS and does integer math on
it and does not handle human readable units....

> >  USING THE FSQA SUITE
> >  ______________________
> > diff --git a/common/config b/common/config
> > index de3aba15..ec723ac4 100644
> > --- a/common/config
> > +++ b/common/config
> > @@ -896,5 +896,10 @@ else
> >  	fi
> >  fi
> >  
> > +# check the size value in $MKFS_OPTIONS is an integer
> > +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> 
> ...because this regex will break /any/ MKFS_OPTIONS with a number
> followed by a letter.  mkfs.xfs handles units just fine, so I don't
> understand why you're adding this blunt instrument.
> 
> MKFS_OPTIONS='-b size=1k' works just fine for XFS.

Except it where it doesn't.

For example, _scratch_mkfs_sized does unexpected stuff
because it assumes filesystem block sizes are all in bytes when it
tries to parse the custom test block sizes out of MKFS_OPTIONS to
set the default block size.

https://lore.kernel.org/fstests/20220616043845.14320-1-lan@suse.com/

In this case, using "1k" in the XFS mkfs specifications results in
the default block size is incorrectly calculated:

$ echo "-bsize=1024" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
1024
$ echo "-bsize=1k" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
1
$

So it sets the block size to 1, not 1024.

But then this bug is covered up later on by this code:

                # don't override MKFS_OPTIONS that set a block size.
                echo $MKFS_OPTIONS |egrep -q "b?size="
                if [ $? -eq 0 ]; then
                        _scratch_mkfs_xfs -d size=$fssize $rt_ops
                else
                        _scratch_mkfs_xfs -d size=$fssize $rt_ops -b size=$blocksize
                fi

Which omits the poorly calculated block size because there's
already a block size specified in the MKFS_OPTIONS so it ignores the
fact it calculated the block size incorrectly from MKFS_OPTIONS.

But it also means that if the test specifies a block size via

_scratch_mkfs_sized $((256 * 1024 * 1024)) 2048

Then _scratch_mkfs_sized does the wrong thing for XFS if there is
a MKFS_OPTIONS specified block size.....

Other filesystems, like ext4, don't have this magic "don't override
MKFS_OPTIONS" code that XFS does here, so they are exposed to the
block size calculation bugs when human readable units are used. But
then they don't have the "ignore test specified block size" bug and
<head explodes>

The whole thing is a mess of bugs and technical debt. Randomly
adding more complex regexes and string parsing to random bits of
fstests to support doing integer math on human readable units in
random variables doesn't improve this situation at all.

Hence my suggestion that we set a requirement that all block size
specifications in MKFS_OPTIONS are in byte units, and we check and
enforce that once at startup. With that requirement in place, we can
clean up all this mess knowing that we only have to support integer
units...

What I expected to see was a function like:

_check_mkfs_block_options()
{
	case $FSTYP:
	xfs)
		# check for -b?size= option in integer format
		# exit if not valid
	ext4)
		# check for -b <num> option in integer format
		# exit if not valid
	.....
}

called from get_next_config() similar to how we call _check_device
on every block device for existence after reading them from the
config file.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Long An]

On Fri, 2022-06-24 at 12:41 +1000, Dave Chinner wrote:
> On Thu, Jun 23, 2022 at 11:17:44AM -0700, Darrick J. Wong wrote:
> > On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> > > _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.
> > > 
> > > Therefore, add validation check to force MKFS_OPTIONS to use pure
> > > integer in bytes for any size value.
> > > 
> > > Signed-off-by: An Long <lan@suse.com>
> > > ---
> > >  README        | 3 ++-
> > >  common/config | 5 +++++
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/README b/README
> > > index 80d148be..7c2d3334 100644
> > > --- a/README
> > > +++ b/README
> > > @@ -241,7 +241,8 @@ Misc:
> > >     this option is supported for all filesystems currently only
> > > -overlay is
> > >     expected to run without issues. For other filesystems
> > > additional patches
> > >     and fixes to the test suite might be needed.
> > > -
> > > + - If MKFS_OPTIONS is used, the size value must be an integer
> > > number of bytes
> > > +   without units. For example, set MKFS_OPTIONS="-b 4096" but
> > > not "-b 4k".
> > 
> > IDGI.  mkfs.$FSTYP does its own input validation, which means that
> > fstest
> > runners are required to set MKFS_OPTIONS to something that mkfs
> > will
> > accept.  It's not the job of fstests to add /another/ layer of
> > validation...
> 
> It's not validating it - it refelecting the fact that fstests parses
> the block device size out of MKFS_OPTIONS and does integer math on
> it and does not handle human readable units....
> 
> > >  USING THE FSQA SUITE
> > >  ______________________
> > > diff --git a/common/config b/common/config
> > > index de3aba15..ec723ac4 100644
> > > --- a/common/config
> > > +++ b/common/config
> > > @@ -896,5 +896,10 @@ else
> > >  	fi
> > >  fi
> > >  
> > > +# check the size value in $MKFS_OPTIONS is an integer
> > > +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> > 
> > ...because this regex will break /any/ MKFS_OPTIONS with a number
> > followed by a letter.  mkfs.xfs handles units just fine, so I don't
> > understand why you're adding this blunt instrument.
> > 
> > MKFS_OPTIONS='-b size=1k' works just fine for XFS.
> 
> Except it where it doesn't.
> 
> For example, _scratch_mkfs_sized does unexpected stuff
> because it assumes filesystem block sizes are all in bytes when it
> tries to parse the custom test block sizes out of MKFS_OPTIONS to
> set the default block size.
> 
> https://lore.kernel.org/fstests/20220616043845.14320-1-lan@suse.com/
> 
> In this case, using "1k" in the XFS mkfs specifications results in
> the default block size is incorrectly calculated:
> 
> $ echo "-bsize=1024" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
> 1024
> $ echo "-bsize=1k" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
> 1
> $
> 
> So it sets the block size to 1, not 1024.
> 
> But then this bug is covered up later on by this code:
> 
>                 # don't override MKFS_OPTIONS that set a block size.
>                 echo $MKFS_OPTIONS |egrep -q "b?size="
>                 if [ $? -eq 0 ]; then
>                         _scratch_mkfs_xfs -d size=$fssize $rt_ops
>                 else
>                         _scratch_mkfs_xfs -d size=$fssize $rt_ops -b
> size=$blocksize
>                 fi
> 
> Which omits the poorly calculated block size because there's
> already a block size specified in the MKFS_OPTIONS so it ignores the
> fact it calculated the block size incorrectly from MKFS_OPTIONS.
> 
> But it also means that if the test specifies a block size via
> 
> _scratch_mkfs_sized $((256 * 1024 * 1024)) 2048
> 
> Then _scratch_mkfs_sized does the wrong thing for XFS if there is
> a MKFS_OPTIONS specified block size.....
> 
> Other filesystems, like ext4, don't have this magic "don't override
> MKFS_OPTIONS" code that XFS does here, so they are exposed to the
> block size calculation bugs when human readable units are used. But
> then they don't have the "ignore test specified block size" bug and
> <head explodes>
> 
> The whole thing is a mess of bugs and technical debt. Randomly
> adding more complex regexes and string parsing to random bits of
> fstests to support doing integer math on human readable units in
> random variables doesn't improve this situation at all.
> 
> Hence my suggestion that we set a requirement that all block size
> specifications in MKFS_OPTIONS are in byte units, and we check and
> enforce that once at startup. With that requirement in place, we can
> clean up all this mess knowing that we only have to support integer
> units...
> 
> What I expected to see was a function like:
> 
> _check_mkfs_block_options()
> {
> 	case $FSTYP:
> 	xfs)
> 		# check for -b?size= option in integer format
> 		# exit if not valid
> 	ext4)
> 		# check for -b <num> option in integer format
> 		# exit if not valid
> 	.....
> }
> 
Hello Dave,

Actually, I have tried the above methods. It's works, but we have to
add specific function for specific type of size?
Such as node size, sector size, device size or cluster size.

Thanks,
An Long

> called from get_next_config() similar to how we call _check_device
> on every block device for existence after reading them from the
> config file.
> 
> Cheers,
> 
> Dave.

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Darrick J. Wong]

On Fri, Jun 24, 2022 at 03:29:43AM +0000, Long An wrote:
> On Fri, 2022-06-24 at 12:41 +1000, Dave Chinner wrote:
> > On Thu, Jun 23, 2022 at 11:17:44AM -0700, Darrick J. Wong wrote:
> > > On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> > > > _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.
> > > > 
> > > > Therefore, add validation check to force MKFS_OPTIONS to use pure
> > > > integer in bytes for any size value.
> > > > 
> > > > Signed-off-by: An Long <lan@suse.com>
> > > > ---
> > > >  README        | 3 ++-
> > > >  common/config | 5 +++++
> > > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/README b/README
> > > > index 80d148be..7c2d3334 100644
> > > > --- a/README
> > > > +++ b/README
> > > > @@ -241,7 +241,8 @@ Misc:
> > > >     this option is supported for all filesystems currently only
> > > > -overlay is
> > > >     expected to run without issues. For other filesystems
> > > > additional patches
> > > >     and fixes to the test suite might be needed.
> > > > -
> > > > + - If MKFS_OPTIONS is used, the size value must be an integer
> > > > number of bytes
> > > > +   without units. For example, set MKFS_OPTIONS="-b 4096" but
> > > > not "-b 4k".
> > > 
> > > IDGI.  mkfs.$FSTYP does its own input validation, which means that
> > > fstest
> > > runners are required to set MKFS_OPTIONS to something that mkfs
> > > will
> > > accept.  It's not the job of fstests to add /another/ layer of
> > > validation...
> > 
> > It's not validating it - it refelecting the fact that fstests parses
> > the block device size out of MKFS_OPTIONS and does integer math on
> > it and does not handle human readable units....
> > 
> > > >  USING THE FSQA SUITE
> > > >  ______________________
> > > > diff --git a/common/config b/common/config
> > > > index de3aba15..ec723ac4 100644
> > > > --- a/common/config
> > > > +++ b/common/config
> > > > @@ -896,5 +896,10 @@ else
> > > >  	fi
> > > >  fi
> > > >  
> > > > +# check the size value in $MKFS_OPTIONS is an integer
> > > > +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> > > 
> > > ...because this regex will break /any/ MKFS_OPTIONS with a number
> > > followed by a letter.  mkfs.xfs handles units just fine, so I don't
> > > understand why you're adding this blunt instrument.
> > > 
> > > MKFS_OPTIONS='-b size=1k' works just fine for XFS.
> > 
> > Except it where it doesn't.
> > 
> > For example, _scratch_mkfs_sized does unexpected stuff
> > because it assumes filesystem block sizes are all in bytes when it
> > tries to parse the custom test block sizes out of MKFS_OPTIONS to
> > set the default block size.
> > 
> > https://lore.kernel.org/fstests/20220616043845.14320-1-lan@suse.com/
> > 
> > In this case, using "1k" in the XFS mkfs specifications results in
> > the default block size is incorrectly calculated:
> > 
> > $ echo "-bsize=1024" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
> > 1024
> > $ echo "-bsize=1k" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
> > 1
> > $
> > 
> > So it sets the block size to 1, not 1024.
> > 
> > But then this bug is covered up later on by this code:
> > 
> >                 # don't override MKFS_OPTIONS that set a block size.
> >                 echo $MKFS_OPTIONS |egrep -q "b?size="
> >                 if [ $? -eq 0 ]; then
> >                         _scratch_mkfs_xfs -d size=$fssize $rt_ops
> >                 else
> >                         _scratch_mkfs_xfs -d size=$fssize $rt_ops -b
> > size=$blocksize
> >                 fi
> > 
> > Which omits the poorly calculated block size because there's
> > already a block size specified in the MKFS_OPTIONS so it ignores the
> > fact it calculated the block size incorrectly from MKFS_OPTIONS.
> > 
> > But it also means that if the test specifies a block size via
> > 
> > _scratch_mkfs_sized $((256 * 1024 * 1024)) 2048
> > 
> > Then _scratch_mkfs_sized does the wrong thing for XFS if there is
> > a MKFS_OPTIONS specified block size.....
> > 
> > Other filesystems, like ext4, don't have this magic "don't override
> > MKFS_OPTIONS" code that XFS does here, so they are exposed to the
> > block size calculation bugs when human readable units are used. But
> > then they don't have the "ignore test specified block size" bug and
> > <head explodes>
> > 
> > The whole thing is a mess of bugs and technical debt. Randomly
> > adding more complex regexes and string parsing to random bits of
> > fstests to support doing integer math on human readable units in
> > random variables doesn't improve this situation at all.

Ah, ok.  I hadn't realized there was /more/ history to this patch than
just some random change that looked weird on its own.

> > Hence my suggestion that we set a requirement that all block size
> > specifications in MKFS_OPTIONS are in byte units, and we check and
> > enforce that once at startup. With that requirement in place, we can
> > clean up all this mess knowing that we only have to support integer
> > units...
> > 
> > What I expected to see was a function like:
> > 
> > _check_mkfs_block_options()
> > {
> > 	case $FSTYP:
> > 	xfs)
> > 		# check for -b?size= option in integer format
> > 		# exit if not valid
> > 	ext4)
> > 		# check for -b <num> option in integer format
> > 		# exit if not valid
> > 	.....
> > }
> > 
> Hello Dave,
> 
> Actually, I have tried the above methods. It's works, but we have to
> add specific function for specific type of size?
> Such as node size, sector size, device size or cluster size.

Personally I thought the v2 approach of fixing _scratch_mkfs_sized was
better, since it's a much smaller change than making fstests reject all
unit-having numbers and then everyone has to update their fstests
wrappers.  I know we like to pretend that nobody wraps fstests, but
everyone wraps fstests in even more bash goo.

Also I didn't like the fact that (I think) this patch would reject
something like MKFS_OPTIONS='-m dirhandling=3bsd' or something like
that.

Anyway, I'll go focus on other things.

--D

> Thanks,
> An Long
> 
> > called from get_next_config() similar to how we call _check_device
> > on every block device for existence after reading them from the
> > config file.
> > 
> > Cheers,
> > 
> > Dave.

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Zorro Lang]

On Fri, Jun 24, 2022 at 08:15:27PM -0700, Darrick J. Wong wrote:
> On Fri, Jun 24, 2022 at 03:29:43AM +0000, Long An wrote:
> > On Fri, 2022-06-24 at 12:41 +1000, Dave Chinner wrote:
> > > On Thu, Jun 23, 2022 at 11:17:44AM -0700, Darrick J. Wong wrote:
> > > > On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> > > > > _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.
> > > > > 
> > > > > Therefore, add validation check to force MKFS_OPTIONS to use pure
> > > > > integer in bytes for any size value.
> > > > > 
> > > > > Signed-off-by: An Long <lan@suse.com>
> > > > > ---
> > > > >  README        | 3 ++-
> > > > >  common/config | 5 +++++
> > > > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/README b/README
> > > > > index 80d148be..7c2d3334 100644
> > > > > --- a/README
> > > > > +++ b/README
> > > > > @@ -241,7 +241,8 @@ Misc:
> > > > >     this option is supported for all filesystems currently only
> > > > > -overlay is
> > > > >     expected to run without issues. For other filesystems
> > > > > additional patches
> > > > >     and fixes to the test suite might be needed.
> > > > > -
> > > > > + - If MKFS_OPTIONS is used, the size value must be an integer
> > > > > number of bytes
> > > > > +   without units. For example, set MKFS_OPTIONS="-b 4096" but
> > > > > not "-b 4k".
> > > > 
> > > > IDGI.  mkfs.$FSTYP does its own input validation, which means that
> > > > fstest
> > > > runners are required to set MKFS_OPTIONS to something that mkfs
> > > > will
> > > > accept.  It's not the job of fstests to add /another/ layer of
> > > > validation...
> > > 
> > > It's not validating it - it refelecting the fact that fstests parses
> > > the block device size out of MKFS_OPTIONS and does integer math on
> > > it and does not handle human readable units....
> > > 
> > > > >  USING THE FSQA SUITE
> > > > >  ______________________
> > > > > diff --git a/common/config b/common/config
> > > > > index de3aba15..ec723ac4 100644
> > > > > --- a/common/config
> > > > > +++ b/common/config
> > > > > @@ -896,5 +896,10 @@ else
> > > > >  	fi
> > > > >  fi
> > > > >  
> > > > > +# check the size value in $MKFS_OPTIONS is an integer
> > > > > +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> > > > 
> > > > ...because this regex will break /any/ MKFS_OPTIONS with a number
> > > > followed by a letter.  mkfs.xfs handles units just fine, so I don't
> > > > understand why you're adding this blunt instrument.
> > > > 
> > > > MKFS_OPTIONS='-b size=1k' works just fine for XFS.
> > > 
> > > Except it where it doesn't.
> > > 
> > > For example, _scratch_mkfs_sized does unexpected stuff
> > > because it assumes filesystem block sizes are all in bytes when it
> > > tries to parse the custom test block sizes out of MKFS_OPTIONS to
> > > set the default block size.
> > > 
> > > https://lore.kernel.org/fstests/20220616043845.14320-1-lan@suse.com/
> > > 
> > > In this case, using "1k" in the XFS mkfs specifications results in
> > > the default block size is incorrectly calculated:
> > > 
> > > $ echo "-bsize=1024" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
> > > 1024
> > > $ echo "-bsize=1k" | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'
> > > 1
> > > $
> > > 
> > > So it sets the block size to 1, not 1024.
> > > 
> > > But then this bug is covered up later on by this code:
> > > 
> > >                 # don't override MKFS_OPTIONS that set a block size.
> > >                 echo $MKFS_OPTIONS |egrep -q "b?size="
> > >                 if [ $? -eq 0 ]; then
> > >                         _scratch_mkfs_xfs -d size=$fssize $rt_ops
> > >                 else
> > >                         _scratch_mkfs_xfs -d size=$fssize $rt_ops -b
> > > size=$blocksize
> > >                 fi
> > > 
> > > Which omits the poorly calculated block size because there's
> > > already a block size specified in the MKFS_OPTIONS so it ignores the
> > > fact it calculated the block size incorrectly from MKFS_OPTIONS.
> > > 
> > > But it also means that if the test specifies a block size via
> > > 
> > > _scratch_mkfs_sized $((256 * 1024 * 1024)) 2048
> > > 
> > > Then _scratch_mkfs_sized does the wrong thing for XFS if there is
> > > a MKFS_OPTIONS specified block size.....
> > > 
> > > Other filesystems, like ext4, don't have this magic "don't override
> > > MKFS_OPTIONS" code that XFS does here, so they are exposed to the
> > > block size calculation bugs when human readable units are used. But
> > > then they don't have the "ignore test specified block size" bug and
> > > <head explodes>
> > > 
> > > The whole thing is a mess of bugs and technical debt. Randomly
> > > adding more complex regexes and string parsing to random bits of
> > > fstests to support doing integer math on human readable units in
> > > random variables doesn't improve this situation at all.
> 
> Ah, ok.  I hadn't realized there was /more/ history to this patch than
> just some random change that looked weird on its own.
> 
> > > Hence my suggestion that we set a requirement that all block size
> > > specifications in MKFS_OPTIONS are in byte units, and we check and
> > > enforce that once at startup. With that requirement in place, we can
> > > clean up all this mess knowing that we only have to support integer
> > > units...
> > > 
> > > What I expected to see was a function like:
> > > 
> > > _check_mkfs_block_options()
> > > {
> > > 	case $FSTYP:
> > > 	xfs)
> > > 		# check for -b?size= option in integer format
> > > 		# exit if not valid
> > > 	ext4)
> > > 		# check for -b <num> option in integer format
> > > 		# exit if not valid
> > > 	.....
> > > }
> > > 
> > Hello Dave,
> > 
> > Actually, I have tried the above methods. It's works, but we have to
> > add specific function for specific type of size?
> > Such as node size, sector size, device size or cluster size.
> 
> Personally I thought the v2 approach of fixing _scratch_mkfs_sized was
> better, since it's a much smaller change than making fstests reject all
> unit-having numbers and then everyone has to update their fstests
> wrappers.  I know we like to pretend that nobody wraps fstests, but
> everyone wraps fstests in even more bash goo.
> 
> Also I didn't like the fact that (I think) this patch would reject
> something like MKFS_OPTIONS='-m dirhandling=3bsd' or something like
> that.

Hmm.. you're right. That's too radicalized if refuse any "[0-9]+[^0-9\ ]"
in MKFS_OPTIONS ...

Let's back to the original problem we tried to fix:
https://lore.kernel.org/fstests/20220616043845.14320-3-lan@suse.com/

So the real problem is in _scratch_mkfs_sized() function:

  _scratch_mkfs_sized()
  {
        local fssize=$1
        local blocksize=$2
        local def_blksz

        case $FSTYP in
        xfs)
                def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?size= ?+([0-9]+).*/\1/p'`
                ;;
        btrfs)
                def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-s ?+([0-9]+).*/\1/p'`
                ;;
        ext2|ext3|ext4|ext4dev|udf|reiser4|ocfs2|reiserfs)
                def_blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
                ;;
        jfs)
                def_blksz=4096
                ;;
        esac

        [ -n "$def_blksz" ] && blocksize=$def_blksz
        [ -z "$blocksize" ] && blocksize=4096
	...
	...

Due to _scratch_mkfs_sized() hope to get blocksize from MKFS_OPTIONS, but it
can't deal with blocksize with an unit (e.g. "k") correctly.

Others _scratch_mkfs_* functions (_scratch_mkfs_ext4, _scratch_mkfs_xfs,
_scratch_mkfs_geom, _scratch_mkfs_blocksized, _scratch_mkfs_richacl) don't
have this problem, due to they don't try to get anything from MKFS_OPTIONS
(_scratch_mkfs_geom just trys to replace something with proper pattern).

If only focus on the issue in _scratch_mkfs_sized() simply. We have three
choices, if MKFS_OPTIONS doesn't use pure integer:

1) As V2 patch, convert it to pure integer.
2) Do _fail or _notrun directly in this function, to warn that pure integer
   is necessary.
3) Do nothing, just document it.

The 1st choice might bring in some performance lose, if the users set something
in local.config likes MKFS_OPTIONS="-b size=1k". But nearly no effect, if the
users use pure integer.

The 2nd choice is similar with the 1st, if use pure integer. But it save some
time if it's not pure integer, due to it exit directly, won't try to calculate.

The 3rd choice changes nothing, push the problem back to users, and depends on if
they read the doc carefully.

I originally would like to take the 1st one. But I can accept the 2nd one too,
if there're strong objections to the 1st one. About the 3rd one, I'm wondering
how to help most of users to notice that.

What do you think?

Thanks,
Zorro

> 
> Anyway, I'll go focus on other things.
> 
> --D
> 
> > Thanks,
> > An Long
> > 
> > > called from get_next_config() similar to how we call _check_device
> > > on every block device for existence after reading them from the
> > > config file.
> > > 
> > > Cheers,
> > > 
> > > Dave.
> 

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Theodore Ts'o]

On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
>  
> +# check the size value in $MKFS_OPTIONS is an integer
> +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> +        _fatal "error: \$MKFS_OPTIONS: Only number in bytes is allowed, no units."
> +fi

This will break configs, since ext4 has a feature 64bit, so there
might be MKFS_OPTIONS that contain "-O 64bit", or "-O ^64bit".

For that matter, this wll also break things like "-E encoding=12.1"
which sets the Unicode encoding to 12.1.

... or "-E lazy_itable_init=1,lazy_journal_init=1".

Bottom line, blindly assuming that any number followed by a non-number
is a unit, is just not going to work.

I think the real problem is that we have fstests trying to parse
MKFS_OPTIONS at all in the first place.  For that matter, I'm not fond
of tests that try to override MKFS_OPTIONS for their own nefarious
purposes, because sometimes certain MKFS_OPTIONS imply something about
what must be in MOUNT_OPTIONS, or vice versa.  I do understand why
that has to be done for certain tests, but... yelch.  Especially in
centralized functions like _scratch_mkfs_sized, it just leads to tech
debt.

I'll note that generic tests and functions in common/rc that do unholy
things with MKFS_OPTIONS already have to have per-file system hacks as
it is.  So having more per-file system hacks for check_block_options()
is par for the course, so long as we accept that we want to have
generic tests and generic code violate the abstraction barrier and try
to read or override MKFS_OPTIONS.

If we want to go down that approach, perhaps another approach would be
to specify the desired blocksize diretly in the fs config.  Something
like "FS_BLOCK_SIZE=1024"?  We can then have the fs-specific mkfs
commands translate that into the appropriate "-b 1024" or whatever
else might be appropriate for a particular file system.  And then
tests that want to override the block size can just override
FS_BLOCK_SIZE and then the _mkfs function can use it as appropriate.

(BTW, this has been less important for ext4, since we allow the block
size to be specified more than once, with the last option being the
one which controls.  So "mkfs.ext4 -b 4096 ... -b 1024 ..." is OK for
ext4, but it results in an error for mkfs.xfs.  Otherwise we could
probably solve this problem more easily.)

I know my proposed approach will require all of the fstests wrapers to
be changed, and may require auditing a lot of tests to see what
breaks.  But quite frankly, what we have write now with tests messing
with MKFS_OPTIONS is kind of a mess already, and if we're going to do
clean up the tech debt, it's going to requrie some pain.

Or we could use the suggestion of more per-fs case in a
_check_mkfs_options function.  Which is a bit ugly, but IMHO, not as
ugly as the existing hacks where we have generic code trying to mess
with MKFS_OPTIONS....

						- Ted

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Dave Chinner]

On Sat, Jun 25, 2022 at 12:39:27PM -0400, Theodore Ts'o wrote:
> On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
> >  
> > +# check the size value in $MKFS_OPTIONS is an integer
> > +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> > +        _fatal "error: \$MKFS_OPTIONS: Only number in bytes is allowed, no units."
> > +fi
> 
> This will break configs, since ext4 has a feature 64bit, so there
> might be MKFS_OPTIONS that contain "-O 64bit", or "-O ^64bit".
> 
> For that matter, this wll also break things like "-E encoding=12.1"
> which sets the Unicode encoding to 12.1.
> 
> ... or "-E lazy_itable_init=1,lazy_journal_init=1".
> 
> Bottom line, blindly assuming that any number followed by a non-number
> is a unit, is just not going to work.
> 
> I think the real problem is that we have fstests trying to parse
> MKFS_OPTIONS at all in the first place.

Yup. That's exactly the issue.

> For that matter, I'm not fond
> of tests that try to override MKFS_OPTIONS for their own nefarious
> purposes, because sometimes certain MKFS_OPTIONS imply something about
> what must be in MOUNT_OPTIONS, or vice versa.  I do understand why
> that has to be done for certain tests, but... yelch.  Especially in
> centralized functions like _scratch_mkfs_sized, it just leads to tech
> debt.
> 
> I'll note that generic tests and functions in common/rc that do unholy
> things with MKFS_OPTIONS already have to have per-file system hacks as
> it is.  So having more per-file system hacks for check_block_options()
> is par for the course, so long as we accept that we want to have
> generic tests and generic code violate the abstraction barrier and try
> to read or override MKFS_OPTIONS.
> 
> If we want to go down that approach, perhaps another approach would be
> to specify the desired blocksize diretly in the fs config.  Something
> like "FS_BLOCK_SIZE=1024"?  We can then have the fs-specific mkfs
> commands translate that into the appropriate "-b 1024" or whatever
> else might be appropriate for a particular file system.  And then
> tests that want to override the block size can just override
> FS_BLOCK_SIZE and then the _mkfs function can use it as appropriate.

This is the right way to fix the problem.

My plan was that once there was a single point of checking of
MKFS_OPTIONS, I'd use that to extract the default value ifor the
entire fstests run and convert everything else to use it. And then
add config section support for the variable so that it can easily be
specified on a config section by section basis.

I started writing an email that said exactly this, but then decided
half way through that it was a waste of time because people would
just nod, go their own way and yet again nothing would happen unless
I did it myself....

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

[Theodore Ts'o]

On Sun, Jun 26, 2022 at 08:47:08AM +1000, Dave Chinner wrote:
> > If we want to go down that approach, perhaps another approach would be
> > to specify the desired blocksize diretly in the fs config.  Something
> > like "FS_BLOCK_SIZE=1024"?  We can then have the fs-specific mkfs
> > commands translate that into the appropriate "-b 1024" or whatever
> > else might be appropriate for a particular file system.  And then
> > tests that want to override the block size can just override
> > FS_BLOCK_SIZE and then the _mkfs function can use it as appropriate.
> 
> This is the right way to fix the problem.
> 
> My plan was that once there was a single point of checking of
> MKFS_OPTIONS, I'd use that to extract the default value ifor the
> entire fstests run and convert everything else to use it. And then
> add config section support for the variable so that it can easily be
> specified on a config section by section basis.

Nice, that sounds like a good way to go.

And if we start down this path, for those file systems that support a
clustered allocation mode, what I'd then propose adding is a
FS_CLUSTER_SIZE parameter, so that the cluster size could be specified
in the config, hich could then get translated by the _mkfs_XXX
function into the appropriate mkfs option --- AND, so that generic
tests that are testing quota can just check the value of
$FS_CLUSTER_SIZE, and if it's set, use that to determine the
granularity of quota tracking.

This avoids having tests need to use fs-specific tools (such as
dumpe2fs) to determine if (a) clustered allocation is enabled, and (b)
what the cluster size is.

More generally, for any file system feature which is supported by more
than one file system[1], instead of explicitly specifying it via
MKFS_OPTIONS and/or MOUNT_OPTION, we could specify it abstractly,
which will make it easier for those tests who either need to override
that setting, or test to see what that setting might be.  This should
allow us to reduce the number of instances of "case $FSTYPE ..." in
tests/generic/*, which would be a very good thing.

[1] Examples of file system features that could use this include
fscrypt, case folding and maybe DAX.  We're mostly doing this for
external log devices already.

					- Ted