]> granicus.if.org Git - zfs/commitdiff
Revert "Fix "snapdev" property inheritance behaviour"
authorBrian Behlendorf <behlendorf1@llnl.gov>
Fri, 26 May 2017 18:40:44 +0000 (11:40 -0700)
committerGitHub <noreply@github.com>
Fri, 26 May 2017 18:40:44 +0000 (11:40 -0700)
This reverts commit 959f56b99366c8727647b5b19fb3d47555c96cf3.
An issue was uncovered by the new zvol_misc_snapdev test case
which needs to be investigated and resolved.

Reviewed-by: loli10K <ezomori.nozomu@gmail.com>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #6174
Issue #6131

module/zfs/zfs_ioctl.c
module/zfs/zvol.c
tests/runfiles/linux.run
tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am
tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh [deleted file]

index 4fda36c7f047a742c199064e3e397b2b2a8722aa..c6a5321239a23b510f56276a56eff80fc60a825c 100644 (file)
@@ -2761,65 +2761,66 @@ zfs_ioc_inherit_prop(zfs_cmd_t *zc)
        zprop_source_t source = (received
            ? ZPROP_SRC_NONE            /* revert to received value, if any */
            : ZPROP_SRC_INHERITED);     /* explicitly inherit */
-       nvlist_t *dummy;
-       nvpair_t *pair;
-       zprop_type_t type;
-       int err;
 
-       if (!received) {
+       if (received) {
+               nvlist_t *dummy;
+               nvpair_t *pair;
+               zprop_type_t type;
+               int err;
+
                /*
-                * Only check this in the non-received case. We want to allow
-                * 'inherit -S' to revert non-inheritable properties like quota
-                * and reservation to the received or default values even though
-                * they are not considered inheritable.
+                * zfs_prop_set_special() expects properties in the form of an
+                * nvpair with type info.
                 */
-               if (prop != ZPROP_INVAL && !zfs_prop_inheritable(prop))
-                       return (SET_ERROR(EINVAL));
-       }
+               if (prop == ZPROP_INVAL) {
+                       if (!zfs_prop_user(propname))
+                               return (SET_ERROR(EINVAL));
 
-       if (prop == ZPROP_INVAL) {
-               if (!zfs_prop_user(propname))
+                       type = PROP_TYPE_STRING;
+               } else if (prop == ZFS_PROP_VOLSIZE ||
+                   prop == ZFS_PROP_VERSION) {
                        return (SET_ERROR(EINVAL));
+               } else {
+                       type = zfs_prop_get_type(prop);
+               }
 
-               type = PROP_TYPE_STRING;
-       } else if (prop == ZFS_PROP_VOLSIZE || prop == ZFS_PROP_VERSION) {
-               return (SET_ERROR(EINVAL));
-       } else {
-               type = zfs_prop_get_type(prop);
-       }
-
-       /*
-        * zfs_prop_set_special() expects properties in the form of an
-        * nvpair with type info.
-        */
-       dummy = fnvlist_alloc();
+               VERIFY(nvlist_alloc(&dummy, NV_UNIQUE_NAME, KM_SLEEP) == 0);
 
-       switch (type) {
-       case PROP_TYPE_STRING:
-               VERIFY(0 == nvlist_add_string(dummy, propname, ""));
-               break;
-       case PROP_TYPE_NUMBER:
-       case PROP_TYPE_INDEX:
-               VERIFY(0 == nvlist_add_uint64(dummy, propname, 0));
-               break;
-       default:
-               err = SET_ERROR(EINVAL);
-               goto errout;
-       }
+               switch (type) {
+               case PROP_TYPE_STRING:
+                       VERIFY(0 == nvlist_add_string(dummy, propname, ""));
+                       break;
+               case PROP_TYPE_NUMBER:
+               case PROP_TYPE_INDEX:
+                       VERIFY(0 == nvlist_add_uint64(dummy, propname, 0));
+                       break;
+               default:
+                       nvlist_free(dummy);
+                       return (SET_ERROR(EINVAL));
+               }
 
-       pair = nvlist_next_nvpair(dummy, NULL);
-       if (pair == NULL) {
-               err = SET_ERROR(EINVAL);
-       } else {
+               pair = nvlist_next_nvpair(dummy, NULL);
+               if (pair == NULL) {
+                       nvlist_free(dummy);
+                       return (SET_ERROR(EINVAL));
+               }
                err = zfs_prop_set_special(zc->zc_name, source, pair);
-               if (err == -1) /* property is not "special", needs handling */
-                       err = dsl_prop_inherit(zc->zc_name, zc->zc_value,
-                           source);
+               nvlist_free(dummy);
+               if (err != -1)
+                       return (err); /* special property already handled */
+       } else {
+               /*
+                * Only check this in the non-received case. We want to allow
+                * 'inherit -S' to revert non-inheritable properties like quota
+                * and reservation to the received or default values even though
+                * they are not considered inheritable.
+                */
+               if (prop != ZPROP_INVAL && !zfs_prop_inheritable(prop))
+                       return (SET_ERROR(EINVAL));
        }
 
-errout:
-       nvlist_free(dummy);
-       return (err);
+       /* property name has been validated by zfs_secpolicy_inherit_prop() */
+       return (dsl_prop_inherit(zc->zc_name, zc->zc_value, source));
 }
 
 static int
