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