From f583c323df68d612fffe87d30ea2168b64eccf5a Mon Sep 17 00:00:00 2001 From: Alexander Motin Date: Thu, 19 Dec 2024 11:54:44 -0500 Subject: [PATCH 1/4] Fix readonly check for vdev user properties VDEV_PROP_USERPROP is equal do VDEV_PROP_INVAL and so is not a real property. That's why vdev_prop_readonly() does not work right for it. In particular it may declare all vdev user properties readonly on FreeBSD. Signed-off-by: Alexander Motin Sponsored by: iXsystems, Inc. --- module/zfs/vdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/zfs/vdev.c b/module/zfs/vdev.c index 250590f062ea..85b6ee32158d 100644 --- a/module/zfs/vdev.c +++ b/module/zfs/vdev.c @@ -5969,7 +5969,7 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl) goto end; } - if (vdev_prop_readonly(prop)) { + if (prop != VDEV_PROP_USERPROP && vdev_prop_readonly(prop)) { error = EROFS; goto end; } From 6a9fddf76ce44b2f01c651bccbd5f729d02df6c4 Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Thu, 19 Dec 2024 20:11:54 +1100 Subject: [PATCH 2/4] zpool_get_vdev_prop_value: show missing vdev userprops If a vdev userprop is not found, present it as value '-', default source, so it matches the output from pool userprops. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris --- lib/libzfs/libzfs_pool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/libzfs/libzfs_pool.c b/lib/libzfs/libzfs_pool.c index f256535e8ea0..64f9d1f6eb49 100644 --- a/lib/libzfs/libzfs_pool.c +++ b/lib/libzfs/libzfs_pool.c @@ -5342,7 +5342,8 @@ zpool_get_vdev_prop_value(nvlist_t *nvprop, vdev_prop_t prop, char *prop_name, strval = fnvlist_lookup_string(nv, ZPROP_VALUE); } else { /* user prop not found */ - return (-1); + src = ZPROP_SRC_DEFAULT; + strval = "-"; } (void) strlcpy(buf, strval, len); if (srctype) From 59bae27310e7370ec453ffdc4e768cf62b3862ba Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Thu, 19 Dec 2024 20:11:54 +1100 Subject: [PATCH 3/4] spa_sync_props: remove pool userprops by setting empty-string People have noted there's no way to remove a pool userprop, only zero it. Turns vdev userprops had a method, by setting empty-string. So this makes pool userprops follow the same behaviour. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris --- module/zfs/spa.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/module/zfs/spa.c b/module/zfs/spa.c index b83c982c13fd..c93c7945f192 100644 --- a/module/zfs/spa.c +++ b/module/zfs/spa.c @@ -9683,9 +9683,17 @@ spa_sync_props(void *arg, dmu_tx_t *tx) if (nvpair_type(elem) == DATA_TYPE_STRING) { ASSERT(proptype == PROP_TYPE_STRING); strval = fnvpair_value_string(elem); - VERIFY0(zap_update(mos, - spa->spa_pool_props_object, propname, - 1, strlen(strval) + 1, strval, tx)); + if (strlen(strval) == 0) { + /* remove the property if value == "" */ + (void) zap_remove(mos, + spa->spa_pool_props_object, + propname, tx); + } else { + VERIFY0(zap_update(mos, + spa->spa_pool_props_object, + propname, 1, strlen(strval) + 1, + strval, tx)); + } spa_history_log_internal(spa, "set", tx, "%s=%s", elemname, strval); } else if (nvpair_type(elem) == DATA_TYPE_UINT64) { From d67914eb466f768201e0c075f379d3927f44fae3 Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Thu, 19 Dec 2024 20:11:54 +1100 Subject: [PATCH 4/4] ZTS: test clearing pool and vdev userprops Confirming that clearing pool and vdev userprops produce the same result: an empty value, with default source. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris --- tests/runfiles/common.run | 3 +- tests/zfs-tests/tests/Makefile.am | 1 + .../zpool_set/zpool_set_clear_userprop.ksh | 44 +++++++++++++++++++ .../zpool_set/zpool_set_common.kshlib | 40 ++++++++++++++++- 4 files changed, 85 insertions(+), 3 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_clear_userprop.ksh diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index a69d36df2f98..6abb3b4213bb 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -544,7 +544,8 @@ tags = ['functional', 'cli_root', 'zpool_scrub'] [tests/functional/cli_root/zpool_set] tests = ['zpool_set_001_pos', 'zpool_set_002_neg', 'zpool_set_003_neg', 'zpool_set_ashift', 'zpool_set_features', 'vdev_set_001_pos', - 'user_property_001_pos', 'user_property_002_neg'] + 'user_property_001_pos', 'user_property_002_neg', + 'zpool_set_clear_userprop'] tags = ['functional', 'cli_root', 'zpool_set'] [tests/functional/cli_root/zpool_split] diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index 67630cb564ae..588249be45da 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -1243,6 +1243,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/cli_root/zpool_set/user_property_001_pos.ksh \ functional/cli_root/zpool_set/user_property_002_neg.ksh \ functional/cli_root/zpool_set/zpool_set_features.ksh \ + functional/cli_root/zpool_set/zpool_set_clear_userprop.ksh \ functional/cli_root/zpool_split/cleanup.ksh \ functional/cli_root/zpool_split/setup.ksh \ functional/cli_root/zpool_split/zpool_split_cliargs.ksh \ diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_clear_userprop.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_clear_userprop.ksh new file mode 100755 index 000000000000..d9395ea8a15b --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_clear_userprop.ksh @@ -0,0 +1,44 @@ +#!/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 https://opensource.org/licenses/CDDL-1.0. +# 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 (c) 2024, Klara, Inc. +# + +. $STF_SUITE/tests/functional/cli_root/zpool_set/zpool_set_common.kshlib + +verify_runnable "both" + +log_assert "Setting a user-defined property to the empty string removes it." +log_onexit cleanup_user_prop $TESTPOOL + +log_must zpool set cool:pool=hello $TESTPOOL +log_must check_user_prop $TESTPOOL cool:pool hello local +log_must zpool set cool:pool= $TESTPOOL +log_must check_user_prop $TESTPOOL cool:pool '-' default + +log_must zpool set cool:vdev=goodbye $TESTPOOL root +log_must check_vdev_user_prop $TESTPOOL root cool:vdev goodbye local +log_must zpool set cool:vdev= $TESTPOOL root +log_must check_vdev_user_prop $TESTPOOL root cool:vdev '-' default + +log_pass "Setting a user-defined property to the empty string removes it." diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_common.kshlib b/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_common.kshlib index 346e4a16b2ad..e095d315c2b5 100644 --- a/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_common.kshlib +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_common.kshlib @@ -160,19 +160,55 @@ function user_property_value random_string ALL_CHAR $len } +function _check_user_prop +{ + typeset pool="$1" + typeset vdev="$2" + typeset user_prop="$3" + typeset expect_value="$4" + typeset expect_source="$5" + + typeset -a \ + v=($(zpool get -p -H -o value,source "$user_prop" $pool $vdev 2>&1)) + + [[ "$expect_value" == "${v[0]}" && \ + -z "$expect_source" || "$expect_source" == "${v[1]}" ]] +} + # # Check if the user-defined property is identical to the expected value. # # $1 pool # $2 user property # $3 expected value +# $4 expected source (optional) # function check_user_prop { typeset pool=$1 typeset user_prop="$2" typeset expect_value="$3" - typeset value=$(zpool get -p -H -o value "$user_prop" $pool 2>&1) + typeset expect_source="${4:-}" + + _check_user_prop $pool '' $user_prop $expect_value $expect_source +} + +# +# Check if the user-defined property is identical to the expected value. +# +# $1 pool +# $2 vdev +# $3 user property +# $4 expected value +# $5 expected source (optional) +# +function check_vdev_user_prop +{ + typeset pool="$1" + typeset vdev="$2" + typeset user_prop="$3" + typeset expect_value="$4" + typeset expect_source="${5:-}" - [ "$expect_value" = "$value" ] + _check_user_prop $pool $vdev $user_prop $expect_value $expect_source }