index 33b1f4321e22614473cb3736b0d02ec1fcb4b436..121f75e4e257de77e5e637cb64dc062104446a70 100644 (file)
@@ -2216,18 +2216,20 @@ zvol_set_snapdev_check(void *arg, dmu_tx_t *tx)
        return (error);
 }
 
-/* ARGSUSED */
 static int
 zvol_set_snapdev_sync_cb(dsl_pool_t *dp, dsl_dataset_t *ds, void *arg)
 {
+       zvol_set_snapdev_arg_t *zsda = arg;
        char dsname[MAXNAMELEN];
        zvol_task_t *task;
-       uint64_t snapdev;
 
        dsl_dataset_name(ds, dsname);
-       if (dsl_prop_get_int_ds(ds, "snapdev", &snapdev) != 0)
-               return (0);
-       task = zvol_task_alloc(ZVOL_ASYNC_SET_SNAPDEV, dsname, NULL, snapdev);
+       dsl_prop_set_sync_impl(ds, zfs_prop_to_name(ZFS_PROP_SNAPDEV),
+           zsda->zsda_source, sizeof (zsda->zsda_value), 1,
+           &zsda->zsda_value, zsda->zsda_tx);
+
+       task = zvol_task_alloc(ZVOL_ASYNC_SET_SNAPDEV, dsname,
+           NULL, zsda->zsda_value);
        if (task == NULL)
                return (0);
 
@@ -2237,11 +2239,7 @@ zvol_set_snapdev_sync_cb(dsl_pool_t *dp, dsl_dataset_t *ds, void *arg)
 }
 
 /*
- * Traverse all child datasets and apply snapdev appropriately.
- * We call dsl_prop_set_sync_impl() here to set the value only on the toplevel
- * dataset and read the effective "snapdev" on every child in the callback
- * function: this is because the value is not guaranteed to be the same in the
- * whole dataset hierarchy.
+ * Traverse all child snapshot datasets and apply snapdev appropriately.
  */
 static void
 zvol_set_snapdev_sync(void *arg, dmu_tx_t *tx)
@@ -2249,19 +2247,10 @@ zvol_set_snapdev_sync(void *arg, dmu_tx_t *tx)
        zvol_set_snapdev_arg_t *zsda = arg;
        dsl_pool_t *dp = dmu_tx_pool(tx);
        dsl_dir_t *dd;
-       dsl_dataset_t *ds;
-       int error;
 
        VERIFY0(dsl_dir_hold(dp, zsda->zsda_name, FTAG, &dd, NULL));
        zsda->zsda_tx = tx;
 
