[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]

T24gVHVlLCAyMDE4LTA0LTAzIGF0IDAyOjQ1IC0wNzAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gT24gVGh1LCBNYXIgMjksIDIwMTggYXQgMTE6MzQ6MzlBTSAtMDQwMCwgQmVuamFtaW4g
Q29kZGluZ3RvbiB3cm90ZToNCj4gPiBXZSdkIGxpa2UgdG8gcnVuIGdlbmVyaWMgdGVzdHMgZm9y
IE5GUywgYnV0IG9mdGVuIGhhdmUgc2xpZ2h0bHkNCj4gPiBkaWZmZXJlbnQNCj4gPiBvdXRwdXQg
Zm9yIG91ciByZXN1bHRzLiAgT25lIGluc3RhbmNlIGlzIHRoYXQgZm9yIHRoZSBORlMgY2xpZW50
DQo+ID4gdGhlDQo+ID4gcmVtb3ZhbCBvZiBhbiBvcGVuIGZpbGUgb3IgZGlyZWN0b3J5IGlzIGhh
bmRsZWQgZGlmZmVyZW50bHkgdGhhbg0KPiA+IGZvciBhDQo+ID4gbG9jYWwgZmlsZXN5c3RlbS4g
IFdlIGNhbiBleHBlY3QgbmxpbmsgdG8gYmUgMSBmb3IgZmlsZXMsIGFuZCB0bw0KPiA+IHJlY2Vp
dmUNCj4gPiAtRVNUQUxFIGZvciBvcGVyYXRpb25zIG9uIGRlbGV0ZWQgZGlyZWN0b3JpZXMsIGlz
bid0IHRoYXQgc2lsbHk/DQo+IA0KPiBORlMgaXMgc2ltcGx5IGJ1Z2d5IGluIHRoaXMgY2FzZSwg
YW5kIHdlIHNob3VsZCBhdCBsZWFzdCB4ZmFpbCB0aGUNCj4gdGVzdA0KPiBjYXNlLCBub3QgbWFr
ZSBpdCBsb29rIGZpbmUuDQo+IA0KPiBJJ2QgcmF0aGVyIGhhdmUgYSBmaWxlIHRoYXQgbGlzdHMg
ZXhwZWN0ZWQgZmFpbHMgcGVyIGZpbGUgc3lzdGVtIHdpdGgNCj4gYW4NCj4gZXhwbGFuYXRpb24g
dGhhbiBhIGhhY2sgbGlrZSB0aGlzIHRoYXQgcGFwZXJzIG92ZXIgdGhlIGlzc3VlLg0KDQpJSVJD
IHRoYXQgRVNUQUxFIHRlc3QgaXMgaGl0dGluZyBhIHByb3RvY29sIGlzc3VlOiBORlMgZG9lc24n
dCBoYXZlDQpzdGF0ZWZ1bCByZWFkZGlyKCkgKG9yIGFueSBzdGF0ZWZ1bCBkaXJlY3Rvcnkgb3Bl
cmF0aW9ucyksIGFuZCBzbyB0aGVyZQ0KaXMgbm90aGluZyB0byB0ZWxsIHRoZSBzZXJ2ZXIgdG8g
cGluIHRoZSByZW1vdmVkIGRpcmVjdG9yeSB3aGlsZSB3ZQ0KaGF2ZSBpdCBvcGVuIGluIHRoZSBW
RlMgbGF5ZXIgb24gdGhlIGNsaWVudC4NCg0KSSdtIGZpbmUgZWl0aGVyIHdheSwgYnV0IGlmIHdl
IG1ha2UgdGhlIGRlY2lzaW9uIHRvIGNhbGwgb3V0IHByb3RvY29sDQppc3N1ZXMgYXMgJ2V4cGVj
dGVkIGZhaWx1cmUnIHRoZW4gd2UgbmVlZCB0byBtYWtlIHRoYXQgYSBjb25zaXN0ZW50DQpwb2xp
Y3kgZm9yIGFsbCB4ZnN0ZXN0cy4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBj
bGllbnQgbWFpbnRhaW5lciwgUHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0
YS5jb20NCg==

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.