[PATCH 0/9] fstests: few updates

[PATCH 0/9] fstests: few updates

[Luis R. Rodriguez]

I've deployed fstests on a relatively new system and ran into a few setup
snags which can be fixed easily. Other than this I also ran into a few
issues running a few tests which can easily also be fixed.

I've added a few new groups to help avoiding running tests with a basic
section. Some of these tests are better designed to be run with a custom
section and grouping them up helps with this.

Luis R. Rodriguez (9):
  generic/381: use username fsgqa-381
  README: document group fsgqa is required
  generic/group: add 304 to dedupe group
  build: update AC_PACKAGE_WANT_GDBM() and src/dbtest.c to build
  tests/xfs/group: add group for tests which require a logdev
  tests/ext4/group: add group for tests which require a logdev
  tests/xfs/group: add realtimedev group
  tests/xfs/group: add group for tests which require mkfs v4_5
  tests/xfs/group: add injection group

 README                |  3 ++-
 m4/package_gdbmdev.m4 |  8 ++++++++
 src/dbtest.c          |  1 +
 tests/ext4/group      |  2 +-
 tests/generic/381     | 16 ++++++++--------
 tests/generic/381.out |  4 ++--
 tests/generic/group   |  2 +-
 tests/xfs/group       | 52 +++++++++++++++++++++++++--------------------------
 8 files changed, 49 insertions(+), 39 deletions(-)

-- 
2.15.0

[PATCH 1/9] generic/381: use username fsgqa-381

[Luis R. Rodriguez]

Some systems are not allowing usernames prefixed with a number now.
One can however use numbers as a postfix so use that.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 README                |  2 +-
 tests/generic/381     | 16 ++++++++--------
 tests/generic/381.out |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/README b/README
index ed69332e774e..e05142be1a87 100644
--- a/README
+++ b/README
@@ -20,7 +20,7 @@ _______________________
 - run make
 - run make install
 - create fsgqa test user ("sudo useradd fsgqa")
-- create 123456-fsgqa test user ("sudo useradd 123456-fsgqa")
+- create fsgqa-381 test user ("sudo useradd fsgqa-381")
 	
 ______________________
 USING THE FSQA SUITE
diff --git a/tests/generic/381 b/tests/generic/381
index 006f0d879638..cdc29c2e029e 100755
--- a/tests/generic/381
+++ b/tests/generic/381
@@ -3,7 +3,7 @@
 #
 # Test xfs_quota when user or names beginning with digits.
 # For example, create a 'limit' for a user or group named
-# '12345678-abcd', then query this user and group.
+# 'fsgqa-381', then query this user and group.
 #
 #-----------------------------------------------------------------------
 # Copyright (c) 2015 Red Hat Inc. All Rights Reserved.
@@ -53,9 +53,9 @@ _require_scratch
 _require_quota
 _require_xfs_quota_foreign
 
-# need user and group named 123456-fsgqa
-_require_user 123456-fsgqa
-_require_group 123456-fsgqa
+# need user and group named fsgqa-381
+_require_user fsgqa-381
+_require_group fsgqa-381
 
 _scratch_mkfs >/dev/null 2>&1
 _qmount_option "usrquota,grpquota"
@@ -63,17 +63,17 @@ _qmount
 
 # user test
 echo "== user test =="
-$XFS_QUOTA_PROG -x -c "limit -u bsoft=100m bhard=200m 123456-fsgqa" $SCRATCH_MNT
+$XFS_QUOTA_PROG -x -c "limit -u bsoft=100m bhard=200m fsgqa-381" $SCRATCH_MNT
 echo "=== quota command output ==="
-$XFS_QUOTA_PROG -c "quota -u -b -N -v 123456-fsgqa" $SCRATCH_MNT | _filter_quota
+$XFS_QUOTA_PROG -c "quota -u -b -N -v fsgqa-381" $SCRATCH_MNT | _filter_quota
 echo "=== report command output ==="
 $XFS_QUOTA_PROG -x -c "report -u -b -N" $SCRATCH_MNT | grep -v "^root " | _filter_quota
 
 # group test
 echo "== group test =="
-$XFS_QUOTA_PROG -x -c "limit -g bsoft=100m bhard=200m 123456-fsgqa" $SCRATCH_MNT
+$XFS_QUOTA_PROG -x -c "limit -g bsoft=100m bhard=200m fsgqa-381" $SCRATCH_MNT
 echo "=== quota command output ==="
-$XFS_QUOTA_PROG -c "quota -g -b -N -v 123456-fsgqa" $SCRATCH_MNT | _filter_quota
+$XFS_QUOTA_PROG -c "quota -g -b -N -v fsgqa-381 " $SCRATCH_MNT | _filter_quota
 echo "=== report command output ==="
 $XFS_QUOTA_PROG -x -c "report -u -b -N" $SCRATCH_MNT | grep -v "^root " | _filter_quota
 
