Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized

Dave Chinner
2022-06-17T22:24:11Z

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@fromorbit.com