[PATCH 0/4] fstests: generic test for NFS handles

[PATCH 0/4] fstests: generic test for NFS handles

[Amir Goldstein]

Eryu,

I am working on NFS export support for overlayfs [1].
Before testing with NFS client I wanted to test the file handle API,
but apparently xfstests have only tests for the XFS specific ioctl.

So I converted the stale_handle xfs/238 test to a generic test and
added some more test cases to it. 

On the bright side, if I disable the drop_caches in the test, the
test already passes on overlayfs in my test branch, but I still have
some work to do for full NFS export support.

When I am done with that, I will add some more overlay specific
exportfs tests (e.g. export handle from lower and decode after copy up).

In the mean while, running this new test on overlayfs yields:

generic/426 3s ... [not run] overlay does not support NFS export

Tested this on ext4, xfs, btrfs, tmpfs.

Thanks,
Amir.

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

Amir Goldstein (4):
  src/open_by_handle: helper to test open_by_handle_at() syscall
  src/open_by_handle: flexible usage options
  fstests: add helper _require_exportfs
  fstests: add generic test for file handles

 common/rc             |   9 +++
 src/Makefile          |   2 +-
 src/open_by_handle.c  | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/426     |  73 +++++++++++++++++
 tests/generic/426.out |   2 +
 tests/generic/group   |   1 +
 6 files changed, 298 insertions(+), 1 deletion(-)
 create mode 100644 src/open_by_handle.c
 create mode 100755 tests/generic/426
 create mode 100644 tests/generic/426.out

-- 
2.7.4

[PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[Amir Goldstein]

This is a clone of src/stale_handle.c test that uses generic
open_by_handle_at() syscall instead of the xfs specific ioctl.

No test is using this helper yet.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/Makefile         |   2 +-
 src/open_by_handle.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 src/open_by_handle.c

diff --git a/src/Makefile b/src/Makefile
index e62d7a9..6b38e77 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -22,7 +22,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
 	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
 	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
-	dio-invalidate-cache stat_test
+	dio-invalidate-cache stat_test open_by_handle
 
 SUBDIRS =
 
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
new file mode 100644
index 0000000..8f04865
--- /dev/null
+++ b/src/open_by_handle.c
@@ -0,0 +1,145 @@
+/*
+ * open_by_handle.c - attempt to create a file handle and open it
+ *                    with open_by_handle_at() syscall
+ *
+ * Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+ * Author: Amir Goldstein <amir73il@gmail.com>
+ * 
+ * from:
+ *  stale_handle.c
+ *
+ *  Copyright (C) 2010 Red Hat, Inc. All Rights reserved.
+ *
+ *  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; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will 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 to the Free Software
+ *  Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+#define TEST_UTIME
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <errno.h>
+#include <linux/limits.h>
+
+#define NUMFILES 1024
+
+struct handle {
+	struct file_handle fh;
+	unsigned char fid[MAX_HANDLE_SZ];
+} handle[NUMFILES];
+
+int main(int argc, char **argv)
+{
+	int	i;
+	int	fd;
+	int	ret;
+	int	failed = 0;
+	char	fname[PATH_MAX];
+	char	*test_dir;
+	int	mount_fd, mount_id;
+
+	if (argc != 2) {
+		fprintf(stderr, "usage: open_by_handle <test_dir>\n");
+		return EXIT_FAILURE;
+	}
+
+	test_dir = argv[1];
+	mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
+	if (mount_fd < 0) {
+		perror("open test_dir");
+		return EXIT_FAILURE;
+	}
+
+	/*
+	 * create a large number of files to force allocation of new inode
+	 * chunks on disk.
+	 */
+	for (i=0; i < NUMFILES; i++) {
+		sprintf(fname, "%s/file%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);
+			perror(fname);
+			return EXIT_FAILURE;
+		}
+		close(fd);
+	}
+
+	/* sync to get the new inodes to hit the disk */
+	sync();
+
+	/* create the handles */
+	for (i=0; i < NUMFILES; i++) {
+		sprintf(fname, "%s/file%06d", test_dir, i);
+		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");
+			return EXIT_FAILURE;
+		}
+	}
+
+	/* unlink the files */
+	for (i=0; i < NUMFILES; i++) {
+		sprintf(fname, "%s/file%06d", test_dir, i);
+		ret = unlink(fname);
+		if (ret < 0) {
+			perror("unlink");
+			return EXIT_FAILURE;
+		}
+	}
+
+	/* sync to get log forced for unlink transactions to hit the disk */
+	sync();
+
+	/* sync once more FTW */
+	sync();
+
+	/*
+	 * now drop the caches so that unlinked inodes are reclaimed and
+	 * buftarg page cache is emptied so that the inode cluster has to be
+	 * fetched from disk again for the open_by_handle() call.
+	 */
+	ret = system("echo 3 > /proc/sys/vm/drop_caches");
+	if (ret < 0) {
+		perror("drop_caches");
+		return EXIT_FAILURE;
+	}
+
+	/*
+	 * now try to open the files by the stored handles. Expecting ENOENT
+	 * for all of them.
+	 */
+	for (i=0; i < NUMFILES; i++) {
+		errno = 0;
+		fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
+		if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
+			continue;
+		}
+		if (fd >= 0) {
+			printf("open_by_handle(%d) opened an unlinked file!\n", i);
+			close(fd);
+		} else
+			printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
+		failed++;
+	}
+	if (failed)
+		return EXIT_FAILURE;
+	return EXIT_SUCCESS;
+}
-- 
2.7.4

[PATCH 2/4] src/open_by_handle: flexible usage options

[Amir Goldstein]

More usage options for testing open_by_handle, which are needed
for testing stable handles across copy up in overlayfs.

usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]

Examples:

1. Create N test files (nlink=1) under test_dir and exit:
 $ open_by_handle -c <test_dir> [N]

2. Get file handles, drop caches and try to open by handle
   (expects success):
 $ open_by_handle <test_dir> [N]

3. Get file handles, create hardlinks to test files (nlink=2),
   drop caches and try to open by handle (expects success):
 $ open_by_handle -l <test_dir> [N]

4. Get file handles, unlink test files w/o the hardlinks (nlink=1),
   drop caches and try to open by handle (expects success):
 $ open_by_handle -u <test_dir> [N]

5. Get file handles, unlink test files and hardlinks (nlink=0),
   drop caches and try to open by handle (expects failure):
 $ open_by_handle -d <test_dir> [N]

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

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 8f04865..c33a4aa 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -37,12 +37,12 @@
 #include <errno.h>
 #include <linux/limits.h>
 