diff --git a/tests/generic/381.out b/tests/generic/381.out
index 50a1a27b0167..cc0ee4a1bb95 100644
--- a/tests/generic/381.out
+++ b/tests/generic/381.out
@@ -3,11 +3,11 @@ QA output created by 381
 === quota command output ===
 SCRATCH_DEV 0 102400 204800 00 [--------] SCRATCH_MNT
 === report command output ===
-123456-fsgqa 0 102400 204800 00 [--------]
+fsgqa-381 0 102400 204800 00 [--------]
 
 == group test ==
 === quota command output ===
 SCRATCH_DEV 0 102400 204800 00 [--------] SCRATCH_MNT
 === report command output ===
-123456-fsgqa 0 102400 204800 00 [--------]
+fsgqa-381 0 102400 204800 00 [--------]
 
-- 
2.15.0

Re: [PATCH 1/9] generic/381: use username fsgqa-381

[Theodore Ts'o]

On Tue, Dec 12, 2017 at 04:45:11PM -0800, Luis R. Rodriguez wrote:
> Some systems are not allowing usernames prefixed with a number now.
> One can however use numbers as a postfix so use that.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  README                |  2 +-
>  tests/generic/381     | 16 ++++++++--------
>  tests/generic/381.out |  4 ++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/README b/README
> index ed69332e774e..e05142be1a87 100644
> --- a/README
> +++ b/README
> @@ -20,7 +20,7 @@ _______________________
>  - run make
>  - run make install
>  - create fsgqa test user ("sudo useradd fsgqa")
> -- create 123456-fsgqa test user ("sudo useradd 123456-fsgqa")
> +- create fsgqa-381 test user ("sudo useradd fsgqa-381")

I'd suggest using the username "fsgqa2" so that other tests that want
to use a second username can do so using a more logically name username.

       	 		     	- Ted

Re: [PATCH 1/9] generic/381: use username fsgqa-381

[Dave Chinner]

On Tue, Dec 12, 2017 at 04:45:11PM -0800, Luis R. Rodriguez wrote:
> Some systems are not allowing usernames prefixed with a number now.
> One can however use numbers as a postfix so use that.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  README                |  2 +-
>  tests/generic/381     | 16 ++++++++--------
>  tests/generic/381.out |  4 ++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/README b/README
> index ed69332e774e..e05142be1a87 100644
> --- a/README
> +++ b/README
> @@ -20,7 +20,7 @@ _______________________
>  - run make
>  - run make install
>  - create fsgqa test user ("sudo useradd fsgqa")
> -- create 123456-fsgqa test user ("sudo useradd 123456-fsgqa")
> +- create fsgqa-381 test user ("sudo useradd fsgqa-381")
>  	
>  ______________________
>  USING THE FSQA SUITE
> diff --git a/tests/generic/381 b/tests/generic/381
> index 006f0d879638..cdc29c2e029e 100755
> --- a/tests/generic/381
> +++ b/tests/generic/381
> @@ -3,7 +3,7 @@
>  #
>  # Test xfs_quota when user or names beginning with digits.
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This isn't a "test names with digits" test, but "test names begining
with digits" test.  Changing the username to not begin with digits
invalidates the entire purpose of the test which is to ensure that
xfs_quota can differentiate between UIDs and names beginning with
numbers....

So from that perspective, NAK.

IF there are distros not allowing usernames to start with digits,
then this test needs a _requires check and to _notrun on those
systems.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/9] README: document group fsgqa is required

[Luis R. Rodriguez]

The group fsgqa is also required.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 README | 1 +
 1 file changed, 1 insertion(+)

diff --git a/README b/README
index e05142be1a87..271d7df22b56 100644
--- a/README
+++ b/README
@@ -20,6 +20,7 @@ _______________________
 - run make
 - run make install
 - create fsgqa test user ("sudo useradd fsgqa")
+- create fsgqa group ("sudo groupadd fsgqa")
 - create fsgqa-381 test user ("sudo useradd fsgqa-381")
 	
 ______________________
-- 
2.15.0

[PATCH 3/9] generic/group: add 304 to dedupe group

[Luis R. Rodriguez]

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/generic/group | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/group b/tests/generic/group
index 6c3bb03a9973..4e2fb0f720ba 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -306,7 +306,7 @@
 301 auto quick clone
 302 auto quick clone
 303 auto quick clone
-304 auto quick clone
+304 auto quick clone dedupe
 305 auto quick clone
 306 auto quick rw
 307 auto quick acl
-- 
2.15.0

