Re: [PATCH] generic/456: add check for fallocate flags

[Eryu Guan <eguan@redhat.com>]

On Tue, Sep 12, 2017 at 10:38:49AM +0800, Xiao Yang wrote:
> Hi Amir,
> 
> Thanks for your comments. :-)
> Could you tell me which patch has fixed the ext4 bug?
> 
> On 2017/09/11 19:03, Amir Goldstein wrote:
> > On Mon, Sep 11, 2017 at 12:25 PM, Xiao Yang<yangx.jy@cn.fujitsu.com>  wrote:
> > > On RHEL6.9GA, this case could not emulate a crash and passed due
> > > to unsupported collapse_range and zero_range instead of no bug.
> > > 
> > > We added check for fallocate flags to avoid confusion.
> > > 
> > I am not sure I understand the confusion.
> > 
> > A bug was allegedly introduced to ext4 when introducing
> > collapse_range and/or insert_range and this is a regression test
> > for this alleged regression.
> > 
> > In what way is it confusing that the test passes on an old kernel?
> > There are a lot of tests in xfstests that test for regressions that
> > were introduced by commit XYZ. I don't see those tests checking
> > that they are running on kernel>  XYZ.
> > 
> > BTW, this test also passes on btrfs and xfs, but it does not include
> > _supported_fs ext4 against confusion.
> On an old kernel(e.g. RHEL6.9GA), the test passed and got the following
> message in ext4.
> ----------------------------------------------------------------------
> # /var/lib/xfstests/ltp/fsx -d --replay-ops /tmp/733.fsxops
> --record-ops=/tmp/733.dupops /mnt/xfstests/scratch/testfile
> main: filesystem does not support fallocate mode FALLOC_FL_ZERO_RANGE,
> disabling!
> main: filesystem does not support fallocate mode FALLOC_FL_COLLAPSE_RANGE,
> disabling!
> main: filesystem does not support fallocate mode FALLOC_FL_INSERT_RANGE,
> disabling!
> 1 write 0x137dd thru    0x21445 (0xdc69 bytes)
> fallocating to largest ever: 0x16ade
> 2 falloc        from 0xb531 to 0x16ade (0xb5ad bytes)
> 4 write 0x3e5ec thru    0x3ffff (0x1a14 bytes)
> 6 mapwrite      0x216ad thru    0x23dfb (0x274f bytes)
> ----------------------------------------------------------------------
> 
> We skip collapse_range and zero_range operations and cannot trigger the
> expected bug in ext4.
> 
> I want to distinguish between unsupported flags and no bug.  Do you think it
> needs to distinguish?

If I understand the bug correctly, it's a bug in
{collapse,zero,insert}_range implementation, if the old kernels don't
support such operations, it's fair to say the old kernels have no such
bugs. And it's no harm to run some more tests even if the underlying
filesystem doesn't support such operations, because we replayed and
tested write and mapwrite operations too.

So I think it's fine to leave the test as it is.

> > 
> > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> > > ---
> > >   tests/generic/456 | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/generic/456 b/tests/generic/456
> > > index 8debd3f..b72acea 100755
> > > --- a/tests/generic/456
> > > +++ b/tests/generic/456
> > > @@ -67,11 +67,16 @@ write 0x3e5ec 0x1a14 0x21446
> > >   zero_range 0x20fac 0x6d9c 0x40000 keep_size
> > >   mapwrite 0x216ad 0x274f 0x40000
> > >   EOF
> > > -run_check $here/ltp/fsx -d --replay-ops $fsxops $SCRATCH_MNT/testfile
> > > +touch $tmp.dupops
> > > +run_check $here/ltp/fsx -d --replay-ops $fsxops --record-ops=$tmp.dupops $SCRATCH_MNT/testfile
> > > 
> > >   _flakey_drop_and_remount
> > >   _unmount_flakey
> > >   _cleanup_flakey
> > > +
> > > +ops_name=$(awk '/skip/ {printf "%s ", $2}' $tmp.dupops)
> > > +[ -n "$ops_name" ]&&  _notrun "fallocate does not support $ops_name"
> > > +
> > If you must add some check, please add
> > _require_xfs_io_command "fcollapse"
> > _require_xfs_io_command "fzero"
> > 
> > It is not really a must for this test and its not even really testing if fs
> > supports those commands, but that is de-facto standard for not
> > running fcollapse/fzero tests.
> IMO, _require_xfs_io_command only check if xfs_io command supports
> collapse_range or zero_range,
> and it does not mean that fallocate(2)  supports collapse_range or
> zero_range.
> 
> I am not sure it is necessary to add some check.

xfs_io commands fcollapse, fzero, finsert are actually run by
_require_xfs_io_command on a file in $TEST_DIR, so it does check if the
underlying filesystem support such operations or not, not only the
xfs_io command.

Thanks,
Eryu