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

Dave Chinner
2022-06-20T23:14:48Z

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