[PATCH 4/9] build: update AC_PACKAGE_WANT_GDBM() and src/dbtest.c to build

[Luis R. Rodriguez]

Modern gdbm-devel packages bundle together gdbm.h and ndbm.h.
The old m4 macro had detection support for some old gdbm libraries
but not for new ones.

We fix compilation of src/dbtest.c by making the autoconf helper
check for this new arrangement:

If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true,
and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already
had a HAVE_GDBM_H but there was never a respective autoconf settter for
it. We can just re-use this and fix it for new arrangement.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 m4/package_gdbmdev.m4 | 8 ++++++++
 src/dbtest.c          | 1 +
 2 files changed, 9 insertions(+)

diff --git a/m4/package_gdbmdev.m4 b/m4/package_gdbmdev.m4
index 734a192baf4d..e96343168478 100644
--- a/m4/package_gdbmdev.m4
+++ b/m4/package_gdbmdev.m4
@@ -28,6 +28,14 @@ AC_DEFUN([AC_PACKAGE_WANT_GDBM],
         AC_CHECK_HEADER(gdbm/ndbm.h, [ gdbm_ndbm_=true; have_db=true ], [ gdbm_ndbm_=false; have_db=false ])
         if test $gdbm_ndbm_ = true; then
             AC_DEFINE(HAVE_GDBM_NDBM_H_, [1], [Define to 1 if you have the <gdbm/ndbm.h> header file.])
+        else
+            AC_CHECK_HEADER(gdbm.h, [ gdbm_ndbm_=true; have_db=true ], [ gdbm_ndbm_=false; have_db=false ])
+            AC_CHECK_HEADER(ndbm.h, [ ndbm_=true ], [ ndbm_=false ])
+                if test $gdbm_ndbm_ = true; then
+                    if test $ndbm_ = true; then
+                        AC_DEFINE(HAVE_GDBM_H, [1], [Define to 1 if you have both <gdbm.h> and <ndbm.h> header files.])
+                    fi
+                fi
         fi
     fi
 
diff --git a/src/dbtest.c b/src/dbtest.c
index ec8db0b93a4e..f45db4ac2b19 100644
--- a/src/dbtest.c
+++ b/src/dbtest.c
@@ -24,6 +24,7 @@
 #include <gdbm-ndbm.h>
 #elif HAVE_GDBM_H
 #include <gdbm.h>
+#include <ndbm.h>
 #elif HAVE_NDBM_H
 #include <ndbm.h>
 #else
-- 
2.15.0

Re: [PATCH 4/9] build: update AC_PACKAGE_WANT_GDBM() and src/dbtest.c to build

[Eryu Guan]

On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote:
> Modern gdbm-devel packages bundle together gdbm.h and ndbm.h.
> The old m4 macro had detection support for some old gdbm libraries
> but not for new ones.
> 
> We fix compilation of src/dbtest.c by making the autoconf helper
> check for this new arrangement:
> 
> If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true,
                     ^^^^^^ ndbm.h?
> and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already
> had a HAVE_GDBM_H but there was never a respective autoconf settter for
> it. We can just re-use this and fix it for new arrangement.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

This looks fine to me.

The only system I have by hand that have both <gdbm.h> and <ndbm.h> but
not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed. Without this patch,
dbtest was not built on openSUSE, and was built successfully with this
patch applied. And dbtest is still built on RHEL6/7 and Fedora.

BTW, I'll queue patch 3 and this patch for next fstests release, while
other patches seem not necessary, I agreed with Dave that groups are not
for excluding tests, the required tools and environments should be
detected by tests and _notrun if not met. (The README change looks fine,
but it doesn't apply due to the "fsgqa-381" change, so I drop it too for
now.)

Thanks,
Eryu

Re: [PATCH 4/9] build: update AC_PACKAGE_WANT_GDBM() and src/dbtest.c to build

[Luis R. Rodriguez]

On Thu, Dec 14, 2017 at 01:51:02PM +0800, Eryu Guan wrote:
> On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote:
> > Modern gdbm-devel packages bundle together gdbm.h and ndbm.h.
> > The old m4 macro had detection support for some old gdbm libraries
> > but not for new ones.
> > 
> > We fix compilation of src/dbtest.c by making the autoconf helper
> > check for this new arrangement:
> > 
> > If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true,
>                      ^^^^^^ ndbm.h?
> > and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already
> > had a HAVE_GDBM_H but there was never a respective autoconf settter for
> > it. We can just re-use this and fix it for new arrangement.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> This looks fine to me.
> 
> The only system I have by hand that have both <gdbm.h> and <ndbm.h> but
> not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed.

Indeed, openSUSE and SLE releases.

> Without this patch,
> dbtest was not built on openSUSE, and was built successfully with this
> patch applied.

Yeap.

> And dbtest is still built on RHEL6/7 and Fedora.

Feel free to modify the commit log accordingly then. Curious, what packages
does Fedora/ RHEL6/7 use for the requirement here?

We just have one:

$ rpm -ql gdbm-devel-1.12-1.282.x86_64
/usr/bin/gdbm_dump
/usr/bin/gdbm_load
/usr/bin/gdbmtool
/usr/include/dbm.h
/usr/include/gdbm.h
/usr/include/ndbm.h
/usr/lib64/libgdbm.a
/usr/lib64/libgdbm.so
/usr/lib64/libgdbm_compat.a
/usr/lib64/libgdbm_compat.so
/usr/lib64/libndbm.a
/usr/lib64/libndbm.so
/usr/share/info/gdbm.info.gz
/usr/share/man/man1/gdbm_dump.1.gz
/usr/share/man/man1/gdbm_load.1.gz
/usr/share/man/man1/gdbmtool.1.gz
/usr/share/man/man3/gdbm.3.gz

> BTW, I'll queue patch 3 and this patch for next fstests release, while
> other patches seem not necessary,

I think patch 2 is fine too.

> I agreed with Dave that groups are not
> for excluding tests, the required tools and environments should be
> detected by tests and _notrun if not met.

Yeah makes sense now. I think we should also document when adding
a group makes sense as well.

> (The README change looks fine,
> but it doesn't apply due to the "fsgqa-381" change, so I drop it too for
> now.)

Feel free to modify it, its not a big deal.

  Luis

Re: [PATCH 4/9] build: update AC_PACKAGE_WANT_GDBM() and src/dbtest.c to build

[Eryu Guan]

On Thu, Dec 14, 2017 at 06:55:03PM +0100, Luis R. Rodriguez wrote:
> On Thu, Dec 14, 2017 at 01:51:02PM +0800, Eryu Guan wrote:
> > On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote:
> > > Modern gdbm-devel packages bundle together gdbm.h and ndbm.h.
> > > The old m4 macro had detection support for some old gdbm libraries
> > > but not for new ones.
> > > 
> > > We fix compilation of src/dbtest.c by making the autoconf helper
> > > check for this new arrangement:
> > > 
> > > If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true,
> >                      ^^^^^^ ndbm.h?
> > > and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already
> > > had a HAVE_GDBM_H but there was never a respective autoconf settter for
> > > it. We can just re-use this and fix it for new arrangement.
> > > 
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > 
> > This looks fine to me.
> > 
> > The only system I have by hand that have both <gdbm.h> and <ndbm.h> but
> > not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed.
> 
> Indeed, openSUSE and SLE releases.
> 
> > Without this patch,
> > dbtest was not built on openSUSE, and was built successfully with this
> > patch applied.
> 
> Yeap.
> 
> > And dbtest is still built on RHEL6/7 and Fedora.
> 
> Feel free to modify the commit log accordingly then. Curious, what packages
> does Fedora/ RHEL6/7 use for the requirement here?
> 
> We just have one:
> 
> $ rpm -ql gdbm-devel-1.12-1.282.x86_64
> /usr/bin/gdbm_dump
> /usr/bin/gdbm_load
> /usr/bin/gdbmtool
> /usr/include/dbm.h
> /usr/include/gdbm.h
> /usr/include/ndbm.h
> /usr/lib64/libgdbm.a
> /usr/lib64/libgdbm.so
> /usr/lib64/libgdbm_compat.a
> /usr/lib64/libgdbm_compat.so
> /usr/lib64/libndbm.a
> /usr/lib64/libndbm.so
> /usr/share/info/gdbm.info.gz
> /usr/share/man/man1/gdbm_dump.1.gz
> /usr/share/man/man1/gdbm_load.1.gz
> /usr/share/man/man1/gdbmtool.1.gz
> /usr/share/man/man3/gdbm.3.gz

gdbm-devel too, but it has gdbm/[gn]dbm.h pointing to ../[gn]dbm.h, so
there's no such problem and dbtest is building normally.

# rpm -ql gdbm-devel
/usr/include/dbm.h
/usr/include/gdbm
/usr/include/gdbm.h
/usr/include/gdbm/dbm.h
/usr/include/gdbm/gdbm.h
/usr/include/gdbm/ndbm.h
/usr/include/ndbm.h
/usr/lib64/libgdbm.so
/usr/lib64/libgdbm_compat.so
/usr/share/info/gdbm.info.gz
/usr/share/man/man3/gdbm.3.gz

> 
> > BTW, I'll queue patch 3 and this patch for next fstests release, while
> > other patches seem not necessary,
> 
> I think patch 2 is fine too.
> 
> > I agreed with Dave that groups are not
> > for excluding tests, the required tools and environments should be
> > detected by tests and _notrun if not met.
> 
> Yeah makes sense now. I think we should also document when adding
> a group makes sense as well.
> 
> > (The README change looks fine,
> > but it doesn't apply due to the "fsgqa-381" change, so I drop it too for
> > now.)
> 
> Feel free to modify it, its not a big deal.

OK, I'll modify on commit, thanks!

Thanks,
Eryu

Re: [PATCH 4/9] build: update AC_PACKAGE_WANT_GDBM() and src/dbtest.c to build

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2815 bytes --]

