[PATCH 0/7] More NFS file handle unit tests

[PATCH 0/7] More NFS file handle unit tests

[Amir Goldstein]

Eryu,

This series enhances test coverage for generic NFS file handles
encode/decode functionality and adds a new gereric/exportfs test.

Please note that the new test output includes the temporary test
number 500, so don't forget to fix those when renaming the test.

The enhanced open_by_handle program is going to be used later on for
overlayfs specific exportfs tests [1].

The open_by_handle program is limited to encoding "non-connectable"
file handles (used by nfsd on 'no_subtree_check' exports), because there
is no user available API (that I know of) to encode a "connecctable" file
handle (used by nfsd on 'subtree_check' exports). I used a test patch
"test connectable file handles", available on my tree [1] to tun the tests
with "connectable" file handles.

I verified that the new test passes on xfs, ext4, ext2, btrfs, f2fs.
However, the test fails on tmpfs due to:
"open_by_handle() returned 116 incorrectly on an unlinked open file!"

This happens because tmpfs uses d_find_alias() to get a decoded dentry,
but d_find_alias() skips unhashed (deleted with refcount) dentries.

I don't know if being able to decode a file handle of a deleted but open
file is a requirement for nfsd or just a recommendation, but IMO it is a
common case that is worth testing, even if tmpfs (or other file systems)
choose not to fix this.

Bruce, Jeff,

What is your view on this issue?

Thanks,
Amir.

[1] https://github.com/amir73il/xfstests/commits/ovl-nfs-export
[2] https://github.com/amir73il/linux/commits/ovl-nfs-export-v1

Amir Goldstein (7):
  open_by_handle: add filename to error reports
  open_by_handle: test file handles of renamed files
  open_by_handle: test content of open file handle
  open_by_handle: test directory file handle
  open_by_handle: test file handles of open files
  generic/426: factor out helper functions
  fstests: add test with more open by file handle use cases

 src/open_by_handle.c  | 247 ++++++++++++++++++++++++++++++++++++++++++++------
 tests/generic/426     |  40 +++++---
 tests/generic/426.out |   4 +
 tests/generic/500     | 119 ++++++++++++++++++++++++
 tests/generic/500.out |  10 ++
 tests/generic/group   |   1 +
 6 files changed, 383 insertions(+), 38 deletions(-)
 create mode 100755 tests/generic/500
 create mode 100644 tests/generic/500.out

-- 
2.7.4

[PATCH 1/7] open_by_handle: add filename to error reports

[Amir Goldstein]

This makes it easier to understand which of the tests
failed when testing on overlay lower and upper files.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/open_by_handle.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 63ebdac..52973c6 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -156,7 +156,7 @@ int main(int argc, char **argv)
 		sprintf(fname2, "%s/link%06d", test_dir, i);
 		fd = open(fname, O_RDWR | O_CREAT | O_TRUNC, 0644);
 		if (fd < 0) {
-			printf("Warning (%s,%d), open(%s) failed.\n", __FILE__, __LINE__, fname);
+			strcat(fname, ": open");
 			perror(fname);
 			return EXIT_FAILURE;
 		}
@@ -164,7 +164,8 @@ int main(int argc, char **argv)
 		/* blow up leftovers hardlinks if they exist */
 		ret = unlink(fname2);
 		if (ret < 0 && errno != ENOENT) {
-			perror("unlink");
+			strcat(fname2, ": unlink");
+			perror(fname2);
 			return EXIT_FAILURE;
 		}
 	}
@@ -178,7 +179,8 @@ int main(int argc, char **argv)
 		handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
 		ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
 		if (ret < 0) {
-			perror("name_to_handle");
+			strcat(fname, ": name_to_handle");
+			perror(fname);
 			return EXIT_FAILURE;
 		}
 	}
@@ -193,7 +195,8 @@ int main(int argc, char **argv)
 		sprintf(fname2, "%s/link%06d", test_dir, i);
 		ret = link(fname, fname2);
 		if (ret < 0) {
-			perror("link");
+			strcat(fname2, ": link");
+			perror(fname2);
 			return EXIT_FAILURE;
 		}
 	}
@@ -204,14 +207,16 @@ int main(int argc, char **argv)
 		sprintf(fname2, "%s/link%06d", test_dir, i);
 		ret = unlink(fname);
 		if (ret < 0) {
-			perror("unlink");
+			strcat(fname, ": unlink");
+			perror(fname);
 			return EXIT_FAILURE;
 		}
 		/* with -d flag, delete the hardlink if it exists */
 		if (!nlink)
 			ret = unlink(fname2);
 		if (ret < 0 && errno != ENOENT) {
-			perror("unlink");
+			strcat(fname2, ": unlink");
+			perror(fname2);
 			return EXIT_FAILURE;
 		}
 	}
@@ -246,11 +251,13 @@ int main(int argc, char **argv)
 		} else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
 			continue;
 		}
+		sprintf(fname, "%s/file%06d", test_dir, i);
 		if (fd >= 0) {
-			printf("open_by_handle(%d) opened an unlinked file!\n", i);
+			printf("open_by_handle(%s) opened an unlinked file!\n", fname);
 			close(fd);
 		} else {
-			printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
+			printf("open_by_handle(%s) returned %d incorrectly on %s file!\n",
+					fname, errno,
 					nlink ? "a linked" : "an unlinked");
 		}
 		failed++;
-- 
2.7.4

[PATCH 2/7] open_by_handle: test file handles of renamed files

[Amir Goldstein]

usage: open_by_handle -m <test_dir> [N]

Get file handles for existing test set, rename all test files,
drop cache and try to open all files by handle.

