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. Every "lets make this pretty parsing" or "lets do complex parsing" change that replaces a simple, straight forward integer or construct adds bloat, overhead and runtime. It's a death-by-a-thousand-cuts scenario - each individual change can be justified, but suddenly we've adding another 15 minutes of runtime to fstests even for people who require/use none of that functionality. Bloat, overhead and runtime are the three main things we need to remove from fstests, so I feel that the right thing to do is document that sizes for filesystems, block sizes, etc must always be in integer units rather than adding code to "be flexible". > As this issue, we have to accept the size unit(k/m/g ...) or have to > tell the users that they must use pure integer, any of "k/m/g" is > unacceptable. I prefer the former, so I'd like to have this change, > except anyone has a better idea :) I'd much prefer we document the existing behaviour rather than add more complexity and overhead.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
