OpenZFS 7336 - vfork and O_CLOEXEC causes zfs_mount EBUSY

Porting notes:
- statvfs64 is replaced by statfs64.
- ZFS_SUPER_MAGIC definition moved in include/sys/fs/zfs.h
  to share it between user and kernel space.

Authored by: Prakash Surya <prakash.surya@delphix.com>
Reviewed by: Matt Ahrens <mahrens@delphix.com>
Reviewed by: Paul Dagnelie <pcd@delphix.com>
Reviewed by: Robert Mustacchi <rm@joyent.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Ported-by: George Melikov <mail@gmelikov.ru>

OpenZFS-issue: https://www.illumos.org/issues/7336
OpenZFS-commit: https://github.com/openzfs/openzfs/commit/dd862f6d
Closes #5651

include/sys/fs/zfs.h

@@ -924,6 +924,8 @@ typedef struct ddt_histogram {
 #define	ZFS_DEV		"/dev/zfs"
 #define	ZFS_SHARETAB	"/etc/dfs/sharetab"
 
+#define	ZFS_SUPER_MAGIC	0x2fc12fc1
+
 /* general zvol path */
 #define	ZVOL_DIR	"/dev"
 

include/sys/zfs_vfsops.h

@@ -119,8 +119,6 @@ typedef struct zfs_sb {
 	kmutex_t	*z_hold_locks;	/* znode hold locks */
 } zfs_sb_t;
 
-#define	ZFS_SUPER_MAGIC	0x2fc12fc1
-
 #define	ZSB_XATTR	0x0001		/* Enable user xattrs */
 
 /*

lib/libzfs/libzfs_mount.c

@@ -75,6 +75,7 @@
 #include <sys/mntent.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
+#include <sys/vfs.h>
 
 #include <libzfs.h>
 
@@ -170,13 +171,32 @@ is_shared(libzfs_handle_t *hdl, const char *mountpoint, zfs_share_proto_t proto)
 	return (SHARED_NOT_SHARED);
 }
 
-/*
- * Returns true if the specified directory is empty.  If we can't open the
- * directory at all, return true so that the mount can fail with a more
- * informative error message.
- */
 static boolean_t
-dir_is_empty(const char *dirname)
+dir_is_empty_stat(const char *dirname)
+{
+	struct stat st;
+
+	/*
+	 * We only want to return false if the given path is a non empty
+	 * directory, all other errors are handled elsewhere.
+	 */
+	if (stat(dirname, &st) < 0 || !S_ISDIR(st.st_mode)) {
+		return (B_TRUE);
+	}
+
+	/*
+	 * An empty directory will still have two entries in it, one
+	 * entry for each of "." and "..".
+	 */
+	if (st.st_size > 2) {
+		return (B_FALSE);
+	}
+
+	return (B_TRUE);
+}
+
+static boolean_t
+dir_is_empty_readdir(const char *dirname)
 {
 	DIR *dirp;
 	struct dirent64 *dp;
@@ -205,6 +225,42 @@ dir_is_empty(const char *dirname)
 	return (B_TRUE);
 }
 
+/*
+ * Returns true if the specified directory is empty.  If we can't open the
+ * directory at all, return true so that the mount can fail with a more
+ * informative error message.
+ */
+static boolean_t
+dir_is_empty(const char *dirname)
+{
+	struct statfs64 st;
+
+	/*
+	 * If the statvfs call fails or the filesystem is not a ZFS
+	 * filesystem, fall back to the slow path which uses readdir.
+	 */
+	if ((statfs64(dirname, &st) != 0) ||
+	    (st.f_type != ZFS_SUPER_MAGIC)) {
+		return (dir_is_empty_readdir(dirname));
+	}
+
+	/*
+	 * At this point, we know the provided path is on a ZFS
+	 * filesystem, so we can use stat instead of readdir to
+	 * determine if the directory is empty or not. We try to avoid
+	 * using readdir because that requires opening "dirname"; this
+	 * open file descriptor can potentially end up in a child
+	 * process if there's a concurrent fork, thus preventing the
+	 * zfs_mount() from otherwise succeeding (the open file
+	 * descriptor inherited by the child process will cause the
+	 * parent's mount to fail with EBUSY). The performance
+	 * implications of replacing the open, read, and close with a
+	 * single stat is nice; but is not the main motivation for the
+	 * added complexity.
+	 */
+	return (dir_is_empty_stat(dirname));
+}
+
 /*
  * Checks to see if the mount is active.  If the filesystem is mounted, we fill
  * in 'where' with the current mountpoint, and return 1.  Otherwise, we return

tests/runfiles/linux.run

@@ -131,7 +131,7 @@ tests = ['zfs_inherit_001_neg', 'zfs_inherit_002_neg']
 [tests/functional/cli_root/zfs_mount]
 tests = ['zfs_mount_001_pos', 'zfs_mount_002_pos', 'zfs_mount_003_pos',
     'zfs_mount_004_pos', 'zfs_mount_005_pos', 'zfs_mount_008_pos',
-    'zfs_mount_010_neg', 'zfs_mount_011_neg']
+    'zfs_mount_010_neg', 'zfs_mount_011_neg', 'zfs_mount_012_neg']
 
 [tests/functional/cli_root/zfs_promote]
 tests = ['zfs_promote_001_pos', 'zfs_promote_002_pos', 'zfs_promote_003_pos',

tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am

@@ -15,4 +15,5 @@ dist_pkgdata_SCRIPTS = \
 	zfs_mount_009_neg.ksh \
 	zfs_mount_010_neg.ksh \
 	zfs_mount_011_neg.ksh \
+	zfs_mount_012_neg.ksh \
 	zfs_mount_all_001_pos.ksh

tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_neg.ksh

@@ -0,0 +1,50 @@
+#!/bin/ksh -p
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2015 by Delphix. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+# Verify that zfs mount should fail with a non-empty directory
+#
+# STRATEGY:
+# 1. Unmount the dataset
+# 2. Create a new empty directory
+# 3. Set the dataset's mountpoint
+# 4. Attempt to mount the dataset
+# 5. Verify the mount succeeds
+# 6. Unmount the dataset
+# 7. Create a file in the directory created in step 2
+# 8. Attempt to mount the dataset
+# 9. Verify the mount fails
+#
+
+verify_runnable "both"
+
+log_assert "zfs mount fails with non-empty directory"
+
+fs=$TESTPOOL/$TESTFS
+
+log_must $ZFS umount $fs
+log_must mkdir -p $TESTDIR
+log_must $ZFS set mountpoint=$TESTDIR $fs
+log_must $ZFS mount $fs
+log_must $ZFS umount $fs
+log_must touch $TESTDIR/testfile.$$
+log_mustnot $ZFS mount $fs
+log_must rm -rf $TESTDIR
+
+log_pass "zfs mount fails non-empty directory as expected."