This is needed for testing that overlayfs can find the upper
inode from lower file handle even when upper is not in the same
path as lower.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 52973c6..8b12df3 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -27,7 +27,7 @@
 
 /*
 
-usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
+usage: open_by_handle [-cludm] <test_dir> [num_files]
 
 Examples:
 
@@ -48,7 +48,12 @@ Examples:
 
    open_by_handle -d <test_dir> [N]
 
-4. Get file handles for existing test set, hardlink all test files,
+4. Get file handles for existing test set, rename all test files,
+   drop caches, try to open all files by handle (should work):
+
+   open_by_handle -m <test_dir> [N]
+
+5. Get file handles for existing test set, hardlink all test files,
    then unlink the original files, drop caches and try to open all
    files by handle (should work):
 
@@ -84,13 +89,14 @@ struct handle {
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludm] <test_dir> [num_files]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
 	exit(EXIT_FAILURE);
 }
 
@@ -105,12 +111,12 @@ int main(int argc, char **argv)
 	char	*test_dir;
 	int	mount_fd, mount_id;
 	int	numfiles = 1;
-	int	create = 0, delete = 0, nlink = 1;
+	int	create = 0, delete = 0, nlink = 1, move = 0;
 
 	if (argc < 2 || argc > 4)
 		usage();
 
-	while ((c = getopt(argc, argv, "clud")) != -1) {
+	while ((c = getopt(argc, argv, "cludm")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
@@ -126,6 +132,9 @@ int main(int argc, char **argv)
 			delete = 1;
 			nlink = 0;
 			break;
+		case 'm':
+			move = 1;
+			break;
 		default:
 			fprintf(stderr, "illegal option '%s'\n", argv[optind]);
 		case 'h':
@@ -201,6 +210,18 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/* rename the files */
+	for (i=0; move && i < numfiles; i++) {
+		sprintf(fname, "%s/file%06d", test_dir, i);
+		sprintf(fname2, "%s/link%06d", test_dir, i);
+		ret = rename(fname, fname2);
+		if (ret < 0) {
+			strcat(fname2, ": rename");
+			perror(fname2);
+			return EXIT_FAILURE;
+		}
+	}
+
 	/* unlink the files */
 	for (i=0; delete && i < numfiles; i++) {
 		sprintf(fname, "%s/file%06d", test_dir, i);
-- 
2.7.4

Re: [PATCH 2/7] open_by_handle: test file handles of renamed files

[Eryu Guan]

On Thu, Nov 02, 2017 at 12:15:34PM +0200, Amir Goldstein wrote:
> usage: open_by_handle -m <test_dir> [N]
> 
> Get file handles for existing test set, rename all test files,
> drop cache and try to open all files by handle.
> 
> This is needed for testing that overlayfs can find the upper
> inode from lower file handle even when upper is not in the same
> path as lower.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 52973c6..8b12df3 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -27,7 +27,7 @@
>  
>  /*
>  
> -usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
> +usage: open_by_handle [-cludm] <test_dir> [num_files]
>  
>  Examples:
>  
> @@ -48,7 +48,12 @@ Examples:
>  
>     open_by_handle -d <test_dir> [N]
>  
> -4. Get file handles for existing test set, hardlink all test files,
> +4. Get file handles for existing test set, rename all test files,
> +   drop caches, try to open all files by handle (should work):
> +
> +   open_by_handle -m <test_dir> [N]
> +
> +5. Get file handles for existing test set, hardlink all test files,
>     then unlink the original files, drop caches and try to open all
>     files by handle (should work):
>  
> @@ -84,13 +89,14 @@ struct handle {
>  
>  void usage(void)
>  {
> -	fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
> +	fprintf(stderr, "usage: open_by_handle [-cludm] <test_dir> [num_files]\n");
>  	fprintf(stderr, "\n");
>  	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
>  	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
>  	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
>  	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
>  	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> +	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
>  	exit(EXIT_FAILURE);
>  }
>  
> @@ -105,12 +111,12 @@ int main(int argc, char **argv)
>  	char	*test_dir;
>  	int	mount_fd, mount_id;
>  	int	numfiles = 1;
> -	int	create = 0, delete = 0, nlink = 1;
> +	int	create = 0, delete = 0, nlink = 1, move = 0;
>  
>  	if (argc < 2 || argc > 4)
>  		usage();
>  
> -	while ((c = getopt(argc, argv, "clud")) != -1) {
> +	while ((c = getopt(argc, argv, "cludm")) != -1) {
>  		switch (c) {
>  		case 'c':
>  			create = 1;
> @@ -126,6 +132,9 @@ int main(int argc, char **argv)
>  			delete = 1;
>  			nlink = 0;
>  			break;
> +		case 'm':
> +			move = 1;
> +			break;
>  		default:
>  			fprintf(stderr, "illegal option '%s'\n", argv[optind]);
>  		case 'h':
> @@ -201,6 +210,18 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> +	/* rename the files */
> +	for (i=0; move && i < numfiles; i++) {
> +		sprintf(fname, "%s/file%06d", test_dir, i);
> +		sprintf(fname2, "%s/link%06d", test_dir, i);

Should we choose another prefix for moved files? "link" is used for
hardlink files already, use something like "move"? (I can fix it up if
you agree to do so.)

Thanks,
Eryu

> +		ret = rename(fname, fname2);
> +		if (ret < 0) {
> +			strcat(fname2, ": rename");
> +			perror(fname2);
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
>  	/* unlink the files */
>  	for (i=0; delete && i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
> -- 
> 2.7.4
> 

Re: [PATCH 2/7] open_by_handle: test file handles of renamed files

[Eryu Guan]

On Wed, Nov 08, 2017 at 12:10:08PM +0800, Eryu Guan wrote:
> On Thu, Nov 02, 2017 at 12:15:34PM +0200, Amir Goldstein wrote:
> > usage: open_by_handle -m <test_dir> [N]
> > 
> > Get file handles for existing test set, rename all test files,
> > drop cache and try to open all files by handle.
> > 
> > This is needed for testing that overlayfs can find the upper
> > inode from lower file handle even when upper is not in the same
> > path as lower.
> > 
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
> >  1 file changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index 52973c6..8b12df3 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -27,7 +27,7 @@
> >  
> >  /*
> >  
> > -usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
> > +usage: open_by_handle [-cludm] <test_dir> [num_files]
> >  
> >  Examples:
> >  
> > @@ -48,7 +48,12 @@ Examples:
> >  
> >     open_by_handle -d <test_dir> [N]
> >  
> > -4. Get file handles for existing test set, hardlink all test files,
> > +4. Get file handles for existing test set, rename all test files,
> > +   drop caches, try to open all files by handle (should work):
> > +
> > +   open_by_handle -m <test_dir> [N]
> > +
> > +5. Get file handles for existing test set, hardlink all test files,
> >     then unlink the original files, drop caches and try to open all
> >     files by handle (should work):
> >  
> > @@ -84,13 +89,14 @@ struct handle {
> >  
> >  void usage(void)
> >  {
> > -	fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
> > +	fprintf(stderr, "usage: open_by_handle [-cludm] <test_dir> [num_files]\n");
> >  	fprintf(stderr, "\n");
> >  	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> >  	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
> >  	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
> >  	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> >  	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> > +	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
> >  	exit(EXIT_FAILURE);
> >  }
> >  
> > @@ -105,12 +111,12 @@ int main(int argc, char **argv)
> >  	char	*test_dir;
> >  	int	mount_fd, mount_id;
> >  	int	numfiles = 1;
> > -	int	create = 0, delete = 0, nlink = 1;
> > +	int	create = 0, delete = 0, nlink = 1, move = 0;
> >  
> >  	if (argc < 2 || argc > 4)
> >  		usage();
> >  
> > -	while ((c = getopt(argc, argv, "clud")) != -1) {
> > +	while ((c = getopt(argc, argv, "cludm")) != -1) {
> >  		switch (c) {
> >  		case 'c':
> >  			create = 1;
> > @@ -126,6 +132,9 @@ int main(int argc, char **argv)
> >  			delete = 1;
> >  			nlink = 0;
> >  			break;
> > +		case 'm':
> > +			move = 1;
> > +			break;
> >  		default:
> >  			fprintf(stderr, "illegal option '%s'\n", argv[optind]);
> >  		case 'h':
> > @@ -201,6 +210,18 @@ int main(int argc, char **argv)
> >  		}
> >  	}
> >  
> > +	/* rename the files */
> > +	for (i=0; move && i < numfiles; i++) {
> > +		sprintf(fname, "%s/file%06d", test_dir, i);
> > +		sprintf(fname2, "%s/link%06d", test_dir, i);
> 
> Should we choose another prefix for moved files? "link" is used for
> hardlink files already, use something like "move"? (I can fix it up if
> you agree to do so.)

Ah, seems this is not a trivial fixup as I thought, patch 4 depends on
the file name prefix of moved files to be "link" too.

Thanks,
Eryu

> 
> > +		ret = rename(fname, fname2);
> > +		if (ret < 0) {
> > +			strcat(fname2, ": rename");
> > +			perror(fname2);
> > +			return EXIT_FAILURE;
> > +		}
> > +	}
> > +
> >  	/* unlink the files */
> >  	for (i=0; delete && i < numfiles; i++) {
> >  		sprintf(fname, "%s/file%06d", test_dir, i);
> > -- 
> > 2.7.4
> > 
> --
> 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 2/7] open_by_handle: test file handles of renamed files

[Amir Goldstein]

On Wed, Nov 8, 2017 at 7:07 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Nov 08, 2017 at 12:10:08PM +0800, Eryu Guan wrote:
>> On Thu, Nov 02, 2017 at 12:15:34PM +0200, Amir Goldstein wrote:
>> > usage: open_by_handle -m <test_dir> [N]
>> >
>> > Get file handles for existing test set, rename all test files,
>> > drop cache and try to open all files by handle.
>> >
>> > This is needed for testing that overlayfs can find the upper
>> > inode from lower file handle even when upper is not in the same
>> > path as lower.
>> >
>> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> > ---
>> >  src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
>> >  1 file changed, 26 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
>> > index 52973c6..8b12df3 100644
>> > --- a/src/open_by_handle.c
>> > +++ b/src/open_by_handle.c
>> > @@ -27,7 +27,7 @@
>> >
>> >  /*
>> >
>> > -usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
>> > +usage: open_by_handle [-cludm] <test_dir> [num_files]
>> >
>> >  Examples:
>> >
>> > @@ -48,7 +48,12 @@ Examples:
>> >
>> >     open_by_handle -d <test_dir> [N]
>> >
>> > -4. Get file handles for existing test set, hardlink all test files,
>> > +4. Get file handles for existing test set, rename all test files,
>> > +   drop caches, try to open all files by handle (should work):
>> > +
>> > +   open_by_handle -m <test_dir> [N]
>> > +
>> > +5. Get file handles for existing test set, hardlink all test files,
>> >     then unlink the original files, drop caches and try to open all
>> >     files by handle (should work):
>> >
>> > @@ -84,13 +89,14 @@ struct handle {
>> >
>> >  void usage(void)
>> >  {
>> > -   fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
>> > +   fprintf(stderr, "usage: open_by_handle [-cludm] <test_dir> [num_files]\n");
>> >     fprintf(stderr, "\n");
>> >     fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
>> >     fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
>> >     fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
>> >     fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
>> >     fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
>> > +   fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
>> >     exit(EXIT_FAILURE);
>> >  }
>> >
>> > @@ -105,12 +111,12 @@ int main(int argc, char **argv)
>> >     char    *test_dir;
>> >     int     mount_fd, mount_id;
>> >     int     numfiles = 1;
>> > -   int     create = 0, delete = 0, nlink = 1;
>> > +   int     create = 0, delete = 0, nlink = 1, move = 0;
>> >
>> >     if (argc < 2 || argc > 4)
>> >             usage();
>> >
>> > -   while ((c = getopt(argc, argv, "clud")) != -1) {
>> > +   while ((c = getopt(argc, argv, "cludm")) != -1) {
>> >             switch (c) {
>> >             case 'c':
>> >                     create = 1;
>> > @@ -126,6 +132,9 @@ int main(int argc, char **argv)
>> >                     delete = 1;
>> >                     nlink = 0;
>> >                     break;
>> > +           case 'm':
>> > +                   move = 1;
>> > +                   break;
>> >             default:
>> >                     fprintf(stderr, "illegal option '%s'\n", argv[optind]);
>> >             case 'h':
>> > @@ -201,6 +210,18 @@ int main(int argc, char **argv)
>> >             }
>> >     }
>> >
>> > +   /* rename the files */
>> > +   for (i=0; move && i < numfiles; i++) {
>> > +           sprintf(fname, "%s/file%06d", test_dir, i);
>> > +           sprintf(fname2, "%s/link%06d", test_dir, i);
>>
>> Should we choose another prefix for moved files? "link" is used for
>> hardlink files already, use something like "move"? (I can fix it up if
>> you agree to do so.)
>
> Ah, seems this is not a trivial fixup as I thought, patch 4 depends on
> the file name prefix of moved files to be "link" too.
>

Move is very similar to link+unlink origin in the context of this test,
to the point that nfsd wouldn't be able to tell the difference between
the two cases.
When it is time to decode the file handle, in both cases there is
nlink == 1 and origin file path is unlinked.

Amir.

[PATCH 3/7] open_by_handle: test content of open file handle

[Amir Goldstein]

usage: open_by_handle -rwa <test_dir> [N]

Get file handles for existing test set, write data to files,
drop caches, open all files by handle, read data and verify old data,
write new data to files.

This is needed for testing that overlay decoded file handles are not
pointing the lower inodes after new data is already written to upper
inodes after copy up.

open_by_handle -a is needed for testing copy up of disconnected overlay
decoded file handles (to index dir).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/open_by_handle.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 7 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 8b12df3..581dc05 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -27,7 +27,7 @@
 
 /*
 
-usage: open_by_handle [-cludm] <test_dir> [num_files]
+usage: open_by_handle [-cludmrwa] <test_dir> [num_files]
 
 Examples:
 
@@ -43,17 +43,23 @@ Examples:
 
    open_by_handle <test_dir> [N]
 
-3. Get file handles for existing test set, unlink all test files,
+3. Get file handles for existing test set, write data to files,
+   drop caches, open all files by handle, read and verify written
+   data, write new data to file:
+
+   open_by_handle -rwa <test_dir> [N]
+
+4. Get file handles for existing test set, unlink all test files,
    drop caches, try to open all files by handle and expect ESTALE:
 
    open_by_handle -d <test_dir> [N]
 
-4. Get file handles for existing test set, rename all test files,
+5. Get file handles for existing test set, rename all test files,
    drop caches, try to open all files by handle (should work):
 
    open_by_handle -m <test_dir> [N]
 
-5. Get file handles for existing test set, hardlink all test files,
+6. Get file handles for existing test set, hardlink all test files,
    then unlink the original files, drop caches and try to open all
    files by handle (should work):
 
@@ -89,10 +95,13 @@ struct handle {
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-cludm] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludmrwa] <test_dir> [num_files]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -w <test_dir> [N] - write data to test files before open by handle\n");
+	fprintf(stderr, "open_by_handle -r <test_dir> [N] - read data from test files after open by handle and verify written data\n");
+	fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
 	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
@@ -112,15 +121,28 @@ int main(int argc, char **argv)
 	int	mount_fd, mount_id;
 	int	numfiles = 1;
 	int	create = 0, delete = 0, nlink = 1, move = 0;
+	int	rd = 0, wr = 0, wrafter = 0;
 
 	if (argc < 2 || argc > 4)
 		usage();
 
-	while ((c = getopt(argc, argv, "cludm")) != -1) {
+	while ((c = getopt(argc, argv, "cludmrwa")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
 			break;
+		case 'w':
+			/* Write data before open_by_handle_at() */
+			wr = 1;
+			break;
+		case 'r':
+			/* Read data after open_by_handle_at() */
+			rd = 1;
+			break;
+		case 'a':
+			/* Write data after open_by_handle_at() */
+			wrafter = 1;
+			break;
 		case 'l':
 			nlink = 2;
 			break;
@@ -194,6 +216,23 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/* write data to files */
+	for (i=0; wr && i < numfiles; i++) {
+		sprintf(fname, "%s/file%06d", test_dir, i);
+		fd = open(fname, O_WRONLY, 0644);
+		if (fd < 0) {
+			strcat(fname, ": open");
+			perror(fname);
+			return EXIT_FAILURE;
+		}
+		if (write(fd, "aaaa", 4) != 4) {
+			strcat(fname, ": write before");
+			perror(fname);
+			return EXIT_FAILURE;
+		}
+		close(fd);
+	}
+
 	/* after creating test set only check that fs supports exportfs */
 	if (create)
 		return EXIT_SUCCESS;
@@ -265,8 +304,25 @@ int main(int argc, char **argv)
 	 */
 	for (i=0; i < numfiles; i++) {
 		errno = 0;
-		fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
+		fd = open_by_handle_at(mount_fd, &handle[i].fh, wrafter ? O_RDWR : O_RDONLY);
 		if (nlink && fd >= 0) {
+			if (rd) {
+				char buf[4] = {0};
+				int size = read(fd, buf, 4);
+				if (size < 0) {
+					strcat(fname, ": read");
+					perror(fname);
+					return EXIT_FAILURE;
+				}
+				if (size < 4 || memcmp(buf, "aaaa", 4)) {
+					printf("open_by_handle(%s) returned stale data '%.*s'!\n", fname, size, buf);
+				}
+			}
+			if (wrafter && write(fd, "aaaa", 4) != 4) {
+				strcat(fname, ": write after");
+				perror(fname);
+				return EXIT_FAILURE;
+			}
 			close(fd);
 			continue;
 		} else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
-- 
2.7.4

[PATCH 4/7] open_by_handle: test directory file handle

[Amir Goldstein]

usage: open_by_handle -cp <test_dir> [N]
usage: open_by_handle  -p <test_dir> [N]
usage: open_by_handle -dp <test_dir> [N]

With -p flag, create/delete also the test_dir itself and try to open by
handle also test_dir itself after droping caches and use dir fd to test
faccessat() of a file inside dir.
mount_fd argument to open_by_handle_at() is open fd of test_dir's parent.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/open_by_handle.c | 114 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 101 insertions(+), 13 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 581dc05..e8edcf5 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -27,13 +27,13 @@
 
 /*
 
-usage: open_by_handle [-cludmrwa] <test_dir> [num_files]
+usage: open_by_handle [-cludmrwap] <test_dir> [num_files]
 
 Examples:
 
-1. Create test set of N files and try to get their NFS handles:
+1. Create test set of of test_dir with N files and try to get their NFS handles:
 
-   open_by_handle -c <test_dir> [N]
+   open_by_handle -cp <test_dir> [N]
 
    This is used by new helper _require_exportfs() to check
    if filesystem supports exportfs
@@ -41,7 +41,7 @@ Examples:
 2. Get file handles for existing test set, drop caches and try to
    open all files by handle:
 
-   open_by_handle <test_dir> [N]
+   open_by_handle -p <test_dir> [N]
 
 3. Get file handles for existing test set, write data to files,
    drop caches, open all files by handle, read and verify written
@@ -50,9 +50,10 @@ Examples:
    open_by_handle -rwa <test_dir> [N]
 
 4. Get file handles for existing test set, unlink all test files,
-   drop caches, try to open all files by handle and expect ESTALE:
+   remove test_dir, drop caches, try to open all files by handle
+   and expect ESTALE:
 
-   open_by_handle -d <test_dir> [N]
+   open_by_handle -dp <test_dir> [N]
 
 5. Get file handles for existing test set, rename all test files,
    drop caches, try to open all files by handle (should work):
@@ -85,17 +86,18 @@ Examples:
 #include <sys/types.h>
 #include <errno.h>
 #include <linux/limits.h>
+#include <libgen.h>
 
 #define MAXFILES 1024
 
 struct handle {
 	struct file_handle fh;
 	unsigned char fid[MAX_HANDLE_SZ];
-} handle[MAXFILES];
+} handle[MAXFILES], dir_handle;
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-cludmrwa] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludmrwap] <test_dir> [num_files]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
@@ -106,6 +108,7 @@ void usage(void)
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
 	exit(EXIT_FAILURE);
 }
 
@@ -113,20 +116,22 @@ int main(int argc, char **argv)
 {
 	int	i, c;
 	int	fd;
-	int	ret;
+	int	ret = 0;
 	int	failed = 0;
+	char	dname[PATH_MAX];
 	char	fname[PATH_MAX];
 	char	fname2[PATH_MAX];
 	char	*test_dir;
+	char	*mount_dir;
 	int	mount_fd, mount_id;
 	int	numfiles = 1;
 	int	create = 0, delete = 0, nlink = 1, move = 0;
-	int	rd = 0, wr = 0, wrafter = 0;
+	int	rd = 0, wr = 0, wrafter = 0, parent = 0;
 
 	if (argc < 2 || argc > 4)
 		usage();
 
-	while ((c = getopt(argc, argv, "cludmrwa")) != -1) {
+	while ((c = getopt(argc, argv, "cludmrwap")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
@@ -157,6 +162,9 @@ int main(int argc, char **argv)
 		case 'm':
 			move = 1;
 			break;
+		case 'p':
+			parent = 1;
+			break;
 		default:
 			fprintf(stderr, "illegal option '%s'\n", argv[optind]);
 		case 'h':
@@ -173,9 +181,27 @@ int main(int argc, char **argv)
 		usage();
 	}
 
-	mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
+	if (parent) {
+		strcpy(dname, test_dir);
+		/*
+		 * -p flag implies that test_dir is NOT a mount point,
+		 * so its parent can be used as mount_fd for open_by_handle_at.
+		 */
+		mount_dir = dirname(dname);
+		if (create)
+			ret = mkdir(test_dir, 0700);
+		if (ret < 0 && errno != EEXIST) {
+			strcat(dname, ": mkdir");
+			perror(dname);
+			return EXIT_FAILURE;
+		}
+	} else {
+		mount_dir = test_dir;
+	}
+
+	mount_fd = open(mount_dir, O_RDONLY|O_DIRECTORY);
 	if (mount_fd < 0) {
-		perror(test_dir);
+		perror(mount_dir);
 		return EXIT_FAILURE;
 	}
 
@@ -216,6 +242,16 @@ int main(int argc, char **argv)
 		}
 	}
 
+	if (parent) {
+		dir_handle.fh.handle_bytes = MAX_HANDLE_SZ;
+		ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
+		if (ret < 0) {
+			strcat(dname, ": name_to_handle");
+			perror(dname);
+			return EXIT_FAILURE;
+		}
+	}
+
 	/* write data to files */
 	for (i=0; wr && i < numfiles; i++) {
 		sprintf(fname, "%s/file%06d", test_dir, i);
@@ -281,6 +317,15 @@ int main(int argc, char **argv)
 		}
 	}
 
+	if (parent && delete && !nlink) {
+		ret = rmdir(test_dir);
+		if (ret < 0) {
+			strcat(dname, ": rmdir");
+			perror(dname);
+			return EXIT_FAILURE;
+		}
+	}
+
 	/* sync to get log forced for unlink transactions to hit the disk */
 	sync();
 
@@ -339,6 +384,49 @@ int main(int argc, char **argv)
 		}
 		failed++;
 	}
