[RFC PATCH 0/4] re-enable tests that require scratch dev on NFS

[RFC PATCH 0/4] re-enable tests that require scratch dev on NFS

[Eryu Guan]

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

[PATCH 1/4] common: re-enable tests that require scratch dev on NFS

[Eryu Guan]

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 <eguan@redhat.com>
---
 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

Re: [PATCH 1/4] common: re-enable tests that require scratch dev on NFS

[Christoph Hellwig]

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.

Re: [PATCH 1/4] common: re-enable tests that require scratch dev on NFS

[Eryu Guan]

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

[PATCH 2/4] common: add _require_block_device() helper

[Eryu Guan]

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 <eguan@redhat.com>
---
 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

Re: [PATCH 2/4] common: add _require_block_device() helper

[Christoph Hellwig]

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 <hch@lst.de>

[PATCH 3/4] common: skip atime related tests on NFS

[Eryu Guan]

>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 <eguan@redhat.com>
---
 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

nfs atime semantics, was: Re: [PATCH 3/4] common: skip atime related tests on NFS

[Christoph Hellwig]

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?

Re: nfs atime semantics, was: Re: [PATCH 3/4] common: skip atime related tests on NFS

[Eryu Guan]

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

Re: nfs atime semantics, was: Re: [PATCH 3/4] common: skip atime related tests on NFS

[Trond Myklebust]

On Thu, Oct 30, 2014 at 11:03 AM, Christoph Hellwig <hch@infradead.org> 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

trond.myklebust@primarydata.com

Re: nfs atime semantics, was: Re: [PATCH 3/4] common: skip atime related tests on NFS

[Boaz Harrosh]

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

Re: nfs atime semantics, was: Re: [PATCH 3/4] common: skip atime related tests on NFS

[Christoph Hellwig]

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.

[PATCH 4/4] generic/277: add _require_attrs

[Eryu Guan]

NFS doesn't support attr yet, add _require_attrs in generic/277 to avoid
failure when testing on NFS.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 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

Re: [PATCH 4/4] generic/277: add _require_attrs

[Christoph Hellwig]

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 <hch@lst.de>