From 39a2d68fe8ed4a271beb3ad3983d2a2790389f98 Mon Sep 17 00:00:00 2001 From: Umer Saleem Date: Tue, 5 Sep 2023 13:27:53 +0500 Subject: [PATCH] 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 Reviewed-by: Alexander Motin Reviewed-by: Ameer Hamza Signed-off-by: Umer Saleem Closes #15240 --- cmd/zfs/zfs_main.c | 7 ++++--- lib/libzfs/libzfs_changelist.c | 8 ++++---- .../cli_root/zfs_mount/zfs_mount_006_pos.ksh | 2 +- .../cli_root/zfs_mount/zfs_mount_008_pos.ksh | 4 ++-- .../cli_root/zfs_mount/zfs_mount_012_pos.ksh | 15 +++++++-------- .../cli_root/zfs_set/mountpoint_002_pos.ksh | 12 +++++++++--- .../cli_root/zfs_set/zfs_set_003_neg.ksh | 10 +++++++--- 7 files changed, 34 insertions(+), 24 deletions(-) diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 5ed25d1ea720..b0f5d7024951 100644 --- 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 index dd14c570ec03..4b0f66964346 100644 --- 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 index 2a2466f65c02..e9ab472795eb 100755 --- 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 index 2c1029d551cf..0437c61a2c40 100755 --- 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 index 5ff094d2c479..66958f2f0884 100755 --- 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 index a5785226e02e..c227a6fb8aa8 100755 --- 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 index 3afb0eb7010e..5901ba7dc461 100755 --- 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"