+
+	if (parent) {
+		fd = open_by_handle_at(mount_fd, &dir_handle.fh, O_RDONLY|O_DIRECTORY);
+		if (fd >= 0) {
+			if (!nlink) {
+				printf("open_by_handle(%s) opened an unlinked dir!\n", dname);
+				return EXIT_FAILURE;
+			} else {
+				/*
+				 * Sanity check dir fd - expect to access orig file IFF
+				 * it was not unlinked nor renamed.
+				 */
+				strcpy(fname, "file000000");
+				ret = faccessat(fd, fname, F_OK, 0);
+				if ((ret == 0) != (!delete && !move) ||
+				    ((ret < 0) && errno != ENOENT)) {
+					strcat(fname, ": unexpected result from faccessat");
+					perror(fname);
+					return EXIT_FAILURE;
+				}
+				/*
+				 * Expect to access link file if ran test with -l flag
+				 * (nlink > 1), -m flag (orig moved to link name) or
+				 * -u flag (which implied previous -l run).
+				 */
+				strcpy(fname2, "link000000");
+				ret = faccessat(fd, fname2, F_OK, 0);
+				if (ret < 0 && (nlink > 1 || delete || move ||
+						errno != ENOENT)) {
+					strcat(fname2, ": unexpected result from faccessat");
+					perror(fname2);
+					return EXIT_FAILURE;
+				}
+			}
+			close(fd);
+		} else if (nlink || !(errno == ENOENT || errno == ESTALE)) {
+			printf("open_by_handle(%s) returned %d incorrectly on %s dir!\n",
+					dname, errno,
+					nlink ? "a linked" : "an unlinked");
+			return EXIT_FAILURE;
+		}
+	}
+
 	if (failed)
 		return EXIT_FAILURE;
 	return EXIT_SUCCESS;
