| Message ID | e82df2bc4db926b051307ba4066d77b616c79b0c.1522337478.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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