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.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
