[PATCH xfstests] generic/035: Override output for NFS testing

[PATCH xfstests] generic/035: Override output for NFS testing

[Benjamin Coddington]

We'd like to run generic tests for NFS, but often have slightly different
output for our results.  One instance is that for the NFS client the
removal of an open file or directory is handled differently than for a
local filesystem.  We can expect nlink to be 1 for files, and to receive
-ESTALE for operations on deleted directories, isn't that silly?

Override the default output when FSTYP == "nfs".

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 .gitignore                                 | 1 +
 tests/generic/035                          | 3 +++
 tests/generic/035.cfg                      | 1 +
 tests/generic/{035.out => 035.out.default} | 0
 tests/generic/035.out.nfs                  | 5 +++++
 5 files changed, 10 insertions(+)
 create mode 100644 tests/generic/035.cfg
 rename tests/generic/{035.out => 035.out.default} (100%)
 create mode 100644 tests/generic/035.out.nfs

diff --git a/.gitignore b/.gitignore
index 368d11c84a66..b2419862aff9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -246,6 +246,7 @@
 /tests/xfs/033.out
 /tests/xfs/071.out
 /tests/xfs/096.out
+/tests/generic/035.out
 
 # cscope files
 cscope.*
diff --git a/tests/generic/035 b/tests/generic/035
index 443ddd57bfc0..37423f32dddd 100755
--- a/tests/generic/035
+++ b/tests/generic/035
@@ -21,6 +21,7 @@
 #-----------------------------------------------------------------------
 #
 
+seqfull=$0
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
 echo "QA output created by $seq"
@@ -44,6 +45,8 @@ _supported_os Linux
 
 _require_test
 
+_link_out_file $FSTYP
+
 # real QA test starts here
 
 rename_dir=$TEST_DIR/$$
diff --git a/tests/generic/035.cfg b/tests/generic/035.cfg
new file mode 100644
index 000000000000..d02b0ce907d4
--- /dev/null
+++ b/tests/generic/035.cfg
@@ -0,0 +1 @@
+nfs: nfs
diff --git a/tests/generic/035.out b/tests/generic/035.out.default
similarity index 100%
rename from tests/generic/035.out
rename to tests/generic/035.out.default
diff --git a/tests/generic/035.out.nfs b/tests/generic/035.out.nfs
new file mode 100644
index 000000000000..6359197f1d04
--- /dev/null
+++ b/tests/generic/035.out.nfs
@@ -0,0 +1,5 @@
+QA output created by 035
+overwriting regular file:
+nlink is 1, should be 0
+overwriting directory:
+t_rename_overwrite: fstat(3): Stale file handle
-- 
2.9.3

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[Anna Schumaker]

I like this patch!  It's weird to see generic/035 actually pass for a change :)

Anna

On 03/29/2018 11:34 AM, Benjamin Coddington wrote:
> We'd like to run generic tests for NFS, but often have slightly different
> output for our results.  One instance is that for the NFS client the
> removal of an open file or directory is handled differently than for a
> local filesystem.  We can expect nlink to be 1 for files, and to receive
> -ESTALE for operations on deleted directories, isn't that silly?
> 
> Override the default output when FSTYP == "nfs".
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  .gitignore                                 | 1 +
>  tests/generic/035                          | 3 +++
>  tests/generic/035.cfg                      | 1 +
>  tests/generic/{035.out => 035.out.default} | 0
>  tests/generic/035.out.nfs                  | 5 +++++
>  5 files changed, 10 insertions(+)
>  create mode 100644 tests/generic/035.cfg
>  rename tests/generic/{035.out => 035.out.default} (100%)
>  create mode 100644 tests/generic/035.out.nfs
> 
> diff --git a/.gitignore b/.gitignore
> index 368d11c84a66..b2419862aff9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -246,6 +246,7 @@
>  /tests/xfs/033.out
>  /tests/xfs/071.out
>  /tests/xfs/096.out
> +/tests/generic/035.out
>  
>  # cscope files
>  cscope.*
> diff --git a/tests/generic/035 b/tests/generic/035
> index 443ddd57bfc0..37423f32dddd 100755
> --- a/tests/generic/035
> +++ b/tests/generic/035
> @@ -21,6 +21,7 @@
>  #-----------------------------------------------------------------------
>  #
>  
> +seqfull=$0
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
>  echo "QA output created by $seq"
> @@ -44,6 +45,8 @@ _supported_os Linux
>  
>  _require_test
>  
> +_link_out_file $FSTYP
> +
>  # real QA test starts here
>  
>  rename_dir=$TEST_DIR/$$
> diff --git a/tests/generic/035.cfg b/tests/generic/035.cfg
> new file mode 100644
> index 000000000000..d02b0ce907d4
> --- /dev/null
> +++ b/tests/generic/035.cfg
> @@ -0,0 +1 @@
> +nfs: nfs
> diff --git a/tests/generic/035.out b/tests/generic/035.out.default
> similarity index 100%
> rename from tests/generic/035.out
> rename to tests/generic/035.out.default
> diff --git a/tests/generic/035.out.nfs b/tests/generic/035.out.nfs
> new file mode 100644
> index 000000000000..6359197f1d04
> --- /dev/null
> +++ b/tests/generic/035.out.nfs
> @@ -0,0 +1,5 @@
> +QA output created by 035
> +overwriting regular file:
> +nlink is 1, should be 0
> +overwriting directory:
> +t_rename_overwrite: fstat(3): Stale file handle
> 

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[Eryu Guan]

