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

[Zorro Lang <zlang@redhat.com>]

On Mon, Nov 07, 2022 at 12:09:58PM -0800, Darrick J. Wong wrote:
> On Mon, Nov 07, 2022 at 11:35:16AM -0500, Theodore Ts'o wrote:
> > On Mon, Nov 07, 2022 at 10:02:36AM +0800, Zorro Lang wrote:
> > > I think it doesn't make sense to use $FSX_AVOID in `fsx --replay-ops` cases.
> > > Due to generally the operations which a cases would like to replay are exact
> > > steps to reproduce to a known bug. If we skip some operations (e.g. -F), it
> > > doesn't make sense for this reproducer.
> > > 
> > > The recommended way for this kind of cases is making sure current fs/system
> > > support the operations will be run by fsx, especially those features are not
> > > common on different fs/system....
> > >
> > > So it uses below _require_* helpers to make sure these operations are supported,
> > > before testing:
> > > 
> > >   _require_xfs_io_command "falloc"
> > >   _require_xfs_io_command "falloc" "-k"
> > >   _require_xfs_io_command "fzero"
> > >   _require_xfs_io_command "fcollapse"
> > > 
> > > That's my point, hope I didn't misunderstand what you said :)
> > 
> > No, you didn't understand me.  :-)

Wow, sorry I didn't realize that's a longer story :)

> > 
> > For context, I have an out of tree patch (see attached), which I had
> > tried upstreaming a while back, but it got rejected, so I've continued
> > to keep it in my personal tree.  The basic idea is sometimes you might
> > want to suppress a test even *though* _require_xfs_io_command seems to
> > indicate that operation was supported.
> > 
> > This might either be because the test didn't know about ext4
> > bigalloc's cluster alignment requirements, 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.
> > 
> > It was rejected because the claim was that you could just exclude by
> > group instead (e.g., "punch", "collapse") but I didn't trust that the
> > group list would be kept up to date, so I never really agreed with

I agree that most of people don't pay much attention to the group names
when they write a test case. And there're new group names sometimes,
so we always need to supplement some group names later. But the group
name is still helpful, so if you feel some cases missed some groups, please
feel free to tell us :)

> > that line of reasoning.  These days, given that group declaration are
> > kept in the test script, it's much less likely to happen, but I've
> > kept the patch in my tree because it's occasionally useful.
> > 
> > At this point, it's admittedly pretty rarely needed since ext4's
> > collapse and insert range commands are pretty solid modulo tests not
> > understanding cluster alignment, but still, it's not much effort for
> > me to keep carrying the patch and I don't expect it will ever get
> > upstreamed.
> 
> 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 ?

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

Thanks,
Zorro

> 
> --D
> 
> > 
> > 					- Ted
> > 
> > commit c9d25475a94d5e53d7f18d247a17088999522862
> > Author: Theodore Ts'o <tytso@mit.edu>
> > Date:   Sat Oct 17 14:39:26 2015 -0400
> > 
> >     common: introduce XFS_IO_AVOID env var
> >     
> >     Like FSSTRESS_AVOID and FSX_AVOID, XFS_IO_AVOID can be used to avoid
> >     using various advanced file system features such as "fpunch"
> >     "fcollapse", "finsert", or "zero".  Tests that require an xfs_io
> >     command which is included in the space-separated list found in the
> >     XFS_IO_AVOID environment variable will be skipped using _notrun.
> >     
> >     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > 
> > diff --git a/README b/README
> > index 4c4f22f85..42baff07b 100644
> > --- a/README
> > +++ b/README
> > @@ -245,6 +245,10 @@ Misc:
> >     this option is supported for all filesystems currently only -overlay is
> >     expected to run without issues. For other filesystems additional patches
> >     and fixes to the test suite might be needed.
> > + - setenv XFS_IO_AVOID, which may contain a list of space separated
> > +   xfs_io commands which will be avoided in case you want to exclude
> > +   tests that require the use of certain file system operations such
> > +   as "fpunch", "fcollapse", "finsert", or "zero".
> >  
> >  ______________________
> >  USING THE FSQA SUITE
> > diff --git a/common/rc b/common/rc
> > index eb67e0cdc..d1c07a4d0 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2485,6 +2485,11 @@ _require_xfs_io_command()
> >  	local opts=""
> >  	local attr_info=""
> >  
> > +	if echo "$XFS_IO_AVOID" | grep -wq -- "$command"
> > +	then
> > +		_notrun "Avoiding xfs_io $command"
> > +	fi
> > +
> >  	local testfile=$TEST_DIR/$$.xfs_io
> >  	local testio
> >  	case $command in
>