-       error = dsl_dataset_hold(dp, zsda->zsda_name, FTAG, &ds);
-       if (error == 0) {
-               dsl_prop_set_sync_impl(ds, zfs_prop_to_name(ZFS_PROP_SNAPDEV),
-                   zsda->zsda_source, sizeof (zsda->zsda_value), 1,
-                   &zsda->zsda_value, zsda->zsda_tx);
-               dsl_dataset_rele(ds, FTAG);
-       }
        dmu_objset_find_dp(dp, dd->dd_object, zvol_set_snapdev_sync_cb,
            zsda, DS_FIND_CHILDREN);
 
index 674684f1fdc0283886ec36113946f8622cd81714..7e64c67140b12b4f080b0d49fa3d946f42f7dc38 100644 (file)
@@ -564,8 +564,7 @@ tests = ['zvol_cli_001_pos', 'zvol_cli_002_pos', 'zvol_cli_003_neg']
 
 [tests/functional/zvol/zvol_misc]
 tests = ['zvol_misc_001_neg', 'zvol_misc_002_pos', 'zvol_misc_003_neg',
-    'zvol_misc_004_pos', 'zvol_misc_005_neg', 'zvol_misc_006_pos',
-    'zvol_misc_snapdev']
+    'zvol_misc_004_pos', 'zvol_misc_005_neg', 'zvol_misc_006_pos']
 
 [tests/functional/zvol/zvol_swap]
 tests = ['zvol_swap_001_pos', 'zvol_swap_002_pos', 'zvol_swap_003_pos',
index f729704902596322f8a5a778f7546e2c8581d833..b52088cccdc33011a93e32198a903452ca0dffcf 100644 (file)
@@ -7,5 +7,4 @@ dist_pkgdata_SCRIPTS = \
        zvol_misc_003_neg.ksh \
        zvol_misc_004_pos.ksh \
        zvol_misc_005_neg.ksh \
-       zvol_misc_006_pos.ksh \
-       zvol_misc_snapdev.ksh
+       zvol_misc_006_pos.ksh
diff --git a/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh b/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh
deleted file mode 100755 (executable)
index a2f8d47..0000000
+++ /dev/null
@@ -1,159 +0,0 @@
-#!/bin/ksh -p
-#
-# CDDL HEADER START
-#
-# The contents of this file are subject to the terms of the
-# Common Development and Distribution License (the "License").
-# You may not use this file except in compliance with the License.
-#
-# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
-# or http://www.opensolaris.org/os/licensing.
-# See the License for the specific language governing permissions
-# and limitations under the License.
-#
-# When distributing Covered Code, include this CDDL HEADER in each
-# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
-# If applicable, add the following below this CDDL HEADER, with the
-# fields enclosed by brackets "[]" replaced with your own identifying
-# information: Portions Copyright [yyyy] [name of copyright owner]
-#
-# CDDL HEADER END
-#
-
-#
-# Copyright 2017, loli10K <ezomori.nozomu@gmail.com>. All rights reserved.
-#
-
-. $STF_SUITE/include/libtest.shlib
-. $STF_SUITE/tests/functional/cli_root/zfs_set/zfs_set_common.kshlib
-. $STF_SUITE/tests/functional/zvol/zvol_common.shlib
-
-#
-# DESCRIPTION:
-# Verify that ZFS volume property "snapdev" works as intended.
-#
-# STRATEGY:
-# 1. Verify "snapdev" property does not accept invalid values
-# 2. Verify "snapdev" adds and removes device nodes when updated
-# 3. Verify "snapdev" is inherited correctly
-#
-
-verify_runnable "global"
-
-function cleanup
-{
-       datasetexists $VOLFS && log_must zfs destroy -r $VOLFS
-       datasetexists $ZVOL && log_must zfs destroy -r $ZVOL
-       log_must zfs inherit snapdev $TESTPOOL
-       block_device_wait
-}
-
-#
-# Verify $device exists and is a block device
-#
-function blockdev_exists # device
-{
-       typeset device="$1"
-
-       if [[ ! -b "$device" ]]; then
-               log_fail "$device does not exist as a block device"
-       fi
-}
-
-#
-# Verify $device does not exist
-#
-function check_missing # device
-{
-       typeset device="$1"
-
-       if [[ -e "$device" ]]; then
-               log_fail "$device exists when not expected"
-       fi
-}
-
-#
-# Verify $property on $dataset is inherited by $parent and is set to $value
-#
-function verify_inherited # property value dataset parent
-{
-       typeset property="$1"
-       typeset value="$2"
-       typeset dataset="$3"
-       typeset parent="$4"
-
-       typeset val=$(get_prop "$property" "$dataset")
-       typeset src=$(get_source "$property" "$dataset")
-       if [[ "$val" != "$value" || "$src" != "inherited from $parent" ]]
-       then
-               log_fail "Dataset $dataset did not inherit $property properly:"\
-                   "expected=$value, value=$val, source=$src."
-       fi
-
-}
-
-log_assert "Verify that ZFS volume property 'snapdev' works as expected."
-log_onexit cleanup
-
-VOLFS="$TESTPOOL/volfs"
-ZVOL="$TESTPOOL/vol"
-SNAP="$ZVOL@snap"
-SNAPDEV="${ZVOL_DEVDIR}/$SNAP"
-SUBZVOL="$VOLFS/subvol"
-SUBSNAP="$SUBZVOL@snap"
-SUBSNAPDEV="${ZVOL_DEVDIR}/$SUBSNAP"
-
-log_must zfs create -o mountpoint=none $VOLFS
-log_must zfs create -V $VOLSIZE -s $ZVOL
-log_must zfs create -V $VOLSIZE -s $SUBZVOL
-
-# 1. Verify "snapdev" property does not accept invalid values
-typeset badvals=("off" "on" "1" "nope" "-")
-for badval in ${badvals[@]}
-do
-       log_mustnot zfs set snapdev="$badval" $ZVOL
-done
-
-# 2. Verify "snapdev" adds and removes device nodes when updated
-# 2.1 First create a snapshot then change snapdev property
-log_must zfs snapshot $SNAP
-log_must zfs set snapdev=visible $ZVOL
-block_device_wait
-blockdev_exists $SNAPDEV
-log_must zfs set snapdev=hidden $ZVOL
-block_device_wait
-check_missing $SNAPDEV
-log_must zfs destroy $SNAP
-# 2.2 First set snapdev property then create a snapshot
-log_must zfs set snapdev=visible $ZVOL
-log_must zfs snapshot $SNAP
-block_device_wait
-blockdev_exists $SNAPDEV
-log_must zfs destroy $SNAP
-block_device_wait
-check_missing $SNAPDEV
-
-# 3. Verify "snapdev" is inherited correctly
-# 3.1 Check snapdev=visible case
-log_must zfs snapshot $SNAP
-log_must zfs inherit snapdev $ZVOL
-log_must zfs set snapdev=visible $TESTPOOL
-verify_inherited 'snapdev' 'visible' $ZVOL $TESTPOOL
-block_device_wait
-blockdev_exists $SNAPDEV
-# 3.2 Check snapdev=hidden case
-log_must zfs set snapdev=hidden $TESTPOOL
-verify_inherited 'snapdev' 'hidden' $ZVOL $TESTPOOL
-block_device_wait
-check_missing $SNAPDEV
-# 3.3 Check inheritance on multiple levels
-log_must zfs snapshot $SUBSNAP
-log_must zfs inherit snapdev $SUBZVOL
-log_must zfs set snapdev=hidden $VOLFS
-log_must zfs set snapdev=visible $TESTPOOL
-verify_inherited 'snapdev' 'hidden' $SUBZVOL $VOLFS
-block_device_wait
-check_missing $SUBSNAPDEV
-blockdev_exists $SNAPDEV
-
-log_pass "ZFS volume property 'snapdev' works as expected"