[PATCH 00/12] xfstests: fix up various tmpfs failures

[PATCH 00/12] xfstests: fix up various tmpfs failures

[Theodore Ts'o]

I've been trying to sync up our internal xfststs tree at $WORK with
the latest upstream xfstests tree, and have been cleaning up the
patches so that they will hopefully be suitable for upstream merging.
This is most of the changes we have for tmpfs support.  I've left out
a few that I suspect should be fixed in other ways (e.g., in our
automated test framework).

With these changes all of the auto group tests are passing with tmpfs
with a the exception of generic/80 (which looks like a genuine test
failure) and generic/269 and generic/273 which are causing an OOM
kill (tmpfs allows an infinite number of inodes to be created until
kmalloc() fails or the test or the test runner gets killed with an OOM
kill).

I've disabled generic/027 for tmpfs for similar ENOSPC-related
reasons; we may want to do something similar for generic/269 and
generic/273 but I want to do a more in-depth examination of those
tests to be sure that there isn't an easy way to fix those tests for
tmpfs.

Hugh Dickins (8):
  common: _scratch_mkfs_sized() for tmpfs
  generic: use mount point instead of device name
  generic: add _require_odirect to three more tests
  generic: require fiemap for generic/009
  xfstests: fix generic/312 on tmpfs, ignore /proc/partitions
  xfstests: generic/079 requires chattr, not xattrs
  xfstests: add executable permission to tests
  xfstests: increase tmpfs memory size

Junho Ryu (2):
  xfstests: do not unmount tmpfs during remount
  generic: do not unmount before calling _check_scratch_fs()

Theodore Ts'o (2):
  check: avoid error messages of tests/$FS does not exist
  generic: disable generic/027 for tmpfs

 check                 |  6 ++++++
 common/config         |  4 ++--
 common/rc             | 35 ++++++++++++++++++++++++++++++-----
 tests/generic/003     | 12 ++++--------
 tests/generic/009     |  2 ++
 tests/generic/027     |  1 +
 tests/generic/053     |  2 --
 tests/generic/053.out |  2 --
 tests/generic/058     |  0
 tests/generic/060     |  0
 tests/generic/061     |  0
 tests/generic/063     |  0
 tests/generic/079     |  2 +-
 tests/generic/113     |  1 +
 tests/generic/125     |  1 +
 tests/generic/135     | 17 +++--------------
 tests/generic/169     | 20 ++++++--------------
 tests/generic/169.out |  6 ++----
 tests/generic/192     |  3 +--
 tests/generic/214     |  1 +
 tests/generic/226     |  3 +--
 tests/generic/258     |  3 +--
 tests/generic/273     |  2 +-
 tests/generic/306     |  3 +--
 tests/generic/312     | 11 ++++++-----
 tests/generic/317     |  3 +--
 tests/generic/318     |  3 +--
 27 files changed, 73 insertions(+), 70 deletions(-)
 mode change 100644 => 100755 tests/generic/058
 mode change 100644 => 100755 tests/generic/060
 mode change 100644 => 100755 tests/generic/061
 mode change 100644 => 100755 tests/generic/063

-- 
2.5.0

[PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Theodore Ts'o]

There are no tmpfs specific tests, so tests/tmpfs does not exist.
Avoid print an error message caused by trying to read the file
tests/tmpfs/group (for example).

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 check | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/check b/check
index c40d2a8..2986d84 100755
--- a/check
+++ b/check
@@ -90,6 +90,9 @@ get_group_list()
 	grp=$1
 
 	for d in $SRC_GROUPS $FSTYP; do
+		if ! test -d "$SRC_DIR/$d" ; then
+			continue
+		fi
 		l=$(sed -n < $SRC_DIR/$d/group \
 			-e 's/#.*//' \
 			-e 's/$/ /' \
@@ -105,6 +108,9 @@ get_all_tests()
 {
 	touch $tmp.list
 	for d in $SRC_GROUPS $FSTYP; do
+		if ! test -d "$SRC_DIR/$d" ; then
+			continue
+		fi
 		ls $SRC_DIR/$d/* | \
 			grep -v "\..*" | \
 			grep "^$SRC_DIR/$d/$VALID_TEST_NAME"| \
-- 
2.5.0

Re: [PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Dave Chinner]

On Tue, Feb 09, 2016 at 08:49:50PM -0500, Theodore Ts'o wrote:
> There are no tmpfs specific tests, so tests/tmpfs does not exist.
> Avoid print an error message caused by trying to read the file
> tests/tmpfs/group (for example).
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Where does this error message get emitted? I would have expected
lots of people to notice this (e.g. NFS, CIFS, ext2, ext3, gfs2)
if this was causing some kind of problem long before this...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 04:45:31PM +1100, Dave Chinner wrote:
> On Tue, Feb 09, 2016 at 08:49:50PM -0500, Theodore Ts'o wrote:
> > There are no tmpfs specific tests, so tests/tmpfs does not exist.
> > Avoid print an error message caused by trying to read the file
> > tests/tmpfs/group (for example).
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> Where does this error message get emitted? I would have expected
> lots of people to notice this (e.g. NFS, CIFS, ext2, ext3, gfs2)
> if this was causing some kind of problem long before this...

It doesn't really cause a problem; it's more of a cosmetic issue, in
that it prints a harmless/annoying error message during the test
startup.  It shows up if you specify a group, e.g. "check -g quick":

BEGIN TEST tmpfs: tmpfs Wed Feb 10 10:31:09 EST 2016
DEVICE: test:/tmp
MK2FS OPTIONS:
MOUNT OPTIONS: -o block_validity
./check: line 96: tests/tmpfs/group: No such file or directory
FSTYP         -- tmpfs
PLATFORM      -- Linux/i686 kvm-xfstests 4.5.0-rc2ext4-00002-g6df2762
MKFS_OPTIONS  -- test:/scratch
MOUNT_OPTIONS -- -o size=1G test:/scratch /test/scratch

generic/001   	 [10:31:10][    5.811742] run fstests generic/001
    ...

Cheers,

						- Ted

Re: [PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Christoph Hellwig]

On Wed, Feb 10, 2016 at 10:34:55AM -0500, Theodore Ts'o wrote:
> It doesn't really cause a problem; it's more of a cosmetic issue, in
> that it prints a harmless/annoying error message during the test
> startup.  It shows up if you specify a group, e.g. "check -g quick":

I found this annoying when testing nfs, and it's been on my todo list
for a while to fix it.  Never high enough to actually do it, but I'd
love to see the fix merged.

Re: [PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 08:28:40AM -0800, Christoph Hellwig wrote:
> On Wed, Feb 10, 2016 at 10:34:55AM -0500, Theodore Ts'o wrote:
> > It doesn't really cause a problem; it's more of a cosmetic issue, in
> > that it prints a harmless/annoying error message during the test
> > startup.  It shows up if you specify a group, e.g. "check -g quick":
> 
> I found this annoying when testing nfs, and it's been on my todo list
> for a while to fix it.  Never high enough to actually do it, but I'd
> love to see the fix merged.

Yeah, it triggered my inner OCD, but if it weren't for the fact that I
was making a bunch of other tmpfs changes it probably wouldn't have
risen to the top of my list either.

					- Ted

Re: [PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Dave Chinner]

On Wed, Feb 10, 2016 at 10:34:55AM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 04:45:31PM +1100, Dave Chinner wrote:
> > On Tue, Feb 09, 2016 at 08:49:50PM -0500, Theodore Ts'o wrote:
> > > There are no tmpfs specific tests, so tests/tmpfs does not exist.
> > > Avoid print an error message caused by trying to read the file
> > > tests/tmpfs/group (for example).
> > > 
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > 
> > Where does this error message get emitted? I would have expected
> > lots of people to notice this (e.g. NFS, CIFS, ext2, ext3, gfs2)
> > if this was causing some kind of problem long before this...
> 
> It doesn't really cause a problem; it's more of a cosmetic issue, in
> that it prints a harmless/annoying error message during the test
> startup.  It shows up if you specify a group, e.g. "check -g quick":
> 
> BEGIN TEST tmpfs: tmpfs Wed Feb 10 10:31:09 EST 2016
> DEVICE: test:/tmp
> MK2FS OPTIONS:
> MOUNT OPTIONS: -o block_validity
> ./check: line 96: tests/tmpfs/group: No such file or directory
> FSTYP         -- tmpfs
> PLATFORM      -- Linux/i686 kvm-xfstests 4.5.0-rc2ext4-00002-g6df2762
> MKFS_OPTIONS  -- test:/scratch
> MOUNT_OPTIONS -- -o size=1G test:/scratch /test/scratch
> 
> generic/001   	 [10:31:10][    5.811742] run fstests generic/001

Can you please rewrite the commit message to include this?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/12] check: avoid error messages of tests/$FS does not exist

[Theodore Ts'o]

On Thu, Feb 11, 2016 at 09:19:59AM +1100, Dave Chinner wrote:
> On Wed, Feb 10, 2016 at 10:34:55AM -0500, Theodore Ts'o wrote:
> > On Wed, Feb 10, 2016 at 04:45:31PM +1100, Dave Chinner wrote:
> > > On Tue, Feb 09, 2016 at 08:49:50PM -0500, Theodore Ts'o wrote:
> > > > There are no tmpfs specific tests, so tests/tmpfs does not exist.
> > > > Avoid print an error message caused by trying to read the file
> > > > tests/tmpfs/group (for example).
> > > > 
> > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > 
> > > Where does this error message get emitted? I would have expected
> > > lots of people to notice this (e.g. NFS, CIFS, ext2, ext3, gfs2)
> > > if this was causing some kind of problem long before this...
> > 
> > It doesn't really cause a problem; it's more of a cosmetic issue, in
> > that it prints a harmless/annoying error message during the test
> > startup.  It shows up if you specify a group, e.g. "check -g quick":
> > 
> > BEGIN TEST tmpfs: tmpfs Wed Feb 10 10:31:09 EST 2016
> > DEVICE: test:/tmp
> > MK2FS OPTIONS:
> > MOUNT OPTIONS: -o block_validity
> > ./check: line 96: tests/tmpfs/group: No such file or directory
> > FSTYP         -- tmpfs
> > PLATFORM      -- Linux/i686 kvm-xfstests 4.5.0-rc2ext4-00002-g6df2762
> > MKFS_OPTIONS  -- test:/scratch
> > MOUNT_OPTIONS -- -o size=1G test:/scratch /test/scratch
> > 
> > generic/001   	 [10:31:10][    5.811742] run fstests generic/001
> 
> Can you please rewrite the commit message to include this?

Sure, will do.

						- Ted

[PATCH 02/12] common: _scratch_mkfs_sized() for tmpfs

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

_scratch_mkfs_sized() avoid blockdev and update MOUNT_OPTIONS with
required size on tmpfs, so those tests using it can now run.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Junho Ryu <jayr@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/rc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 76e02e4..aca723f 100644
--- a/common/rc
+++ b/common/rc
@@ -813,7 +813,7 @@ _scratch_mkfs_sized()
 
     blocks=`expr $fssize / $blocksize`
 
-    if [ "$HOSTOS" == "Linux" ]; then
+    if [ "$HOSTOS" == "Linux" -a -b "$SCRATCH_DEV" ]; then
 	devsize=`blockdev --getsize64 $SCRATCH_DEV`
 	[ "$fssize" -gt "$devsize" ] && _notrun "Scratch device too small"
     fi
@@ -849,6 +849,9 @@ _scratch_mkfs_sized()
 	sector_size=`blockdev --getss $SCRATCH_DEV`
 	$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
 	;;
+    tmpfs)
+	export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
+	;;
     *)
 	_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
 	;;
-- 
2.5.0

Re: [PATCH 02/12] common: _scratch_mkfs_sized() for tmpfs

[Dave Chinner]

On Tue, Feb 09, 2016 at 08:49:51PM -0500, Theodore Ts'o wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> _scratch_mkfs_sized() avoid blockdev and update MOUNT_OPTIONS with
> required size on tmpfs, so those tests using it can now run.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Junho Ryu <jayr@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  common/rc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 76e02e4..aca723f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -813,7 +813,7 @@ _scratch_mkfs_sized()
>  
>      blocks=`expr $fssize / $blocksize`
>  
> -    if [ "$HOSTOS" == "Linux" ]; then
> +    if [ "$HOSTOS" == "Linux" -a -b "$SCRATCH_DEV" ]; then
>  	devsize=`blockdev --getsize64 $SCRATCH_DEV`
>  	[ "$fssize" -gt "$devsize" ] && _notrun "Scratch device too small"
>      fi
> @@ -849,6 +849,9 @@ _scratch_mkfs_sized()
>  	sector_size=`blockdev --getss $SCRATCH_DEV`
>  	$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
>  	;;
> +    tmpfs)
> +	export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"

So what happens when the test asks for 10GB of device space, and you
only have 4GB of RAM?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 02/12] common: _scratch_mkfs_sized() for tmpfs

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 05:00:58PM +1100, Dave Chinner wrote:
> > +    tmpfs)
> > +	export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
> 
> So what happens when the test asks for 10GB of device space, and you
> only have 4GB of RAM?

The largest amount of storage space requested by shared/generic tests
via _scratch_mkfs_sized is 2G, and that's from generic/27[345].

We currently could run into problems today if the storage device only
had 3G of space, and some test, such as f2fs/001, called
_scratch_mkfs_sized requesting 4G of space.  We currently aren't
checking error returns from the mkfs command; we just assume that it
will succeed.

Because of the someone interesting behavior of tmpfs assuming inode
space is free, we already have problems where even with 8G of memory,
generic/269 and generic/273 will result in the test getting OOM
killed, and if you use less than 4G, there are a more tests that end
up getting OOM killed as a result.

So I think it would be fair to simply document that if you are testing
tmpfs, you need a VM or or a memory container with at least 8G of
memory.  If we start limiting the number of inodes in a tmpfs mount,
we could probably bring that requirement down to 4G, which is
certainly fair.  We do document minimum requirements for the size of
the TEST_DEV and SCRATCH_DEV, don't we?   (Minimum requirements
certainly do exist in practice, in any case.)

Cheers,

					- Ted

Re: [PATCH 02/12] common: _scratch_mkfs_sized() for tmpfs

[Dave Chinner]

On Wed, Feb 10, 2016 at 10:58:37AM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 05:00:58PM +1100, Dave Chinner wrote:
> > > +    tmpfs)
> > > +	export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
> > 
> > So what happens when the test asks for 10GB of device space, and you
> > only have 4GB of RAM?
> 
> The largest amount of storage space requested by shared/generic tests
> via _scratch_mkfs_sized is 2G, and that's from generic/27[345].

Sure, but that doesn't doesn't invalidate my question.

I run xfstests on machines with 1GB RAM, so those tests you list
above are going to have problems with tmpfs, yes?

> We currently could run into problems today if the storage device only
> had 3G of space, and some test, such as f2fs/001, called
> _scratch_mkfs_sized requesting 4G of space.  We currently aren't
> checking error returns from the mkfs command; we just assume that it
> will succeed.

That would be a test bug.  We have a general rule of thumb that
test/scratch devices are typically a couple of GB in size, so if the
test needs less space than that it doesn't need to check. If the
test needs more than 1-2GB of space, then it needs to call
_require_fs_space to abort the test if the device is too small.
Hence we end up with tests being not run with:

generic/312      [not run] this test requires $SCRATCH_DEV has 5368709120B space

This needs to work for tmpfs, too, so that we don't run tests that
will overcommit RAM on machines with limited memory.

> Because of the someone interesting behavior of tmpfs assuming inode
> space is free, we already have problems where even with 8G of memory,
> generic/269 and generic/273 will result in the test getting OOM
> killed, and if you use less than 4G, there are a more tests that end
> up getting OOM killed as a result.

As I said, this is a tmpfs bug that needs fixing, not working around
in xfstests. If tmpfs is limited to 512MB or 1GB of space, then
having it be able to consume multiple times it's requested size in
memory is simply wrong.

> So I think it would be fair to simply document that if you are
> testing tmpfs, you need a VM or or a memory container with at
> least 8G of memory. If we start limiting the number of inodes in a
> tmpfs mount, we could probably bring that requirement down to 4G,
> which is certainly fair.  We do document minimum requirements for
> the size of the TEST_DEV and SCRATCH_DEV, don't we? 

No, not that I know of.

> (Minimum
> requirements certainly do exist in practice, in any case.)

See above.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 03/12] generic: use mount point instead of device name

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

A tmpfs mount does not involve any block device, its $SCRATCH_DEV is
nothing but a place-holder, so apply 'df' or 'stat' to its mount point
$SCRATCH_MNT instead of to $SCRATCH_DEV.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Junho Ryu <jayr@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/273 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/273 b/tests/generic/273
index e5af7f1..0a4a600 100755
--- a/tests/generic/273
+++ b/tests/generic/273
@@ -68,7 +68,7 @@ _file_create()
 
 	cd $SCRATCH_MNT/origin
 
-	_disksize=`$DF_PROG -B 1 $SCRATCH_DEV | tail -1 | $AWK_PROG '{ print $5 }'`
+	_disksize=`$DF_PROG -B 1 $SCRATCH_MNT | tail -1 | $AWK_PROG '{ print $5 }'`
 	_disksize=$(($_disksize / 3))
 	_num=$(($_disksize / $count / $threads / 4096))
 	_count=$count
-- 
2.5.0

[PATCH 04/12] generic: add _require_odirect to three more tests

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

generic/113, generic/125, generic/214 each use O_DIRECT at some
stage in their tests, so check O_DIRECT support before running them.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/113 | 1 +
 tests/generic/125 | 1 +
 tests/generic/214 | 1 +
 3 files changed, 3 insertions(+)

diff --git a/tests/generic/113 b/tests/generic/113
index 7208fa2..54d6191 100755
--- a/tests/generic/113
+++ b/tests/generic/113
@@ -77,6 +77,7 @@ _do_test()
 _supported_fs generic
 _supported_os Linux
 _require_test
+_require_odirect
 
 [ -x $here/ltp/aio-stress ] || _notrun "aio-stress not built for this platform"
 
diff --git a/tests/generic/125 b/tests/generic/125
index bcf9b3e..67eb63f 100755
--- a/tests/generic/125
+++ b/tests/generic/125
@@ -41,6 +41,7 @@ _supported_os Linux
 
 _require_test
 _require_user
+_require_odirect
 
 TESTDIR=$TEST_DIR/ftrunc
 TESTFILE=$TESTDIR/ftrunc.tmp
diff --git a/tests/generic/214 b/tests/generic/214
index dff267e..7838047 100755
--- a/tests/generic/214
+++ b/tests/generic/214
@@ -55,6 +55,7 @@ rm -f $seqres.full
 rm -f $TEST_DIR/ouch*
 
 _require_xfs_io_command "falloc"
+_require_odirect
 
 # Ok, off we go.
 
-- 
2.5.0

Re: [PATCH 04/12] generic: add _require_odirect to three more tests

[Eryu Guan]

On Tue, Feb 09, 2016 at 08:49:53PM -0500, Theodore Ts'o wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> generic/113, generic/125, generic/214 each use O_DIRECT at some
> stage in their tests, so check O_DIRECT support before running them.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  tests/generic/113 | 1 +
>  tests/generic/125 | 1 +
>  tests/generic/214 | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/tests/generic/113 b/tests/generic/113
> index 7208fa2..54d6191 100755
> --- a/tests/generic/113
> +++ b/tests/generic/113
> @@ -77,6 +77,7 @@ _do_test()
>  _supported_fs generic
>  _supported_os Linux
>  _require_test
> +_require_odirect
>  
>  [ -x $here/ltp/aio-stress ] || _notrun "aio-stress not built for this platform"
>  
> diff --git a/tests/generic/125 b/tests/generic/125
> index bcf9b3e..67eb63f 100755
> --- a/tests/generic/125
> +++ b/tests/generic/125
> @@ -41,6 +41,7 @@ _supported_os Linux
>  
>  _require_test
>  _require_user
> +_require_odirect

generic/125 only runs "./src/ftrunc" and I don't see where O_DIRECT is
being used, did I miss anything?

Thanks,
Eryu

>  
>  TESTDIR=$TEST_DIR/ftrunc
>  TESTFILE=$TESTDIR/ftrunc.tmp
> diff --git a/tests/generic/214 b/tests/generic/214
> index dff267e..7838047 100755
> --- a/tests/generic/214
> +++ b/tests/generic/214
> @@ -55,6 +55,7 @@ rm -f $seqres.full
>  rm -f $TEST_DIR/ouch*
>  
>  _require_xfs_io_command "falloc"
> +_require_odirect
>  
>  # Ok, off we go.
>  
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/12] generic: add _require_odirect to three more tests

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 05:15:13PM +0800, Eryu Guan wrote:
> 
> generic/125 only runs "./src/ftrunc" and I don't see where O_DIRECT is
> being used, did I miss anything?

generic/125 also calls "src/trunc", which does use O_DIRECT.

(Hmm, and it assumes '.' is in the test's path, which I don't think is
a safe thing to do.)

					- Ted

Re: [PATCH 04/12] generic: add _require_odirect to three more tests

[Dave Chinner]

On Wed, Feb 10, 2016 at 11:11:35AM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 05:15:13PM +0800, Eryu Guan wrote:
> > 
> > generic/125 only runs "./src/ftrunc" and I don't see where O_DIRECT is
> > being used, did I miss anything?
> 
> generic/125 also calls "src/trunc", which does use O_DIRECT.
> 
> (Hmm, and it assumes '.' is in the test's path, which I don't think is
> a safe thing to do.)

Perhaps one should question whether generic/125 is actually testing
anything useful in the first place.

i.e. it's writing 16k, then truncating it back to 1000 bytes, then
reading it with direct IO for 60s to see if it returns the correct
data for that entire time.

What, exactly, is going to cause that test to fail?

Keep in mind this was ported from a CXFS test, where the metadata
server did the truncation operation (including the data zeroing),
but the reads are being done from the client.  i.e. it was designed
to test whether a remote machine is doing a truncate correctly on a
access shared disk.

IMO, I think we just remove the test, src/trunc.c and src/ftrunc.c
because it's 60s of testing that doesn't actually provide any value
to us.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 04/12] generic: add _require_odirect to three more tests

[Theodore Ts'o]

On Thu, Feb 11, 2016 at 09:51:23AM +1100, Dave Chinner wrote:
> Keep in mind this was ported from a CXFS test, where the metadata
> server did the truncation operation (including the data zeroing),
> but the reads are being done from the client.  i.e. it was designed
> to test whether a remote machine is doing a truncate correctly on a
> access shared disk.

Maybe the Ceph folks would care?  But I'm happy to replace this with a
test that just nukes generic/125.

							- Ted

[PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Theodore Ts'o]

From: Junho Ryu <jayr@google.com>

Several tests unmount then re-mount the scratch filesystem, to check
that the content is unchanged; but unmounting a tmpfs is designed to
lose its content, which causes such tests to fail unnecessarily.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Junho Ryu <jayr@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/rc             | 30 ++++++++++++++++++++++++++----
 tests/generic/003     | 12 ++++--------
 tests/generic/135     | 17 +++--------------
 tests/generic/169     | 20 ++++++--------------
 tests/generic/169.out |  6 ++----
 tests/generic/192     |  3 +--
 tests/generic/226     |  3 +--
 tests/generic/258     |  3 +--
 tests/generic/306     |  3 +--
 tests/generic/317     |  3 +--
 tests/generic/318     |  3 +--
 11 files changed, 47 insertions(+), 56 deletions(-)

diff --git a/common/rc b/common/rc
index aca723f..2508fb5 100644
--- a/common/rc
+++ b/common/rc
@@ -331,8 +331,19 @@ _scratch_unmount()
 
 _scratch_remount()
 {
-    _scratch_unmount
-    _scratch_mount
+    case $FSTYP in
+    tmpfs)
+        OPTS="$@"
+	if test -n "$OPTS"; then
+	   OPTS=$(echo $OPTS | sed -e 's/-o /-o remount,/')
+	   mount $OPTS $SCRATCH_MNT
+	fi
+        ;;
+    *)
+        _scratch_unmount
+        _scratch_mount "$@"
+        ;;
+    esac
 }
 
 _test_mount()
@@ -356,8 +367,19 @@ _test_unmount()
 
 _test_remount()
 {
-    _test_unmount
-    _test_mount
+    case $FSTYP in
+    tmpfs)
+        OPTS="$@"
+	if test -n "$OPTS"; then
+	   OPTS=$(echo $OPTS | sed -e 's/-o /-o remount,/')
+	   mount $OPTS $SCRATCH_DIR
+	fi
+        ;;
+    *)
+        _test_unmount
+        _test_mount "$@"
+       ;;
+    esac
 }
 
 _scratch_mkfs_options()
diff --git a/tests/generic/003 b/tests/generic/003
index 7ffd09a..2ccaf2c 100755
--- a/tests/generic/003
+++ b/tests/generic/003
@@ -108,8 +108,7 @@ _compare_stat_times NNN "$file1_stat_after_first_access" \
 	"$file1_stat_after_second_access" "after accessing file1 second time"
 
 # Remounting with nodiratime option
-_scratch_unmount
-_scratch_mount "-o nodiratime"
+_scratch_remount "-o nodiratime"
 file1_stat_after_remount=`_stat $TPATH/dir1/file1`
 _compare_stat_times NNN "$file1_stat_after_second_access" \
 	"$file1_stat_after_remount" "for file1 after remount"
@@ -135,8 +134,7 @@ _compare_stat_times NNN "$dir2_stat_after_file_creation" \
 	"$dir2_stat_after_file_access" "for dir2 after file access"
 
 # Remounting with noatime option, creating a file and accessing it
-_scratch_unmount
-_scratch_mount "-o noatime"
+_scratch_remount "-o noatime"
 echo "ccc" > $TPATH/dir2/file3
 file3_stat_before_first_access=`_stat $TPATH/dir2/file3`
 sleep 1
@@ -160,8 +158,7 @@ _compare_stat_times NNY "$file1_stat_after_modify" \
 
 # Remounting with strictatime option and
 # accessing a previously created file twice
-_scratch_unmount
-_scratch_mount "-o strictatime"
+_scratch_remount "-o strictatime"
 cat $TPATH/dir2/file3 > /dev/null
 file3_stat_after_second_access=`_stat $TPATH/dir2/file3`
 _compare_stat_times YNN "$file3_stat_after_first_access" \
@@ -194,8 +191,7 @@ fi
 sleep 1
 dir2_stat_before_ro_mount=`_stat $TPATH/dir2`
 file3_stat_before_ro_mount=`_stat $TPATH/dir2/file3`
-_scratch_unmount
-_scratch_mount "-o ro,strictatime"
+_scratch_remount "-o ro,strictatime"
 ls $TPATH/dir2 > /dev/null
 cat $TPATH/dir2/file3 > /dev/null
 dir2_stat_after_ro_mount=`_stat $TPATH/dir2`
diff --git a/tests/generic/135 b/tests/generic/135
index 4400672..b250587 100755
--- a/tests/generic/135
+++ b/tests/generic/135
@@ -41,19 +41,8 @@ _supported_os Linux IRIX
 
 _require_odirect
 _require_scratch
-_scratch_mkfs >/dev/null 2>&1
-
-_umount_mount()
-{
-    CWD=`pwd`
-    cd /
-    # pipe error into /dev/null, in case not mounted (after _require_scratch)
-    _scratch_unmount 2>/dev/null
-    _scratch_mount
-    cd "$CWD"
-}
-
-_umount_mount
+_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount > /dev/null 2>&1 || _fail "mount failed"
 
 cd $SCRATCH_MNT
 
@@ -71,7 +60,7 @@ $XFS_IO_PROG -f -c 'pwrite -b 4k -S 0x78 0 4k' trunc_file > /dev/null
 $XFS_IO_PROG -f -c 'truncate 2k' trunc_file > /dev/null
 $XFS_IO_PROG -c 'pwrite 1k 0 1k' trunc_file > /dev/null
 
-_umount_mount
+_scratch_remount
 
 # check file size and contents
 od -Ad -x async_file
diff --git a/tests/generic/169 b/tests/generic/169
index 839ff9d..ebfb106 100755
--- a/tests/generic/169
+++ b/tests/generic/169
@@ -73,13 +73,9 @@ $XFS_IO_PROG -a -c "pwrite 0 5k" -c "fsync" \
 	$SCRATCH_MNT/testfile \
 	| _show_wrote_and_stat_only
 
-echo "# unmounting scratch"
-_scratch_unmount>>$seqres.full 2>&1 \
-    || _fail "unmount failed"
-
-echo "# mounting scratch"
-_scratch_mount >>$seqres.full 2>&1 \
-    || _fail "mount failed: $MOUNT_OPTIONS"
+echo "# remounting scratch"
+_scratch_remount >>$seqres.full 2>&1 \
+    || _fail "remount failed: $MOUNT_OPTIONS"
 
 echo "# stating file to confirm correct size"
 $XFS_IO_PROG -r -c "stat" $SCRATCH_MNT/testfile \
@@ -90,13 +86,9 @@ $XFS_IO_PROG -f -c "pwrite 0 5" -c s -c "pwrite 5 5" \
 	-c "stat" $SCRATCH_MNT/nextfile \
 	| _show_wrote_and_stat_only
 
-echo "# unmounting scratch"
-_scratch_unmount>>$seqres.full 2>&1 \
-    || _fail "unmount failed"
-
-echo "# mounting scratch"
-_scratch_mount >>$seqres.full 2>&1 \
-    || _fail "mount failed: $MOUNT_OPTIONS"
+echo "# remounting scratch"
+_scratch_remount >>$seqres.full 2>&1 \
+    || _fail "remount failed: $MOUNT_OPTIONS"
 
 echo "# stating file to confirm correct size"
 $XFS_IO_PROG -r -c "stat" $SCRATCH_MNT/nextfile \
diff --git a/tests/generic/169.out b/tests/generic/169.out
index 22a5b77..5f7df39 100644
--- a/tests/generic/169.out
+++ b/tests/generic/169.out
@@ -5,15 +5,13 @@ wrote 5120/5120 bytes at offset 0
 wrote 5120/5120 bytes at offset 5120
 wrote 5120/5120 bytes at offset 10240
 stat.size = 15360
-# unmounting scratch
-# mounting scratch
+# remounting scratch
 # stating file to confirm correct size
 stat.size = 15360
 # appending 10 bytes to new file, sync at 5 bytes
 wrote 5/5 bytes at offset 0
 wrote 5/5 bytes at offset 5
 stat.size = 10
-# unmounting scratch
-# mounting scratch
+# remounting scratch
 # stating file to confirm correct size
 stat.size = 10
diff --git a/tests/generic/192 b/tests/generic/192
index ebabea2..c64b954 100755
--- a/tests/generic/192
+++ b/tests/generic/192
@@ -78,8 +78,7 @@ cat $testfile
 time2=`_access_time $testfile | tee -a $seqres.full`
 
 cd /
-_test_unmount
-_test_mount
+_test_remount
 time3=`_access_time $testfile | tee -a $seqres.full`
 
 delta1=`expr $time2 - $time1`
diff --git a/tests/generic/226 b/tests/generic/226
index b12965a..253e82c 100755
--- a/tests/generic/226
+++ b/tests/generic/226
@@ -61,8 +61,7 @@ for I in `seq 1 $loops`; do
 done
 
 echo
-_scratch_unmount
-_scratch_mount
+_scratch_remount
 
 echo "--> $loops direct 64m writes in a loop"
 for I in `seq 1 $loops`; do
diff --git a/tests/generic/258 b/tests/generic/258
index 285a422..dd5c970 100755
--- a/tests/generic/258
+++ b/tests/generic/258
@@ -62,8 +62,7 @@ fi
 
 # unmount, remount, and check the timestamp
 echo "Remounting to flush cache"
-_test_unmount
-_test_mount
+_test_remount
 
 # Should yield -315593940 (prior to epoch)
 echo "Testing for negative seconds since epoch"
diff --git a/tests/generic/306 b/tests/generic/306
index 64d8cde..3660926 100755
--- a/tests/generic/306
+++ b/tests/generic/306
@@ -67,8 +67,7 @@ touch $BINDFILE || _fail "Could not create bind mount file"
 touch $TARGET || _fail "Could not create symlink target"
 ln -s $TARGET $SYMLINK
 
-_scratch_unmount || _fail "Could not unmount scratch device"
-_scratch_mount -o ro || _fail "Could not mount scratch readonly"
+_scratch_remount -o ro || _fail "Could not remount scratch readonly"
 
 # We should be able to read & write to/from these devices even on an RO fs
 echo "== try to create new file"
diff --git a/tests/generic/317 b/tests/generic/317
index 68f231c..0aaf10e 100755
--- a/tests/generic/317
+++ b/tests/generic/317
@@ -96,8 +96,7 @@ echo ""
 echo "*** Remounting ***"
 echo ""
 sync
-_scratch_unmount >>$seqres.full 2>&1
-_scratch_mount      >>$seqres.full 2>&1 || _fail "mount failed"
+_scratch_remount      >>$seqres.full 2>&1 || _fail "remount failed"
 
 _print_numeric_uid
 
diff --git a/tests/generic/318 b/tests/generic/318
index c730b50..2d39b21 100755
--- a/tests/generic/318
+++ b/tests/generic/318
@@ -109,8 +109,7 @@ _print_getfacls
 echo "*** Remounting ***"
 echo ""
 sync
-_scratch_unmount >>$seqres.full 2>&1
-_scratch_mount      >>$seqres.full 2>&1 || _fail "mount failed"
+_scratch_remount      >>$seqres.full 2>&1 || _fail "remount failed"
 
 _print_getfacls
 
-- 
2.5.0

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Dave Chinner]

On Tue, Feb 09, 2016 at 08:49:54PM -0500, Theodore Ts'o wrote:
> From: Junho Ryu <jayr@google.com>
> 
> Several tests unmount then re-mount the scratch filesystem, to check
> that the content is unchanged; but unmounting a tmpfs is designed to
> lose its content, which causes such tests to fail unnecessarily.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Junho Ryu <jayr@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
....
>  _scratch_remount()
>  {
> -    _scratch_unmount
> -    _scratch_mount
> +    case $FSTYP in
> +    tmpfs)
> +        OPTS="$@"
> +	if test -n "$OPTS"; then
> +	   OPTS=$(echo $OPTS | sed -e 's/-o /-o remount,/')
> +	   mount $OPTS $SCRATCH_MNT
> +	fi
> +        ;;
> +    *)
> +        _scratch_unmount
> +        _scratch_mount "$@"
> +        ;;
> +    esac
>  }

If really don't like the different definitions of "remount" being
used here, especially now that new parameters are being passed in.
i.e.

> --- a/tests/generic/003
> +++ b/tests/generic/003
> @@ -108,8 +108,7 @@ _compare_stat_times NNN "$file1_stat_after_first_access" \
>  	"$file1_stat_after_second_access" "after accessing file1 second time"
>  
>  # Remounting with nodiratime option
> -_scratch_unmount
> -_scratch_mount "-o nodiratime"
> +_scratch_remount "-o nodiratime"

This makes me go "no, that can't work, nodiratime is not an option
that we allow on remount."

So, at minimum, the name of the helper needs to get changed so that
it doesn't imply that a "-o remount" with new options is being
done...

>  _require_scratch
> -_scratch_mkfs >/dev/null 2>&1
> -
> -_umount_mount()
> -{
> -    CWD=`pwd`
> -    cd /
> -    # pipe error into /dev/null, in case not mounted (after _require_scratch)
> -    _scratch_unmount 2>/dev/null
> -    _scratch_mount
> -    cd "$CWD"
> -}
> -
> -_umount_mount
> +_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
> +_scratch_mount > /dev/null 2>&1 || _fail "mount failed"

Please don't add _fail to mkfs/mount like this, especially where the
test doesn't already have them.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 05:07:16PM +1100, Dave Chinner wrote:
> >  # Remounting with nodiratime option
> > -_scratch_unmount
> > -_scratch_mount "-o nodiratime"
> > +_scratch_remount "-o nodiratime"
> 
> This makes me go "no, that can't work, nodiratime is not an option
> that we allow on remount."

Hmm, yes, we're not consistent here.  xfs doesn't allow nodiratime on
remounts.  ext4 allows nodiratime on remount, but not diratime, so
there's no way to clear nodiratime once its set.  tmpfs allows
diratime and nodiratime on remount.

I wonder if it's worth it make things more consistent across the
various file systems.  What do you think?

> So, at minimum, the name of the helper needs to get changed so that
> it doesn't imply that a "-o remount" with new options is being
> done...

Hmm, how about having two helper functions: _scratch_remount that
doesn't take any arguments, and a _scratch_change_mount_opts which
does?

> > -_umount_mount
> > +_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
> > +_scratch_mount > /dev/null 2>&1 || _fail "mount failed"
> 
> Please don't add _fail to mkfs/mount like this, especially where the
> test doesn't already have them.

Could you say more about why?  We do have tests that do use _fail if
the mkfs or mount fails.  Should we be changing them to remove it?
But if we do that, and the mount fails for some reason, then won't
things stagger on and perhaps make life harder to debug.

Cheers,

						- Ted

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Theodore Ts'o]

How about this?

				- Ted

>From e03fcd72962fc29e198173662635d4fa10779128 Mon Sep 17 00:00:00 2001
From: Junho Ryu <jayr@google.com>
Date: Wed, 18 Dec 2013 10:16:01 -0800
Subject: [PATCH] xfstests: do not unmount tmpfs during remount

Several tests unmount then re-mount the scratch filesystem, to check
that the content is unchanged; but unmounting a tmpfs is designed to
lose its content, which causes such tests to fail unnecessarily.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Junho Ryu <jayr@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/rc             | 23 +++++++++++++++++++++++
 tests/generic/003     | 12 ++++--------
 tests/generic/135     | 17 +++--------------
 tests/generic/169     | 20 ++++++--------------
 tests/generic/169.out |  6 ++----
 tests/generic/192     |  3 +--
 tests/generic/226     |  3 +--
 tests/generic/258     |  3 +--
 tests/generic/306     |  3 +--
 tests/generic/317     |  3 +--
 tests/generic/318     |  3 +--
 11 files changed, 44 insertions(+), 52 deletions(-)

diff --git a/common/rc b/common/rc
index aca723f..69c9586 100644
--- a/common/rc
+++ b/common/rc
@@ -331,10 +331,30 @@ _scratch_unmount()
 
 _scratch_remount()
 {
+    if [ "$FSTYP" = tmpfs ]; then
+	return
+    fi
     _scratch_unmount
     _scratch_mount
 }
 
+_scratch_change_mount_opts()
+{
+    case $FSTYP in
+    tmpfs)
+        OPTS="$@"
+	if test -n "$OPTS"; then
+	   OPTS=$(echo $OPTS | sed -e 's/-o /-o remount,/')
+	   mount $OPTS $SCRATCH_MNT
+	fi
+        ;;
+    *)
+        _scratch_unmount
+        _scratch_mount "$@"
+        ;;
+    esac
+}
+
 _test_mount()
 {
     if [ "$FSTYP" == "overlay" ]; then
@@ -356,6 +376,9 @@ _test_unmount()
 
 _test_remount()
 {
+    if [ "$FSTYP" = tmpfs ]; then
+	return
+    fi
     _test_unmount
     _test_mount
 }
diff --git a/tests/generic/003 b/tests/generic/003
index 7ffd09a..e6cb3c4 100755
--- a/tests/generic/003
+++ b/tests/generic/003
@@ -108,8 +108,7 @@ _compare_stat_times NNN "$file1_stat_after_first_access" \
 	"$file1_stat_after_second_access" "after accessing file1 second time"
 
 # Remounting with nodiratime option
-_scratch_unmount
-_scratch_mount "-o nodiratime"
+_scratch_change_mount_opts "-o nodiratime"
 file1_stat_after_remount=`_stat $TPATH/dir1/file1`
 _compare_stat_times NNN "$file1_stat_after_second_access" \
 	"$file1_stat_after_remount" "for file1 after remount"
@@ -135,8 +134,7 @@ _compare_stat_times NNN "$dir2_stat_after_file_creation" \
 	"$dir2_stat_after_file_access" "for dir2 after file access"
 
 # Remounting with noatime option, creating a file and accessing it
-_scratch_unmount
-_scratch_mount "-o noatime"
+_scratch_change_mount_opts "-o noatime"
 echo "ccc" > $TPATH/dir2/file3
 file3_stat_before_first_access=`_stat $TPATH/dir2/file3`
 sleep 1
@@ -160,8 +158,7 @@ _compare_stat_times NNY "$file1_stat_after_modify" \
 
 # Remounting with strictatime option and
 # accessing a previously created file twice
-_scratch_unmount
-_scratch_mount "-o strictatime"
+_scratch_change_mount_opts "-o strictatime"
 cat $TPATH/dir2/file3 > /dev/null
 file3_stat_after_second_access=`_stat $TPATH/dir2/file3`
 _compare_stat_times YNN "$file3_stat_after_first_access" \
@@ -194,8 +191,7 @@ fi
 sleep 1
 dir2_stat_before_ro_mount=`_stat $TPATH/dir2`
 file3_stat_before_ro_mount=`_stat $TPATH/dir2/file3`
-_scratch_unmount
-_scratch_mount "-o ro,strictatime"
+_scratch_change_mount_opts "-o ro,strictatime"
 ls $TPATH/dir2 > /dev/null
 cat $TPATH/dir2/file3 > /dev/null
 dir2_stat_after_ro_mount=`_stat $TPATH/dir2`
diff --git a/tests/generic/135 b/tests/generic/135
index 4400672..9872c7b 100755
--- a/tests/generic/135
+++ b/tests/generic/135
@@ -41,19 +41,8 @@ _supported_os Linux IRIX
 
 _require_odirect
 _require_scratch
-_scratch_mkfs >/dev/null 2>&1
-
-_umount_mount()
-{
-    CWD=`pwd`
-    cd /
-    # pipe error into /dev/null, in case not mounted (after _require_scratch)
-    _scratch_unmount 2>/dev/null
-    _scratch_mount
-    cd "$CWD"
-}
-
-_umount_mount
+_scratch_mkfs >/dev/null
+_scratch_mount
 
 cd $SCRATCH_MNT
 
@@ -71,7 +60,7 @@ $XFS_IO_PROG -f -c 'pwrite -b 4k -S 0x78 0 4k' trunc_file > /dev/null
 $XFS_IO_PROG -f -c 'truncate 2k' trunc_file > /dev/null
 $XFS_IO_PROG -c 'pwrite 1k 0 1k' trunc_file > /dev/null
 
-_umount_mount
+_scratch_remount
 
 # check file size and contents
 od -Ad -x async_file
diff --git a/tests/generic/169 b/tests/generic/169
index 839ff9d..ebfb106 100755
--- a/tests/generic/169
+++ b/tests/generic/169
@@ -73,13 +73,9 @@ $XFS_IO_PROG -a -c "pwrite 0 5k" -c "fsync" \
 	$SCRATCH_MNT/testfile \
 	| _show_wrote_and_stat_only
 
-echo "# unmounting scratch"
-_scratch_unmount>>$seqres.full 2>&1 \
-    || _fail "unmount failed"
-
-echo "# mounting scratch"
-_scratch_mount >>$seqres.full 2>&1 \
-    || _fail "mount failed: $MOUNT_OPTIONS"
+echo "# remounting scratch"
+_scratch_remount >>$seqres.full 2>&1 \
+    || _fail "remount failed: $MOUNT_OPTIONS"
 
 echo "# stating file to confirm correct size"
 $XFS_IO_PROG -r -c "stat" $SCRATCH_MNT/testfile \
@@ -90,13 +86,9 @@ $XFS_IO_PROG -f -c "pwrite 0 5" -c s -c "pwrite 5 5" \
 	-c "stat" $SCRATCH_MNT/nextfile \
 	| _show_wrote_and_stat_only
 
-echo "# unmounting scratch"
-_scratch_unmount>>$seqres.full 2>&1 \
-    || _fail "unmount failed"
-
-echo "# mounting scratch"
-_scratch_mount >>$seqres.full 2>&1 \
-    || _fail "mount failed: $MOUNT_OPTIONS"
+echo "# remounting scratch"
+_scratch_remount >>$seqres.full 2>&1 \
+    || _fail "remount failed: $MOUNT_OPTIONS"
 
 echo "# stating file to confirm correct size"
 $XFS_IO_PROG -r -c "stat" $SCRATCH_MNT/nextfile \
diff --git a/tests/generic/169.out b/tests/generic/169.out
index 22a5b77..5f7df39 100644
--- a/tests/generic/169.out
+++ b/tests/generic/169.out
@@ -5,15 +5,13 @@ wrote 5120/5120 bytes at offset 0
 wrote 5120/5120 bytes at offset 5120
 wrote 5120/5120 bytes at offset 10240
 stat.size = 15360
-# unmounting scratch
-# mounting scratch
+# remounting scratch
 # stating file to confirm correct size
 stat.size = 15360
 # appending 10 bytes to new file, sync at 5 bytes
 wrote 5/5 bytes at offset 0
 wrote 5/5 bytes at offset 5
 stat.size = 10
-# unmounting scratch
-# mounting scratch
+# remounting scratch
 # stating file to confirm correct size
 stat.size = 10
diff --git a/tests/generic/192 b/tests/generic/192
index ebabea2..c64b954 100755
--- a/tests/generic/192
+++ b/tests/generic/192
@@ -78,8 +78,7 @@ cat $testfile
 time2=`_access_time $testfile | tee -a $seqres.full`
 
 cd /
-_test_unmount
-_test_mount
+_test_remount
 time3=`_access_time $testfile | tee -a $seqres.full`
 
 delta1=`expr $time2 - $time1`
diff --git a/tests/generic/226 b/tests/generic/226
index b12965a..253e82c 100755
--- a/tests/generic/226
+++ b/tests/generic/226
@@ -61,8 +61,7 @@ for I in `seq 1 $loops`; do
 done
 
 echo
-_scratch_unmount
-_scratch_mount
+_scratch_remount
 
 echo "--> $loops direct 64m writes in a loop"
 for I in `seq 1 $loops`; do
diff --git a/tests/generic/258 b/tests/generic/258
index 285a422..dd5c970 100755
--- a/tests/generic/258
+++ b/tests/generic/258
@@ -62,8 +62,7 @@ fi
 
 # unmount, remount, and check the timestamp
 echo "Remounting to flush cache"
-_test_unmount
-_test_mount
+_test_remount
 
 # Should yield -315593940 (prior to epoch)
 echo "Testing for negative seconds since epoch"
diff --git a/tests/generic/306 b/tests/generic/306
index 64d8cde..725520b 100755
--- a/tests/generic/306
+++ b/tests/generic/306
@@ -67,8 +67,7 @@ touch $BINDFILE || _fail "Could not create bind mount file"
 touch $TARGET || _fail "Could not create symlink target"
 ln -s $TARGET $SYMLINK
 
-_scratch_unmount || _fail "Could not unmount scratch device"
-_scratch_mount -o ro || _fail "Could not mount scratch readonly"
+_scratch_change_mount_opts -o ro || _fail "Could not remount scratch readonly"
 
 # We should be able to read & write to/from these devices even on an RO fs
 echo "== try to create new file"
diff --git a/tests/generic/317 b/tests/generic/317
index 68f231c..0aaf10e 100755
--- a/tests/generic/317
+++ b/tests/generic/317
@@ -96,8 +96,7 @@ echo ""
 echo "*** Remounting ***"
 echo ""
 sync
-_scratch_unmount >>$seqres.full 2>&1
-_scratch_mount      >>$seqres.full 2>&1 || _fail "mount failed"
+_scratch_remount      >>$seqres.full 2>&1 || _fail "remount failed"
 
 _print_numeric_uid
 
diff --git a/tests/generic/318 b/tests/generic/318
index c730b50..2d39b21 100755
--- a/tests/generic/318
+++ b/tests/generic/318
@@ -109,8 +109,7 @@ _print_getfacls
 echo "*** Remounting ***"
 echo ""
 sync
-_scratch_unmount >>$seqres.full 2>&1
-_scratch_mount      >>$seqres.full 2>&1 || _fail "mount failed"
+_scratch_remount      >>$seqres.full 2>&1 || _fail "remount failed"
 
 _print_getfacls
 
-- 
2.5.0

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Dave Chinner]

On Wed, Feb 10, 2016 at 11:07:32AM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 05:07:16PM +1100, Dave Chinner wrote:
> > >  # Remounting with nodiratime option
> > > -_scratch_unmount
> > > -_scratch_mount "-o nodiratime"
> > > +_scratch_remount "-o nodiratime"
> > 
> > This makes me go "no, that can't work, nodiratime is not an option
> > that we allow on remount."
> 
> Hmm, yes, we're not consistent here.  xfs doesn't allow nodiratime on
> remounts.  ext4 allows nodiratime on remount, but not diratime, so
> there's no way to clear nodiratime once its set.  tmpfs allows
> diratime and nodiratime on remount.
> 
> I wonder if it's worth it make things more consistent across the
> various file systems.  What do you think?

Different problem, care factor approaching zero right now. :/

Talk to Eric - he's futzing with this stuff on XFS right now.

> > So, at minimum, the name of the helper needs to get changed so that
> > it doesn't imply that a "-o remount" with new options is being
> > done...
> 
> Hmm, how about having two helper functions: _scratch_remount that
> doesn't take any arguments, and a _scratch_change_mount_opts which
> does?

No, it's not really the options that are the problem here. The
problem is -o remount vs unmount/mount and what the test is actually
expecting.

I'd say "_scratch_remount" should do "-o remount" unconditionally
(least surprise) and the current _scratch_remount should be changed
to something like _scratch_cycle_mount(). That way both can take
options, but it's clear they do different things. tmpfs can simply
implement them the same way.

> > > -_umount_mount
> > > +_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
> > > +_scratch_mount > /dev/null 2>&1 || _fail "mount failed"
> > 
> > Please don't add _fail to mkfs/mount like this, especially where the
> > test doesn't already have them.
> 
> Could you say more about why?  We do have tests that do use _fail if
> the mkfs or mount fails.  Should we be changing them to remove it?

Because, in general, scratch_mkfs should not ever fail, and nor
should a mounting the scratch device. If they do fail, something has
already gone wrong...

> But if we do that, and the mount fails for some reason, then won't
> things stagger on and perhaps make life harder to debug.

Yes, but we don't care that they make lots of noise or that they
stagger on, because that staggering on can exercise failure and
other unexpected paths that wouldn't otherwise get exercised. I've
lost count of the number of times that I've had a stuck process
prevent an unmount and a subsequent test has tripped over and
uncovered a previously unknown bug because we allowed mkfs and mount
to fail....

As for debugging - you always have to go back to the first failure
you see in xfstests and work from there, so the subsequent noise of
other tests failing can simply be ignored. Yes, it can make it a
little harder to identify the first failure, but it only takes a few
seconds looking at log files to identify that these functions have
failed...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Theodore Ts'o]

On Thu, Feb 11, 2016 at 10:07:00AM +1100, Dave Chinner wrote:
> No, it's not really the options that are the problem here. The
> problem is -o remount vs unmount/mount and what the test is actually
> expecting.
> 
> I'd say "_scratch_remount" should do "-o remount" unconditionally
> (least surprise) and the current _scratch_remount should be changed
> to something like _scratch_cycle_mount(). That way both can take
> options, but it's clear they do different things. tmpfs can simply
> implement them the same way.

Well, I can do that, but it's going to be a huge patch --- the vast
majority of the calls to _scratch_remount in the tree (over 100) would
have to be changed to _scratch_cycle_mount, because they are just
doing a _scratch_umount / _scratch_mount without taking any arguments
to change the mount option.

It is this patch that adds the calls that is using -o remount to
change mount options for generic/003 and generic/306 and tmpfs.

This is why I suggested adding _scratch_change_mount_opts because it
changes the smallest number of things, and keeps _scratch_mount to
have the same semantic value.

But if you want me to sweep through the entire tree changing
_scratch_remount to _scratch_cycle_mount, I suppose I can do that.
It's not going to be a small patch, though......

						- Ted

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Dave Chinner]

On Wed, Feb 10, 2016 at 06:28:26PM -0500, Theodore Ts'o wrote:
> On Thu, Feb 11, 2016 at 10:07:00AM +1100, Dave Chinner wrote:
> > No, it's not really the options that are the problem here. The
> > problem is -o remount vs unmount/mount and what the test is actually
> > expecting.
> > 
> > I'd say "_scratch_remount" should do "-o remount" unconditionally
> > (least surprise) and the current _scratch_remount should be changed
> > to something like _scratch_cycle_mount(). That way both can take
> > options, but it's clear they do different things. tmpfs can simply
> > implement them the same way.
> 
> Well, I can do that, but it's going to be a huge patch --- the vast
> majority of the calls to _scratch_remount in the tree (over 100) would
> have to be changed to _scratch_cycle_mount, because they are just
> doing a _scratch_umount / _scratch_mount without taking any arguments
> to change the mount option.

Yup, but we do this sort of tree-wide cleanup fairly often if it
makes sense. In this case, it's just an initial patch taht
does sed -i -e 's/_scratch_remount/_scratch_cycle_mount/' ....

And, let's put things in context: changing 108 lines of code is a
pretty damn small patch in the greater scheme of things. Indeed,
it's smaller than most patches that add a new regression test.

A "huge" patch is something like the series Darrick posted earlier
in the week - something like 20 patches, including somewhere in the
order of 30 new tests, a couple of new binary test programs, a heap
of cleanups across all 80-90 existing reflink/dedupe tests, and a
bunch of bug fixes to go with them.

IOWs, s/_scratch_remount/_scratch_cycle_mount/ is the sort of
no-brainer change that takes less time to write, test and review
than it did for me to write this email....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Theodore Ts'o]

On Thu, Feb 11, 2016 at 02:07:51PM +1100, Dave Chinner wrote:
> 
> IOWs, s/_scratch_remount/_scratch_cycle_mount/ is the sort of
> no-brainer change that takes less time to write, test and review
> than it did for me to write this email....

Fair enough, I'll make that change in the next rev of the patches.
The current set should reflect all of the other review comments,
though.

						- Ted

Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount

[Darrick J. Wong]

On Thu, Feb 11, 2016 at 10:25:08AM -0500, Theodore Ts'o wrote:
> On Thu, Feb 11, 2016 at 02:07:51PM +1100, Dave Chinner wrote:
> > 
> > IOWs, s/_scratch_remount/_scratch_cycle_mount/ is the sort of
> > no-brainer change that takes less time to write, test and review
> > than it did for me to write this email....
> 
> Fair enough, I'll make that change in the next rev of the patches.
> The current set should reflect all of the other review comments,
> though.

You might wait for the latest batch of reflink tests (the huge batch Dave
referred to earlier in this thread) to land, as I use _scratch_remount in most
of them.

--D

> 
> 						- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/12] generic: do not unmount before calling _check_scratch_fs()

[Theodore Ts'o]

From: Junho Ryu <jayr@google.com>

Fix generic/053 so it works on tmpfs by relying on _check_scratch_fs
to unmount before checking the file system and remounting it
afterwards.  Many other tests rely on this, and since tmpfs does not
have a file system consistency checker, this allows the test to
succeed because the files don't disappear when the tmpfs file system
is unmounted.

Signed-off-by: Junho Ryu <jayr@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/053     | 2 --
 tests/generic/053.out | 2 --
 2 files changed, 4 deletions(-)

diff --git a/tests/generic/053 b/tests/generic/053
index a0e7280..cf46a93 100755
--- a/tests/generic/053
+++ b/tests/generic/053
@@ -81,9 +81,7 @@ list_acls()
 
 echo "acls before repair:"
 list_acls
-_do 'unmount $SCRATCH_DEV' '_scratch_unmount'
 _do 'repair filesystem' '_check_scratch_fs'
-_do 'mount filesytem' '_scratch_mount'
 echo "acls after repair: "
 list_acls
 
diff --git a/tests/generic/053.out b/tests/generic/053.out
index cd71cbb..d57857a 100644
--- a/tests/generic/053.out
+++ b/tests/generic/053.out
@@ -10,9 +10,7 @@ $SCRATCH_MNT/test.4 [u::---,g::r-x,o::rwx]
 $SCRATCH_MNT/test.5 [u::---,u:id2:r-x,g::---,m::rwx,o::---]
 $SCRATCH_MNT/test.6 [u::rwx,g::r-x,o::r--]
 $SCRATCH_MNT/test.7 [u::---,g::---,g:id2:r-x,m::-w-,o::---]
-unmount $SCRATCH_DEV... done
 repair filesystem... done
-mount filesytem... done
 acls after repair: 
 $SCRATCH_MNT/test.0 [u::r--,g::rwx,o::rw-]
 $SCRATCH_MNT/test.1 [u::r-x,g::---,o::---]
-- 
2.5.0

[PATCH 07/12] generic: require fiemap for generic/009

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

Require xfs_io commands fiemap and falloc as well as fzero: fzero
without falloc is unlikely, but tmpfs may later support fzero, though
probably never fiemap (and in v3.15 wrongly claimed to support fzero).

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/009 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/generic/009 b/tests/generic/009
index 7fbec34..5902afd 100755
--- a/tests/generic/009
+++ b/tests/generic/009
@@ -45,6 +45,8 @@ trap "_cleanup ; exit \$status" 0 1 2 3 15
 # real QA test starts here
 _supported_os Linux
 _require_xfs_io_command "fzero"
+_require_xfs_io_command "fiemap"
+_require_xfs_io_command "falloc"
 _require_test
 
 testfile=$TEST_DIR/009.$$
-- 
2.5.0

[PATCH 08/12] xfstests: fix generic/312 on tmpfs, ignore /proc/partitions

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

312 avoid reading /proc/partitions for tmpfs, just assume enough space.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/312 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tests/generic/312 b/tests/generic/312
index b570814..a826071 100755
--- a/tests/generic/312
+++ b/tests/generic/312
@@ -51,12 +51,13 @@ _require_scratch
 
 # 5G in byte
 fssize=$((2**30 * 5))
-required_blocks=$(($fssize / 1024))
-dev_blocks=$(grep -w $(_short_dev $SCRATCH_DEV) /proc/partitions | $AWK_PROG '{print $3}')
-if [ $required_blocks -gt $dev_blocks ];then
-	_notrun "this test requires \$SCRATCH_DEV has ${fssize}B space"
+if [ "$FSTYP" != "tmpfs" ]; then
+	required_blocks=$(($fssize / 1024))
+	dev_blocks=$(grep -w $(_short_dev $SCRATCH_DEV) /proc/partitions | $AWK_PROG '{print $3}')
+	if [ $required_blocks -gt $dev_blocks ];then
+	    _notrun "this test requires \$SCRATCH_DEV has ${fssize}B space"
+	fi
 fi
-
 rm -f $seqres.full
 _scratch_mkfs_sized $fssize >>$seqres.full 2>&1
 _scratch_mount >>$seqres.full 2>&1
-- 
2.5.0

Re: [PATCH 08/12] xfstests: fix generic/312 on tmpfs, ignore /proc/partitions

[Dave Chinner]

On Tue, Feb 09, 2016 at 08:49:57PM -0500, Theodore Ts'o wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> 312 avoid reading /proc/partitions for tmpfs, just assume enough space.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  tests/generic/312 | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/generic/312 b/tests/generic/312
> index b570814..a826071 100755
> --- a/tests/generic/312
> +++ b/tests/generic/312
> @@ -51,12 +51,13 @@ _require_scratch
>  
>  # 5G in byte
>  fssize=$((2**30 * 5))
> -required_blocks=$(($fssize / 1024))
> -dev_blocks=$(grep -w $(_short_dev $SCRATCH_DEV) /proc/partitions | $AWK_PROG '{print $3}')
> -if [ $required_blocks -gt $dev_blocks ];then
> -	_notrun "this test requires \$SCRATCH_DEV has ${fssize}B space"
> +if [ "$FSTYP" != "tmpfs" ]; then
> +	required_blocks=$(($fssize / 1024))
> +	dev_blocks=$(grep -w $(_short_dev $SCRATCH_DEV) /proc/partitions | $AWK_PROG '{print $3}')
> +	if [ $required_blocks -gt $dev_blocks ];then
> +	    _notrun "this test requires \$SCRATCH_DEV has ${fssize}B space"
> +	fi
>  fi

_scratch_mkfs
_scratch_mount
_require_fs_space $SCRATCH_MNT $((fssize / 1024))
_scratch_unmount
_scratch_mkfs_sized $fssize

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 08/12] xfstests: fix generic/312 on tmpfs, ignore /proc/partitions

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 04:54:55PM +1100, Dave Chinner wrote:
> 
> _scratch_mkfs
> _scratch_mount
> _require_fs_space $SCRATCH_MNT $((fssize / 1024))
> _scratch_unmount
> _scratch_mkfs_sized $fssize

This doesn't work because _require_fs_space uses df -klP, and the -l
file systems with names such as "test:/scratch" to be filtered out ---
and xfstests requires us to use NFS-looking names as a hint that the
file system has no block device.

This results in "_require_fs_space" failing on tmpfs file systems with
the error messages:

    df: no file systems processed
    ./common/rc: line 1860: [: -lt: unary operator expected

I'm not sure why the -l option is being used at all in the first
place.  Any objections if I remove it?  Presumably this is also
causing _require_fs_space to blow up on any tests run using NFS....

Alternatively I could just remove if it $FSTYP == tmpfs, but that
assumes there was some reason for using the -l option in the first
place.

			     	     	- Ted

Re: [PATCH 08/12] xfstests: fix generic/312 on tmpfs, ignore /proc/partitions

[Dave Chinner]

On Wed, Feb 10, 2016 at 06:39:11PM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 04:54:55PM +1100, Dave Chinner wrote:
> > 
> > _scratch_mkfs
> > _scratch_mount
> > _require_fs_space $SCRATCH_MNT $((fssize / 1024))
> > _scratch_unmount
> > _scratch_mkfs_sized $fssize
> 
> This doesn't work because _require_fs_space uses df -klP, and the -l
> file systems with names such as "test:/scratch" to be filtered out ---
> and xfstests requires us to use NFS-looking names as a hint that the
> file system has no block device.
> 
> This results in "_require_fs_space" failing on tmpfs file systems with
> the error messages:
> 
>     df: no file systems processed
>     ./common/rc: line 1860: [: -lt: unary operator expected
> 
> I'm not sure why the -l option is being used at all in the first
> place.  Any objections if I remove it?  Presumably this is also
> causing _require_fs_space to blow up on any tests run using NFS....

No idea on why -l is used. If there is a reason to be found, it
will be in the commit history.

> Alternatively I could just remove if it $FSTYP == tmpfs, but that
> assumes there was some reason for using the -l option in the first
> place.

Remove it, see what breaks/who complains.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 09/12] xfstests: generic/079 requires chattr, not xattrs

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/079 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/079 b/tests/generic/079
index 041d9c0..a2c3286 100755
--- a/tests/generic/079
+++ b/tests/generic/079
@@ -48,7 +48,7 @@ _cleanup()
 _supported_fs generic
 _supported_os Linux
 
-_require_attrs
+_require_chattr
 _require_scratch
 
 [ -x $timmutable ] || _notrun "t_immutable was not built for this platform"
-- 
2.5.0

Re: [PATCH 09/12] xfstests: generic/079 requires chattr, not xattrs

[Eryu Guan]

On Tue, Feb 09, 2016 at 08:49:58PM -0500, Theodore Ts'o wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  tests/generic/079 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/079 b/tests/generic/079
> index 041d9c0..a2c3286 100755
> --- a/tests/generic/079
> +++ b/tests/generic/079
> @@ -48,7 +48,7 @@ _cleanup()
>  _supported_fs generic
>  _supported_os Linux
>  
> -_require_attrs
> +_require_chattr

I don't see _require_chattr in any of the common files, did I miss
anything?

Thanks,
Eryu

>  _require_scratch
>  
>  [ -x $timmutable ] || _notrun "t_immutable was not built for this platform"
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/12] xfstests: generic/079 requires chattr, not xattrs

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 05:09:10PM +0800, Eryu Guan wrote:
> On Tue, Feb 09, 2016 at 08:49:58PM -0500, Theodore Ts'o wrote:
> > From: Hugh Dickins <hughd@google.com>
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  tests/generic/079 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/generic/079 b/tests/generic/079
> > index 041d9c0..a2c3286 100755
> > --- a/tests/generic/079
> > +++ b/tests/generic/079
> > @@ -48,7 +48,7 @@ _cleanup()
> >  _supported_fs generic
> >  _supported_os Linux
> >  
> > -_require_attrs
> > +_require_chattr
> 
> I don't see _require_chattr in any of the common files, did I miss
> anything?

Oops, I forgot to pull that into the patch.  Thanks for catching this!

      	       	       	    	     	     	    - Ted

[PATCH 10/12] generic: disable generic/027 for tmpfs

[Theodore Ts'o]

The generic/027 test creates a large number of 1k files until the file
creation fails.  On other file systems there will be a large number
zero length files because the block allocation has failed, but there
are still inodes available, and so the file creation loop runs until
the file system runs out of inodes.

With tmpfs, we can limit the size of the file system, which limits the
block allocation, but there is no limit to the number of inodes that
can be created until kmalloc() fails --- or the OOM killer kills the
test.  So this causes this test to run for a long, long time, and in
some cases the test or the test runner will get OOM killed instead.
We have other ENOSPC tests, so given that tmpfs is just so different
from all other file systems, it's simpler just to disable this test
for tmpfs than to try to make it work.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/027 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/generic/027 b/tests/generic/027
index d2e59d6..f89a4ca 100755
--- a/tests/generic/027
+++ b/tests/generic/027
@@ -59,6 +59,7 @@ create_file()
 # real QA test starts here
 _supported_fs generic
 _supported_os Linux
+[ "$FSTYP" == tmpfs ] && _notrun "Test is not suitable for tmpfs"
 
 _require_scratch
 
-- 
2.5.0

Re: [PATCH 10/12] generic: disable generic/027 for tmpfs

[Dave Chinner]

On Tue, Feb 09, 2016 at 08:49:59PM -0500, Theodore Ts'o wrote:
> The generic/027 test creates a large number of 1k files until the file
> creation fails.  On other file systems there will be a large number
> zero length files because the block allocation has failed, but there
> are still inodes available, and so the file creation loop runs until
> the file system runs out of inodes.
> 
> With tmpfs, we can limit the size of the file system, which limits the
> block allocation, but there is no limit to the number of inodes that
> can be created until kmalloc() fails --- or the OOM killer kills the
> test.  So this causes this test to run for a long, long time, and in
> some cases the test or the test runner will get OOM killed instead.
> We have other ENOSPC tests, so given that tmpfs is just so different
> from all other file systems, it's simpler just to disable this test
> for tmpfs than to try to make it work.

This sounds like a bug in tmpfs and a potential user level DOS
vector, too. Hence I dont think not running the test is the right
thing to do here - tmpfs should be handling this gracefully by
applying sane resource limits.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 10/12] generic: disable generic/027 for tmpfs

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 04:58:39PM +1100, Dave Chinner wrote:
> > With tmpfs, we can limit the size of the file system, which limits the
> > block allocation, but there is no limit to the number of inodes that
> > can be created until kmalloc() fails --- or the OOM killer kills the
> > test.  So this causes this test to run for a long, long time, and in
> > some cases the test or the test runner will get OOM killed instead.
> > We have other ENOSPC tests, so given that tmpfs is just so different
> > from all other file systems, it's simpler just to disable this test
> > for tmpfs than to try to make it work.
> 
> This sounds like a bug in tmpfs and a potential user level DOS
> vector, too. Hence I dont think not running the test is the right
> thing to do here - tmpfs should be handling this gracefully by
> applying sane resource limits.

Well, it's not really that interesting of a DOS vector since if the
goal is to use up all available memory, there are other ways to do it
that are much more efficient.  If you are using a memory cgroup, the
kmalloc does eventually fail and so the O_CREAT open(2) call will
return ENOMEM --- but it can take 20+ hours with a 8G memory
container.  Without a memory cgroup in general the test runner gets
OOM killed first, but a much better DOS vector is to open one too many
tabs in Chrome, at which point your machine thrashes to death and the
X server goes unresponsive (and this is why many people have started
running Chrome inside a memory container).

OTOH, the fact that tmpfs doesn't have a inode limits is a bit weird.
What about having tmpfs enforce a per-mount "maxinodes" restriction
which by default is "maxsize / 1k", and which can be overriden using a
maxinodes mount option?  Does that sound sane to you?

Or we could charge a minimum 512 bytes per inode against the size,
since between the kernel data structures and the file name, it's not
like a zero-length tmpfs file is free.

						- Ted

Re: [PATCH 10/12] generic: disable generic/027 for tmpfs

[Dave Chinner]

On Wed, Feb 10, 2016 at 10:48:10AM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 04:58:39PM +1100, Dave Chinner wrote:
> > > With tmpfs, we can limit the size of the file system, which limits the
> > > block allocation, but there is no limit to the number of inodes that
> > > can be created until kmalloc() fails --- or the OOM killer kills the
> > > test.  So this causes this test to run for a long, long time, and in
> > > some cases the test or the test runner will get OOM killed instead.
> > > We have other ENOSPC tests, so given that tmpfs is just so different
> > > from all other file systems, it's simpler just to disable this test
> > > for tmpfs than to try to make it work.
> > 
> > This sounds like a bug in tmpfs and a potential user level DOS
> > vector, too. Hence I dont think not running the test is the right
> > thing to do here - tmpfs should be handling this gracefully by
> > applying sane resource limits.
> 
> Well, it's not really that interesting of a DOS vector since if the
> goal is to use up all available memory, there are other ways to do it
> that are much more efficient.

Of course, but that doesn't mean the behaviour is valid. If we are
relying on the OOM killer to kill the right thing or require memcgs
to prevent trivial OOM killer invocations, then we've got a problem
that needs fixing.

....

> OTOH, the fact that tmpfs doesn't have a inode limits is a bit weird.
> What about having tmpfs enforce a per-mount "maxinodes" restriction
> which by default is "maxsize / 1k", and which can be overriden using a
> maxinodes mount option?  Does that sound sane to you?
> 
> Or we could charge a minimum 512 bytes per inode against the size,
> since between the kernel data structures and the file name, it's not
> like a zero-length tmpfs file is free.

I'd suggest simply charging sizeof(struct shmem_inode_info) against
the space usage when inode allocation is requested.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 11/12] xfstests: add executable permission to tests

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/058 | 0
 tests/generic/060 | 0
 tests/generic/061 | 0
 tests/generic/063 | 0
 4 files changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 tests/generic/058
 mode change 100644 => 100755 tests/generic/060
 mode change 100644 => 100755 tests/generic/061
 mode change 100644 => 100755 tests/generic/063

diff --git a/tests/generic/058 b/tests/generic/058
old mode 100644
new mode 100755
diff --git a/tests/generic/060 b/tests/generic/060
old mode 100644
new mode 100755
diff --git a/tests/generic/061 b/tests/generic/061
old mode 100644
new mode 100755
diff --git a/tests/generic/063 b/tests/generic/063
old mode 100644
new mode 100755
-- 
2.5.0

Re: [PATCH 11/12] xfstests: add executable permission to tests

[Eryu Guan]

On Tue, Feb 09, 2016 at 08:50:00PM -0500, Theodore Ts'o wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

I noticed that there're more tests have the wrong mode

tests/xfs/140
tests/btrfs/104
tests/btrfs/026
tests/btrfs/114
tests/generic/061
tests/generic/103
tests/generic/063
tests/generic/094
tests/generic/058
tests/generic/060
tests/generic/092

I think they can be updated in one patch.

Thanks,
Eryu

> ---
>  tests/generic/058 | 0
>  tests/generic/060 | 0
>  tests/generic/061 | 0
>  tests/generic/063 | 0
>  4 files changed, 0 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 tests/generic/058
>  mode change 100644 => 100755 tests/generic/060
>  mode change 100644 => 100755 tests/generic/061
>  mode change 100644 => 100755 tests/generic/063
> 
> diff --git a/tests/generic/058 b/tests/generic/058
> old mode 100644
> new mode 100755
> diff --git a/tests/generic/060 b/tests/generic/060
> old mode 100644
> new mode 100755
> diff --git a/tests/generic/061 b/tests/generic/061
> old mode 100644
> new mode 100755
> diff --git a/tests/generic/063 b/tests/generic/063
> old mode 100644
> new mode 100755
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/12] xfstests: increase tmpfs memory size

[Theodore Ts'o]

From: Hugh Dickins <hughd@google.com>

512M is not enough for generic/129.  Raise default tmpfs size to 1G.

Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Junho Ryu <jayr@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/config | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/config b/common/config
index c97facf..cacd815 100644
--- a/common/config
+++ b/common/config
@@ -316,8 +316,8 @@ _mount_opts()
 		export MOUNT_OPTIONS="-o acl $GFS2_MOUNT_OPTIONS"
 		;;
 	tmpfs)
-		# We need to specify the size at mount, use 512 MB by default
-		export MOUNT_OPTIONS="-o size=512M $TMPFS_MOUNT_OPTIONS"
+		# We need to specify the size at mount, use 1G by default
+		export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
 		;;
 	*)
 		;;
-- 
2.5.0

Re: [PATCH 00/12] xfstests: fix up various tmpfs failures

[Theodore Ts'o]

Here is the output of "gce-xfstests -c tmpfs -g auto" with these
patches.

						- Ted

CMDLINE: -c tmpfs -g auto
FSTESTVER: e2fsprogs	v1.43-WIP-2015-05-18-64-g2334bd3 (Sat, 5 Sep 2015 22:21:35 -0400)
FSTESTVER: fio		fio-2.6-8-ge6989e1 (Thu, 4 Feb 2016 12:09:48 -0700)
FSTESTVER: quota		7367f5d (Wed, 27 Jan 2016 12:16:25 +0100)
FSTESTVER: xfsprogs	v4.2.0 (Mon, 7 Sep 2015 10:14:31 +1000)
FSTESTVER: xfstests-bld	0f62f62 (Tue, 9 Feb 2016 20:11:45 -0500)
FSTESTVER: xfstests	linux-v3.8-906-ga687f46 (Tue, 9 Feb 2016 20:07:42 -0500)
FSTESTVER: kernel	4.5.0-rc2-ext4-00002-g6df2762 #155 SMP Mon Feb 8 12:32:59 EST 2016 x86_64
FSTESTCFG: "tmpfs"
FSTESTSET: "-g auto"
FSTESTEXC: ""
FSTESTOPT: "aex"
MNTOPTS: ""
CPUS: "2"
MEM: "7477.45"
MEM: 7680 MB (Max capacity)
BEGIN TEST tmpfs: tmpfs Tue Feb  9 20:56:48 EST 2016
MKFS_OPTIONS  -- test:/scratch
MOUNT_OPTIONS -- -o size=1G test:/scratch /test/scratch
Ran: generic/001 generic/002 generic/003 generic/004 generic/005 generic/006 generic/007 generic/011 generic/013 generic/014 generic/015 generic/023 generic/024 generic/025 generic/028 generic/029 generic/030 generic/035 generic/053 generic/067 generic/069 generic/071 generic/074 generic/075 generic/078 generic/080 generic/083 generic/084 generic/086 generic/087 generic/088 generic/089 generic/095 generic/098 generic/100 generic/102 generic/105 generic/109 generic/112 generic/120 generic/123 generic/124 generic/126 generic/127 generic/128 generic/129 generic/131 generic/132 generic/141 generic/169 generic/184 generic/192 generic/193 generic/204 generic/213 generic/215 generic/221 generic/224 generic/228 generic/236 generic/237 generic/241 generic/245 generic/246 generic/247 generic/248 generic/249 generic/256 generic/257 generic/258 generic/274 generic/275 generic/285 generic/286 generic/294 generic/306 generic/307 generic/308 generic/309 generic/310 generic/312 generic/313 !
 generic/314 generic/315 generic/319 generic/320
Failures: generic/080
END TEST: tmpfs Tue Feb  9 21:03:53 EST 2016