On 12/14/17 12:51 AM, Eryu Guan wrote:
> On Tue, Dec 12, 2017 at 04:45:14PM -0800, Luis R. Rodriguez wrote:
>> Modern gdbm-devel packages bundle together gdbm.h and ndbm.h.
>> The old m4 macro had detection support for some old gdbm libraries
>> but not for new ones.
>>
>> We fix compilation of src/dbtest.c by making the autoconf helper
>> check for this new arrangement:
>>
>> If both gdbm.h and gdbm.h are found define set both gdbm_ndbm_=true,
>                      ^^^^^^ ndbm.h?
>> and have_db=true, and define HAVE_GDBM_H. The src/dbtest.c already
>> had a HAVE_GDBM_H but there was never a respective autoconf settter for
>> it. We can just re-use this and fix it for new arrangement.
>>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> This looks fine to me.
> 
> The only system I have by hand that have both <gdbm.h> and <ndbm.h> but
> not any <gdbm/[gn]dbm.h> is openSUSE Tumbleweed. Without this patch,
> dbtest was not built on openSUSE, and was built successfully with this
> patch applied. And dbtest is still built on RHEL6/7 and Fedora.
> 
> BTW, I'll queue patch 3 and this patch for next fstests release, while
> other patches seem not necessary, I agreed with Dave that groups are not
> for excluding tests, the required tools and environments should be
> detected by tests and _notrun if not met. (The README change looks fine,
> but it doesn't apply due to the "fsgqa-381" change, so I drop it too for
> now.)

