From 261c013fbf79431ac79def2cdf56d9d82009cd4d Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Fri, 26 May 2017 11:40:44 -0700 Subject: [PATCH] Revert "Fix "snapdev" property inheritance behaviour" 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 Reviewed-by: George Melikov Signed-off-by: Brian Behlendorf Closes #6174 Issue #6131 --- module/zfs/zfs_ioctl.c | 97 +++++------ module/zfs/zvol.c | 27 +-- tests/runfiles/linux.run | 3 +- .../functional/zvol/zvol_misc/Makefile.am | 3 +- .../zvol/zvol_misc/zvol_misc_snapdev.ksh | 159 ------------------ 5 files changed, 59 insertions(+), 230 deletions(-) delete mode 100755 tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index 4fda36c7f0..c6a5321239 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -2761,12 +2761,54 @@ 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; + + /* + * zfs_prop_set_special() expects properties in the form of an + * nvpair with type info. + */ + if (prop == ZPROP_INVAL) { + if (!zfs_prop_user(propname)) + return (SET_ERROR(EINVAL)); + + 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); + } + + 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: + nvlist_free(dummy); + return (SET_ERROR(EINVAL)); + } + + 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); + 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 @@ -2777,49 +2819,8 @@ zfs_ioc_inherit_prop(zfs_cmd_t *zc) return (SET_ERROR(EINVAL)); } - if (prop == ZPROP_INVAL) { - if (!zfs_prop_user(propname)) - return (SET_ERROR(EINVAL)); - - 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(); - - 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; - } - - pair = nvlist_next_nvpair(dummy, NULL); - if (pair == NULL) { - err = SET_ERROR(EINVAL); - } else { - 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); - } - -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 diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c index 33b1f4321e..121f75e4e2 100644 --- a/module/zfs/zvol.c +++ b/module/zfs/zvol.c @@ -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); diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index 674684f1fd..7e64c67140 100644 --- a/tests/runfiles/linux.run +++ b/tests/runfiles/linux.run @@ -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', diff --git a/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am b/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am index f729704902..b52088cccd 100644 --- a/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am +++ b/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am @@ -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 index a2f8d47517..0000000000 --- a/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh +++ /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 . 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"