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

From: Zorro Lang <zlang@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: fstests@vger.kernel.org
Subject: Re: [PATCH v2 0/2] Fix input value to _scratch_mkfs_sized
Date: Sat, 18 Jun 2022 11:14:13 +0800	[thread overview]
Message-ID: <20220618031413.j6pgaucpinl5csc5@zlang-mailbox> (raw)
In-Reply-To: <20220617222405.GA1098723@dread.disaster.area>

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. I didn't
test, but I think it's fast enough. We even can put this judgement to be
the first judgement, let the pure integer won't be affected much.

Then we document that "recommend pure integer in MKFS_OPTIONS". How about
that?

Due to if we only tell the users to use pure integer in document, if they
don't follow that, and cause some tests actually didn't run as expected,
but we don't report any failures. That looks I'm all talk, no clear
"bad sequent" if no one follow.

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?

Thanks,
Zorro

> 
> 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
> 