Re: [PATCH] generic: add missing $FSX_AVOID to fsx invocations

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Nov 08, 2022 at 11:56:05PM +0800, Zorro Lang wrote:
> On Tue, Nov 08, 2022 at 10:08:03AM -0500, Theodore Ts'o wrote:
> > On Tue, Nov 08, 2022 at 10:44:55AM +0800, Zorro Lang wrote:
> > > > If it's collapse/insert range you're specifically worried about, perhaps
> > > > its time to implement _get_file_block_size for ext4 so that
> > > > _test_congruent_file_oplen can exclude those tests that will get the
> > > > alignment wrong?
> > > 
> > > Thanks Darrick, I'm thinking about this helper you wrote recently :)
> > > (The real name is _require_congruent_file_oplen in common/rc.)
> > > 
> > > Hi Ted, is this helpful if you write a _ext4_get_file_block_size (refer to
> > > _xfs_get_file_block_size in common/xfs), then call it in _get_file_block_size
> > > to help _require_congruent_file_oplen to get the correct length which is an
> > > integer multiple of the file's allocation unit size ?
> > 
> > Well, it's a bit more complicated than that, since there's a
> > distinction between the cluster size and block size.  The cluster size
> > is the allocation unit.  However, other things are done in units of
> > the block size, including how we report fiemap results, etc.
> > 
> > For example, take a look at generic/206.  It will try to create a file
> > system where the file system block size is the one fourth of the page
> > size --- so for x86, 1k.  However, for ext4, the default cluster size
> > for 16 times the block size, so for this file system the allocation
> > unit size is 16k.  If we make _get_file_block_size return 64k for all
> > files, then generic/206 will fail:
> > 
> >     pagesz=$(getconf PAGE_SIZE)
> >     blksz=$((pagesz / 4))
> >     ...
> >     _scratch_mkfs_blocksized $blksz > $seqres.full 2>&1
> >     ...
> >     real_blksz=$(_get_file_block_size $testdir)
> 
> Hmm... I'm wondering is it possible that g/206 need _get_block_size() at here?
> 
> Due to from the logic of _get_file_block_size() [1], extN call _get_block_size()
> directly. So if you feel a specific _ext4_get_file_block_size() isn't suit for
> g/206, maybe it turely want _get_block_size() at here?
> 
> [1]
> _get_file_block_size()
> {
>         if [ -z $1 ] || [ ! -d $1 ]; then
>                 echo "Missing mount point argument for _get_file_block_size"
>                 exit 1
>         fi
> 
>         case "$FSTYP" in
>         "ocfs2")
>                 stat -c '%o' $1
>                 ;;
>         "xfs")
>                 _xfs_get_file_block_size $1
>                 ;;
>         *)
>                 _get_block_size $1
>                 ;;
>         esac
> }
> 
> >     test $real_blksz != $blksz && _notrun "Failed to format with small blocksize."
> > 
> > I assume this works for xfs because only files in the real-time block
> > size will have a larger allocation unit size, but the root directory
> > has a allocation unit size of the "real" block size?
> > 
> > I suppose I could make a hypothetical _ext4_get_file_block_size lie
> > and return the "real" block size when it's called on the mount point,
> > but that seems kinda of gross, and it's also a lie, since the
> > allocation unit size for the root directory actually is the cluster
> > size, not the block size.
> > 
> > If I were going to be doing things from scratch, I'd make a
> > distinction between _get_file_allocation_size and
> > _get_file_system_block_size, which would be a lot *clearer* about what
> 
> Currently _get_block_size is more like _get_file_system_block_size, and
> _get_file_block_size is more like _get_file_allocation_size. Actually
> I'm confused about these two names before :)

Yes, both statements are correct.  They're both crappily named, one of
them by me. :(

> > is going on.  Even then, I could imagine some tests getting confused
> > with how, say, fiemap behaves with an ext4 file system with a 4k block
> > size and a 64k allocation unit size, so I'm not sure it's a complete
> > solution.  And doing this now would require quite a bit of code churn
> > in xfstests.

Yes, it has been, uh, fun to fix fstests so that xfs realtime with a 28k
allocation unit doesn't spray false failures everywhere.  I'm mostly
done now, I hope? ;)

(Or at least I haven't heard any complaints from Leah in a solid 2
months...)

> > > If this's helpful for your first concern [1], please tell me. Then we can talk
> > > about your second concern [2], if it's still your main concern now :)
> > > 
> > > "This might *[1]* either be because the test didn't know about ext4
> > >  bigalloc's cluster alignment requirements, *[2]* or because a particular
> > >  operation might just be *buggy* and being able to run tests as if a
> > >  particular command wasn't supported was useful."
> > 
> > So [1] is a real concern, and at the moment, we just suppress all of
> > the tests that try to use collapse/insert range.  For example, from
> > the ext4/bigalloc config file:
> > 
> >     # Until we can teach xfstests the difference between cluster size and
> >     # block size, avoid collapse_range and insert_range since these will
> >     # fail due the fact that these operations require cluster-aligned
> >     # ranges.
> >     export FSX_AVOID="-C -I"
> >     export FSSTRESS_AVOID="-f collapse=0 -f insert=0"
> >     export XFS_IO_AVOID="fcollapse finsert"
> >     TEST_SET_EXCLUDE="-x collapse,insert"
> 
> Thanks for pointing this out, I'll help to check all fcollapse and finsert
> related cases, and add them into collapse or insert group. We're keep improving
> group problem, most of cases really not take too much care about its group.
> And I'll pay more attention about the groups when I review patches.

<nod>

> > 
> > That's not ideal, and it's been on my todo list to try to fix it, when
> > I could get one of those round tuit's.  However, I had assumed that we
> > would split _get_file_block_size somehow, given the observation I've
> > made above.  So this was always been something that has put me off,
> > because it looked like a much larger project than say, "gee, I have an
> > hour or two on a weekend, let me see if I can fix this".
> 
> I'd like to help your team to deal with the fstests problems you hit in your
> using procedure, feel free to tell us the part you hope to fix/improve. I
> have to care about other teams too, so might need more time to evaluate and
> talk, sorry about that.
> 
> > 
> > [2] is practically not _really_ a concern any more.  It used to be
> > that one of the ways that I would root cause a failing test was to
> > suppress one of the advanced fallocate modes, whether it be
> > collapse/insert range, or going even further back, punch hole.  If
> > test then passed, then I could say, "ah, hah; the problem can probably
> > be localized to a certain part of the fs code."
> > 
> > However, that's mainly tests that are using fsstress and fsx, and we
> > have solutions for that.  And for other tests, I can examine the test
> > and see whether or not it's using collapse/insert range by inspection,
> > so it's really not that big of a deal.  I could imagine other file
> > systems who might find this useful in the future, if they were trying
> > to growing support for the more advanced fallocate modes --- but that
> > wouldn't *my* concern, and arguably those file systems could solve
> > problems alternate ways, such as having mount options that suppress
> > those fallocate modes entirely which could be used when running
> > xfstests.
> > 
> > 						- Ted
> > 
> > P.S.  I have noticed some tests that use collapse/insert range but
> > don't declare that they are in the collapse or insert groups.
> > Fortunately, all of these tests are also in the clone group, and so
> > they don't apply to ext4, so _I_ don't care.  :-)
> 
> Thanks, I'll try to check all collapse/insert related cases.

<nod> Bad throwback to the days when we didn't scrutinize group names
all that closely.  Someone probably ought to write a dumb linter that
can look for obvious patterns in a testcase and nominate group names.

--D

> Thanks,
> Zorro
> 
> > 
>