-#define NUMFILES 1024
+#define MAXFILES 1024
 
 struct handle {
 	struct file_handle fh;
 	unsigned char fid[MAX_HANDLE_SZ];
-} handle[NUMFILES];
+} handle[MAXFILES];
 
 int main(int argc, char **argv)
 {
@@ -51,18 +51,60 @@ int main(int argc, char **argv)
 	int	ret;
 	int	failed = 0;
 	char	fname[PATH_MAX];
+	char	fname2[PATH_MAX];
 	char	*test_dir;
 	int	mount_fd, mount_id;
+	int	argi = 1, numfiles = 1;
+	int	create = 0, delete = 0, nlink = 1;
 
-	if (argc != 2) {
-		fprintf(stderr, "usage: open_by_handle <test_dir>\n");
+	if (argc < 2 || argc > 4) {
+usage:
+		fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
+		fprintf(stderr, "\n");
+		fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, get file handles and exit\n");
+		fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles, drop caches and try to open by handle (expects success)\n");
+		fprintf(stderr, "open_by_handle -l <test_dir> [N] - get file handles, create hardlinks to test files (nlink=2), drop caches and try to open by handle (expects success)\n");
+		fprintf(stderr, "open_by_handle -u <test_dir> [N] - get file handles, unlink test files w/o hardlinks (nlink=1), drop caches and try to open by handle (expects success)\n");
+		fprintf(stderr, "open_by_handle -d <test_dir> [N] - get file handles, unlink test files and hardlinks (nlink=0), drop caches and try to open by handle (expects failure)\n");
 		return EXIT_FAILURE;
 	}
 
-	test_dir = argv[1];
+	if (argv[1][0] == '-') {
+		if (argv[1][2])
+			goto usage;
+		switch (argv[1][1]) {
+		case 'c':
+			create = 1;
+			break;
+		case 'l':
+			nlink = 2;
+			break;
+		case 'u':
+			delete = 1;
+			nlink = 1;
+			break;
+		case 'd':
+			delete = 1;
+			nlink = 0;
+			break;
+		default:
+			fprintf(stderr, "illegal option '%s'\n", argv[1]);
+		case 'h':
+			goto usage;
+		}
+		argi++;
+	}
+	test_dir = argv[argi++];
+	if (argc > argi)
+		numfiles = atoi(argv[argi]);
+	if (!numfiles || numfiles > MAXFILES) {
+		fprintf(stderr, "illegal value '%s' for num_files\n", argv[argi]);
+		goto usage;
+	}
+
 	mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
 	if (mount_fd < 0) {
-		perror("open test_dir");
+		perror(test_dir);
 		return EXIT_FAILURE;
 	}
 
@@ -70,8 +112,9 @@ int main(int argc, char **argv)
 	 * create a large number of files to force allocation of new inode
 	 * chunks on disk.
 	 */
-	for (i=0; i < NUMFILES; i++) {
+	for (i=0; create && i < numfiles; i++) {
 		sprintf(fname, "%s/file%06d", test_dir, i);
+		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);
@@ -79,13 +122,14 @@ int main(int argc, char **argv)
 			return EXIT_FAILURE;
 		}
 		close(fd);
+		ret = unlink(fname2);
 	}
 
 	/* sync to get the new inodes to hit the disk */
 	sync();
 
 	/* create the handles */
-	for (i=0; i < NUMFILES; i++) {
+	for (i=0; i < numfiles; i++) {
 		sprintf(fname, "%s/file%06d", test_dir, i);
 		handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
 		ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
@@ -95,14 +139,32 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/* after creating test set only check that fs supports exportfs */
+	if (create)
+		return EXIT_SUCCESS;
+
+	/* hardlink the files */
+	for (i=0; nlink > 1 && i < numfiles; i++) {
+		sprintf(fname, "%s/file%06d", test_dir, i);
+		sprintf(fname2, "%s/link%06d", test_dir, i);
+		ret = link(fname, fname2);
+		if (ret < 0) {
+			perror("link");
+			return EXIT_FAILURE;
+		}
+	}
+
 	/* unlink the files */
-	for (i=0; i < NUMFILES; i++) {
+	for (i=0; delete && i < numfiles; i++) {
 		sprintf(fname, "%s/file%06d", test_dir, i);
+		sprintf(fname2, "%s/link%06d", test_dir, i);
 		ret = unlink(fname);
 		if (ret < 0) {
 			perror("unlink");
 			return EXIT_FAILURE;
 		}
+		if (!nlink)
+			ret = unlink(fname2);
 	}
 
 	/* sync to get log forced for unlink transactions to hit the disk */
@@ -126,17 +188,22 @@ int main(int argc, char **argv)
 	 * now try to open the files by the stored handles. Expecting ENOENT
 	 * for all of them.
 	 */
-	for (i=0; i < NUMFILES; i++) {
+	for (i=0; i < numfiles; i++) {
 		errno = 0;
 		fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
-		if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
+		if (nlink && fd >= 0) {
+			close(fd);
+			continue;
+		} else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
 			continue;
 		}
 		if (fd >= 0) {
 			printf("open_by_handle(%d) opened an unlinked file!\n", i);
 			close(fd);
-		} else
-			printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
+		} else {
+			printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
+					nlink ? "a linked" : "an unlinked");
+		}
 		failed++;
 	}
 	if (failed)
-- 
2.7.4

[PATCH 3/4] fstests: add helper _require_exportfs

[Amir Goldstein]

Use open_by_handle -c to determine if filesystem supports NFS export
---
 common/rc | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/common/rc b/common/rc
index 685b859..f24a21f 100644
--- a/common/rc
+++ b/common/rc
@@ -2857,6 +2857,15 @@ _require_freeze()
 	[ $result -eq 0 ] || _notrun "$FSTYP does not support freezing"
 }
 
+# Does NFS export work on this fs?
+_require_exportfs()
+{
+	mkdir -p "$TEST_DIR"/export_test
+	$here/src/open_by_handle -c "$TEST_DIR"/export_test 2>&1 \
+		|| _notrun "$FSTYP does not support NFS export"
+}
+
+
 # Does shutdown work on this fs?
 _require_scratch_shutdown()
 {
-- 
2.7.4

[PATCH 4/4] fstests: add generic test for file handles

[Amir Goldstein]

Cloned from xfs specific test xfs/238, which checks
stale file handles of deleted files.

This test uses the generic open_by_handle_at() syscall
and also tests for non-stale file handles of linked files.

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

diff --git a/tests/generic/426 b/tests/generic/426
new file mode 100755
index 0000000..62bb85e
--- /dev/null
+++ b/tests/generic/426
@@ -0,0 +1,73 @@
+#! /bin/bash
+# FS QA Test No. 426
+#
+# Check stale handles pointing to unlinked files
+# and non-stale handles pointing to linked files
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2016 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_scratch
+_require_exportfs
+
+echo "Silence is golden"
+
+_scratch_mkfs > /dev/null 2>&1
+_scratch_mount > /dev/null 2>&1
+
+numfiles=1024
+
+# Check stale handles to deleted files
+src/open_by_handle -c $SCRATCH_MNT $numfiles
+src/open_by_handle -d $SCRATCH_MNT $numfiles
+
+# Check non-stale handles to linked files
+src/open_by_handle -c $SCRATCH_MNT $numfiles
+src/open_by_handle    $SCRATCH_MNT $numfiles
+
+# Check non-stale handles to files that were hardlinked and original deleted
+src/open_by_handle -l $SCRATCH_MNT $numfiles
+src/open_by_handle -u $SCRATCH_MNT $numfiles
+
+status=$?
+exit
diff --git a/tests/generic/426.out b/tests/generic/426.out
new file mode 100644
index 0000000..777cbcd
--- /dev/null
+++ b/tests/generic/426.out
@@ -0,0 +1,2 @@
+QA output created by 426
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 6d6e4f6..f29009c 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -428,3 +428,4 @@
 423 auto quick
 424 auto quick
 425 auto quick attr
+426 auto quick exportfs
-- 
2.7.4

Re: [PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[J . Bruce Fields]

On Tue, Apr 18, 2017 at 09:17:21PM +0300, Amir Goldstein wrote:
> This is a clone of src/stale_handle.c test that uses generic
> open_by_handle_at() syscall instead of the xfs specific ioctl.
> 
> No test is using this helper yet.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/Makefile         |   2 +-
>  src/open_by_handle.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 146 insertions(+), 1 deletion(-)
>  create mode 100644 src/open_by_handle.c
> 
> diff --git a/src/Makefile b/src/Makefile
> index e62d7a9..6b38e77 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -22,7 +22,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
>  	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
>  	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
> -	dio-invalidate-cache stat_test
> +	dio-invalidate-cache stat_test open_by_handle
>  
>  SUBDIRS =
>  
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> new file mode 100644
> index 0000000..8f04865
> --- /dev/null
> +++ b/src/open_by_handle.c
> @@ -0,0 +1,145 @@
> +/*
> + * open_by_handle.c - attempt to create a file handle and open it
> + *                    with open_by_handle_at() syscall
> + *
> + * Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> + * Author: Amir Goldstein <amir73il@gmail.com>
> + * 
> + * from:
> + *  stale_handle.c
> + *
> + *  Copyright (C) 2010 Red Hat, Inc. All Rights reserved.
> + *
> + *  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; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will 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 to the Free Software
> + *  Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
> + */
> +
> +#define TEST_UTIME
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <errno.h>
> +#include <linux/limits.h>
> +
> +#define NUMFILES 1024
> +
> +struct handle {
> +	struct file_handle fh;
> +	unsigned char fid[MAX_HANDLE_SZ];
> +} handle[NUMFILES];
> +
> +int main(int argc, char **argv)
> +{
> +	int	i;
> +	int	fd;
> +	int	ret;
> +	int	failed = 0;
> +	char	fname[PATH_MAX];
> +	char	*test_dir;
> +	int	mount_fd, mount_id;
> +
> +	if (argc != 2) {
> +		fprintf(stderr, "usage: open_by_handle <test_dir>\n");
> +		return EXIT_FAILURE;
> +	}
> +
> +	test_dir = argv[1];
> +	mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
> +	if (mount_fd < 0) {
> +		perror("open test_dir");
> +		return EXIT_FAILURE;
> +	}
> +
> +	/*
> +	 * create a large number of files to force allocation of new inode
> +	 * chunks on disk.
> +	 */
> +	for (i=0; i < NUMFILES; i++) {
> +		sprintf(fname, "%s/file%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);
> +			perror(fname);
> +			return EXIT_FAILURE;
> +		}
> +		close(fd);
> +	}
> +
> +	/* sync to get the new inodes to hit the disk */
> +	sync();
> +
> +	/* create the handles */
> +	for (i=0; i < NUMFILES; i++) {
> +		sprintf(fname, "%s/file%06d", test_dir, i);
> +		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");
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
> +	/* unlink the files */
> +	for (i=0; i < NUMFILES; i++) {
> +		sprintf(fname, "%s/file%06d", test_dir, i);
> +		ret = unlink(fname);
> +		if (ret < 0) {
> +			perror("unlink");
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
> +	/* sync to get log forced for unlink transactions to hit the disk */
> +	sync();
> +
> +	/* sync once more FTW */
> +	sync();
> +
> +	/*
> +	 * now drop the caches so that unlinked inodes are reclaimed and
> +	 * buftarg page cache is emptied so that the inode cluster has to be
> +	 * fetched from disk again for the open_by_handle() call.
> +	 */
> +	ret = system("echo 3 > /proc/sys/vm/drop_caches");
> +	if (ret < 0) {
> +		perror("drop_caches");
> +		return EXIT_FAILURE;

By the way, I noticed some time ago in kernel commit 75a6f82a0d10 that
Al uses a remount for this instead of drop_caches; I wonder if that has
any advantages or if it just amounts to the same thing?

--b.

> +	}
> +
> +	/*
> +	 * now try to open the files by the stored handles. Expecting ENOENT
> +	 * for all of them.
> +	 */
> +	for (i=0; i < NUMFILES; i++) {
> +		errno = 0;
> +		fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
> +		if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
> +			continue;
> +		}
> +		if (fd >= 0) {
> +			printf("open_by_handle(%d) opened an unlinked file!\n", i);
> +			close(fd);
> +		} else
> +			printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
> +		failed++;
> +	}
> +	if (failed)
> +		return EXIT_FAILURE;
> +	return EXIT_SUCCESS;
> +}
> -- 
> 2.7.4

Re: [PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[Amir Goldstein]

On Tue, Apr 18, 2017 at 9:55 PM, J . Bruce Fields <bfields@fieldses.org> wrote:
> On Tue, Apr 18, 2017 at 09:17:21PM +0300, Amir Goldstein wrote:
>> This is a clone of src/stale_handle.c test that uses generic
>> open_by_handle_at() syscall instead of the xfs specific ioctl.
>>
>> No test is using this helper yet.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  src/Makefile         |   2 +-
>>  src/open_by_handle.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 146 insertions(+), 1 deletion(-)
>>  create mode 100644 src/open_by_handle.c
>>
>> diff --git a/src/Makefile b/src/Makefile
>> index e62d7a9..6b38e77 100644
>> --- a/src/Makefile
>> +++ b/src/Makefile
>> @@ -22,7 +22,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>>       seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
>>       renameat2 t_getcwd e4compact test-nextquota punch-alternating \
>>       attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
>> -     dio-invalidate-cache stat_test
>> +     dio-invalidate-cache stat_test open_by_handle
>>
>>  SUBDIRS =
>>
>> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
>> new file mode 100644
>> index 0000000..8f04865
>> --- /dev/null
>> +++ b/src/open_by_handle.c
>> @@ -0,0 +1,145 @@
>> +/*
>> + * open_by_handle.c - attempt to create a file handle and open it
>> + *                    with open_by_handle_at() syscall
>> + *
>> + * Copyright (C) 2017 CTERA Networks. All Rights Reserved.
>> + * Author: Amir Goldstein <amir73il@gmail.com>
>> + *
>> + * from:
>> + *  stale_handle.c
>> + *
>> + *  Copyright (C) 2010 Red Hat, Inc. All Rights reserved.
>> + *
>> + *  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; either version 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  This program is distributed in the hope that it will 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 to the Free Software
>> + *  Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
>> + */
>> +
>> +#define TEST_UTIME
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <fcntl.h>
>> +#include <unistd.h>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <errno.h>
>> +#include <linux/limits.h>
>> +
>> +#define NUMFILES 1024
>> +
>> +struct handle {
>> +     struct file_handle fh;
>> +     unsigned char fid[MAX_HANDLE_SZ];
>> +} handle[NUMFILES];
>> +
>> +int main(int argc, char **argv)
>> +{
>> +     int     i;
>> +     int     fd;
>> +     int     ret;
>> +     int     failed = 0;
>> +     char    fname[PATH_MAX];
>> +     char    *test_dir;
>> +     int     mount_fd, mount_id;
>> +
>> +     if (argc != 2) {
>> +             fprintf(stderr, "usage: open_by_handle <test_dir>\n");
>> +             return EXIT_FAILURE;
>> +     }
>> +
>> +     test_dir = argv[1];
>> +     mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
>> +     if (mount_fd < 0) {
>> +             perror("open test_dir");
>> +             return EXIT_FAILURE;
>> +     }
>> +
>> +     /*
>> +      * create a large number of files to force allocation of new inode
>> +      * chunks on disk.
>> +      */
>> +     for (i=0; i < NUMFILES; i++) {
>> +             sprintf(fname, "%s/file%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);
>> +                     perror(fname);
>> +                     return EXIT_FAILURE;
>> +             }
>> +             close(fd);
>> +     }
>> +
>> +     /* sync to get the new inodes to hit the disk */
>> +     sync();
>> +
>> +     /* create the handles */
>> +     for (i=0; i < NUMFILES; i++) {
>> +             sprintf(fname, "%s/file%06d", test_dir, i);
>> +             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");
>> +                     return EXIT_FAILURE;
>> +             }
>> +     }
>> +
>> +     /* unlink the files */
>> +     for (i=0; i < NUMFILES; i++) {
>> +             sprintf(fname, "%s/file%06d", test_dir, i);
>> +             ret = unlink(fname);
>> +             if (ret < 0) {
>> +                     perror("unlink");
>> +                     return EXIT_FAILURE;
>> +             }
>> +     }
>> +
>> +     /* sync to get log forced for unlink transactions to hit the disk */
>> +     sync();
>> +
>> +     /* sync once more FTW */
>> +     sync();
>> +
>> +     /*
>> +      * now drop the caches so that unlinked inodes are reclaimed and
>> +      * buftarg page cache is emptied so that the inode cluster has to be
>> +      * fetched from disk again for the open_by_handle() call.
>> +      */
>> +     ret = system("echo 3 > /proc/sys/vm/drop_caches");
>> +     if (ret < 0) {
>> +             perror("drop_caches");
>> +             return EXIT_FAILURE;
>
> By the way, I noticed some time ago in kernel commit 75a6f82a0d10 that
> Al uses a remount for this instead of drop_caches; I wonder if that has
> any advantages or if it just amounts to the same thing?
>

It is not the same thing. drop_caches is less powerful.
For example an inotify watch can keep an inode in cache, but umount
will remove that watch and evict the inode.
There are probably more examples.
In any case, after umount you *know* all caches are gone.
After drop caches you don't know that.

I plan to modify this test later to store handles into a dump file and read them
from the file later, so the drop_caches part will be done by the test script
that will also be able to do remount (as it should).

But for now, I just copied that test as it is from stale_handle.c, because
it is sufficient for my immediate needs.

BTW, I wanted to ask you guys, going forward with my implementation,
is there any NFS testsuite I should use to validate exporting overlayfs?

And another question for my curiosity -
Right now on my test branch, NFS handles work well as long as caches
are not dropped.
Am I right to assume that on normal workloads, handles are always
served from cache? meaning that NFS LOOKUP populates the
inode/dentry cache and all later decodes are served from cache?

Is it useful to have this basic support for these 'normal' workloads
without having full support (you get ESTALE if inode is not in cache)?

>
>> +     }
>> +
>> +     /*
>> +      * now try to open the files by the stored handles. Expecting ENOENT
>> +      * for all of them.
>> +      */
>> +     for (i=0; i < NUMFILES; i++) {
>> +             errno = 0;
>> +             fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
>> +             if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>> +                     continue;
>> +             }
>> +             if (fd >= 0) {
>> +                     printf("open_by_handle(%d) opened an unlinked file!\n", i);
>> +                     close(fd);
>> +             } else
>> +                     printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
>> +             failed++;
>> +     }
>> +     if (failed)
>> +             return EXIT_FAILURE;
>> +     return EXIT_SUCCESS;
>> +}
>> --
>> 2.7.4

Re: [PATCH 2/4] src/open_by_handle: flexible usage options

[J . Bruce Fields]

On Tue, Apr 18, 2017 at 09:17:22PM +0300, Amir Goldstein wrote:
> More usage options for testing open_by_handle, which are needed
> for testing stable handles across copy up in overlayfs.
> 
> usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
> 
> Examples:
> 
> 1. Create N test files (nlink=1) under test_dir and exit:
>  $ open_by_handle -c <test_dir> [N]
> 
> 2. Get file handles, drop caches and try to open by handle
>    (expects success):
>  $ open_by_handle <test_dir> [N]
> 
> 3. Get file handles, create hardlinks to test files (nlink=2),
>    drop caches and try to open by handle (expects success):
>  $ open_by_handle -l <test_dir> [N]

I guess I don't see the point of this particular test case.  It might be
more interesting if it then unlinked the original names, to test for the
case when a filesystem depends on the filename or parent directory being
encoded in the filehandle?

Or maybe you are doing that in one of these tests, I'm not sure I follow
the descriptions.

--b.

> 4. Get file handles, unlink test files w/o the hardlinks (nlink=1),
>    drop caches and try to open by handle (expects success):
>  $ open_by_handle -u <test_dir> [N]
> 
> 5. Get file handles, unlink test files and hardlinks (nlink=0),
>    drop caches and try to open by handle (expects failure):
>  $ open_by_handle -d <test_dir> [N]
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/open_by_handle.c | 93 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 8f04865..c33a4aa 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -37,12 +37,12 @@
>  #include <errno.h>
>  #include <linux/limits.h>
>  
> -#define NUMFILES 1024
> +#define MAXFILES 1024
>  
>  struct handle {
>  	struct file_handle fh;
>  	unsigned char fid[MAX_HANDLE_SZ];
> -} handle[NUMFILES];
> +} handle[MAXFILES];
>  
>  int main(int argc, char **argv)
>  {
> @@ -51,18 +51,60 @@ int main(int argc, char **argv)
>  	int	ret;
>  	int	failed = 0;
>  	char	fname[PATH_MAX];
> +	char	fname2[PATH_MAX];
>  	char	*test_dir;
>  	int	mount_fd, mount_id;
> +	int	argi = 1, numfiles = 1;
> +	int	create = 0, delete = 0, nlink = 1;
>  
> -	if (argc != 2) {
> -		fprintf(stderr, "usage: open_by_handle <test_dir>\n");
> +	if (argc < 2 || argc > 4) {
> +usage:
> +		fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
> +		fprintf(stderr, "\n");
> +		fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, get file handles and exit\n");
> +		fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles, drop caches and try to open by handle (expects success)\n");
> +		fprintf(stderr, "open_by_handle -l <test_dir> [N] - get file handles, create hardlinks to test files (nlink=2), drop caches and try to open by handle (expects success)\n");
> +		fprintf(stderr, "open_by_handle -u <test_dir> [N] - get file handles, unlink test files w/o hardlinks (nlink=1), drop caches and try to open by handle (expects success)\n");
> +		fprintf(stderr, "open_by_handle -d <test_dir> [N] - get file handles, unlink test files and hardlinks (nlink=0), drop caches and try to open by handle (expects failure)\n");
>  		return EXIT_FAILURE;
>  	}
>  
> -	test_dir = argv[1];
> +	if (argv[1][0] == '-') {
> +		if (argv[1][2])
> +			goto usage;
> +		switch (argv[1][1]) {
> +		case 'c':
> +			create = 1;
> +			break;
> +		case 'l':
> +			nlink = 2;
> +			break;
> +		case 'u':
> +			delete = 1;
> +			nlink = 1;
> +			break;
> +		case 'd':
> +			delete = 1;
> +			nlink = 0;
> +			break;
> +		default:
> +			fprintf(stderr, "illegal option '%s'\n", argv[1]);
> +		case 'h':
> +			goto usage;
> +		}
> +		argi++;
> +	}
> +	test_dir = argv[argi++];
> +	if (argc > argi)
> +		numfiles = atoi(argv[argi]);
> +	if (!numfiles || numfiles > MAXFILES) {
> +		fprintf(stderr, "illegal value '%s' for num_files\n", argv[argi]);
> +		goto usage;
> +	}
> +
>  	mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
>  	if (mount_fd < 0) {
> -		perror("open test_dir");
> +		perror(test_dir);
>  		return EXIT_FAILURE;
>  	}
>  
> @@ -70,8 +112,9 @@ int main(int argc, char **argv)
>  	 * create a large number of files to force allocation of new inode
>  	 * chunks on disk.
>  	 */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; create && i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
> +		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);
> @@ -79,13 +122,14 @@ int main(int argc, char **argv)
>  			return EXIT_FAILURE;
>  		}
>  		close(fd);
> +		ret = unlink(fname2);
>  	}
>  
>  	/* sync to get the new inodes to hit the disk */
>  	sync();
>  
>  	/* create the handles */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
>  		handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
>  		ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> @@ -95,14 +139,32 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> +	/* after creating test set only check that fs supports exportfs */
> +	if (create)
> +		return EXIT_SUCCESS;
> +
> +	/* hardlink the files */
> +	for (i=0; nlink > 1 && i < numfiles; i++) {
> +		sprintf(fname, "%s/file%06d", test_dir, i);
> +		sprintf(fname2, "%s/link%06d", test_dir, i);
> +		ret = link(fname, fname2);
> +		if (ret < 0) {
> +			perror("link");
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
>  	/* unlink the files */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; delete && i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
> +		sprintf(fname2, "%s/link%06d", test_dir, i);
>  		ret = unlink(fname);
>  		if (ret < 0) {
>  			perror("unlink");
>  			return EXIT_FAILURE;
>  		}
> +		if (!nlink)
> +			ret = unlink(fname2);
>  	}
>  
>  	/* sync to get log forced for unlink transactions to hit the disk */
> @@ -126,17 +188,22 @@ int main(int argc, char **argv)
>  	 * now try to open the files by the stored handles. Expecting ENOENT
>  	 * for all of them.
>  	 */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; i < numfiles; i++) {
>  		errno = 0;
>  		fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
> -		if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
> +		if (nlink && fd >= 0) {
> +			close(fd);
> +			continue;
> +		} else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>  			continue;
>  		}
>  		if (fd >= 0) {
>  			printf("open_by_handle(%d) opened an unlinked file!\n", i);
>  			close(fd);
> -		} else
> -			printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
> +		} else {
> +			printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
> +					nlink ? "a linked" : "an unlinked");
> +		}
>  		failed++;
>  	}
>  	if (failed)
> -- 
> 2.7.4

