From c24fa4b19a1b117945f3235e014f926fe93b0c5a Mon Sep 17 00:00:00 2001 From: loli10K Date: Tue, 7 Jan 2020 00:40:06 +0100 Subject: [PATCH] Fix "zpool add -n" for dedup, special and log devices For dedup, special and log devices "zpool add -n" does not print correctly their vdev type: ~# zpool add -n pool dedup /tmp/dedup special /tmp/special log /tmp/log would update 'pool' to the following configuration: pool /tmp/normal /tmp/dedup /tmp/special /tmp/log This could lead storage administrators to modify their ZFS pools to unexpected and unintended vdev configurations. Reviewed-by: Brian Behlendorf Signed-off-by: loli10K Closes #9783 Closes #9390 --- cmd/zpool/zpool_main.c | 39 ++++++++---- include/zfs_comutil.h | 2 +- module/zcommon/zfs_comutil.c | 8 ++- module/zfs/spa.c | 2 +- .../cli_root/zpool_add/zpool_add_003_pos.ksh | 61 ++++++++++++------- 5 files changed, 75 insertions(+), 37 deletions(-) diff --git a/cmd/zpool/zpool_main.c b/cmd/zpool/zpool_main.c index f7696caac..77c826ba8 100644 --- a/cmd/zpool/zpool_main.c +++ b/cmd/zpool/zpool_main.c @@ -936,20 +936,35 @@ zpool_do_add(int argc, char **argv) print_vdev_tree(zhp, NULL, nvroot, 0, "", name_flags); /* print other classes: 'dedup', 'special', and 'log' */ - print_vdev_tree(zhp, "dedup", poolnvroot, 0, - VDEV_ALLOC_BIAS_DEDUP, name_flags); - print_vdev_tree(zhp, NULL, nvroot, 0, VDEV_ALLOC_BIAS_DEDUP, - name_flags); + if (zfs_special_devs(poolnvroot, VDEV_ALLOC_BIAS_DEDUP)) { + print_vdev_tree(zhp, "dedup", poolnvroot, 0, + VDEV_ALLOC_BIAS_DEDUP, name_flags); + print_vdev_tree(zhp, NULL, nvroot, 0, + VDEV_ALLOC_BIAS_DEDUP, name_flags); + } else if (zfs_special_devs(nvroot, VDEV_ALLOC_BIAS_DEDUP)) { + print_vdev_tree(zhp, "dedup", nvroot, 0, + VDEV_ALLOC_BIAS_DEDUP, name_flags); + } - print_vdev_tree(zhp, "special", poolnvroot, 0, - VDEV_ALLOC_BIAS_SPECIAL, name_flags); - print_vdev_tree(zhp, NULL, nvroot, 0, VDEV_ALLOC_BIAS_SPECIAL, - name_flags); + if (zfs_special_devs(poolnvroot, VDEV_ALLOC_BIAS_SPECIAL)) { + print_vdev_tree(zhp, "special", poolnvroot, 0, + VDEV_ALLOC_BIAS_SPECIAL, name_flags); + print_vdev_tree(zhp, NULL, nvroot, 0, + VDEV_ALLOC_BIAS_SPECIAL, name_flags); + } else if (zfs_special_devs(nvroot, VDEV_ALLOC_BIAS_SPECIAL)) { + print_vdev_tree(zhp, "special", nvroot, 0, + VDEV_ALLOC_BIAS_SPECIAL, name_flags); + } - print_vdev_tree(zhp, "logs", poolnvroot, 0, VDEV_ALLOC_BIAS_LOG, - name_flags); - print_vdev_tree(zhp, NULL, nvroot, 0, VDEV_ALLOC_BIAS_LOG, - name_flags); + if (num_logs(poolnvroot) > 0) { + print_vdev_tree(zhp, "logs", poolnvroot, 0, + VDEV_ALLOC_BIAS_LOG, name_flags); + print_vdev_tree(zhp, NULL, nvroot, 0, + VDEV_ALLOC_BIAS_LOG, name_flags); + } else if (num_logs(nvroot) > 0) { + print_vdev_tree(zhp, "logs", nvroot, 0, + VDEV_ALLOC_BIAS_LOG, name_flags); + } /* Do the same for the caches */ if (nvlist_lookup_nvlist_array(poolnvroot, ZPOOL_CONFIG_L2CACHE, diff --git a/include/zfs_comutil.h b/include/zfs_comutil.h index 7cdc6d693..17b07d9e4 100644 --- a/include/zfs_comutil.h +++ b/include/zfs_comutil.h @@ -34,7 +34,7 @@ extern "C" { #endif extern boolean_t zfs_allocatable_devs(nvlist_t *); -extern boolean_t zfs_special_devs(nvlist_t *); +extern boolean_t zfs_special_devs(nvlist_t *, char *); extern void zpool_get_load_policy(nvlist_t *, zpool_load_policy_t *); extern int zfs_zpl_version_map(int spa_version); diff --git a/module/zcommon/zfs_comutil.c b/module/zcommon/zfs_comutil.c index a3ff7d8e6..1cec60ac1 100644 --- a/module/zcommon/zfs_comutil.c +++ b/module/zcommon/zfs_comutil.c @@ -68,7 +68,7 @@ zfs_allocatable_devs(nvlist_t *nv) * Are there special vdevs? */ boolean_t -zfs_special_devs(nvlist_t *nv) +zfs_special_devs(nvlist_t *nv, char *type) { char *bias; uint_t c; @@ -84,7 +84,11 @@ zfs_special_devs(nvlist_t *nv) &bias) == 0) { if (strcmp(bias, VDEV_ALLOC_BIAS_SPECIAL) == 0 || strcmp(bias, VDEV_ALLOC_BIAS_DEDUP) == 0) { - return (B_TRUE); + if (type != NULL && strcmp(bias, type) == 0) { + return (B_TRUE); + } else if (type == NULL) { + return (B_TRUE); + } } } } diff --git a/module/zfs/spa.c b/module/zfs/spa.c index 6cadefe91..39b59d5ce 100644 --- a/module/zfs/spa.c +++ b/module/zfs/spa.c @@ -5690,7 +5690,7 @@ spa_create(const char *pool, nvlist_t *nvroot, nvlist_t *props, return (error); } } - if (!has_allocclass && zfs_special_devs(nvroot)) { + if (!has_allocclass && zfs_special_devs(nvroot, NULL)) { spa_deactivate(spa); spa_remove(spa); mutex_exit(&spa_namespace_lock); diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_add/zpool_add_003_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_add/zpool_add_003_pos.ksh index cfdc29d95..f27004130 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zpool_add/zpool_add_003_pos.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_add/zpool_add_003_pos.ksh @@ -34,26 +34,23 @@ # # DESCRIPTION: -# 'zpool add -n ...' can display the configuration without -# adding the specified devices to given pool +# 'zpool add -n ...' can display the configuration without adding +# the specified devices to given pool # # STRATEGY: -# 1. Create a storage pool -# 2. Use -n to add a device to the pool -# 3. Verify the device is not added actually +# 1. Create a storage pool +# 2. Use -n to add devices to the pool +# 3. Verify the devices are not added actually +# 4. Add devices to the pool for real this time, verify the vdev tree is the +# same printed by the dryrun iteration # verify_runnable "global" function cleanup { - poolexists $TESTPOOL && \ - destroy_pool $TESTPOOL - - partition_cleanup - - [[ -e $tmpfile ]] && \ - log_must rm -f $tmpfile + destroy_pool $TESTPOOL + rm -f $TMPFILE_PREFIX* $VDEV_PREFIX* } log_assert "'zpool add -n ...' can display the configuration" \ @@ -61,18 +58,40 @@ log_assert "'zpool add -n ...' can display the configuration" \ log_onexit cleanup -tmpfile="$TEST_BASE_DIR/zpool_add_003.tmp$$" +typeset TMPFILE_PREFIX="$TEST_BASE_DIR/zpool_add_003" +typeset STR_DRYRUN="would update '$TESTPOOL' to the following configuration:" +typeset VDEV_PREFIX="$TEST_BASE_DIR/filedev" +typeset -a VDEV_TYPES=("" "dedup" "special" "log" "cache") -create_pool "$TESTPOOL" "${disk}${SLICE_PREFIX}${SLICE0}" +vdevs="" +config="" + +# 1. Create a storage pool +log_must truncate -s $SPA_MINDEVSIZE "$VDEV_PREFIX-root" +log_must zpool create "$TESTPOOL" "$VDEV_PREFIX-root" log_must poolexists "$TESTPOOL" +for vdevtype in "${VDEV_TYPES[@]}"; do + log_must truncate -s $SPA_MINDEVSIZE "$VDEV_PREFIX-$vdevtype" + vdevs="$vdevs $VDEV_PREFIX-$vdevtype" + config="$config $vdevtype $VDEV_PREFIX-$vdevtype" +done -zpool add -n "$TESTPOOL" ${disk}${SLICE_PREFIX}${SLICE1} > $tmpfile +# 2. Use -n to add devices to the pool +log_must eval "zpool add -f -n $TESTPOOL $config > $TMPFILE_PREFIX-dryrun" +log_must grep -q "$STR_DRYRUN" "$TMPFILE_PREFIX-dryrun" -log_mustnot vdevs_in_pool "$TESTPOOL" "${disk}${SLICE_PREFIX}${SLICE1}" +# 3. Verify the devices are not added actually +for vdev in $vdevs; do + log_mustnot vdevs_in_pool "$TESTPOOL" "$vdev" +done -str="would update '$TESTPOOL' to the following configuration:" -cat $tmpfile | grep "$str" >/dev/null 2>&1 -(( $? != 0 )) && \ - log_fail "'zpool add -n ...' is executed as unexpected" +# 4. Add devices to the pool for real this time, verify the vdev tree is the +# same printed by the dryrun iteration +log_must zpool add -f $TESTPOOL $config +zpool status $TESTPOOL | awk 'NR == 1, /NAME/ { next } /^$/ {exit} + {print $1}' > "$TMPFILE_PREFIX-vdevtree" +cat "$TMPFILE_PREFIX-dryrun" | awk 'NR == 1, /would/ {next} + {print $1}' > "$TMPFILE_PREFIX-vdevtree-n" +log_must eval "diff $TMPFILE_PREFIX-vdevtree-n $TMPFILE_PREFIX-vdevtree" -log_pass "'zpool add -n ...'executes successfully." +log_pass "'zpool add -n ...' executes successfully."