Skip to contentSkip to conversation

More NFS file handle unit tests

PATCH In review Amir Goldstein 4 participants 15 messages Nov 2, 2017 – Nov 8, 2017 VFS

lore
Share & tools
lore
#0 lore raw

cover More NFS file handle unit tests

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
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/7 open_by_handle: add filename to error reports

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

---
 src/open_by_handle.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)
src/open_by_handle.c +15 -8
index 63ebdac..52973c6 100644
@@ -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++;
Signature · 1 line
2.7.4
#2 lore raw

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

In reply to 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.

---
 src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
src/open_by_handle.c +26 -5
index 52973c6..8b12df3 100644
@@ -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);
Signature · 1 line
2.7.4
#3 lore raw

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

Quoted 83 lines · 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.

---
 src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
src/open_by_handle.c +14 -5
index 52973c6..8b12df3 100644
@@ -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':

src/open_by_handle.c int main(int argc, char **argv) · line 216

Quoted 7 lines
src/open_by_handle.c +4 -0
@@ -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

Quoted 14 lines
src/open_by_handle.c +8 -0
+		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);
Signature · 1 line
2.7.4
#4 lore raw

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

Quoted 87 lines · Eryu Guan
Quoted 86 lines · 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.

---
 src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
src/open_by_handle.c +14 -5
index 52973c6..8b12df3 100644
@@ -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)
 		}
 	}
 
Quoted 8 lines

src/open_by_handle.c int main(int argc, char **argv) · line 216

Quoted 4 lines
src/open_by_handle.c +4 -0
+	/* 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

Quoted 19 lines
Quoted 15 lines
src/open_by_handle.c +8 -0
+		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);
Signature · 1 line
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

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

Quoted 97 lines · Eryu Guan
Quoted 87 lines · Eryu Guan
Quoted 86 lines · 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.

---
 src/open_by_handle.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
src/open_by_handle.c +14 -5
index 52973c6..8b12df3 100644
@@ -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)
            }
    }
 
Quoted 8 lines

src/open_by_handle.c int main(int argc, char **argv) · line 216

Quoted 4 lines
src/open_by_handle.c +4 -0
+   /* 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.)

Quoted 3 lines

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.

#6 lore raw

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

In reply to 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).

---
 src/open_by_handle.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 7 deletions(-)
src/open_by_handle.c +63 -7
index 8b12df3..581dc05 100644
@@ -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)) {
Signature · 1 line
2.7.4
#7 lore raw

PATCH 4/7 open_by_handle: test directory file handle

In reply to 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.

---
 src/open_by_handle.c | 114 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 101 insertions(+), 13 deletions(-)
src/open_by_handle.c +101 -13
index 581dc05..e8edcf5 100644
@@ -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;
Signature · 1 line
2.7.4
#8 lore raw

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

In reply to 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.

---
 src/open_by_handle.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)
src/open_by_handle.c +31 -10
index e8edcf5..dbc5b0f 100644
@@ -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++;
 	}
Signature · 1 line
2.7.4
#9 lore raw

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

In reply to 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.

---
 tests/generic/426     | 40 +++++++++++++++++++++++++++++-----------
 tests/generic/426.out |  4 ++++
 2 files changed, 33 insertions(+), 11 deletions(-)
tests/generic/426 +29 -11
index 2d96c1d..68ff169 100755
@@ -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
tests/generic/426.out +4 -0
index 777cbcd..9a25123 100644
@@ -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
Signature · 1 line
2.7.4
#10 lore raw

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

In reply to 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.

---
 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
tests/generic/500 new file +119 -0
new file mode 100755
index 0000000..ab3d119
@@ -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 <[email protected]>
+#
+# 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
tests/generic/500.out new file +10 -0
new file mode 100644
index 0000000..0035d21
@@ -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
tests/generic/group +1 -0
index fbe0a7f..0b41c15 100644
@@ -468,3 +468,4 @@
 463 auto quick clone dangerous
 464 auto rw
 465 auto rw quick aio
+500 auto quick exportfs
Signature · 1 line
2.7.4
#11 lore raw
In reply to Amir Goldstein
Quoted 25 lines · 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.

Quoted 10 lines

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.

Quoted 25 lines

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
Signature · 1 line
Jeff Layton <[email protected]>
#12 lore raw
Quoted 37 lines · Jeff Layton
Quoted 25 lines · 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.

Quoted 10 lines

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?

Quoted 5 lines

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.

Signature · 1 line
Jeff Layton <[email protected]>
#13 lore raw
Quoted 38 lines · Jeff Layton
Quoted 37 lines · Jeff Layton
Quoted 35 lines · 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?

Quoted 5 lines
Quoted 5 lines

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.

Quoted 10 lines

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.

#14 lore raw
In reply to Amir Goldstein
Quoted 4 lines · Amir Goldstein

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.