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

[Long An <lan@suse.com>]

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,
> Zorro
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> >