On Thu, Mar 29, 2018 at 11:34:39AM -0400, Benjamin Coddington wrote:
> We'd like to run generic tests for NFS, but often have slightly different
> output for our results.  One instance is that for the NFS client the
> removal of an open file or directory is handled differently than for a
> local filesystem.  We can expect nlink to be 1 for files, and to receive
> -ESTALE for operations on deleted directories, isn't that silly?
> 
> Override the default output when FSTYP == "nfs".
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>

Thanks for the patch!

> ---
>  .gitignore                                 | 1 +
>  tests/generic/035                          | 3 +++
>  tests/generic/035.cfg                      | 1 +
>  tests/generic/{035.out => 035.out.default} | 0
>  tests/generic/035.out.nfs                  | 5 +++++
>  5 files changed, 10 insertions(+)
>  create mode 100644 tests/generic/035.cfg
>  rename tests/generic/{035.out => 035.out.default} (100%)
>  create mode 100644 tests/generic/035.out.nfs
> 
> diff --git a/.gitignore b/.gitignore
> index 368d11c84a66..b2419862aff9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -246,6 +246,7 @@
>  /tests/xfs/033.out
>  /tests/xfs/071.out
>  /tests/xfs/096.out
> +/tests/generic/035.out
>  
>  # cscope files
>  cscope.*
> diff --git a/tests/generic/035 b/tests/generic/035
> index 443ddd57bfc0..37423f32dddd 100755
> --- a/tests/generic/035
> +++ b/tests/generic/035
> @@ -21,6 +21,7 @@
>  #-----------------------------------------------------------------------
>  #
>  
> +seqfull=$0
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
>  echo "QA output created by $seq"
> @@ -44,6 +45,8 @@ _supported_os Linux
>  
>  _require_test
>  
> +_link_out_file $FSTYP
> +

We usually _link_out_file according to the features enabled at mkfs
time, so linking a .out file based on $FSTYP makes me wonder if it's
really a 'generic' test then.

So I think we could 'edit' the output for NFS a bit, e.g.

-src/t_rename_overwrite $file1 $file2
+# comments about why we special-case nfs here
+src/t_rename_overwrite $file1 $file2 >$tmp.file 2>&1
+if [ "$FSTYP" = "nfs" ]; then
+       sed -i '/nlink is 1/d' $tmp.file
+fi
+cat $tmp.file

Similar 'edit' can be done to the dir case too.

Thanks,
Eryu

>  # real QA test starts here
>  
>  rename_dir=$TEST_DIR/$$
> diff --git a/tests/generic/035.cfg b/tests/generic/035.cfg
> new file mode 100644
> index 000000000000..d02b0ce907d4
> --- /dev/null
> +++ b/tests/generic/035.cfg
> @@ -0,0 +1 @@
> +nfs: nfs
> diff --git a/tests/generic/035.out b/tests/generic/035.out.default
> similarity index 100%
> rename from tests/generic/035.out
> rename to tests/generic/035.out.default
> diff --git a/tests/generic/035.out.nfs b/tests/generic/035.out.nfs
> new file mode 100644
> index 000000000000..6359197f1d04
> --- /dev/null
> +++ b/tests/generic/035.out.nfs
> @@ -0,0 +1,5 @@
> +QA output created by 035
> +overwriting regular file:
> +nlink is 1, should be 0
> +overwriting directory:
> +t_rename_overwrite: fstat(3): Stale file handle
> -- 
> 2.9.3
> 
> --
> 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 xfstests] generic/035: Override output for NFS testing

[Christoph Hellwig]

On Thu, Mar 29, 2018 at 11:34:39AM -0400, Benjamin Coddington wrote:
> We'd like to run generic tests for NFS, but often have slightly different
> output for our results.  One instance is that for the NFS client the
> removal of an open file or directory is handled differently than for a
> local filesystem.  We can expect nlink to be 1 for files, and to receive
> -ESTALE for operations on deleted directories, isn't that silly?

NFS is simply buggy in this case, and we should at least xfail the test
case, not make it look fine.

I'd rather have a file that lists expected fails per file system with an
explanation than a hack like this that papers over the issue.

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[Trond Myklebust]

