diff mbox

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

Message ID 1455069001-17846-6-git-send-email-tytso@mit.edu (mailing list archive)
State New, archived
Headers show

Commit Message

Theodore Ts'o Feb. 10, 2016, 1:49 a.m. UTC 
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(-)

Comments

Dave Chinner Feb. 10, 2016, 6:07 a.m. UTC | #1 
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.
Theodore Ts'o Feb. 10, 2016, 4:07 p.m. UTC | #2 
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

--
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
Dave Chinner Feb. 10, 2016, 11:07 p.m. UTC | #3 
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.
Theodore Ts'o Feb. 10, 2016, 11:28 p.m. UTC | #4 
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
--
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
Dave Chinner Feb. 11, 2016, 3:07 a.m. UTC | #5 
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.
Theodore Ts'o Feb. 11, 2016, 3:25 p.m. UTC | #6 
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
--
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
Darrick J. Wong Feb. 11, 2016, 5:36 p.m. UTC | #7 
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
--
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
diff mbox

Patch

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