Re: [PATCH 2/4] src/open_by_handle: flexible usage options

[Amir Goldstein]

On Tue, Apr 18, 2017 at 10:14 PM, J . Bruce Fields <bfields@fieldses.org> wrote:
> On Tue, Apr 18, 2017 at 09:17:22PM +0300, Amir Goldstein wrote:
>> More usage options for testing open_by_handle, which are needed
>> for testing stable handles across copy up in overlayfs.
>>
>> usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
>>
>> Examples:
>>
>> 1. Create N test files (nlink=1) under test_dir and exit:
>>  $ open_by_handle -c <test_dir> [N]
>>
>> 2. Get file handles, drop caches and try to open by handle
>>    (expects success):
>>  $ open_by_handle <test_dir> [N]
>>
>> 3. Get file handles, create hardlinks to test files (nlink=2),
>>    drop caches and try to open by handle (expects success):
>>  $ open_by_handle -l <test_dir> [N]
>
> I guess I don't see the point of this particular test case.  It might be
> more interesting if it then unlinked the original names, to test for the
> case when a filesystem depends on the filename or parent directory being
> encoded in the filehandle?
>
> Or maybe you are doing that in one of these tests, I'm not sure I follow
> the descriptions.
>

I am doing that, but with 2 separate invocations:
open_by_handle -l <test_dir> [N]
open_by_handle -u <test_dir> [N]