On Tue, 2018-04-03 at 02:45 -0700, Christoph Hellwig wrote:
> On Thu, Mar 29, 2018 at 11:34:39AM -0400, Benjamin Coddington wrote:
> > We'd like to run generic tests for NFS, but often have slightly
> > different
> > output for our results.  One instance is that for the NFS client
> > the
> > removal of an open file or directory is handled differently than
> > for a
> > local filesystem.  We can expect nlink to be 1 for files, and to
> > receive
> > -ESTALE for operations on deleted directories, isn't that silly?
> 
> NFS is simply buggy in this case, and we should at least xfail the
> test
> case, not make it look fine.
> 
> I'd rather have a file that lists expected fails per file system with
> an
> explanation than a hack like this that papers over the issue.

IIRC that ESTALE test is hitting a protocol issue: NFS doesn't have
stateful readdir() (or any stateful directory operations), and so there
is nothing to tell the server to pin the removed directory while we
have it open in the VFS layer on the client.

I'm fine either way, but if we make the decision to call out protocol
issues as 'expected failure' then we need to make that a consistent
policy for all xfstests.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[Benjamin Coddington]

On 3 Apr 2018, at 5:45, Christoph Hellwig wrote:

> On Thu, Mar 29, 2018 at 11:34:39AM -0400, Benjamin Coddington wrote:
>> We'd like to run generic tests for NFS, but often have slightly different
>> output for our results.  One instance is that for the NFS client the
>> removal of an open file or directory is handled differently than for a
>> local filesystem.  We can expect nlink to be 1 for files, and to receive
>> -ESTALE for operations on deleted directories, isn't that silly?
>
> NFS is simply buggy in this case, and we should at least xfail the test
> case, not make it look fine.

No, having nlink == 1 is not a bug and we should expect that behavior, the
same with the -ESTALE return for a directory.  This is true, at least, for
the linux client.

> I'd rather have a file that lists expected fails per file system with an
> explanation than a hack like this that papers over the issue.

I'd like that as well, since there are a number of tests that just don't
make sense at all for NFS..  I'll figure out a way to do that.  We have
groups of tests right now, and NFS is one, but those seem to be tests that
should be run only by NFS.

Ben

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[Christoph Hellwig]

On Tue, Apr 03, 2018 at 08:10:36AM -0400, Benjamin Coddington wrote:
> No, having nlink == 1 is not a bug and we should expect that behavior, the
> same with the -ESTALE return for a directory.  This is true, at least, for
> the linux client.

In terms of Linux semantics is plain and simple is a bug.  It is an
expected bug in NFS, but that doesn't make it correct.

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[Benjamin Coddington]

On 3 Apr 2018, at 8:25, Christoph Hellwig wrote:

> On Tue, Apr 03, 2018 at 08:10:36AM -0400, Benjamin Coddington wrote:
>> No, having nlink == 1 is not a bug and we should expect that behavior, the
>> same with the -ESTALE return for a directory.  This is true, at least, for
>> the linux client.
>
> In terms of Linux semantics is plain and simple is a bug.  It is an
> expected bug in NFS, but that doesn't make it correct.

Ok yes.  I'd still like to test for it, since it's possible we can get this
wrong.  Maybe a better approach is to copy this one to an NFS-only test,
with the expected buggy output, and then everything in generic/ can go back
to not having any output overrides.

That keeps us from setting a precedent that any generic/ tests may be
papered over, rather than expected to fail for a particular file system.

Ben

Re: [PATCH xfstests] generic/035: Override output for NFS testing

[J. Bruce Fields]

On Tue, Apr 03, 2018 at 08:36:35AM -0400, Benjamin Coddington wrote:
> On 3 Apr 2018, at 8:25, Christoph Hellwig wrote:
> 
> > On Tue, Apr 03, 2018 at 08:10:36AM -0400, Benjamin Coddington wrote:
> >> No, having nlink == 1 is not a bug and we should expect that behavior, the
> >> same with the -ESTALE return for a directory.  This is true, at least, for
> >> the linux client.
> >
> > In terms of Linux semantics is plain and simple is a bug.  It is an
> > expected bug in NFS, but that doesn't make it correct.
> 
> Ok yes.  I'd still like to test for it, since it's possible we can get this
> wrong.

In the regular file case this is fixable with current protocol[1].

If/when we fix this then we'll want a test like this one to verify the
fix.  So I think I'm won over to Christoph's point of view here.

Agreed that it'd be nice to have expected failures reported separately
somehow, though, as sorting through this kind of thing is an obstacle to
new NFS developers starting with xfstests.

--b.

[1] Grep for "OPEN4_RESULT_PRESERVE_UNLINKED" in RFC 5661.  NFSv4 opens
can already hold a reference to an unlinked file, the remaining work is
to figure out how to persist that across server reboot.  Maybe we'd do a
sillyrename server-side into a hidden directory and fix up nlink to hide
the extra link in this case.  Then we'd need to teach the client to stop
doing sillyrename when the PRESERVE_UNLINKED flag is set.