Fix changelist mounted-dataset iteration

Commit 0c6d093 caused a regression in the inherit codepath.
The fix is to restrict the changelist iteration on mountpoints and
add proper handling for 'legacy' mountpoints

Reviewed by: Serapheim Dimitropoulos <serapheim.dimitro@delphix.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alek Pinchuk <apinchuk@datto.com>
Closes #7988 
Closes #7991

include/libzfs_impl.h

@@ -22,6 +22,7 @@
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2011, 2015 by Delphix. All rights reserved.
+ * Copyright (c) 2018 Datto Inc.
  */
 
 #ifndef	_LIBZFS_IMPL_H
@@ -146,6 +147,10 @@ int zprop_expand_list(libzfs_handle_t *hdl, zprop_list_t **plp,
  * mounted.
  */
 #define	CL_GATHER_MOUNT_ALWAYS	1
+/*
+ * changelist_gather() flag to force it to iterate on mounted datasets only
+ */
+#define	CL_GATHER_ITER_MOUNTED	2
 
 typedef struct prop_changelist prop_changelist_t;
 

lib/libzfs/libzfs_changelist.c

@@ -553,39 +553,50 @@ change_one(zfs_handle_t *zhp, void *data)
 	return (0);
 }
 
-/*ARGSUSED*/
 static int
-compare_mountpoints(const void *a, const void *b, void *unused)
+compare_props(const void *a, const void *b, zfs_prop_t prop)
 {
 	const prop_changenode_t *ca = a;
 	const prop_changenode_t *cb = b;
 
-	char mounta[MAXPATHLEN];
+	char propa[MAXPATHLEN];
-	char mountb[MAXPATHLEN];
+	char propb[MAXPATHLEN];
 
-	boolean_t hasmounta, hasmountb;
+	boolean_t haspropa, haspropb;
 
+	haspropa = (zfs_prop_get(ca->cn_handle, prop, propa, sizeof (propa),
+	    NULL, NULL, 0, B_FALSE) == 0);
+	haspropb = (zfs_prop_get(cb->cn_handle, prop, propb, sizeof (propb),
+	    NULL, NULL, 0, B_FALSE) == 0);
+
+	if (!haspropa && haspropb)
+		return (-1);
+	else if (haspropa && !haspropb)
+		return (1);
+	else if (!haspropa && !haspropb)
+		return (0);
+	else
+		return (strcmp(propb, propa));
+}
+
+/*ARGSUSED*/
+static int
+compare_mountpoints(const void *a, const void *b, void *unused)
+{
 	/*
 	 * When unsharing or unmounting filesystems, we need to do it in
 	 * mountpoint order.  This allows the user to have a mountpoint
 	 * hierarchy that is different from the dataset hierarchy, and still
-	 * allow it to be changed.  However, if either dataset doesn't have a
+	 * allow it to be changed.
-	 * mountpoint (because it is a volume or a snapshot), we place it at the
-	 * end of the list, because it doesn't affect our change at all.
 	 */
-	hasmounta = (zfs_prop_get(ca->cn_handle, ZFS_PROP_MOUNTPOINT, mounta,
+	return (compare_props(a, b, ZFS_PROP_MOUNTPOINT));
-	    sizeof (mounta), NULL, NULL, 0, B_FALSE) == 0);
+}
-	hasmountb = (zfs_prop_get(cb->cn_handle, ZFS_PROP_MOUNTPOINT, mountb,
-	    sizeof (mountb), NULL, NULL, 0, B_FALSE) == 0);
 
-	if (!hasmounta && hasmountb)
+/*ARGSUSED*/
-		return (-1);
+static int
-	else if (hasmounta && !hasmountb)
+compare_dataset_names(const void *a, const void *b, void *unused)
-		return (1);
+{
-	else if (!hasmounta && !hasmountb)
+	return (compare_props(a, b, ZFS_PROP_NAME));
-		return (0);
-	else
-		return (strcmp(mountb, mounta));
 }
 
 /*
@@ -630,7 +641,7 @@ changelist_gather(zfs_handle_t *zhp, zfs_prop_t prop, int gather_flags,
 	clp->cl_pool = uu_avl_pool_create("changelist_pool",
 	    sizeof (prop_changenode_t),
 	    offsetof(prop_changenode_t, cn_treenode),
-	    compare_mountpoints, 0);
+	    legacy ? compare_dataset_names : compare_mountpoints, 0);
 	if (clp->cl_pool == NULL) {
 		assert(uu_error() == UU_ERROR_NO_MEMORY);
 		(void) zfs_error(zhp->zfs_hdl, EZFS_NOMEM, "internal error");
@@ -687,7 +698,7 @@ changelist_gather(zfs_handle_t *zhp, zfs_prop_t prop, int gather_flags,
 		clp->cl_shareprop = ZFS_PROP_SHARENFS;
 
 	if (clp->cl_prop == ZFS_PROP_MOUNTPOINT &&
-	    (clp->cl_gflags & CL_GATHER_MOUNT_ALWAYS) == 0) {
+	    (clp->cl_gflags & CL_GATHER_ITER_MOUNTED)) {
 		/*
 		 * Instead of iterating through all of the dataset children we
 		 * gather mounted dataset children from MNTTAB

lib/libzfs/libzfs_dataset.c

@@ -30,6 +30,7 @@
  * Copyright 2017 Nexenta Systems, Inc.
  * Copyright 2016 Igor Kozhukhov <ikozhukhov@gmail.com>
  * Copyright 2017-2018 RackTop Systems.
+ * Copyright (c) 2018 Datto Inc.
  */
 
 #include <ctype.h>