-u differs from -d because it expects the handle to be opened.

The reason I separated -l (link) and -u (unlink) is because
for overlayfs I need to test linking in lower layer and unlinking
from overlay.

I guess my intentions will become more clear after I post some
overlay specific tests.
It's kind of weird for me to post these tests when overlay
doesn't have exportfs support yet, but I may do it anyway.


>
>> 4. Get file handles, unlink test files w/o the hardlinks (nlink=1),
>>    drop caches and try to open by handle (expects success):
>>  $ open_by_handle -u <test_dir> [N]
>>
>> 5. Get file handles, unlink test files and hardlinks (nlink=0),
>>    drop caches and try to open by handle (expects failure):
>>  $ open_by_handle -d <test_dir> [N]
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  src/open_by_handle.c | 93 ++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 80 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
>> index 8f04865..c33a4aa 100644
>> --- a/src/open_by_handle.c
>> +++ b/src/open_by_handle.c
>> @@ -37,12 +37,12 @@
>>  #include <errno.h>
>>  #include <linux/limits.h>
>>
>> -#define NUMFILES 1024
>> +#define MAXFILES 1024
>>
>>  struct handle {
>>       struct file_handle fh;
>>       unsigned char fid[MAX_HANDLE_SZ];
>> -} handle[NUMFILES];
>> +} handle[MAXFILES];
>>
>>  int main(int argc, char **argv)
>>  {
>> @@ -51,18 +51,60 @@ int main(int argc, char **argv)
>>       int     ret;
>>       int     failed = 0;
>>       char    fname[PATH_MAX];
>> +     char    fname2[PATH_MAX];
>>       char    *test_dir;
>>       int     mount_fd, mount_id;
>> +     int     argi = 1, numfiles = 1;
>> +     int     create = 0, delete = 0, nlink = 1;
>>
>> -     if (argc != 2) {
>> -             fprintf(stderr, "usage: open_by_handle <test_dir>\n");
>> +     if (argc < 2 || argc > 4) {
>> +usage:
>> +             fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
>> +             fprintf(stderr, "\n");
>> +             fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, get file handles and exit\n");
>> +             fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles, drop caches and try to open by handle (expects success)\n");
>> +             fprintf(stderr, "open_by_handle -l <test_dir> [N] - get file handles, create hardlinks to test files (nlink=2), drop caches and try to open by handle (expects success)\n");
>> +             fprintf(stderr, "open_by_handle -u <test_dir> [N] - get file handles, unlink test files w/o hardlinks (nlink=1), drop caches and try to open by handle (expects success)\n");
>> +             fprintf(stderr, "open_by_handle -d <test_dir> [N] - get file handles, unlink test files and hardlinks (nlink=0), drop caches and try to open by handle (expects failure)\n");
>>               return EXIT_FAILURE;
>>       }
>>
>> -     test_dir = argv[1];
>> +     if (argv[1][0] == '-') {
>> +             if (argv[1][2])
>> +                     goto usage;
>> +             switch (argv[1][1]) {
>> +             case 'c':
>> +                     create = 1;
>> +                     break;
>> +             case 'l':
>> +                     nlink = 2;
>> +                     break;
>> +             case 'u':
>> +                     delete = 1;
>> +                     nlink = 1;
>> +                     break;
>> +             case 'd':
>> +                     delete = 1;
>> +                     nlink = 0;
>> +                     break;
>> +             default:
>> +                     fprintf(stderr, "illegal option '%s'\n", argv[1]);
>> +             case 'h':
>> +                     goto usage;
>> +             }
>> +             argi++;
>> +     }
>> +     test_dir = argv[argi++];
>> +     if (argc > argi)
>> +             numfiles = atoi(argv[argi]);
>> +     if (!numfiles || numfiles > MAXFILES) {
>> +             fprintf(stderr, "illegal value '%s' for num_files\n", argv[argi]);
>> +             goto usage;
>> +     }
>> +
>>       mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
>>       if (mount_fd < 0) {
>> -             perror("open test_dir");
>> +             perror(test_dir);
>>               return EXIT_FAILURE;
>>       }
>>
>> @@ -70,8 +112,9 @@ int main(int argc, char **argv)
>>        * create a large number of files to force allocation of new inode
>>        * chunks on disk.
>>        */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; create && i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>> +             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);
>> @@ -79,13 +122,14 @@ int main(int argc, char **argv)
>>                       return EXIT_FAILURE;
>>               }
>>               close(fd);
>> +             ret = unlink(fname2);
>>       }
>>
>>       /* sync to get the new inodes to hit the disk */
>>       sync();
>>
>>       /* create the handles */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>>               handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
>>               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
>> @@ -95,14 +139,32 @@ int main(int argc, char **argv)
>>               }
>>       }
>>
>> +     /* after creating test set only check that fs supports exportfs */
>> +     if (create)
>> +             return EXIT_SUCCESS;
>> +
>> +     /* hardlink the files */
>> +     for (i=0; nlink > 1 && i < numfiles; i++) {
>> +             sprintf(fname, "%s/file%06d", test_dir, i);
>> +             sprintf(fname2, "%s/link%06d", test_dir, i);
>> +             ret = link(fname, fname2);
>> +             if (ret < 0) {
>> +                     perror("link");
>> +                     return EXIT_FAILURE;
>> +             }
>> +     }
>> +
>>       /* unlink the files */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; delete && i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>> +             sprintf(fname2, "%s/link%06d", test_dir, i);
>>               ret = unlink(fname);
>>               if (ret < 0) {
>>                       perror("unlink");
>>                       return EXIT_FAILURE;
>>               }
>> +             if (!nlink)
>> +                     ret = unlink(fname2);
>>       }
>>
>>       /* sync to get log forced for unlink transactions to hit the disk */
>> @@ -126,17 +188,22 @@ int main(int argc, char **argv)
>>        * now try to open the files by the stored handles. Expecting ENOENT
>>        * for all of them.
>>        */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; i < numfiles; i++) {
>>               errno = 0;
>>               fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
>> -             if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>> +             if (nlink && fd >= 0) {
>> +                     close(fd);
>> +                     continue;
>> +             } else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>>                       continue;
>>               }
>>               if (fd >= 0) {
>>                       printf("open_by_handle(%d) opened an unlinked file!\n", i);
>>                       close(fd);
>> -             } else
>> -                     printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
>> +             } else {
>> +                     printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
>> +                                     nlink ? "a linked" : "an unlinked");
>> +             }
>>               failed++;
>>       }
>>       if (failed)
>> --
>> 2.7.4

