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

[Zorro Lang <zlang@redhat.com>]

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

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

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

Thanks,
Zorro

>