-- 
2.7.4

[PATCH 5/7] open_by_handle: test file handles of open files

[Amir Goldstein]

usage: open_by_handle -dk <test_dir> [N]

Get file handles for existing test set, keep open file handles for all
test files, unlink all test files, drop caches and try to open all files
by handle.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/open_by_handle.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index e8edcf5..dbc5b0f 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -27,7 +27,7 @@
 
 /*
 
-usage: open_by_handle [-cludmrwap] <test_dir> [num_files]
+usage: open_by_handle [-cludmrwapk] <test_dir> [num_files]
 
 Examples:
 
@@ -55,12 +55,18 @@ Examples:
 
    open_by_handle -dp <test_dir> [N]
 
-5. Get file handles for existing test set, rename all test files,
+5. Get file handles for existing test set, keep open file handles for all
+   test files, unlink all test files, drop caches and try to open all files
+   by handle (should work):
+
+   open_by_handle -dk <test_dir> [N]
+
+6. Get file handles for existing test set, rename all test files,
    drop caches, try to open all files by handle (should work):
 
    open_by_handle -m <test_dir> [N]
 
-6. Get file handles for existing test set, hardlink all test files,
+7. Get file handles for existing test set, hardlink all test files,
    then unlink the original files, drop caches and try to open all
    files by handle (should work):
 
@@ -97,10 +103,11 @@ struct handle {
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-cludmrwap] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludmrwapk] <test_dir> [num_files]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -w <test_dir> [N] - write data to test files before open by handle\n");
 	fprintf(stderr, "open_by_handle -r <test_dir> [N] - read data from test files after open by handle and verify written data\n");
 	fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
@@ -127,11 +134,12 @@ int main(int argc, char **argv)
 	int	numfiles = 1;
 	int	create = 0, delete = 0, nlink = 1, move = 0;
 	int	rd = 0, wr = 0, wrafter = 0, parent = 0;
+	int	keepopen = 0;
 
 	if (argc < 2 || argc > 4)
 		usage();
 
-	while ((c = getopt(argc, argv, "cludmrwap")) != -1) {
+	while ((c = getopt(argc, argv, "cludmrwapk")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
@@ -165,6 +173,9 @@ int main(int argc, char **argv)
 		case 'p':
 			parent = 1;
 			break;
+		case 'k':
+			keepopen = 1;
+			break;
 		default:
 			fprintf(stderr, "illegal option '%s'\n", argv[optind]);
 		case 'h':
@@ -213,7 +224,7 @@ int main(int argc, char **argv)
 		sprintf(fname2, "%s/link%06d", test_dir, i);
 		fd = open(fname, O_RDWR | O_CREAT | O_TRUNC, 0644);
 		if (fd < 0) {
-			strcat(fname, ": open");
+			strcat(fname, ": open(O_CREAT)");
 			perror(fname);
 			return EXIT_FAILURE;
 		}
@@ -240,6 +251,15 @@ int main(int argc, char **argv)
 			perror(fname);
 			return EXIT_FAILURE;
 		}
+		if (keepopen) {
+			/* Open without close to keep unlinked files around */
+			fd = open(fname, O_RDONLY);
+			if (fd < 0) {
+				strcat(fname, ": open(O_RDONLY)");
+				perror(fname);
+				return EXIT_FAILURE;
+			}
+		}
 	}
 
 	if (parent) {
@@ -350,7 +370,7 @@ int main(int argc, char **argv)
 	for (i=0; i < numfiles; i++) {
 		errno = 0;
 		fd = open_by_handle_at(mount_fd, &handle[i].fh, wrafter ? O_RDWR : O_RDONLY);
-		if (nlink && fd >= 0) {
+		if ((nlink || keepopen) && fd >= 0) {
 			if (rd) {
 				char buf[4] = {0};
 				int size = read(fd, buf, 4);
@@ -370,7 +390,7 @@ int main(int argc, char **argv)
 			}
 			close(fd);
 			continue;
-		} else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
+		} else if (!nlink && !keepopen && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
 			continue;
 		}
 		sprintf(fname, "%s/file%06d", test_dir, i);
@@ -378,9 +398,10 @@ int main(int argc, char **argv)
 			printf("open_by_handle(%s) opened an unlinked file!\n", fname);
 			close(fd);
 		} else {
-			printf("open_by_handle(%s) returned %d incorrectly on %s file!\n",
+			printf("open_by_handle(%s) returned %d incorrectly on %s%s file!\n",
 					fname, errno,
-					nlink ? "a linked" : "an unlinked");
+					nlink ? "a linked" : "an unlinked",
+					keepopen ? " open" : "");
 		}
 		failed++;
 	}