Hi guys -

This change breaks on older releases like SLES 11 where both <ndbm.h>
and <gdbm.h> define datum, so we get build failures.  The failure is
new, but not because it used to pass and now doesn't.  It's apparently
never built on SLES releases since we ship /usr/include/ndbm.h and then
we notrun the test that uses.  Now that we're looking for gdbm.h and
find it, we attempt to build src/dbtest and fail.

This fix isn't the right solution.  The problem is that we have a couple
layers of old cruft that needs to be cleaned out.

1) As Luis notes, nothing sets HAVE_GDBM_H.  The thing is that there is
no version of gdbm.h that exports the NDBM interface.  Further, looking
at the git history, nothing has ever set HAVE_GDBM_H.  It was dead code
when it was committed initially as best I can tell.
2) openSUSE Tumbleweed doesn't need <gdbm.h> at all.  It needs <ndbm.h>
and this fix works because Luis added it to the HAVE_GDBM_H stanza.
3) AC_PACKAGE_WANT_NDBM used to check for <ndbm.h> but it was a check
for IRIX and the caller was removed ages ago.  It wouldn't matter if it
were called anyway since libndbm is an IRIX library.  Linux, IIRC, has
never shipped a libndbm.

I'll post a few patches following this to clean it up and get it working
on SLES11.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

[PATCH 5/9] tests/xfs/group: add group for tests which require a logdev

[Luis R. Rodriguez]

This should make it easy to run these separately or exclude them.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/xfs/group | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/xfs/group b/tests/xfs/group
index d23006041ea2..cce98847de53 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -42,7 +42,7 @@
 042 fsr ioctl auto
 043 dump ioctl tape
 044 other auto
-045 other auto quick
+045 other auto quick logdev
 046 dump ioctl auto quick
 047 dump ioctl auto
 048 other auto quick
@@ -272,7 +272,7 @@
 272 auto quick rmap fsmap
 273 auto rmap fsmap
 274 auto quick rmap fsmap
-275 auto quick rmap fsmap
+275 auto quick rmap fsmap logdev
 276 auto quick rmap fsmap
 277 auto quick rmap fsmap
 278 repair auto
-- 
2.15.0

Re: [PATCH 5/9] tests/xfs/group: add group for tests which require a logdev

[Dave Chinner]

On Tue, Dec 12, 2017 at 04:45:15PM -0800, Luis R. Rodriguez wrote:
> This should make it easy to run these separately or exclude them.

These should notrun automatically if you don't have an external log
device configured. Every test should either work with an external
logdev or explicitly notrun them, so I'm not sure what you're trying
to acheive here....

> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  tests/xfs/group | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/xfs/group b/tests/xfs/group
> index d23006041ea2..cce98847de53 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -42,7 +42,7 @@
>  042 fsr ioctl auto
>  043 dump ioctl tape
>  044 other auto
> -045 other auto quick
> +045 other auto quick logdev

This change also looks wrong because:

xfs/044  [not run] This test requires a valid $SCRATCH_LOGDEV
xfs/045 1s ... 1s

xfs/044 is the external logdev test, and xfs/045 is a
duplicate uuid mount test that has nothign to do with external
log devices.

And, FWIW, we already have a "log" group to indicate tests that
exercise the log, and that mostly includes all the tests that use
external logs. It would be better to tag all the tests that exercise
the log with "log" rather than create some new group that doesn't
really provide any added benefit....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/9] tests/xfs/group: add group for tests which require a logdev

[Luis R. Rodriguez]

On Thu, Dec 14, 2017 at 08:50:13AM +1100, Dave Chinner wrote:
> On Tue, Dec 12, 2017 at 04:45:15PM -0800, Luis R. Rodriguez wrote:
> > This should make it easy to run these separately or exclude them.
> 
> These should notrun automatically if you don't have an external log
> device configured. Every test should either work with an external
> logdev or explicitly notrun them, so I'm not sure what you're trying
> to acheive here....

The way I'm splitting up tests is one first run with a basic xfs section
on a configuration file, with no external log, which pretty much runs all
tests but excludes all which require external or funky configurations.

A secondary pass then goes through these extra groups and then runs tests
only for the previously excluded groups but with their own respective
section. So for instance in this case I have:

[xfs]
....
[logdev_xfs]
...

Automatic detection if the requirements are met is fine, but this doesn't 
let me easily use say:

	./check -s logdev_xfs -g logdev

> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  tests/xfs/group | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index d23006041ea2..cce98847de53 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -42,7 +42,7 @@
> >  042 fsr ioctl auto
> >  043 dump ioctl tape
> >  044 other auto
> > -045 other auto quick
> > +045 other auto quick logdev
> 
> This change also looks wrong because:
> 
> xfs/044  [not run] This test requires a valid $SCRATCH_LOGDEV
> xfs/045 1s ... 1s
> 
> xfs/044 is the external logdev test, and xfs/045 is a
> duplicate uuid mount test that has nothign to do with external
> log devices.

I see...

> And, FWIW, we already have a "log" group to indicate tests that
> exercise the log, and that mostly includes all the tests that use
> external logs. It would be better to tag all the tests that exercise
> the log with "log" rather than create some new group that doesn't
> really provide any added benefit....

So for my case would one better goal be to just run check without the external
one and one with the external log?

  ./check -s xfs -g log
  ./check -s logdev_xfs -g log

  Luis

Re: [PATCH 5/9] tests/xfs/group: add group for tests which require a logdev

[Dave Chinner]

On Thu, Dec 14, 2017 at 12:00:52AM +0100, Luis R. Rodriguez wrote:
> On Thu, Dec 14, 2017 at 08:50:13AM +1100, Dave Chinner wrote:
> > On Tue, Dec 12, 2017 at 04:45:15PM -0800, Luis R. Rodriguez wrote:
> > > This should make it easy to run these separately or exclude them.
> > 
> > These should notrun automatically if you don't have an external log
> > device configured. Every test should either work with an external
> > logdev or explicitly notrun them, so I'm not sure what you're trying
> > to acheive here....
> 
> The way I'm splitting up tests is one first run with a basic xfs section
> on a configuration file, with no external log, which pretty much runs all
> tests but excludes all which require external or funky configurations.
> 
> A secondary pass then goes through these extra groups and then runs tests
> only for the previously excluded groups but with their own respective
> section. So for instance in this case I have:
> 
> [xfs]
> ....
> [logdev_xfs]
> ...

Which seems to me like a misguided attempt to optimise test
runtimes. i.e. this doesn't provide test coverage of external log
behaviour in all the cases that need to be tested.

Data integrity code paths are affected by having an external log. IO
ordering changes with external logs, which can expose update/crash
recovery problems. external logs can expose data IO race conditions
that are masked by interleaved log IO. etc, etc, etc.

You can't just run an internal log test then add couple of extra
external log tests and say "external logs work fine".

> Automatic detection if the requirements are met is fine, but this doesn't 
> let me easily use say:
> 
> 	./check -s logdev_xfs -g logdev

You can do that if we ignore the fact that a large number of tests
need to be run on both internal and external log devices to cover
the differences in behaviour between them.

