This commit disables tests requires scratch dev running on NFS
c041421 xfstests: stop special casing nfs and udf
Now re-enable them to get a larger test coverage on NFS.
Also do more updates to avoid unnecessary failures on NFS.
I tested against NFSv3 NFSv4.0 NFSv4.1 (both linux server and client,
with 3.18-rc1 kernel), the results look good.
Failures on NFSv3:
generic/035 generic/089 generic/258 generic/294
Failures on NFSv4.0:
generic/035 generic/169 generic/294
Failures on NFSv4.1:
I hit kernel BUG_ON when testing on NFSv4.1 in generic/285
SEEK_DATA/SEEK_HOLE test. I think there's already a patch to fix it.
http://www.spinics.net/lists/linux-nfs/msg47359.html
Note that generic/294 does remount,ro on SCRATCH_DEV, but TEST_DEV is
affected too, so some tests after generic/294 fail because of EROFS.
Run the failed tests seperately and they all passed.
I'm not sure if it's a bug, but I filed one, please see
https://bugzilla.redhat.com/show_bug.cgi?id=1158046
Thanks,
Eryu
---
Eryu Guan (4):
common: re-enable tests that require scratch dev on NFS
common: add _require_block_device() helper
common: skip atime related tests on NFS
generic/277: add _require_attrs
common/rc | 40 ++++++++++++++++++++++++++++++++++++----
tests/generic/003 | 1 +
tests/generic/076 | 1 +
tests/generic/192 | 1 +
tests/generic/277 | 2 ++
5 files changed, 41 insertions(+), 4 deletions(-)
--
1.8.3.1
On Tue, Oct 28, 2014 at 06:22:52AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 28, 2014 at 09:16:08PM +0800, Eryu Guan wrote:
> > index 747cf72..8738da7 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -558,7 +558,11 @@ _scratch_mkfs()
> > _scratch_mkfs_xfs $*
> > ;;
> > nfs*)
> > - # do nothing for nfs
> > + # unable to re-create NFS, just remove all files in $SCRATCH_MNT to
> > + # avoid EEXIST caused by the leftover files created in previous runs
> > + _scratch_mount
> > + rm -rf $SCRATCH_MNT/*
> > + _scratch_unmount
>
> Please move this into a helper, and wire it up for cifs as well.
>
Will do in v2.
Thanks for the review!
Eryu
NFS doesn't support attr yet, add _require_attrs in generic/277 to avoid
failure when testing on NFS.
Signed-off-by: Eryu Guan <[email protected]>
---
tests/generic/277 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/generic/277 b/tests/generic/277
index 8461ad9..39ebdc3 100755
--- a/tests/generic/277
+++ b/tests/generic/277
@@ -38,11 +38,13 @@ trap "_cleanup ; exit \$status" 0 1 2 3 15
# get standard environment, filters and checks
. ./common/rc
. ./common/filter
+. ./common/attr
# real QA test starts here
_supported_fs generic
_supported_os Linux
_require_scratch
+_require_attrs
_scratch_mkfs > /dev/null 2>&1
_scratch_mount
--
1.8.3.1
On Tue, Oct 28, 2014 at 09:16:09PM +0800, Eryu Guan wrote:
> Add _require_block_device() helper and use it in _require_dm_flakey()
> and generic/076.
>
> _require_dm_flakey() assumes $SCRATCH_DEV is a block device, now it can
> also be a NFS export.
>
> generic/076 does "cat $SCRATCH_DEV" which will fail when testing on NFS.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Thu, Oct 30, 2014 at 02:03:06AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 28, 2014 at 09:16:10PM +0800, Eryu Guan wrote:
> > >From nfs(5) we can know that atime related mount options have no effect
> > on NFS mounts, so add _require_atime() helper to skip atime tests on NFS
>
> I' like to use this opportunity to start a discussion on NFS atime
> handlign again, which I think is broken. I think relatime is perfectly
> fine default semantics for NFS, and not supporting it can cause all
> kidns of application breakage. Supporting normal atime semantics when
> explicity requested is also something NFS shouldn't sneak out of.
>
> > Also change the way how _require_relatime() mount $SCRATCH_DEV, use
> > _scratch_mount helper so it's mounted with selinux context, to avoid
> > "same superblock, different selinux context" failure.
>
> Can you please split this part into a separate patch?
>
Sure, will do in v2.
Thanks,
Eryu
>From nfs(5) we can know that atime related mount options have no effect
on NFS mounts, so add _require_atime() helper to skip atime tests on NFS
Also change the way how _require_relatime() mount $SCRATCH_DEV, use
_scratch_mount helper so it's mounted with selinux context, to avoid
"same superblock, different selinux context" failure.
Signed-off-by: Eryu Guan <[email protected]>
---
common/rc | 9 ++++++++-
tests/generic/003 | 1 +
tests/generic/192 | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/common/rc b/common/rc
index 42f00cb..b50577f 100644
--- a/common/rc
+++ b/common/rc
@@ -2379,10 +2379,17 @@ _verify_reflink()
|| echo "$1 and $2 are not reflinks: different extents"
}
+_require_atime()
+{
+ if [ "$FSTYP" == "nfs" ]; then
+ _notrun "atime related mount options have no effect on NFS"
+ fi
+}
+
_require_relatime()
{
_scratch_mkfs > /dev/null 2>&1
- _mount -t $FSTYP -o relatime $SCRATCH_DEV $SCRATCH_MNT || \
+ _scratch_mount -o relatime || \
_notrun "relatime not supported by the current kernel"
_scratch_unmount
}
diff --git a/tests/generic/003 b/tests/generic/003
index 83d6f90..7ffd09a 100755
--- a/tests/generic/003
+++ b/tests/generic/003
@@ -47,6 +47,7 @@ _cleanup()
_supported_fs generic
_supported_os Linux
_require_scratch
+_require_atime
_require_relatime
rm -f $seqres.full
diff --git a/tests/generic/192 b/tests/generic/192
index b2da358..5b6cfbc 100755
--- a/tests/generic/192
+++ b/tests/generic/192
@@ -54,6 +54,7 @@ is_noatime_set() {
_supported_fs generic
_supported_os Linux
_require_test
+_require_atime
#delay=150
#delay=75
#delay=60
--
1.8.3.1
This commit disables tests requires scratch dev running on NFS
c041421 xfstests: stop special casing nfs and udf
Now re-enable them to get a larger test coverage on NFS.
Signed-off-by: Eryu Guan <[email protected]>
---
common/rc | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/common/rc b/common/rc
index 747cf72..8738da7 100644
--- a/common/rc
+++ b/common/rc
@@ -558,7 +558,11 @@ _scratch_mkfs()
_scratch_mkfs_xfs $*
;;
nfs*)
- # do nothing for nfs
+ # unable to re-create NFS, just remove all files in $SCRATCH_MNT to
+ # avoid EEXIST caused by the leftover files created in previous runs
+ _scratch_mount
+ rm -rf $SCRATCH_MNT/*
+ _scratch_unmount
;;
cifs)
# do nothing for cifs
@@ -1032,8 +1036,14 @@ _require_scratch_nocheck()
{
case "$FSTYP" in
nfs*)
- _notrun "requires a scratch device"
- ;;
+ echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1
+ if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then
+ _notrun "this test requires a valid \$SCRATCH_DEV"
+ fi
+ if [ ! -d "$SCRATCH_MNT" ]; then
+ _notrun "this test requires a valid \$SCRATCH_MNT"
+ fi
+ ;;
cifs)
_notrun "requires a scratch device"
;;
--
1.8.3.1
On Tue, Oct 28, 2014 at 09:16:08PM +0800, Eryu Guan wrote:
> index 747cf72..8738da7 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -558,7 +558,11 @@ _scratch_mkfs()
> _scratch_mkfs_xfs $*
> ;;
> nfs*)
> - # do nothing for nfs
> + # unable to re-create NFS, just remove all files in $SCRATCH_MNT to
> + # avoid EEXIST caused by the leftover files created in previous runs
> + _scratch_mount
> + rm -rf $SCRATCH_MNT/*
> + _scratch_unmount
Please move this into a helper, and wire it up for cifs as well.
Add _require_block_device() helper and use it in _require_dm_flakey()
and generic/076.
_require_dm_flakey() assumes $SCRATCH_DEV is a block device, now it can
also be a NFS export.
generic/076 does "cat $SCRATCH_DEV" which will fail when testing on NFS.
Signed-off-by: Eryu Guan <[email protected]>
---
common/rc | 15 +++++++++++++++
tests/generic/076 | 1 +
2 files changed, 16 insertions(+)
diff --git a/common/rc b/common/rc
index 8738da7..42f00cb 100644
--- a/common/rc
+++ b/common/rc
@@ -1237,10 +1237,25 @@ _require_command()
[ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test"
}
+# this test requires the device to be valid block device
+# $1 - device
+_require_block_device()
+{
+ if [ -z "$1" ]; then
+ echo "Usage: _require_block_device <dev>" 1>&2
+ exit 1
+ fi
+ if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then
+ _notrun "require $1 to be valid block disk"
+ fi
+}
+
# this test requires the device mapper flakey target
#
_require_dm_flakey()
{
+ # require SCRATCH_DEV to be a valid block device
+ _require_block_device $SCRATCH_DEV
_require_command $DMSETUP_PROG
modprobe dm-flakey >/dev/null 2>&1
diff --git a/tests/generic/076 b/tests/generic/076
index 02af762..aa0aae0 100755
--- a/tests/generic/076
+++ b/tests/generic/076
@@ -56,6 +56,7 @@ _supported_fs generic
_supported_os IRIX Linux
_require_scratch
+_require_block_device $SCRATCH_DEV
echo "*** init fs"
--
1.8.3.1
On Tue, Oct 28, 2014 at 09:16:11PM +0800, Eryu Guan wrote:
> NFS doesn't support attr yet, add _require_attrs in generic/277 to avoid
> failure when testing on NFS.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Oct 28, 2014 at 09:16:10PM +0800, Eryu Guan wrote:
> >From nfs(5) we can know that atime related mount options have no effect
> on NFS mounts, so add _require_atime() helper to skip atime tests on NFS
I' like to use this opportunity to start a discussion on NFS atime
handlign again, which I think is broken. I think relatime is perfectly
fine default semantics for NFS, and not supporting it can cause all
kidns of application breakage. Supporting normal atime semantics when
explicity requested is also something NFS shouldn't sneak out of.
> Also change the way how _require_relatime() mount $SCRATCH_DEV, use
> _scratch_mount helper so it's mounted with selinux context, to avoid
> "same superblock, different selinux context" failure.
Can you please split this part into a separate patch?
On Thu, Oct 30, 2014 at 11:03 AM, Christoph Hellwig <[email protected]> wrote:
> On Tue, Oct 28, 2014 at 09:16:10PM +0800, Eryu Guan wrote:
>> >From nfs(5) we can know that atime related mount options have no effect
>> on NFS mounts, so add _require_atime() helper to skip atime tests on NFS
>
> I' like to use this opportunity to start a discussion on NFS atime
> handlign again, which I think is broken. I think relatime is perfectly
> fine default semantics for NFS, and not supporting it can cause all
> kidns of application breakage. Supporting normal atime semantics when
> explicity requested is also something NFS shouldn't sneak out of.
If there is no read on the wire, then there is no way to update the
atime without doing an explicit SETATTR. Courtesy of POSIX filesystem
semantics on the server, that means we get a bonus change attribute
and ctime update (no extra charge).
Unless there are new suggestions for how to solve the atime issue that
do not involve introducing this or similar regressions, then the
standing NACK applies.
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On 10/31/2014 01:31 PM, Trond Myklebust wrote:
<>
> If there is no read on the wire, then there is no way to update the
> atime without doing an explicit SETATTR. Courtesy of POSIX filesystem
> semantics on the server, that means we get a bonus change attribute
> and ctime update (no extra charge).
>
> Unless there are new suggestions for how to solve the atime issue that
> do not involve introducing this or similar regressions, then the
> standing NACK applies.
>
Say the user asks, realy (realy^3) nicely, can we instead of sending
SETATTR (BAD) send in its place an async READ of say one byte (or one
word)
It will do what we want. Just need to collect the updates and atime vs rel-atime
correctly, and to not hurt performance as well. (Like only send when no real
READS went through)
> Cheers
> Trond
>
Cheers
Boaz
On Fri, Oct 31, 2014 at 01:31:36PM +0200, Trond Myklebust wrote:
> If there is no read on the wire, then there is no way to update the
> atime without doing an explicit SETATTR. Courtesy of POSIX filesystem
> semantics on the server, that means we get a bonus change attribute
> and ctime update (no extra charge).
>
> Unless there are new suggestions for how to solve the atime issue that
> do not involve introducing this or similar regressions, then the
> standing NACK applies.
I think various network filesystems are fairly lax on updating the atime
on the server. What xfstests generic/192 tests is that we don't lose
an atime update after an unmount/remount. I think we should be able
to expect local atime updates, and updates to the server on unmount.
Or at least claim that the filesystem is mounted by noatime if it is so
that users (including xfstests) notice, and can enable strict atime mode
if really needed, including the above mentioned drawbacks.