-- 
2.7.4

[PATCH 6/7] generic/426: factor out helper functions

[Amir Goldstein]

Helper test_file_handles() outputs the sub-test command to output,
so if errors are detected in one of the sub-tests, it is easier
to know which sub-tests have failed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/generic/426     | 40 +++++++++++++++++++++++++++++-----------
 tests/generic/426.out |  4 ++++
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/tests/generic/426 b/tests/generic/426
index 2d96c1d..68ff169 100755
--- a/tests/generic/426
+++ b/tests/generic/426
@@ -52,25 +52,43 @@ _require_test
 _require_test_program "open_by_handle"
 _require_exportfs
 
-numfiles=1024
+NUMFILES=1024
 testdir=$TEST_DIR/$seq-dir
 mkdir -p $testdir
 
+# Create empty test files in test dir
+create_test_files()
+{
+	local dir=$1
+	local opt=$2
+
+	mkdir -p $dir
+	rm -f $dir/*
+	src/open_by_handle -c $dir $NUMFILES
+}
+
+# Test encode/decode file handles
+test_file_handles()
+{
+	local dir=$1
+	local opt=$2
+
+	echo test_file_handles $* | _filter_test_dir
+	src/open_by_handle $opt $dir $NUMFILES
+}
+
 # Check stale handles to deleted files
-rm -f $testdir/*
-src/open_by_handle -c $testdir $numfiles
-src/open_by_handle -d $testdir $numfiles
+create_test_files $testdir
+test_file_handles $testdir -d
 
 # Check non-stale handles to linked files
-rm -f $testdir/*
-src/open_by_handle -c $testdir $numfiles
-src/open_by_handle    $testdir $numfiles
+create_test_files $testdir
+test_file_handles $testdir
 
 # Check non-stale handles to files that were hardlinked and original deleted
-rm -f $testdir/*
-src/open_by_handle -c $testdir $numfiles
-src/open_by_handle -l $testdir $numfiles
-src/open_by_handle -u $testdir $numfiles
+create_test_files $testdir
+test_file_handles $testdir -l
+test_file_handles $testdir -u
 
 echo "Silence is golden"
 status=0
diff --git a/tests/generic/426.out b/tests/generic/426.out
index 777cbcd..9a25123 100644
--- a/tests/generic/426.out
+++ b/tests/generic/426.out
@@ -1,2 +1,6 @@
 QA output created by 426
+test_file_handles TEST_DIR/426-dir -d
+test_file_handles TEST_DIR/426-dir
+test_file_handles TEST_DIR/426-dir -l
+test_file_handles TEST_DIR/426-dir -u
 Silence is golden
-- 
2.7.4

[PATCH 7/7] fstests: add test with more open by file handle use cases

[Amir Goldstein]

This test is a variant of test generic/426 that tests with less
files and more use cases:
- Create test dir with non empty files with known content and verify
  their content after opening file by handle.
- Check open by handle of directory.
- Check open by handle of files that have been unlinked, but still open.
- Check open by handle of files that have been renamed in same dir,
  moved to new dir and whose parent dir has been renamed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/generic/500     | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/500.out |  10 +++++
 tests/generic/group   |   1 +
 3 files changed, 130 insertions(+)
 create mode 100755 tests/generic/500
 create mode 100644 tests/generic/500.out

diff --git a/tests/generic/500 b/tests/generic/500
new file mode 100755
index 0000000..ab3d119
--- /dev/null
+++ b/tests/generic/500
@@ -0,0 +1,119 @@
+#! /bin/bash
+# FS QA Test No. 500
+#
+# Check open by file handle.
+# This is a variant of test generic/426 that tests with less
+# files and more use cases:
+# - open directory by file handle
+# - verify content integrity of file after opening by file handle
+# - open by file handle of unlinked open files
+# - open by file handle of renamed files
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Author: Amir Goldstein <amir73il@gmail.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_test
+# _require_exportfs already requires open_by_handle, but let's not count on it
+_require_test_program "open_by_handle"
+_require_exportfs
+
+NUMFILES=10
+testdir=$TEST_DIR/$seq-dir
+
+# Create test dir and non-empty test files
+create_test_files()
+{
+	local dir=$1
+	local opt=$2
+
+	rm -rf $dir
+	src/open_by_handle -cwp $dir $NUMFILES
+}
+
+# Test encode/decode file handles
+test_file_handles()
+{
+	local dir=$1
+	local opt=$2
+
+	echo test_file_handles $* | _filter_test_dir
+	src/open_by_handle $opt $dir $NUMFILES
+}
+
+# Check stale handles to deleted files/dir
+create_test_files $testdir
+test_file_handles $testdir -dp
+
+# Check non-stale handles to linked files/dir
+create_test_files $testdir
+test_file_handles $testdir -rp
+
+# Check non-stale handles to unlinked open files
+create_test_files $testdir
+test_file_handles $testdir -dkr
+
+# Check non-stale handles to files that were hardlinked and original deleted
+create_test_files $testdir
+test_file_handles $testdir -lr
+test_file_handles $testdir -ur
+
+# Check non-stale file handles of renamed files
+create_test_files $testdir
+test_file_handles $testdir -mr
+
+# Check non-stale file handles after rename of parent
+create_test_files $testdir
+rm -rf $testdir.renamed
+mv $testdir $testdir.renamed/
+test_file_handles $testdir.renamed -rp
+
+# Check non-stale file handles after move to new parent
+create_test_files $testdir
+rm -rf $testdir.new
+mkdir -p $testdir.new
+mv $testdir/* $testdir.new/
+test_file_handles $testdir.new -rp
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/500.out b/tests/generic/500.out
new file mode 100644
index 0000000..0035d21
--- /dev/null
+++ b/tests/generic/500.out
@@ -0,0 +1,10 @@
+QA output created by 500
+test_file_handles TEST_DIR/500-dir -dp
+test_file_handles TEST_DIR/500-dir -rp
+test_file_handles TEST_DIR/500-dir -dkr
+test_file_handles TEST_DIR/500-dir -lr
+test_file_handles TEST_DIR/500-dir -ur
+test_file_handles TEST_DIR/500-dir -mr
+test_file_handles TEST_DIR/500-dir.renamed -rp
+test_file_handles TEST_DIR/500-dir.new -rp
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index fbe0a7f..0b41c15 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -468,3 +468,4 @@
 463 auto quick clone dangerous
 464 auto rw
 465 auto rw quick aio
+500 auto quick exportfs
-- 
2.7.4

Re: [PATCH 0/7] More NFS file handle unit tests

[Jeff Layton]

On Thu, 2017-11-02 at 12:15 +0200, Amir Goldstein wrote:
> Eryu,
> 
> This series enhances test coverage for generic NFS file handles
> encode/decode functionality and adds a new gereric/exportfs test.
> 
> Please note that the new test output includes the temporary test
> number 500, so don't forget to fix those when renaming the test.
> 
> The enhanced open_by_handle program is going to be used later on for
> overlayfs specific exportfs tests [1].
> 
> The open_by_handle program is limited to encoding "non-connectable"
> file handles (used by nfsd on 'no_subtree_check' exports), because there
> is no user available API (that I know of) to encode a "connecctable" file
> handle (used by nfsd on 'subtree_check' exports). I used a test patch
> "test connectable file handles", available on my tree [1] to tun the tests
> with "connectable" file handles.
> 
> I verified that the new test passes on xfs, ext4, ext2, btrfs, f2fs.
> However, the test fails on tmpfs due to:
> "open_by_handle() returned 116 incorrectly on an unlinked open file!"
> 
> This happens because tmpfs uses d_find_alias() to get a decoded dentry,
> but d_find_alias() skips unhashed (deleted with refcount) dentries.
> 
> I don't know if being able to decode a file handle of a deleted but open
> file is a requirement for nfsd or just a recommendation, but IMO it is a
> common case that is worth testing, even if tmpfs (or other file systems)
> choose not to fix this.
> 
> Bruce, Jeff,
> 
> What is your view on this issue?
> 
> 

It seems like something that should be a requirement.

A client could (for instance) send a READ for a filehandle with one of
the special anonymous stateids. If you can't decode the filehandle, you
really have no way to know what inode against which to issue the read.

> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/xfstests/commits/ovl-nfs-export
> [2] https://github.com/amir73il/linux/commits/ovl-nfs-export-v1
> 
> Amir Goldstein (7):
>   open_by_handle: add filename to error reports
>   open_by_handle: test file handles of renamed files
>   open_by_handle: test content of open file handle
>   open_by_handle: test directory file handle
>   open_by_handle: test file handles of open files
>   generic/426: factor out helper functions
>   fstests: add test with more open by file handle use cases
> 
>  src/open_by_handle.c  | 247 ++++++++++++++++++++++++++++++++++++++++++++------
>  tests/generic/426     |  40 +++++---
>  tests/generic/426.out |   4 +
>  tests/generic/500     | 119 ++++++++++++++++++++++++
>  tests/generic/500.out |  10 ++
>  tests/generic/group   |   1 +
>  6 files changed, 383 insertions(+), 38 deletions(-)
>  create mode 100755 tests/generic/500
>  create mode 100644 tests/generic/500.out
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/7] More NFS file handle unit tests

[Jeff Layton]

On Fri, 2017-11-03 at 08:22 -0400, Jeff Layton wrote:
> On Thu, 2017-11-02 at 12:15 +0200, Amir Goldstein wrote:
> > Eryu,
> > 
> > This series enhances test coverage for generic NFS file handles
> > encode/decode functionality and adds a new gereric/exportfs test.
> > 
> > Please note that the new test output includes the temporary test
> > number 500, so don't forget to fix those when renaming the test.
> > 
> > The enhanced open_by_handle program is going to be used later on for
> > overlayfs specific exportfs tests [1].
> > 
> > The open_by_handle program is limited to encoding "non-connectable"
> > file handles (used by nfsd on 'no_subtree_check' exports), because there
> > is no user available API (that I know of) to encode a "connecctable" file
> > handle (used by nfsd on 'subtree_check' exports). I used a test patch
> > "test connectable file handles", available on my tree [1] to tun the tests
> > with "connectable" file handles.
> > 
> > I verified that the new test passes on xfs, ext4, ext2, btrfs, f2fs.
> > However, the test fails on tmpfs due to:
> > "open_by_handle() returned 116 incorrectly on an unlinked open file!"
> > 
> > This happens because tmpfs uses d_find_alias() to get a decoded dentry,
> > but d_find_alias() skips unhashed (deleted with refcount) dentries.
> > 
> > I don't know if being able to decode a file handle of a deleted but open
> > file is a requirement for nfsd or just a recommendation, but IMO it is a
> > common case that is worth testing, even if tmpfs (or other file systems)
> > choose not to fix this.
> > 
> > Bruce, Jeff,
> > 
> > What is your view on this issue?
> > 
> > 
> 
> It seems like something that should be a requirement.
> 
> A client could (for instance) send a READ for a filehandle with one of
> the special anonymous stateids. If you can't decode the filehandle, you
> really have no way to know what inode against which to issue the read.

That said...

If an open file is unlinked, and the server reboots you're more or less
back in the same situation anyway. That's generally the reason for
sillyrenaming in NFSv4.x instead of just removing the files outright.

The main takeaway is that with NFS in general, it's actually rather
difficult to ensure that behavior across all server failure scenarios.
It'd be nice if tmpfs did this like the others, but it's probably not
fatal if it doesn't.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/7] More NFS file handle unit tests

[J . Bruce Fields]

On Sat, Nov 04, 2017 at 07:23:36PM -0400, Jeff Layton wrote:
> On Fri, 2017-11-03 at 08:22 -0400, Jeff Layton wrote:
> > On Thu, 2017-11-02 at 12:15 +0200, Amir Goldstein wrote:
> > > Eryu,
> > > 
> > > This series enhances test coverage for generic NFS file handles
> > > encode/decode functionality and adds a new gereric/exportfs test.
> > > 
> > > Please note that the new test output includes the temporary test
> > > number 500, so don't forget to fix those when renaming the test.
> > > 
> > > The enhanced open_by_handle program is going to be used later on for
> > > overlayfs specific exportfs tests [1].
> > > 
> > > The open_by_handle program is limited to encoding "non-connectable"
> > > file handles (used by nfsd on 'no_subtree_check' exports), because there
> > > is no user available API (that I know of) to encode a "connecctable" file
> > > handle (used by nfsd on 'subtree_check' exports). I used a test patch
> > > "test connectable file handles", available on my tree [1] to tun the tests
> > > with "connectable" file handles.
> > > 
> > > I verified that the new test passes on xfs, ext4, ext2, btrfs, f2fs.
> > > However, the test fails on tmpfs due to:
> > > "open_by_handle() returned 116 incorrectly on an unlinked open file!"
> > > 
> > > This happens because tmpfs uses d_find_alias() to get a decoded dentry,
> > > but d_find_alias() skips unhashed (deleted with refcount) dentries.
> > > 
> > > I don't know if being able to decode a file handle of a deleted but open
> > > file is a requirement for nfsd or just a recommendation, but IMO it is a
> > > common case that is worth testing, even if tmpfs (or other file systems)
> > > choose not to fix this.
> > > 
> > > Bruce, Jeff,
> > > 
> > > What is your view on this issue?
> > > 
> > > 
> > 
> > It seems like something that should be a requirement.
> > 
> > A client could (for instance) send a READ for a filehandle with one of
> > the special anonymous stateids. If you can't decode the filehandle, you
> > really have no way to know what inode against which to issue the read.

Also even if you use a "real" stateid the current implementation will
error out if the filehandle lookup fails, and that might be complicated
to fix.

> That said...
> 
> If an open file is unlinked, and the server reboots you're more or less
> back in the same situation anyway. That's generally the reason for
> sillyrenaming in NFSv4.x instead of just removing the files outright.
> 
> The main takeaway is that with NFS in general, it's actually rather
> difficult to ensure that behavior across all server failure scenarios.
> It'd be nice if tmpfs did this like the others, but it's probably not
> fatal if it doesn't.

I guess.  The idea of failing on an unlinked inode still makes me
nervous, though.  It's supposed to be a look up by *inode*, not dentry.
NFSv4 client can hold files open after unlink, even if the possibility
of server reboots means applications don't get a real guarantee of that.
tmpfs exports aren't much use across server reboots anyway, though.

Some day we may fix the reboot problem--but possibly just by doing
something like sillyrename on the server side, so that wouldn't affect
this case much.

I think some users would likely notice if we broke the ability to look
up an unlinked file so I'd rather we kept that test in.

--b.

Re: [PATCH 0/7] More NFS file handle unit tests

[J . Bruce Fields]

On Thu, Nov 02, 2017 at 12:15:32PM +0200, Amir Goldstein wrote:
> Eryu,
> 
> This series enhances test coverage for generic NFS file handles
> encode/decode functionality and adds a new gereric/exportfs test.

Look fine to me.

My only worry is the reliance on drop_caches.  It doesn't really
guarantee that the dentries will diseappear.  But I think we've had that
discussion before, and anyway that problem predates these patches.

--b.