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

[Zorro Lang <zlang@redhat.com>]

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