Update the behavior of mountpoint property

There are some inconsistencies in the handling of mountpoint property. This commit updates the behavior and makes it consistent. If mountpoint property is set when dataset is unmounted, this would update the mountpoint property. The mountpoint could be valid or invalid in this case. Setting the mountpoint property would result in success in this case. Dataset would still be unmounted here. On the other hand, if dataset is mounted and mountpoint property is updated to something invalid where mount cannot be successful, for example, setting the mountpoint inside a readonly directory. This would unmount the dataset, set the mountpoint property to requested value and tries to mount the dataset. The mount operation returns error and this error is treated as overall failure of setting the property while the property is actually set. To make the behavior consistent in case dataset is mounted or unmounted, we should try to mount the dataset whenever mountpoint property is updated. This would result in mounting the datasets if canmount property is set to on, regardless if the dataset was previously unmounted. The failure in mount operation while setting the mountpoint property should not be treated as failure, since the property is actually set now to user requested value. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Ameer Hamza <[email protected]> Signed-off-by: Umer Saleem <[email protected]> Closes openzfs#15240
truenas · Sep 20, 2023 · 39a2d68 · 39a2d68
1 parent 93337f5
commit 39a2d68
Show file tree

Hide file tree

Showing 7 changed files with 34 additions and 24 deletions.
diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c
@@ -4198,8 +4198,9 @@ static int
 set_callback(zfs_handle_t *zhp, void *data)
 {
 	nvlist_t *props = data;
+	int ret = zfs_prop_set_list(zhp, props);
 
-	if (zfs_prop_set_list(zhp, props) != 0) {
+	if (ret != 0 || libzfs_errno(g_zfs) != EZFS_SUCCESS) {
 		switch (libzfs_errno(g_zfs)) {
 		case EZFS_MOUNTFAILED:
 			(void) fprintf(stderr, gettext("property may be set "
@@ -4208,11 +4209,11 @@ set_callback(zfs_handle_t *zhp, void *data)
 		case EZFS_SHARENFSFAILED:
 			(void) fprintf(stderr, gettext("property may be set "
 			    "but unable to reshare filesystem\n"));
+			ret = 1;
 			break;
 		}
-		return (1);
 	}
-	return (0);
+	return (ret);
 }
 
 static int

diff --git a/lib/libzfs/libzfs_changelist.c b/lib/libzfs/libzfs_changelist.c
@@ -244,13 +244,13 @@ changelist_postfix(prop_changelist_t *clp)
 		    zfs_is_mounted(cn->cn_handle, NULL);
 
 		if (!mounted && !needs_key && (cn->cn_mounted ||
-		    ((sharenfs || sharesmb || clp->cl_waslegacy) &&
+		    (((clp->cl_prop == ZFS_PROP_MOUNTPOINT &&
+		    clp->cl_prop == clp->cl_realprop) ||
+		    sharenfs || sharesmb || clp->cl_waslegacy) &&
 		    (zfs_prop_get_int(cn->cn_handle,
 		    ZFS_PROP_CANMOUNT) == ZFS_CANMOUNT_ON)))) {
 
-			if (zfs_mount(cn->cn_handle, NULL, 0) != 0)
-				errors++;
-			else
+			if (zfs_mount(cn->cn_handle, NULL, 0) == 0)
 				mounted = TRUE;
 		}
 

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh
@@ -94,7 +94,7 @@ while (( depth < MAXDEPTH )); do
 done
 
 log_must zfs set mountpoint=$mtpt $TESTPOOL/$TESTFS
-log_must zfs $mountcmd $TESTPOOL/$TESTFS
+log_must ismounted $TESTPOOL/$TESTFS
 
 log_must zfs set overlay=off $TESTPOOL/$TESTFS
 if ! is_illumos; then

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh
@@ -71,7 +71,7 @@ log_must mkfile 1M $testfile $testfile1
 
 log_must zfs unmount $fs1
 log_must zfs set mountpoint=$mntpnt $fs1
-log_must zfs mount $fs1
+log_must ismounted $fs1
 log_must zfs unmount $fs1
 log_must zfs mount -O $fs1
 
@@ -85,7 +85,7 @@ log_must ls $mntpnt/$TESTFILE1 $mntpnt/$TESTFILE2
 # Verify $TESTFILE2 was created in $fs1, rather than $fs
 log_must zfs unmount $fs1
 log_must zfs set mountpoint=$mntpnt1 $fs1
-log_must zfs mount $fs1
+log_must ismounted $fs1
 log_must ls $testfile1 $mntpnt1/$TESTFILE2
 
 # Verify $TESTFILE2 was not created in $fs, and $fs is accessible again.

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh
@@ -25,13 +25,12 @@
 # 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 succeeds
+# 3. Set the dataset's mountpoint, this should mount the dataset
+# 4. Verify the mount succeeds
+# 5. Unmount the dataset
+# 6. Create a file in the directory created in step 2
+# 7. Attempt to mount the dataset
+# 8. Verify the mount succeeds
 #
 
 verify_runnable "both"
@@ -43,7 +42,7 @@ 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 ismounted $fs
 log_must zfs umount $fs
 log_must touch $TESTDIR/testfile.$$
 log_must zfs mount $fs

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh
@@ -35,7 +35,9 @@
 #
 # DESCRIPTION:
 #	If ZFS is currently managing the file system but it is currently unmounted,
-#	and the mountpoint property is changed, the file system remains unmounted.
+#	and the mountpoint property is changed, the file system should be mounted
+#	if it is a valid mountpoint and canmount allows to mount, otherwise it
+#	should not be mounted.
 #
 # STRATEGY:
 # 1. Setup a pool and create fs, ctr within it.
@@ -62,7 +64,7 @@ function cleanup
 }
 
 log_assert "Setting a valid mountpoint for an unmounted file system, \
-	it remains unmounted."
+	it gets mounted."
 log_onexit cleanup
 
 old_fs_mpt=$(get_prop mountpoint $TESTPOOL/$TESTFS)
@@ -83,7 +85,11 @@ while (( i < ${#dataset[@]} )); do
 	while (( j < ${#values[@]} )); do
 		set_n_check_prop "${values[j]}" "mountpoint" \
 			"${dataset[i]}"
-		log_mustnot ismounted ${dataset[i]}
+		if [ "${dataset[i]}" = "$TESTPOOL/$TESTFS" ]; then
+			log_must ismounted ${dataset[i]}
+		else
+			log_mustnot ismounted ${dataset[i]}
+		fi
 		(( j += 1 ))
 	done
 	cleanup

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh
@@ -33,7 +33,9 @@
 
 #
 # DESCRIPTION:
-# 'zfs set mountpoint/sharenfs' should fail when the mountpoint is invalid
+# 'zfs set mountpoint/sharenfs' should set the property when mountpoint
+#  is invalid. Setting the property should be successful, but dataset
+# should not be mounted, as mountpoint is invalid.
 #
 # STRATEGY:
 # 1. Create invalid scenarios
@@ -62,10 +64,12 @@ longpath=$(gen_dataset_name 1030 "abcdefg")
 log_must zfs create -o mountpoint=legacy $TESTPOOL/foo
 
 # Do the negative testing about "property may be set but unable to remount filesystem"
-log_mustnot eval "zfs set mountpoint=$badpath $TESTPOOL/foo >/dev/null 2>&1"
+set_n_check_prop "$badpath" "mountpoint" "$TESTPOOL/foo"
+log_mustnot ismounted $TESTPOOL/foo
 
 # Do the negative testing about "property may be set but unable to reshare filesystem"
-log_mustnot eval "zfs set sharenfs=on $TESTPOOL/foo >/dev/null 2>&1"
+set_n_check_prop "on" "sharenfs" "$TESTPOOL/foo"
+log_mustnot ismounted $TESTPOOL/foo
 
 # Do the negative testing about "sharenfs property can not be set to null"
 log_mustnot eval "zfs set sharenfs= $TESTPOOL/foo >/dev/null 2>&1"