Re: [PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[J . Bruce Fields]

On Tue, Apr 18, 2017 at 10:13:14PM +0300, Amir Goldstein wrote:
> On Tue, Apr 18, 2017 at 9:55 PM, J . Bruce Fields <bfields@fieldses.org> wrote:
> > On Tue, Apr 18, 2017 at 09:17:21PM +0300, Amir Goldstein wrote:
> >> This is a clone of src/stale_handle.c test that uses generic
> >> open_by_handle_at() syscall instead of the xfs specific ioctl.
> >>
> >> No test is using this helper yet.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  src/Makefile         |   2 +-
> >>  src/open_by_handle.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 146 insertions(+), 1 deletion(-)
> >>  create mode 100644 src/open_by_handle.c
> >>
> >> diff --git a/src/Makefile b/src/Makefile
> >> index e62d7a9..6b38e77 100644
> >> --- a/src/Makefile
> >> +++ b/src/Makefile
> >> @@ -22,7 +22,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> >>       seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
> >>       renameat2 t_getcwd e4compact test-nextquota punch-alternating \
> >>       attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
> >> -     dio-invalidate-cache stat_test
> >> +     dio-invalidate-cache stat_test open_by_handle
> >>
> >>  SUBDIRS =
> >>
> >> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> >> new file mode 100644
> >> index 0000000..8f04865
> >> --- /dev/null
> >> +++ b/src/open_by_handle.c
> >> @@ -0,0 +1,145 @@
> >> +/*
> >> + * open_by_handle.c - attempt to create a file handle and open it
> >> + *                    with open_by_handle_at() syscall
> >> + *
> >> + * Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> >> + * Author: Amir Goldstein <amir73il@gmail.com>
> >> + *
> >> + * from:
> >> + *  stale_handle.c
> >> + *
> >> + *  Copyright (C) 2010 Red Hat, Inc. All Rights reserved.
> >> + *
> >> + *  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; either version 2 of the License, or
> >> + *  (at your option) any later version.
> >> + *
> >> + *  This program is distributed in the hope that it will 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 to the Free Software
> >> + *  Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
> >> + */
> >> +
> >> +#define TEST_UTIME
> >> +
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <string.h>
> >> +#include <fcntl.h>
> >> +#include <unistd.h>
> >> +#include <sys/stat.h>
> >> +#include <sys/types.h>
> >> +#include <errno.h>
> >> +#include <linux/limits.h>
> >> +
> >> +#define NUMFILES 1024
> >> +
> >> +struct handle {
> >> +     struct file_handle fh;
> >> +     unsigned char fid[MAX_HANDLE_SZ];
> >> +} handle[NUMFILES];
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> +     int     i;
> >> +     int     fd;
> >> +     int     ret;
> >> +     int     failed = 0;
> >> +     char    fname[PATH_MAX];
> >> +     char    *test_dir;
> >> +     int     mount_fd, mount_id;
> >> +
> >> +     if (argc != 2) {
> >> +             fprintf(stderr, "usage: open_by_handle <test_dir>\n");
> >> +             return EXIT_FAILURE;
> >> +     }
> >> +
> >> +     test_dir = argv[1];
> >> +     mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
> >> +     if (mount_fd < 0) {
> >> +             perror("open test_dir");
> >> +             return EXIT_FAILURE;
> >> +     }
> >> +
> >> +     /*
> >> +      * create a large number of files to force allocation of new inode
> >> +      * chunks on disk.
> >> +      */
> >> +     for (i=0; i < NUMFILES; i++) {
> >> +             sprintf(fname, "%s/file%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);
> >> +                     perror(fname);
> >> +                     return EXIT_FAILURE;
> >> +             }
> >> +             close(fd);
> >> +     }
> >> +
> >> +     /* sync to get the new inodes to hit the disk */
> >> +     sync();
> >> +
> >> +     /* create the handles */
> >> +     for (i=0; i < NUMFILES; i++) {
> >> +             sprintf(fname, "%s/file%06d", test_dir, i);
> >> +             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");
> >> +                     return EXIT_FAILURE;
> >> +             }
> >> +     }
> >> +
> >> +     /* unlink the files */
> >> +     for (i=0; i < NUMFILES; i++) {
> >> +             sprintf(fname, "%s/file%06d", test_dir, i);
> >> +             ret = unlink(fname);
> >> +             if (ret < 0) {
> >> +                     perror("unlink");
> >> +                     return EXIT_FAILURE;
> >> +             }
> >> +     }
> >> +
> >> +     /* sync to get log forced for unlink transactions to hit the disk */
> >> +     sync();
> >> +
> >> +     /* sync once more FTW */
> >> +     sync();
> >> +
> >> +     /*
> >> +      * now drop the caches so that unlinked inodes are reclaimed and
> >> +      * buftarg page cache is emptied so that the inode cluster has to be
> >> +      * fetched from disk again for the open_by_handle() call.
> >> +      */
> >> +     ret = system("echo 3 > /proc/sys/vm/drop_caches");
> >> +     if (ret < 0) {
> >> +             perror("drop_caches");
> >> +             return EXIT_FAILURE;
> >
> > By the way, I noticed some time ago in kernel commit 75a6f82a0d10 that
> > Al uses a remount for this instead of drop_caches; I wonder if that has
> > any advantages or if it just amounts to the same thing?
> >
> 
> It is not the same thing. drop_caches is less powerful.
> For example an inotify watch can keep an inode in cache, but umount
> will remove that watch and evict the inode.
> There are probably more examples.
> In any case, after umount you *know* all caches are gone.
> After drop caches you don't know that.

OK. Yes, I ran into that drop_caches behavior when trying to do some
similar testing--I had to create a lot of extra entries if I wanted to
be sure some of them would be evicted.  So maybe the remount trick would
make it easier to get reproduceable results.

(By the way, the particular odd test I was doing was in the
deep-fh-lookup script in:

	git://linux-nfs.org/~bfields/fragments.git

It tries to stress the code that reconnects a dentry to its parents
after it's looked up by filehandle, so it creates a very deep directory
hierarchy, gets the filehandle, drops caches, then tries to look up by
filehandle.  I meant to add that to xfstests and never got around to
it.)

> I plan to modify this test later to store handles into a dump file and read them
> from the file later, so the drop_caches part will be done by the test script
> that will also be able to do remount (as it should).
> 
> But for now, I just copied that test as it is from stale_handle.c, because
> it is sufficient for my immediate needs.
> 
> BTW, I wanted to ask you guys, going forward with my implementation,
> is there any NFS testsuite I should use to validate exporting overlayfs?

You can use xfstests, actually, there's a "check -nfs" option.  We also
use connectathon and pynfs.  I have some crufty scripts to run those at
git://linux-nfs.org/~bfields/testd.git.  You probably don't want to use
those scripts, but maybe the commandlines under
bin/do-{xfstests,pynfs,cthon} might be useful.

> And another question for my curiosity -
> Right now on my test branch, NFS handles work well as long as caches
> are not dropped.
> Am I right to assume that on normal workloads, handles are always
> served from cache? meaning that NFS LOOKUP populates the
> inode/dentry cache and all later decodes are served from cache?
 
Ugh.  I seem to recall filesystems that depended on that, or used to;
maybe fat?  Looking at git.... Yes, see 21b6633d516c.  I guess you could
read that changelog and/or google for any mailing-list mentions of that
work to see if they had any particular motivation.

Note also with NFS it's normally expected that you can reboot the server
without consequences worse than applications hanging during the reboot.

> Is it useful to have this basic support for these 'normal' workloads
> without having full support (you get ESTALE if inode is not in cache)?

I'd personally be terrified to depend on inodes staying in cache, and
certainly wouldn't want to be put in the position of having to support
such behavior.  But I don't have any actual data on how often it would
fail.

--b.

> >> +     }
> >> +
> >> +     /*
> >> +      * now try to open the files by the stored handles. Expecting ENOENT
> >> +      * for all of them.
> >> +      */
> >> +     for (i=0; i < NUMFILES; i++) {
> >> +             errno = 0;
> >> +             fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
> >> +             if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
> >> +                     continue;
> >> +             }
> >> +             if (fd >= 0) {
> >> +                     printf("open_by_handle(%d) opened an unlinked file!\n", i);
> >> +                     close(fd);
> >> +             } else
> >> +                     printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
> >> +             failed++;
> >> +     }
> >> +     if (failed)
> >> +             return EXIT_FAILURE;
> >> +     return EXIT_SUCCESS;
> >> +}
> >> --
> >> 2.7.4

Re: [PATCH 2/4] src/open_by_handle: flexible usage options

[J . Bruce Fields]

On Tue, Apr 18, 2017 at 10:22:20PM +0300, Amir Goldstein wrote:
> On Tue, Apr 18, 2017 at 10:14 PM, J . Bruce Fields <bfields@fieldses.org> wrote:
> > On Tue, Apr 18, 2017 at 09:17:22PM +0300, Amir Goldstein wrote:
> >> More usage options for testing open_by_handle, which are needed
> >> for testing stable handles across copy up in overlayfs.
> >>
> >> usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
> >>
> >> Examples:
> >>
> >> 1. Create N test files (nlink=1) under test_dir and exit:
> >>  $ open_by_handle -c <test_dir> [N]
> >>
> >> 2. Get file handles, drop caches and try to open by handle
> >>    (expects success):
> >>  $ open_by_handle <test_dir> [N]
> >>
> >> 3. Get file handles, create hardlinks to test files (nlink=2),
> >>    drop caches and try to open by handle (expects success):
> >>  $ open_by_handle -l <test_dir> [N]
> >
> > I guess I don't see the point of this particular test case.  It might be
> > more interesting if it then unlinked the original names, to test for the
> > case when a filesystem depends on the filename or parent directory being
> > encoded in the filehandle?
> >
> > Or maybe you are doing that in one of these tests, I'm not sure I follow
> > the descriptions.
> >
> 
> I am doing that, but with 2 separate invocations:
> open_by_handle -l <test_dir> [N]
> open_by_handle -u <test_dir> [N]

Oh, got it, I forgot these were building blocks for tests and not tests
in themselves....

> -u differs from -d because it expects the handle to be opened.
> 
> The reason I separated -l (link) and -u (unlink) is because
> for overlayfs I need to test linking in lower layer and unlinking
> from overlay.
> 
> I guess my intentions will become more clear after I post some
> overlay specific tests.
> It's kind of weird for me to post these tests when overlay
> doesn't have exportfs support yet, but I may do it anyway.

I don't think the filehandle lookup code has very good test coverage, so
it's probably useful to have anyway, thanks for working on this.

--b.

> >> 4. Get file handles, unlink test files w/o the hardlinks (nlink=1),
> >>    drop caches and try to open by handle (expects success):
> >>  $ open_by_handle -u <test_dir> [N]
> >>
> >> 5. Get file handles, unlink test files and hardlinks (nlink=0),
> >>    drop caches and try to open by handle (expects failure):
> >>  $ open_by_handle -d <test_dir> [N]
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  src/open_by_handle.c | 93 ++++++++++++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 80 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> >> index 8f04865..c33a4aa 100644
> >> --- a/src/open_by_handle.c
> >> +++ b/src/open_by_handle.c
> >> @@ -37,12 +37,12 @@
> >>  #include <errno.h>
> >>  #include <linux/limits.h>
> >>
> >> -#define NUMFILES 1024
> >> +#define MAXFILES 1024
> >>
> >>  struct handle {
> >>       struct file_handle fh;
> >>       unsigned char fid[MAX_HANDLE_SZ];
> >> -} handle[NUMFILES];
> >> +} handle[MAXFILES];
> >>
> >>  int main(int argc, char **argv)
> >>  {
> >> @@ -51,18 +51,60 @@ int main(int argc, char **argv)
> >>       int     ret;
> >>       int     failed = 0;
> >>       char    fname[PATH_MAX];
> >> +     char    fname2[PATH_MAX];
> >>       char    *test_dir;
> >>       int     mount_fd, mount_id;
> >> +     int     argi = 1, numfiles = 1;
> >> +     int     create = 0, delete = 0, nlink = 1;
> >>
> >> -     if (argc != 2) {
> >> -             fprintf(stderr, "usage: open_by_handle <test_dir>\n");
> >> +     if (argc < 2 || argc > 4) {
> >> +usage:
> >> +             fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
> >> +             fprintf(stderr, "\n");
> >> +             fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, get file handles and exit\n");
> >> +             fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles, drop caches and try to open by handle (expects success)\n");
> >> +             fprintf(stderr, "open_by_handle -l <test_dir> [N] - get file handles, create hardlinks to test files (nlink=2), drop caches and try to open by handle (expects success)\n");
> >> +             fprintf(stderr, "open_by_handle -u <test_dir> [N] - get file handles, unlink test files w/o hardlinks (nlink=1), drop caches and try to open by handle (expects success)\n");
> >> +             fprintf(stderr, "open_by_handle -d <test_dir> [N] - get file handles, unlink test files and hardlinks (nlink=0), drop caches and try to open by handle (expects failure)\n");
> >>               return EXIT_FAILURE;
> >>       }
> >>
> >> -     test_dir = argv[1];
> >> +     if (argv[1][0] == '-') {
> >> +             if (argv[1][2])
> >> +                     goto usage;
> >> +             switch (argv[1][1]) {
> >> +             case 'c':
> >> +                     create = 1;
> >> +                     break;
> >> +             case 'l':
> >> +                     nlink = 2;
> >> +                     break;
> >> +             case 'u':
> >> +                     delete = 1;
> >> +                     nlink = 1;
> >> +                     break;
> >> +             case 'd':
> >> +                     delete = 1;
> >> +                     nlink = 0;
> >> +                     break;
> >> +             default:
> >> +                     fprintf(stderr, "illegal option '%s'\n", argv[1]);
> >> +             case 'h':
> >> +                     goto usage;
> >> +             }
> >> +             argi++;
> >> +     }
> >> +     test_dir = argv[argi++];
> >> +     if (argc > argi)
> >> +             numfiles = atoi(argv[argi]);
> >> +     if (!numfiles || numfiles > MAXFILES) {
> >> +             fprintf(stderr, "illegal value '%s' for num_files\n", argv[argi]);
> >> +             goto usage;
> >> +     }
> >> +
> >>       mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
> >>       if (mount_fd < 0) {
> >> -             perror("open test_dir");
> >> +             perror(test_dir);
> >>               return EXIT_FAILURE;
> >>       }
> >>
> >> @@ -70,8 +112,9 @@ int main(int argc, char **argv)
> >>        * create a large number of files to force allocation of new inode
> >>        * chunks on disk.
> >>        */
> >> -     for (i=0; i < NUMFILES; i++) {
> >> +     for (i=0; create && i < numfiles; i++) {
> >>               sprintf(fname, "%s/file%06d", test_dir, i);
> >> +             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);
> >> @@ -79,13 +122,14 @@ int main(int argc, char **argv)
> >>                       return EXIT_FAILURE;
> >>               }
> >>               close(fd);
> >> +             ret = unlink(fname2);
> >>       }
> >>
> >>       /* sync to get the new inodes to hit the disk */
> >>       sync();
> >>
> >>       /* create the handles */
> >> -     for (i=0; i < NUMFILES; i++) {
> >> +     for (i=0; i < numfiles; i++) {
> >>               sprintf(fname, "%s/file%06d", test_dir, i);
> >>               handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
> >>               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> >> @@ -95,14 +139,32 @@ int main(int argc, char **argv)
> >>               }
> >>       }
> >>
> >> +     /* after creating test set only check that fs supports exportfs */
> >> +     if (create)
> >> +             return EXIT_SUCCESS;
> >> +
> >> +     /* hardlink the files */
> >> +     for (i=0; nlink > 1 && i < numfiles; i++) {
> >> +             sprintf(fname, "%s/file%06d", test_dir, i);
> >> +             sprintf(fname2, "%s/link%06d", test_dir, i);
> >> +             ret = link(fname, fname2);
> >> +             if (ret < 0) {
> >> +                     perror("link");
> >> +                     return EXIT_FAILURE;
> >> +             }
> >> +     }
> >> +
> >>       /* unlink the files */
> >> -     for (i=0; i < NUMFILES; i++) {
> >> +     for (i=0; delete && i < numfiles; i++) {
> >>               sprintf(fname, "%s/file%06d", test_dir, i);
> >> +             sprintf(fname2, "%s/link%06d", test_dir, i);
> >>               ret = unlink(fname);
> >>               if (ret < 0) {
> >>                       perror("unlink");
> >>                       return EXIT_FAILURE;
> >>               }
> >> +             if (!nlink)
> >> +                     ret = unlink(fname2);
> >>       }
> >>
> >>       /* sync to get log forced for unlink transactions to hit the disk */
> >> @@ -126,17 +188,22 @@ int main(int argc, char **argv)
> >>        * now try to open the files by the stored handles. Expecting ENOENT
> >>        * for all of them.
> >>        */
> >> -     for (i=0; i < NUMFILES; i++) {
> >> +     for (i=0; i < numfiles; i++) {
> >>               errno = 0;
> >>               fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
> >> -             if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
> >> +             if (nlink && fd >= 0) {
> >> +                     close(fd);
> >> +                     continue;
> >> +             } else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
> >>                       continue;
> >>               }
> >>               if (fd >= 0) {
> >>                       printf("open_by_handle(%d) opened an unlinked file!\n", i);
> >>                       close(fd);
> >> -             } else
> >> -                     printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
> >> +             } else {
> >> +                     printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
> >> +                                     nlink ? "a linked" : "an unlinked");
> >> +             }
> >>               failed++;
> >>       }
> >>       if (failed)
> >> --
> >> 2.7.4