> > And, FWIW, we already have a "log" group to indicate tests that
> > exercise the log, and that mostly includes all the tests that use
> > external logs. It would be better to tag all the tests that exercise
> > the log with "log" rather than create some new group that doesn't
> > really provide any added benefit....
> 
> So for my case would one better goal be to just run check without the external
> one and one with the external log?

See above. Your test coverage assumptions are wrong, so what you are
trying to do really doesn't tell you whether external logs work
correctly or not. It's worse that not testing external logs at all,
because it gives the false impression that they have been
exhaustively tested and work just fine when that really isn't the
case.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/9] tests/xfs/group: add group for tests which require a logdev

[Luis R. Rodriguez]

On Thu, Dec 14, 2017 at 10:39:14AM +1100, Dave Chinner wrote:
> You can't just run an internal log test then add couple of extra
> external log tests and say "external logs work fine".
> 
> > Automatic detection if the requirements are met is fine, but this doesn't 
> > let me easily use say:
> > 
> > 	./check -s logdev_xfs -g logdev
> 
> You can do that if we ignore the fact that a large number of tests
> need to be run on both internal and external log devices to cover
> the differences in behaviour between them.
> 
> > > And, FWIW, we already have a "log" group to indicate tests that
> > > exercise the log, and that mostly includes all the tests that use
> > > external logs. It would be better to tag all the tests that exercise
> > > the log with "log" rather than create some new group that doesn't
> > > really provide any added benefit....
> > 
> > So for my case would one better goal be to just run check without the external
> > one and one with the external log?
> 
> See above. Your test coverage assumptions are wrong, so what you are
> trying to do really doesn't tell you whether external logs work
> correctly or not. It's worse that not testing external logs at all,
> because it gives the false impression that they have been
> exhaustively tested and work just fine when that really isn't the
> case.

Makes sense, thanks.

  Luis

[PATCH 6/9] tests/ext4/group: add group for tests which require a logdev

[Luis R. Rodriguez]

This should make it easy to run these separately or exclude them.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/ext4/group | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ext4/group b/tests/ext4/group
index 257bb646f312..6fa2f06432f3 100644
--- a/tests/ext4/group
+++ b/tests/ext4/group
@@ -31,7 +31,7 @@
 026 auto quick attr
 027 auto quick fsmap
 028 auto quick fsmap
-029 auto quick fsmap
+029 auto quick fsmap logdev
 271 auto rw quick
 301 aio auto ioctl rw stress defrag
 302 aio auto ioctl rw stress defrag
-- 
2.15.0

[PATCH 7/9] tests/xfs/group: add realtimedev group

[Luis R. Rodriguez]

This groups up tests which require an external realtime volume,
ie, SCRATCH_RTDEV. This requires CONFIG_XFS_RT and not all kernels
support this.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/xfs/group | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/tests/xfs/group b/tests/xfs/group
index cce98847de53..656b65f5fe7a 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -87,11 +87,11 @@
 087 fuzzers
 088 fuzzers
 089 fuzzers
-090 rw auto
+090 rw auto realtimedev
 091 fuzzers
 092 other auto quick
 093 fuzzers
-094 metadata dir ioctl auto
+094 metadata dir ioctl auto realtimedev
 095 log v2log auto
 096 mkfs v2log auto quick
 097 fuzzers
@@ -128,7 +128,7 @@
 128 auto quick clone fsr
 129 auto quick clone
 130 fuzzers clone
-131 auto quick clone
+131 auto quick clone realtimedev
 132 auto quick clone
 133 dangerous_fuzzers
 134 dangerous_fuzzers
@@ -273,7 +273,7 @@
 273 auto rmap fsmap
 274 auto quick rmap fsmap
 275 auto quick rmap fsmap logdev
-276 auto quick rmap fsmap
+276 auto quick rmap fsmap realtimedev
 277 auto quick rmap fsmap
 278 repair auto
 279 auto mkfs
@@ -331,16 +331,16 @@
 331 auto quick rmap clone
 332 auto quick rmap clone
 333 auto quick rmap
-334 auto quick rmap
-335 auto rmap
-336 auto rmap
-337 fuzzers rmap
-338 auto quick rmap
-339 auto quick rmap
-340 auto quick rmap
-341 auto quick rmap
-342 auto quick rmap
-343 auto quick rmap
+334 auto quick rmap realtimedev
+335 auto rmap realtimedev
+336 auto rmap realtimedev
+337 fuzzers rmap realtimedev
+338 auto quick rmap realtimedev
+339 auto quick rmap realtimedev
+340 auto quick rmap realtimedev
+341 auto quick rmap realtimedev
+342 auto quick rmap realtimedev
+343 auto quick rmap realtimedev
 344 auto quick clone
 345 auto quick clone
 346 auto quick clone
