On Tue, Jun 21, 2022 at 04:25:07AM +0000, Long An wrote: > On Tue, 2022-06-21 at 12:05 +0800, Zorro Lang wrote: > > 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. > > OK， I'll fix it ASAP according to the comments Thanks, no push :) > > > > > Thanks, > > Zorro > > > > > Cheers, > > > > > > Dave. > > > -- > > > Dave Chinner > > > david@xxxxxxxxxxxxx > > >