Re: [PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[Eryu Guan]

On Tue, Apr 18, 2017 at 09:17:21PM +0300, Amir Goldstein wrote:
> This is a clone of src/stale_handle.c test that uses generic
> open_by_handle_at() syscall instead of the xfs specific ioctl.
> 
> No test is using this helper yet.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/Makefile         |   2 +-
>  src/open_by_handle.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 146 insertions(+), 1 deletion(-)
>  create mode 100644 src/open_by_handle.c
> 
> diff --git a/src/Makefile b/src/Makefile
> index e62d7a9..6b38e77 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -22,7 +22,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
>  	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
>  	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
> -	dio-invalidate-cache stat_test
> +	dio-invalidate-cache stat_test open_by_handle

Need an entry in .gitignore file too.

>  
>  SUBDIRS =
>  
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> new file mode 100644
> index 0000000..8f04865
> --- /dev/null
> +++ b/src/open_by_handle.c
> @@ -0,0 +1,145 @@
> +/*
> + * open_by_handle.c - attempt to create a file handle and open it
> + *                    with open_by_handle_at() syscall
> + *
> + * Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> + * Author: Amir Goldstein <amir73il@gmail.com>
> + * 

Trailing whitespace in above line.

> + * from:
> + *  stale_handle.c
> + *
> + *  Copyright (C) 2010 Red Hat, Inc. All Rights reserved.
> + *
> + *  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; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will 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 to the Free Software
> + *  Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
> + */
> +
> +#define TEST_UTIME

This is not used, remove it? (I think it's not used in the original
stale_handle.c too).

> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <errno.h>
> +#include <linux/limits.h>
> +
> +#define NUMFILES 1024
> +
> +struct handle {
> +	struct file_handle fh;
> +	unsigned char fid[MAX_HANDLE_SZ];
> +} handle[NUMFILES];

>From open_by_handle_at(2):

"These system calls first appeared in Linux 2.6.39.  Library support is
provided in glibc since version 2.14."

I think we should check if the system supports open_by_handle_at(2)
syscalls at build time and _require_test_program "open_by_handle" in
tests in case it's not built.

Currently it fails to build on my RHEL6 host:
Building src
    [CC]    open_by_handle
open_by_handle.c:43: error: field 'fh' has incomplete type
open_by_handle.c:44: error: 'MAX_HANDLE_SZ' undeclared here (not in a function)
open_by_handle.c: In function 'main':
open_by_handle.c:135: warning: implicit declaration of function 'name_to_handle_at'
open_by_handle.c:193: warning: implicit declaration of function 'open_by_handle_at'
gmake[2]: *** [open_by_handle] Error 1
gmake[1]: *** [src] Error 2
make: *** [default] Error 2

Thanks,
Eryu

Re: [PATCH 2/4] src/open_by_handle: flexible usage options

[Eryu Guan]

On Tue, Apr 18, 2017 at 09:17:22PM +0300, Amir Goldstein wrote:
> More usage options for testing open_by_handle, which are needed
> for testing stable handles across copy up in overlayfs.
> 
> usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
> 
> Examples:
> 
> 1. Create N test files (nlink=1) under test_dir and exit:
>  $ open_by_handle -c <test_dir> [N]
> 
> 2. Get file handles, drop caches and try to open by handle
>    (expects success):
>  $ open_by_handle <test_dir> [N]
> 
> 3. Get file handles, create hardlinks to test files (nlink=2),
>    drop caches and try to open by handle (expects success):
>  $ open_by_handle -l <test_dir> [N]
> 
> 4. Get file handles, unlink test files w/o the hardlinks (nlink=1),
>    drop caches and try to open by handle (expects success):
>  $ open_by_handle -u <test_dir> [N]
> 
> 5. Get file handles, unlink test files and hardlinks (nlink=0),
>    drop caches and try to open by handle (expects failure):
>  $ open_by_handle -d <test_dir> [N]

"The reason I separated -l (link) and -u (unlink) is because for
overlayfs I need to test linking in lower layer and unlinking from
overlay."

I think it's better to have this in commit log too. (And more
description of the usages of these options? I found that "-c" only
creates test files not hard links, so a subsequent "-u" reports ESTALE,
"-u" is only valid after a "-l". This could confuse testers, I guess.)

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/open_by_handle.c | 93 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 8f04865..c33a4aa 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -37,12 +37,12 @@
>  #include <errno.h>
>  #include <linux/limits.h>
>  
> -#define NUMFILES 1024
> +#define MAXFILES 1024
>  
>  struct handle {
>  	struct file_handle fh;
>  	unsigned char fid[MAX_HANDLE_SZ];
> -} handle[NUMFILES];
> +} handle[MAXFILES];
>  
>  int main(int argc, char **argv)
>  {
> @@ -51,18 +51,60 @@ int main(int argc, char **argv)
>  	int	ret;
>  	int	failed = 0;
>  	char	fname[PATH_MAX];
> +	char	fname2[PATH_MAX];
>  	char	*test_dir;
>  	int	mount_fd, mount_id;
> +	int	argi = 1, numfiles = 1;
> +	int	create = 0, delete = 0, nlink = 1;
>  
> -	if (argc != 2) {
> -		fprintf(stderr, "usage: open_by_handle <test_dir>\n");
> +	if (argc < 2 || argc > 4) {
> +usage:
> +		fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
> +		fprintf(stderr, "\n");
> +		fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, get file handles and exit\n");
> +		fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles, drop caches and try to open by handle (expects success)\n");
> +		fprintf(stderr, "open_by_handle -l <test_dir> [N] - get file handles, create hardlinks to test files (nlink=2), drop caches and try to open by handle (expects success)\n");
> +		fprintf(stderr, "open_by_handle -u <test_dir> [N] - get file handles, unlink test files w/o hardlinks (nlink=1), drop caches and try to open by handle (expects success)\n");
> +		fprintf(stderr, "open_by_handle -d <test_dir> [N] - get file handles, unlink test files and hardlinks (nlink=0), drop caches and try to open by handle (expects failure)\n");
>  		return EXIT_FAILURE;
>  	}
>  
> -	test_dir = argv[1];
> +	if (argv[1][0] == '-') {

Hmm, why not "getopt"?

> +		if (argv[1][2])
> +			goto usage;
> +		switch (argv[1][1]) {
> +		case 'c':
> +			create = 1;
> +			break;
> +		case 'l':
> +			nlink = 2;
> +			break;
> +		case 'u':
> +			delete = 1;
> +			nlink = 1;
> +			break;
> +		case 'd':
> +			delete = 1;
> +			nlink = 0;
> +			break;
> +		default:
> +			fprintf(stderr, "illegal option '%s'\n", argv[1]);
> +		case 'h':
> +			goto usage;
> +		}
> +		argi++;
> +	}
> +	test_dir = argv[argi++];
> +	if (argc > argi)
> +		numfiles = atoi(argv[argi]);
> +	if (!numfiles || numfiles > MAXFILES) {
> +		fprintf(stderr, "illegal value '%s' for num_files\n", argv[argi]);
> +		goto usage;
> +	}
> +
>  	mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
>  	if (mount_fd < 0) {
> -		perror("open test_dir");
> +		perror(test_dir);
>  		return EXIT_FAILURE;
>  	}
>  
> @@ -70,8 +112,9 @@ int main(int argc, char **argv)
>  	 * create a large number of files to force allocation of new inode
>  	 * chunks on disk.
>  	 */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; create && i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
> +		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);
> @@ -79,13 +122,14 @@ int main(int argc, char **argv)
>  			return EXIT_FAILURE;
>  		}
>  		close(fd);
> +		ret = unlink(fname2);

So "-c" also implies unlinking the hard links, mind updating the commit
log and the comment above the for block?

But I'm not sure if it's a good idea for the test to depend on this
subtle cleanup behavior, see my comments to patch #4.