@@ -4548,7 +4549,8 @@ zfs_rename(zfs_handle_t *zhp, const char *target, boolean_t recursive,
 			goto error;
 		}
 	} else if (zhp->zfs_type != ZFS_TYPE_SNAPSHOT) {
-		if ((cl = changelist_gather(zhp, ZFS_PROP_NAME, 0,
+		if ((cl = changelist_gather(zhp, ZFS_PROP_NAME,
+		    CL_GATHER_ITER_MOUNTED,
 		    force_unmount ? MS_FORCE : 0)) == NULL)
 			return (-1);
 

lib/libzfs/libzfs_mount.c

@@ -25,6 +25,7 @@
  * Copyright (c) 2014, 2015 by Delphix. All rights reserved.
  * Copyright 2016 Igor Kozhukhov <ikozhukhov@gmail.com>
  * Copyright 2017 RackTop Systems.
+ * Copyright (c) 2018 Datto Inc.
  */
 
 /*
@@ -699,7 +700,8 @@ zfs_unmountall(zfs_handle_t *zhp, int flags)
 	prop_changelist_t *clp;
 	int ret;
 
-	clp = changelist_gather(zhp, ZFS_PROP_MOUNTPOINT, 0, flags);
+	clp = changelist_gather(zhp, ZFS_PROP_MOUNTPOINT,
+	    CL_GATHER_ITER_MOUNTED, 0);
 	if (clp == NULL)
 		return (-1);
 

tests/runfiles/linux.run

@@ -166,7 +166,8 @@ tests = ['zfs_get_001_pos', 'zfs_get_002_pos', 'zfs_get_003_pos',
 tags = ['functional', 'cli_root', 'zfs_get']
 
 [tests/functional/cli_root/zfs_inherit]
-tests = ['zfs_inherit_001_neg', 'zfs_inherit_002_neg', 'zfs_inherit_003_pos']
+tests = ['zfs_inherit_001_neg', 'zfs_inherit_002_neg', 'zfs_inherit_003_pos',
+    'zfs_inherit_mountpoint']
 tags = ['functional', 'cli_root', 'zfs_inherit']
 
 [tests/functional/cli_root/zfs_load-key]
@@ -218,7 +219,7 @@ tests = ['zfs_rename_001_pos', 'zfs_rename_002_pos', 'zfs_rename_003_pos',
     'zfs_rename_007_pos', 'zfs_rename_008_pos', 'zfs_rename_009_neg',
     'zfs_rename_010_neg', 'zfs_rename_011_pos', 'zfs_rename_012_neg',
     'zfs_rename_013_pos', 'zfs_rename_014_neg', 'zfs_rename_encrypted_child',
-    'zfs_rename_to_encrypted']
+    'zfs_rename_to_encrypted', 'zfs_rename_mountpoint']
 tags = ['functional', 'cli_root', 'zfs_rename']
 
 [tests/functional/cli_root/zfs_reservation]

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

@@ -4,4 +4,5 @@ dist_pkgdata_SCRIPTS = \
 	setup.ksh \
 	zfs_inherit_001_neg.ksh \
 	zfs_inherit_002_neg.ksh \
-	zfs_inherit_003_pos.ksh
+	zfs_inherit_003_pos.ksh \
+	zfs_inherit_mountpoint.ksh

tests/zfs-tests/tests/functional/cli_root/zfs_inherit/zfs_inherit_mountpoint.ksh

@@ -0,0 +1,62 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# 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 is of the CDDL is also available via the Internet
+# at http://www.illumos.org/license/CDDL.
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2018 Datto Inc.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+#	zfs inherit should inherit mountpoint on mountpoint=none children
+#
+# STRATEGY:
+#	1. Create a set of nested datasets with mountpoint=none
+#	2. Verify datasets aren't mounted
+#	3. Inherit mountpoint and verify all datasets are now mounted
+#
+
+verify_runnable "both"
+
+function inherit_cleanup
+{
+	log_must zfs destroy -fR $TESTPOOL/inherit_test
+}
+
+log_onexit inherit_cleanup
+
+
+log_must zfs create -o mountpoint=none $TESTPOOL/inherit_test
+log_must zfs create $TESTPOOL/inherit_test/child
+
+if ismounted $TESTPOOL/inherit_test; then
+	log_fail "$TESTPOOL/inherit_test is mounted"
+fi
+if ismounted $TESTPOOL/inherit_test/child; then
+	log_fail "$TESTPOOL/inherit_test/child is mounted"
+fi
+
+log_must zfs inherit mountpoint $TESTPOOL/inherit_test
+
+if ! ismounted $TESTPOOL/inherit_test; then
+	log_fail "$TESTPOOL/inherit_test is not mounted"
+fi
+if ! ismounted $TESTPOOL/inherit_test/child; then
+	log_fail "$TESTPOOL/inherit_test/child is not mounted"
+fi
+
+log_pass "Verified mountpoint for mountpoint=none children inherited."

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

@@ -17,7 +17,8 @@ dist_pkgdata_SCRIPTS = \
 	zfs_rename_013_pos.ksh \
 	zfs_rename_014_neg.ksh \
 	zfs_rename_encrypted_child.ksh \
-	zfs_rename_to_encrypted.ksh
+	zfs_rename_to_encrypted.ksh \
+	zfs_rename_mountpoint.ksh
 
 dist_pkgdata_DATA = \
 	zfs_rename.cfg \

tests/zfs-tests/tests/functional/cli_root/zfs_rename/zfs_rename_mountpoint.ksh

@@ -0,0 +1,88 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# 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 is of the CDDL is also available via the Internet
+# at http://www.illumos.org/license/CDDL.
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2018 Datto Inc.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+#	zfs rename should rename datasets even for mountpoint=none children
+#
+# STRATEGY:
+#	1. Create a set of nested datasets with mountpoint=none
+#	2. Verify datasets aren't mounted except for the parent
+#	3. Rename mountpoint and verify all child datasets are renamed
+#
+
+verify_runnable "both"
+
+function rename_cleanup
+{
+	log_note zfs destroy -fR $TESTPOOL/rename_test
+	log_note zfs destroy -fR $TESTPOOL/renamed
+}
+
+log_onexit rename_cleanup
+
+
+log_must zfs create $TESTPOOL/rename_test
+log_must zfs create $TESTPOOL/rename_test/ds
+log_must zfs create -o mountpoint=none $TESTPOOL/rename_test/child
+log_must zfs create $TESTPOOL/rename_test/child/grandchild
+
+if ! ismounted $TESTPOOL/rename_test; then
+	log_fail "$TESTPOOL/rename_test is not mounted"
+fi
+if ! ismounted $TESTPOOL/rename_test/ds; then
+	log_fail "$TESTPOOL/rename_test/ds is not mounted"
+fi
+if ismounted $TESTPOOL/rename_test/child; then
+	log_fail "$TESTPOOL/rename_test/child is mounted"
+fi
+if ismounted $TESTPOOL/rename_test/child/grandchild; then
+	log_fail "$TESTPOOL/rename_test/child/grandchild is mounted"
+fi
+
+log_must zfs rename $TESTPOOL/rename_test $TESTPOOL/renamed
+
+log_mustnot zfs list $TESTPOOL/rename_test
+log_mustnot zfs list $TESTPOOL/rename_test/ds
+log_mustnot zfs list $TESTPOOL/rename_test/child
+log_mustnot zfs list $TESTPOOL/rename_test/child/grandchild
+
+log_must zfs list $TESTPOOL/renamed
+log_must zfs list $TESTPOOL/renamed/ds
+log_must zfs list $TESTPOOL/renamed/child
+log_must zfs list $TESTPOOL/renamed/child/grandchild
+
+if ! ismounted $TESTPOOL/renamed; then
+        log_must zfs get all $TESTPOOL/renamed
+	log_fail "$TESTPOOL/renamed is not mounted"
+fi
+if ! ismounted $TESTPOOL/renamed/ds; then
+	log_fail "$TESTPOOL/renamed/ds is not mounted"
+fi
+if ismounted $TESTPOOL/renamed/child; then
+	log_fail "$TESTPOOL/renamed/child is mounted"
+fi
+if ismounted $TESTPOOL/renamed/child/grandchild; then
+	log_fail "$TESTPOOL/renamed/child/grandchild is mounted"
+fi
+
+log_pass "Verified rename for mountpoint=none children."