| Message ID | 20220623160825.31788-1-lan@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | common/config: Fix use of MKFS_OPTIONS | expand |
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 >
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 > > >
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.
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.
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.
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. >
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
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.
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
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
_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(-)