>  	}
>  
>  	/* sync to get the new inodes to hit the disk */
>  	sync();
>  
>  	/* create the handles */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
>  		handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
>  		ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> @@ -95,14 +139,32 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> +	/* after creating test set only check that fs supports exportfs */
> +	if (create)
> +		return EXIT_SUCCESS;

"-c" means "Create N test files (nlink=1) under test_dir and exit", so
we could move it before name_to_handle_at() calls?

> +
> +	/* hardlink the files */
> +	for (i=0; nlink > 1 && i < numfiles; i++) {
> +		sprintf(fname, "%s/file%06d", test_dir, i);
> +		sprintf(fname2, "%s/link%06d", test_dir, i);
> +		ret = link(fname, fname2);
> +		if (ret < 0) {
> +			perror("link");
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
>  	/* unlink the files */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; delete && i < numfiles; i++) {
>  		sprintf(fname, "%s/file%06d", test_dir, i);
> +		sprintf(fname2, "%s/link%06d", test_dir, i);
>  		ret = unlink(fname);
>  		if (ret < 0) {
>  			perror("unlink");
>  			return EXIT_FAILURE;
>  		}
> +		if (!nlink)
> +			ret = unlink(fname2);

I noticed that return values of unlink(fname2) are all ignored, is this
intentional?

>  	}
>  
>  	/* sync to get log forced for unlink transactions to hit the disk */
> @@ -126,17 +188,22 @@ int main(int argc, char **argv)
>  	 * now try to open the files by the stored handles. Expecting ENOENT
>  	 * for all of them.

These comments should be updated too.

Thanks,
Eryu

>  	 */
> -	for (i=0; i < NUMFILES; i++) {
> +	for (i=0; i < numfiles; i++) {
>  		errno = 0;
>  		fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
> -		if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
> +		if (nlink && fd >= 0) {
> +			close(fd);
> +			continue;
> +		} else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>  			continue;
>  		}
>  		if (fd >= 0) {
>  			printf("open_by_handle(%d) opened an unlinked file!\n", i);
>  			close(fd);
> -		} else
> -			printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
> +		} else {
> +			printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
> +					nlink ? "a linked" : "an unlinked");
> +		}
>  		failed++;
>  	}
>  	if (failed)
> -- 
> 2.7.4
> 

Re: [PATCH 3/4] fstests: add helper _require_exportfs

[Eryu Guan]

On Tue, Apr 18, 2017 at 09:17:23PM +0300, Amir Goldstein wrote:
> Use open_by_handle -c to determine if filesystem supports NFS export
> ---
>  common/rc | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 685b859..f24a21f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2857,6 +2857,15 @@ _require_freeze()
>  	[ $result -eq 0 ] || _notrun "$FSTYP does not support freezing"
>  }
>  
> +# Does NFS export work on this fs?
> +_require_exportfs()
> +{
> +	mkdir -p "$TEST_DIR"/export_test
> +	$here/src/open_by_handle -c "$TEST_DIR"/export_test 2>&1 \
> +		|| _notrun "$FSTYP does not support NFS export"

_require_test_program "open_by_handle" first?

Thanks,
Eryu

> +}
> +
> +
>  # Does shutdown work on this fs?
>  _require_scratch_shutdown()
>  {
> -- 
> 2.7.4
> 

Re: [PATCH 3/4] fstests: add helper _require_exportfs

[David Howells]

Amir Goldstein <amir73il@gmail.com> wrote:

> +# Does NFS export work on this fs?
> +_require_exportfs()

Don't forget to add it to doc/requirement-checking.txt!

David

Re: [PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[David Howells]

Amir Goldstein <amir73il@gmail.com> wrote:

> This is a clone of src/stale_handle.c test that uses generic
> open_by_handle_at() syscall instead of the xfs specific ioctl.

Don't forget to add it to doc/auxiliary-programs.txt!

David

Re: [PATCH 4/4] fstests: add generic test for file handles

[Eryu Guan]

On Tue, Apr 18, 2017 at 09:17:24PM +0300, Amir Goldstein wrote:
> Cloned from xfs specific test xfs/238, which checks
> stale file handles of deleted files.
> 
> This test uses the generic open_by_handle_at() syscall
> and also tests for non-stale file handles of linked files.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/generic/426     | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/426.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 76 insertions(+)
>  create mode 100755 tests/generic/426
>  create mode 100644 tests/generic/426.out
> 
> diff --git a/tests/generic/426 b/tests/generic/426
> new file mode 100755
> index 0000000..62bb85e
> --- /dev/null
> +++ b/tests/generic/426
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# FS QA Test No. 426
> +#
> +# Check stale handles pointing to unlinked files
> +# and non-stale handles pointing to linked files
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
                   ^^^^ 2017?
> +# 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_scratch
> +_require_exportfs
> +
> +echo "Silence is golden"
> +
> +_scratch_mkfs > /dev/null 2>&1
> +_scratch_mount > /dev/null 2>&1
> +
> +numfiles=1024
> +
> +# Check stale handles to deleted files
> +src/open_by_handle -c $SCRATCH_MNT $numfiles
> +src/open_by_handle -d $SCRATCH_MNT $numfiles
> +
> +# Check non-stale handles to linked files
> +src/open_by_handle -c $SCRATCH_MNT $numfiles
> +src/open_by_handle    $SCRATCH_MNT $numfiles
> +
> +# Check non-stale handles to files that were hardlinked and original deleted
> +src/open_by_handle -l $SCRATCH_MNT $numfiles
> +src/open_by_handle -u $SCRATCH_MNT $numfiles

This third test depends on test files created in second test, and I
guess this could confuse people at debug time if any of the tests
failed.

So how about remove all the files and call "open_by_handle -c ..."
before each test? e.g. (I use testdir and don't remove lost+found dir
because extN needs it for fsck)

testdir=$SCRATCH_MNT/testdir
mkdir -p $testdir

# Check stale handles to deleted files
src/open_by_handle -c $testdir $numfiles
src/open_by_handle -d $testdir $numfiles

# Check non-stale handles to linked files
rm -f $testdir/*
...

# Check non-stale handles to files that were hardlinked and original
# deleted
rm -f $testdir/*
src/open_by_handle -c $testdir $numfiles
...                -l ..
...                -u ..

Thanks,
Eryu

> +
> +status=$?
> +exit
> diff --git a/tests/generic/426.out b/tests/generic/426.out
> new file mode 100644
> index 0000000..777cbcd
> --- /dev/null
> +++ b/tests/generic/426.out
> @@ -0,0 +1,2 @@
> +QA output created by 426
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 6d6e4f6..f29009c 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -428,3 +428,4 @@
>  423 auto quick
>  424 auto quick
>  425 auto quick attr
> +426 auto quick exportfs
> -- 
> 2.7.4
> 

Re: [PATCH 2/4] src/open_by_handle: flexible usage options

[Amir Goldstein]

On Wed, Apr 19, 2017 at 12:42 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Apr 18, 2017 at 09:17:22PM +0300, Amir Goldstein wrote:
>> More usage options for testing open_by_handle, which are needed
>> for testing stable handles across copy up in overlayfs.
>>
>> usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]
>>
>> Examples:
>>
>> 1. Create N test files (nlink=1) under test_dir and exit:
>>  $ open_by_handle -c <test_dir> [N]
>>
>> 2. Get file handles, drop caches and try to open by handle
>>    (expects success):
>>  $ open_by_handle <test_dir> [N]
>>
>> 3. Get file handles, create hardlinks to test files (nlink=2),
>>    drop caches and try to open by handle (expects success):
>>  $ open_by_handle -l <test_dir> [N]
>>
>> 4. Get file handles, unlink test files w/o the hardlinks (nlink=1),
>>    drop caches and try to open by handle (expects success):
>>  $ open_by_handle -u <test_dir> [N]
>>
>> 5. Get file handles, unlink test files and hardlinks (nlink=0),
>>    drop caches and try to open by handle (expects failure):
>>  $ open_by_handle -d <test_dir> [N]
>
> "The reason I separated -l (link) and -u (unlink) is because for
> overlayfs I need to test linking in lower layer and unlinking from
> overlay."
>
> I think it's better to have this in commit log too. (And more
> description of the usages of these options? I found that "-c" only
> creates test files not hard links, so a subsequent "-u" reports ESTALE,
> "-u" is only valid after a "-l". This could confuse testers, I guess.)
>

Certainly, this tool is dumb. It should be used in very specific ways.
I'll update the documentation with the sane use cases.

>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  src/open_by_handle.c | 93 ++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 80 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
>> index 8f04865..c33a4aa 100644
>> --- a/src/open_by_handle.c
>> +++ b/src/open_by_handle.c
>> @@ -37,12 +37,12 @@
>>  #include <errno.h>
>>  #include <linux/limits.h>
>>
>> -#define NUMFILES 1024
>> +#define MAXFILES 1024
>>
>>  struct handle {
>>       struct file_handle fh;
>>       unsigned char fid[MAX_HANDLE_SZ];
>> -} handle[NUMFILES];
>> +} handle[MAXFILES];
>>
>>  int main(int argc, char **argv)
>>  {
>> @@ -51,18 +51,60 @@ int main(int argc, char **argv)
>>       int     ret;
>>       int     failed = 0;
>>       char    fname[PATH_MAX];
>> +     char    fname2[PATH_MAX];
>>       char    *test_dir;
>>       int     mount_fd, mount_id;
>> +     int     argi = 1, numfiles = 1;
>> +     int     create = 0, delete = 0, nlink = 1;
>>
>> -     if (argc != 2) {
>> -             fprintf(stderr, "usage: open_by_handle <test_dir>\n");
>> +     if (argc < 2 || argc > 4) {
>> +usage:
>> +             fprintf(stderr, "usage: open_by_handle [-c|-l|-u|-d] <test_dir> [num_files]\n");
>> +             fprintf(stderr, "\n");
>> +             fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, get file handles and exit\n");
>> +             fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles, drop caches and try to open by handle (expects success)\n");
>> +             fprintf(stderr, "open_by_handle -l <test_dir> [N] - get file handles, create hardlinks to test files (nlink=2), drop caches and try to open by handle (expects success)\n");
>> +             fprintf(stderr, "open_by_handle -u <test_dir> [N] - get file handles, unlink test files w/o hardlinks (nlink=1), drop caches and try to open by handle (expects success)\n");
>> +             fprintf(stderr, "open_by_handle -d <test_dir> [N] - get file handles, unlink test files and hardlinks (nlink=0), drop caches and try to open by handle (expects failure)\n");
>>               return EXIT_FAILURE;
>>       }
>>
>> -     test_dir = argv[1];
>> +     if (argv[1][0] == '-') {
>
> Hmm, why not "getopt"?
>

No reason, it started small with -c and grew into this. I guess getopt
is called for.

>> +             if (argv[1][2])
>> +                     goto usage;
>> +             switch (argv[1][1]) {
>> +             case 'c':
>> +                     create = 1;
>> +                     break;
>> +             case 'l':
>> +                     nlink = 2;
>> +                     break;
>> +             case 'u':
>> +                     delete = 1;
>> +                     nlink = 1;
>> +                     break;
>> +             case 'd':
>> +                     delete = 1;
>> +                     nlink = 0;
>> +                     break;
>> +             default:
>> +                     fprintf(stderr, "illegal option '%s'\n", argv[1]);
>> +             case 'h':
>> +                     goto usage;
>> +             }
>> +             argi++;
>> +     }
>> +     test_dir = argv[argi++];
>> +     if (argc > argi)
>> +             numfiles = atoi(argv[argi]);
>> +     if (!numfiles || numfiles > MAXFILES) {
>> +             fprintf(stderr, "illegal value '%s' for num_files\n", argv[argi]);
>> +             goto usage;
>> +     }
>> +
>>       mount_fd = open(test_dir, O_RDONLY|O_DIRECTORY);
>>       if (mount_fd < 0) {
>> -             perror("open test_dir");
>> +             perror(test_dir);
>>               return EXIT_FAILURE;
>>       }
>>
>> @@ -70,8 +112,9 @@ int main(int argc, char **argv)
>>        * create a large number of files to force allocation of new inode
>>        * chunks on disk.
>>        */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; create && i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>> +             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);
>> @@ -79,13 +122,14 @@ int main(int argc, char **argv)
>>                       return EXIT_FAILURE;
>>               }
>>               close(fd);
>> +             ret = unlink(fname2);
>
> So "-c" also implies unlinking the hard links, mind updating the commit
> log and the comment above the for block?

