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@xxxxxxxx> > > > > --- > > > > 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@xxxxxxxx/ > > > > 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.