@@ -403,10 +403,10 @@
 403 dangerous_fuzzers dangerous_scrub dangerous_online_repair
 404 dangerous_fuzzers dangerous_scrub dangerous_repair
 405 dangerous_fuzzers dangerous_scrub dangerous_online_repair
-406 dangerous_fuzzers dangerous_scrub dangerous_repair
+406 dangerous_fuzzers dangerous_scrub dangerous_repair realtimedev
 407 dangerous_fuzzers dangerous_scrub dangerous_online_repair
-408 dangerous_fuzzers dangerous_scrub dangerous_repair
-409 dangerous_fuzzers dangerous_scrub dangerous_online_repair
+408 dangerous_fuzzers dangerous_scrub dangerous_repair realtimedev
+409 dangerous_fuzzers dangerous_scrub dangerous_online_repair realtimedev
 410 dangerous_fuzzers dangerous_scrub dangerous_repair
 411 dangerous_fuzzers dangerous_scrub dangerous_online_repair
 412 dangerous_fuzzers dangerous_scrub dangerous_repair
@@ -416,7 +416,7 @@
 416 dangerous_fuzzers dangerous_scrub dangerous_repair
 417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
 418 dangerous_fuzzers dangerous_scrub dangerous_repair
-419 auto quick
+419 auto quick realtimedev
 420 auto quick clone dedupe
 421 auto quick clone dedupe
 422 dangerous_scrub dangerous_online_repair
-- 
2.15.0

[PATCH 8/9] tests/xfs/group: add group for tests which require mkfs v4_5

[Luis R. Rodriguez]

This lets us skip these tests on newer systems.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/xfs/group | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/xfs/group b/tests/xfs/group
index 656b65f5fe7a..ebb1685e22eb 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -93,7 +93,7 @@
 093 fuzzers
 094 metadata dir ioctl auto realtimedev
 095 log v2log auto
-096 mkfs v2log auto quick
+096 mkfs v2log auto quick mkfs_4_5
 097 fuzzers
 098 fuzzers
 099 fuzzers
-- 
2.15.0

Re: [PATCH 8/9] tests/xfs/group: add group for tests which require mkfs v4_5

[Dave Chinner]

On Tue, Dec 12, 2017 at 04:45:18PM -0800, Luis R. Rodriguez wrote:
> This lets us skip these tests on newer systems.

This already _not_runs on new systems. we do not want to be adding
one-off group descriptors to avoid tests like this - the tests
themselves already detect whether they should run or not.

xfs/096 2s ... [not run] Requires older mkfs without strict input checks: the last supported version of xfsprogs is 4.5.

Why are you trying to add groups to define tests that not_run
correctly on systems they aren't supported on?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 9/9] tests/xfs/group: add injection group

[Luis R. Rodriguez]

XFS error injection requires CONFIG_XFS_DEBUG and not all kernels
have this enabled.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/xfs/group | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/xfs/group b/tests/xfs/group
index ebb1685e22eb..1df0ab32b71c 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -54,7 +54,7 @@
 054 auto quick
 055 dump ioctl remote tape
 056 dump ioctl auto quick
-057 auto log
+057 auto log injection
 058 auto quick fuzzers
 059 dump ioctl auto quick
 060 dump ioctl auto quick
@@ -138,7 +138,7 @@
 138 auto quick
 139 auto quick clone
 140 auto clone
-141 auto log metadata
+141 auto log metadata injection
 142 dmapi
 143 dmapi
 144 dmapi
@@ -193,7 +193,7 @@
 193 auto quick clone
 194 rw auto
 195 ioctl dump auto quick
-196 auto quick rw
+196 auto quick rw injection
 197 dir auto quick
 198 auto quick clone
 199 mount auto quick
@@ -315,7 +315,7 @@
 315 auto quick clone
 316 auto quick clone
 317 auto quick rmap
-318 auto quick rw
+318 auto quick rw injection
 319 auto quick clone
 320 auto quick clone
 321 auto quick clone
@@ -430,7 +430,7 @@
 430 dangerous_fuzzers dangerous_scrub dangerous_online_repair
 431 auto quick dangerous
 432 auto quick dir metadata
-433 auto quick attr
+433 auto quick attr injection
 434 auto quick clone fsr
 435 auto quick clone
 436 auto quick clone fsr
-- 
2.15.0

Re: [PATCH 9/9] tests/xfs/group: add injection group

[Dave Chinner]

On Tue, Dec 12, 2017 at 04:45:19PM -0800, Luis R. Rodriguez wrote:
> XFS error injection requires CONFIG_XFS_DEBUG and not all kernels
> have this enabled.

NAK. this should be automatically detected by the tests and they
_not_run when support is not available.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com