Yes, it means create the test set, which implies blowing up previous
test leftovers.
I'll update the comments.

>
> But I'm not sure if it's a good idea for the test to depend on this
> subtle cleanup behavior, see my comments to patch #4.
>
>>       }
>>
>>       /* sync to get the new inodes to hit the disk */
>>       sync();
>>
>>       /* create the handles */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>>               handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
>>               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
>> @@ -95,14 +139,32 @@ int main(int argc, char **argv)
>>               }
>>       }
>>
>> +     /* after creating test set only check that fs supports exportfs */
>> +     if (create)
>> +             return EXIT_SUCCESS;
>
> "-c" means "Create N test files (nlink=1) under test_dir and exit", so
> we could move it before name_to_handle_at() calls?

That's the wording from commit message which is out of date.
usage says -c also tries to get handles, because it is being used for
_require_exportfs

>
>> +
>> +     /* hardlink the files */
>> +     for (i=0; nlink > 1 && i < numfiles; i++) {
>> +             sprintf(fname, "%s/file%06d", test_dir, i);
>> +             sprintf(fname2, "%s/link%06d", test_dir, i);
>> +             ret = link(fname, fname2);
>> +             if (ret < 0) {
>> +                     perror("link");
>> +                     return EXIT_FAILURE;
>> +             }
>> +     }
>> +
>>       /* unlink the files */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; delete && i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>> +             sprintf(fname2, "%s/link%06d", test_dir, i);
>>               ret = unlink(fname);
>>               if (ret < 0) {
>>                       perror("unlink");
>>                       return EXIT_FAILURE;
>>               }
>> +             if (!nlink)
>> +                     ret = unlink(fname2);
>
> I noticed that return values of unlink(fname2) are all ignored, is this
> intentional?
>

Yes. For -d it unlinks the hardlinks if they exist so jus unlink
and ignore the return value instead of stat+unlink.
worse a comment...


>>       }
>>
>>       /* sync to get log forced for unlink transactions to hit the disk */
>> @@ -126,17 +188,22 @@ int main(int argc, char **argv)
>>        * now try to open the files by the stored handles. Expecting ENOENT
>>        * for all of them.
>
> These comments should be updated too.
>

Right.
Thanks!

> Thanks,
> Eryu
>
>>        */
>> -     for (i=0; i < NUMFILES; i++) {
>> +     for (i=0; i < numfiles; i++) {
>>               errno = 0;
>>               fd = open_by_handle_at(mount_fd, &handle[i].fh, O_RDWR);
>> -             if (fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>> +             if (nlink && fd >= 0) {
>> +                     close(fd);
>> +                     continue;
>> +             } else if (!nlink && fd < 0 && (errno == ENOENT || errno == ESTALE)) {
>>                       continue;
>>               }
>>               if (fd >= 0) {
>>                       printf("open_by_handle(%d) opened an unlinked file!\n", i);
>>                       close(fd);
>> -             } else
>> -                     printf("open_by_handle(%d) returned %d incorrectly on an unlinked file!\n", i, errno);
>> +             } else {
>> +                     printf("open_by_handle(%d) returned %d incorrectly on %s file!\n", i, errno,
>> +                                     nlink ? "a linked" : "an unlinked");
>> +             }
>>               failed++;
>>       }
>>       if (failed)
>> --
>> 2.7.4
>>

Re: [PATCH 2/4] src/open_by_handle: flexible usage options

[Eryu Guan]

On Wed, Apr 19, 2017 at 12:57:49PM +0300, Amir Goldstein wrote:
...
> >> @@ -95,14 +139,32 @@ int main(int argc, char **argv)
> >>               }
> >>       }
> >>
> >> +     /* after creating test set only check that fs supports exportfs */
> >> +     if (create)
> >> +             return EXIT_SUCCESS;
> >
> > "-c" means "Create N test files (nlink=1) under test_dir and exit", so
> > we could move it before name_to_handle_at() calls?
> 
> That's the wording from commit message which is out of date.
> usage says -c also tries to get handles, because it is being used for
> _require_exportfs

Right, I missed that. Then yes, commit message should be updated :)

Thanks,
Eryu

Re: [PATCH 4/4] fstests: add generic test for file handles

[Amir Goldstein]

On Wed, Apr 19, 2017 at 12:55 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Apr 18, 2017 at 09:17:24PM +0300, Amir Goldstein wrote:
>> Cloned from xfs specific test xfs/238, which checks
>> stale file handles of deleted files.
>>
>> This test uses the generic open_by_handle_at() syscall
>> and also tests for non-stale file handles of linked files.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  tests/generic/426     | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/generic/426.out |  2 ++
>>  tests/generic/group   |  1 +
>>  3 files changed, 76 insertions(+)
>>  create mode 100755 tests/generic/426
>>  create mode 100644 tests/generic/426.out
>>
>> diff --git a/tests/generic/426 b/tests/generic/426
>> new file mode 100755
>> index 0000000..62bb85e
>> --- /dev/null
>> +++ b/tests/generic/426
>> @@ -0,0 +1,73 @@
>> +#! /bin/bash
>> +# FS QA Test No. 426
>> +#
>> +# Check stale handles pointing to unlinked files
>> +# and non-stale handles pointing to linked files
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
>                    ^^^^ 2017?
>> +# 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_scratch
>> +_require_exportfs
>> +
>> +echo "Silence is golden"
>> +
>> +_scratch_mkfs > /dev/null 2>&1
>> +_scratch_mount > /dev/null 2>&1
>> +
>> +numfiles=1024
>> +
>> +# Check stale handles to deleted files
>> +src/open_by_handle -c $SCRATCH_MNT $numfiles
>> +src/open_by_handle -d $SCRATCH_MNT $numfiles
>> +
>> +# Check non-stale handles to linked files
>> +src/open_by_handle -c $SCRATCH_MNT $numfiles
>> +src/open_by_handle    $SCRATCH_MNT $numfiles
>> +
>> +# Check non-stale handles to files that were hardlinked and original deleted
>> +src/open_by_handle -l $SCRATCH_MNT $numfiles
>> +src/open_by_handle -u $SCRATCH_MNT $numfiles
>
> This third test depends on test files created in second test, and I
> guess this could confuse people at debug time if any of the tests
> failed.
>
> So how about remove all the files and call "open_by_handle -c ..."
> before each test? e.g. (I use testdir and don't remove lost+found dir
> because extN needs it for fsck)
>

Sure. Makes sense.
Going forward, as you might have guessed, I am going to add
_scratch_cycle_mount to the mix.


> testdir=$SCRATCH_MNT/testdir

So I copied using SCRATCH_MNT from xfs/238, but I wonder if
I should go with thsi instead:
testdir=$TEST_DIR/$seq

Maybe some fs is vulnerable to exporting handles from an ages fs
who knows... Any opinion?

> mkdir -p $testdir
>
> # Check stale handles to deleted files
> src/open_by_handle -c $testdir $numfiles
> src/open_by_handle -d $testdir $numfiles
>
> # Check non-stale handles to linked files
> rm -f $testdir/*
> ...
>
> # Check non-stale handles to files that were hardlinked and original
> # deleted
> rm -f $testdir/*
> src/open_by_handle -c $testdir $numfiles
> ...                -l ..
> ...                -u ..
>
> Thanks,
> Eryu
>
>> +
>> +status=$?
>> +exit
>> diff --git a/tests/generic/426.out b/tests/generic/426.out
>> new file mode 100644
>> index 0000000..777cbcd
>> --- /dev/null
>> +++ b/tests/generic/426.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 426
>> +Silence is golden
>> diff --git a/tests/generic/group b/tests/generic/group
>> index 6d6e4f6..f29009c 100644
>> --- a/tests/generic/group
>> +++ b/tests/generic/group
>> @@ -428,3 +428,4 @@
>>  423 auto quick
>>  424 auto quick
>>  425 auto quick attr
>> +426 auto quick exportfs
>> --
>> 2.7.4
>>

Re: [PATCH 1/4] src/open_by_handle: helper to test open_by_handle_at() syscall

[Amir Goldstein]

On Wed, Apr 19, 2017 at 12:51 PM, David Howells <dhowells@redhat.com> wrote:
> Amir Goldstein <amir73il@gmail.com> wrote:
>
>> This is a clone of src/stale_handle.c test that uses generic
>> open_by_handle_at() syscall instead of the xfs specific ioctl.
>
> Don't forget to add it to doc/auxiliary-programs.txt!
>

Will do :) Thanks!

Re: [PATCH 4/4] fstests: add generic test for file handles

[Eryu Guan]

On Wed, Apr 19, 2017 at 01:07:47PM +0300, Amir Goldstein wrote:
...
> > So how about remove all the files and call "open_by_handle -c ..."
> > before each test? e.g. (I use testdir and don't remove lost+found dir
> > because extN needs it for fsck)
> >
> 
> Sure. Makes sense.
> Going forward, as you might have guessed, I am going to add
> _scratch_cycle_mount to the mix.
> 
> 
> > testdir=$SCRATCH_MNT/testdir
> 
> So I copied using SCRATCH_MNT from xfs/238, but I wonder if
> I should go with thsi instead:
> testdir=$TEST_DIR/$seq
> 
> Maybe some fs is vulnerable to exporting handles from an ages fs
> who knows... Any opinion?

Hmm, I have no strong preference on this, seems TEST_DIR should be
sufficient for the test and we can get rid of one test requirement.

Thanks,
Eryu