Fix self-deadlock when setting the "allocating"/"path" vdev property

zfs_ioc_vdev_set_props() acquires the SCL_CONFIG lock as a reader and
holds it across the call to vdev_prop_set(). For the "allocating"
property, vdev_prop_set() calls spa_vdev_noalloc()/spa_vdev_alloc(),
which descend through spa_vdev_enter() into spa_config_enter(spa,
SCL_ALL, RW_WRITER); the "path" property does the same via
spa_vdev_setpath().

Acquiring SCL_CONFIG as a writer while the same thread already holds it
as a reader is a self-deadlock: the writer waits for scl_count to drain
to zero, but scl_count is the thread's own reader, which is not released
until vdev_prop_set() returns. As a result "zpool set allocating=off|on
<vdev>" hangs the calling thread, and txg_sync, which also needs
SCL_CONFIG as a reader, stalls behind it and freezes the pool.

The SCL_CONFIG reader was added by commit d65015938e ("Vdev allocation
bias/class change", #18493) to keep the vdev tree stable across the guid
lookup and the property handling.

The vdev pointer the callers resolved and passed in is unnecessary.
vdev_prop_set() re-resolves the vdev by guid from innvl anyway, so the
pointer was redundant there. vdev_prop_get() did use the passed vdev,
but it also looks up the guid from innvl (which it then never uses), so
it can resolve the vdev itself just as easily. Pass the spa to both
functions instead of a vdev and resolve the vdev by guid inside them
under SCL_CONFIG as a reader. vdev_prop_get() is read-only and holds the
reader across the whole fetch. vdev_prop_set() holds it while touching
the vdev but drops it around the "path" and "allocating" handlers, which
descend into spa_vdev_enter() -> spa_config_enter(spa, SCL_ALL,
RW_WRITER) and would otherwise self-deadlock against this thread's own
reader; the vdev is re-resolved by guid after each.

Setting these properties also went untested; the suite only ever read
them back. Add ZTS coverage that exercises the write paths:
vdev_set_allocating creates a pool with two top-level vdevs, sets
allocating=off then allocating=on on one vdev, and verifies each command
returns and reads back as expected. vdev_set_path creates a pool on its
own file vdev, points a /dev symlink at the leaf, and sets path= to the
symlink (setting "path" only records the string, so a /dev symlink
standing in for the file vdev is enough to exercise the write-taking
path). On an affected build either command hangs in spa_config_enter().

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Chris Longros <[email protected]>
Signed-off-by: Prakash Surya <[email protected]>
Closes #18637

include/sys/vdev.h

@@ -243,8 +243,8 @@ typedef enum {
 
 extern int vdev_label_init(vdev_t *vd, uint64_t txg, vdev_labeltype_t reason);
 
-extern int vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl);
-extern int vdev_prop_get(vdev_t *vd, nvlist_t *nvprops, nvlist_t *outnvl);
+extern int vdev_prop_set(spa_t *spa, nvlist_t *innvl, nvlist_t *outnvl);
+extern int vdev_prop_get(spa_t *spa, nvlist_t *nvprops, nvlist_t *outnvl);
 
 #ifdef	__cplusplus
 }

module/zfs/vdev.c

@@ -6200,22 +6200,14 @@ vdev_props_set_sync(void *arg, dmu_tx_t *tx)
 }
 
 int
-vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
+vdev_prop_set(spa_t *spa, nvlist_t *innvl, nvlist_t *outnvl)
 {
-	spa_t *spa = vd->vdev_spa;
+	vdev_t *vd;
 	nvpair_t *elem = NULL;
 	uint64_t vdev_guid;
 	nvlist_t *nvprops;
 	int error = 0;
 
-	ASSERT(vd != NULL);
-
-	/* Check that vdev has a zap we can use */
-	if (vd->vdev_root_zap == 0 &&
-	    vd->vdev_top_zap == 0 &&
-	    vd->vdev_leaf_zap == 0)
-		return (SET_ERROR(EINVAL));
-
 	if (nvlist_lookup_uint64(innvl, ZPOOL_VDEV_PROPS_SET_VDEV,
 	    &vdev_guid) != 0)
 		return (SET_ERROR(EINVAL));
@@ -6224,8 +6216,31 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 	    &nvprops) != 0)
 		return (SET_ERROR(EINVAL));
 
-	if ((vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE)) == NULL)
+	/*
+	 * Resolve the vdev by guid and hold SCL_CONFIG as a reader so the
+	 * vdev tree can't change beneath us while we touch vd.  The lock is
+	 * dropped around the "path" and "allocating" handlers below: those
+	 * descend into spa_vdev_enter() -> spa_config_enter(SCL_ALL,
+	 * RW_WRITER), and taking SCL_CONFIG as a writer while this same
+	 * thread already holds it as a reader is a self-deadlock (the writer
+	 * waits for scl_count to drain to 0, but scl_count is this thread's
+	 * own reader, which is never released).  Those handlers re-resolve
+	 * the vdev by guid under their own locking, so we re-resolve here
+	 * after each one in case the tree changed.
+	 */
+	spa_config_enter(spa, SCL_CONFIG, FTAG, RW_READER);
+	if ((vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE)) == NULL) {
+		spa_config_exit(spa, SCL_CONFIG, FTAG);
+		return (SET_ERROR(ENOENT));
+	}
+
+	/* Check that vdev has a zap we can use */
+	if (vd->vdev_root_zap == 0 &&
+	    vd->vdev_top_zap == 0 &&
+	    vd->vdev_leaf_zap == 0) {
+		spa_config_exit(spa, SCL_CONFIG, FTAG);
 		return (SET_ERROR(EINVAL));
+	}
 
 	while ((elem = nvlist_next_nvpair(nvprops, elem)) != NULL) {
 		const char *propname = nvpair_name(elem);
@@ -6259,7 +6274,17 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 				error = EINVAL;
 				break;
 			}
+			/*
+			 * spa_vdev_setpath() takes SCL_ALL as a writer, so we
+			 * must not hold SCL_CONFIG across it (see above).  Drop
+			 * it, then re-resolve vd in case the tree changed.
+			 */
+			spa_config_exit(spa, SCL_CONFIG, FTAG);
 			error = spa_vdev_setpath(spa, vdev_guid, strval);
+			spa_config_enter(spa, SCL_CONFIG, FTAG, RW_READER);
+			vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE);
+			if (vd == NULL && error == 0)
+				error = SET_ERROR(ENOENT);
 			break;
 		case VDEV_PROP_ALLOCATING:
 			if (nvpair_value_uint64(elem, &intval) != 0) {
@@ -6268,10 +6293,19 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 			}
 			if (intval != vd->vdev_noalloc)
 				break;
+			/*
+			 * spa_vdev_noalloc()/spa_vdev_alloc() take SCL_ALL as a
+			 * writer; same locking dance as VDEV_PROP_PATH above.
+			 */
+			spa_config_exit(spa, SCL_CONFIG, FTAG);
 			if (intval == 0)
 				error = spa_vdev_noalloc(spa, vdev_guid);
 			else
 				error = spa_vdev_alloc(spa, vdev_guid);
+			spa_config_enter(spa, SCL_CONFIG, FTAG, RW_READER);
+			vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE);
+			if (vd == NULL && error == 0)
+				error = SET_ERROR(ENOENT);
 			break;
 		case VDEV_PROP_FAILFAST:
 			if (nvpair_value_uint64(elem, &intval) != 0 ||
@@ -6444,10 +6478,15 @@ end:
 		if (error != 0) {
 			intval = error;
 			vdev_prop_add_list(outnvl, propname, strval, intval, 0);
-			return (error);
+			break;
 		}
 	}
 
+	spa_config_exit(spa, SCL_CONFIG, FTAG);
+
+	if (error != 0)
+		return (error);
+
 	return (dsl_sync_task(spa->spa_name, NULL, vdev_props_set_sync,
 	    innvl, 6, ZFS_SPACE_CHECK_EXTRA_RESERVED));
 }
@@ -6462,10 +6501,10 @@ vdev_get_child_idx(vdev_t *vd, uint64_t c_guid)
 }
 
 int
-vdev_prop_get(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
+vdev_prop_get(spa_t *spa, nvlist_t *innvl, nvlist_t *outnvl)
 {
-	spa_t *spa = vd->vdev_spa;
 	objset_t *mos = spa->spa_meta_objset;
+	vdev_t *vd;
 	int err = 0;
 	uint64_t objid = 0;
 	uint64_t vdev_guid;
@@ -6477,7 +6516,6 @@ vdev_prop_get(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 	const char *propname = NULL;
 	vdev_prop_t prop;
 
-	ASSERT(vd != NULL);
 	ASSERT(mos != NULL);
 
 	if (nvlist_lookup_uint64(innvl, ZPOOL_VDEV_PROPS_GET_VDEV,
@@ -6486,6 +6524,18 @@ vdev_prop_get(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 
 	nvlist_lookup_nvlist(innvl, ZPOOL_VDEV_PROPS_GET_PROPS, &nvprops);
 
+	/*
+	 * Resolve the vdev by guid and hold SCL_CONFIG as a reader across the
+	 * property fetch so the vdev tree can't change beneath us.  This path
+	 * is read-only and never takes SCL_CONFIG as a writer, so holding the
+	 * reader throughout is safe.
+	 */
+	spa_config_enter(spa, SCL_CONFIG, FTAG, RW_READER);
+	if ((vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE)) == NULL) {
+		spa_config_exit(spa, SCL_CONFIG, FTAG);
+		return (SET_ERROR(ENOENT));
+	}
+
 	/*
 	 * A missing ZAP is normal for spare and L2ARC vdevs, which are
 	 * not part of the main vdev tree and never get ZAPs allocated.
@@ -6997,6 +7047,8 @@ vdev_prop_get(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 	}
 
 	mutex_exit(&spa->spa_props_lock);
+	spa_config_exit(spa, SCL_CONFIG, FTAG);
+
 	if (err && err != ENOENT) {
 		return (err);
 	}

module/zfs/zfs_ioctl.c

@@ -3457,13 +3457,6 @@ zfs_ioc_vdev_set_props(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
 {
 	spa_t *spa;
 	int error;
-	vdev_t *vd;
-	uint64_t vdev_guid;
-
-	/* Early validation */
-	if (nvlist_lookup_uint64(innvl, ZPOOL_VDEV_PROPS_SET_VDEV,
-	    &vdev_guid) != 0)
-		return (SET_ERROR(EINVAL));
 
 	if (outnvl == NULL)
 		return (SET_ERROR(EINVAL));
@@ -3473,15 +3466,7 @@ zfs_ioc_vdev_set_props(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
 
 	ASSERT(spa_writeable(spa));
 
-	spa_config_enter(spa, SCL_CONFIG, FTAG, RW_READER);
-	if ((vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE)) == NULL) {
-		spa_config_exit(spa, SCL_CONFIG, FTAG);
-		spa_close(spa, FTAG);
-		return (SET_ERROR(ENOENT));
-	}
-
-	error = vdev_prop_set(vd, innvl, outnvl);
-	spa_config_exit(spa, SCL_CONFIG, FTAG);
+	error = vdev_prop_set(spa, innvl, outnvl);
 
 	spa_close(spa, FTAG);
 
@@ -3506,13 +3491,6 @@ zfs_ioc_vdev_get_props(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
 {
 	spa_t *spa;
 	int error;
-	vdev_t *vd;
-	uint64_t vdev_guid;
-
-	/* Early validation */
-	if (nvlist_lookup_uint64(innvl, ZPOOL_VDEV_PROPS_GET_VDEV,
-	    &vdev_guid) != 0)
-		return (SET_ERROR(EINVAL));
 
 	if (outnvl == NULL)
 		return (SET_ERROR(EINVAL));
@@ -3520,15 +3498,7 @@ zfs_ioc_vdev_get_props(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
 	if ((error = spa_open(poolname, &spa, FTAG)) != 0)
 		return (error);
 
-	spa_config_enter(spa, SCL_CONFIG, FTAG, RW_READER);
-	if ((vd = spa_lookup_by_guid(spa, vdev_guid, B_TRUE)) == NULL) {
-		spa_config_exit(spa, SCL_CONFIG, FTAG);
-		spa_close(spa, FTAG);
-		return (SET_ERROR(ENOENT));
-	}
-
-	error = vdev_prop_get(vd, innvl, outnvl);
-	spa_config_exit(spa, SCL_CONFIG, FTAG);
+	error = vdev_prop_get(spa, innvl, outnvl);
 
 	spa_close(spa, FTAG);
 

tests/runfiles/common.run

@@ -575,7 +575,8 @@ tags = ['functional', 'cli_root', 'zpool_scrub']
 tests = ['zpool_set_001_pos', 'zpool_set_002_neg', 'zpool_set_003_neg',
     'zpool_set_ashift', 'zpool_set_features', 'zpool_set_inherit',
     'vdev_set_001_pos', 'user_property_001_pos', 'user_property_002_neg',
-    'zpool_set_clear_userprop','vdev_set_scheduler']
+    'zpool_set_clear_userprop','vdev_set_scheduler', 'vdev_set_allocating',
+    'vdev_set_path']
 tags = ['functional', 'cli_root', 'zpool_set']
 
 [tests/functional/cli_root/zpool_split]

tests/zfs-tests/tests/Makefile.am

@@ -1299,6 +1299,8 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/cli_root/zpool/setup.ksh \
 	functional/cli_root/zpool_set/vdev_set_001_pos.ksh \
 	functional/cli_root/zpool_set/vdev_set_scheduler.ksh \
+	functional/cli_root/zpool_set/vdev_set_allocating.ksh \
+	functional/cli_root/zpool_set/vdev_set_path.ksh \
 	functional/cli_root/zpool_set/zpool_set_common.kshlib \
 	functional/cli_root/zpool_set/zpool_set_001_pos.ksh \
 	functional/cli_root/zpool_set/zpool_set_002_neg.ksh \

tests/zfs-tests/tests/functional/cli_root/zpool_set/vdev_set_allocating.ksh

@@ -0,0 +1,80 @@
+#!/bin/ksh -p
+# SPDX-License-Identifier: CDDL-1.0
+#
+# 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) 2026 by Delphix. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+#	Toggling the "allocating" vdev property completes and takes effect.
+#
+#	When zfs_ioc_vdev_set_props() holds SCL_CONFIG as a reader across
+#	vdev_prop_set(), setting "allocating" re-enters
+#	spa_config_enter(spa, SCL_ALL, RW_WRITER) via
+#	spa_vdev_noalloc()/spa_vdev_alloc(); taking SCL_CONFIG as a writer
+#	while the same thread already holds it as a reader self-deadlocks the
+#	calling thread, so "zpool set allocating=off" hangs in
+#	spa_config_enter().
+#
+# STRATEGY:
+#	1. Create a pool with two top-level vdevs (so one can stop allocating
+#	   while a normal vdev remains).
+#	2. Set allocating=off on the first vdev; verify the command returns and
+#	   the property reads back "off".
+#	3. Set allocating=on; verify it reads back "on".
+#
+
+verify_runnable "global"
+
+typeset POOL=alloc_testpool
+typeset VDEV0=$TEST_BASE_DIR/vdev_alloc0.$$
+typeset VDEV1=$TEST_BASE_DIR/vdev_alloc1.$$
+
+function cleanup
+{
+	poolexists $POOL && destroy_pool $POOL
+	rm -f $VDEV0 $VDEV1
+}
+
+log_onexit cleanup
+
+log_assert "toggling the allocating vdev property completes and takes effect"
+
+log_must truncate -s $MINVDEVSIZE $VDEV0 $VDEV1
+log_must zpool create $POOL $VDEV0 $VDEV1
+
+# Both top-level vdevs allocate by default.
+log_must test "$(zpool get -H -o value allocating $POOL $VDEV0)" = "on"
+
+# The operation that previously deadlocked.
+log_must zpool set allocating=off $POOL $VDEV0
+log_must test "$(zpool get -H -o value allocating $POOL $VDEV0)" = "off"
+
+# And back on.
+log_must zpool set allocating=on $POOL $VDEV0
+log_must test "$(zpool get -H -o value allocating $POOL $VDEV0)" = "on"
+
+log_pass "toggling the allocating vdev property completes and takes effect"

tests/zfs-tests/tests/functional/cli_root/zpool_set/vdev_set_path.ksh

@@ -0,0 +1,90 @@
+#!/bin/ksh -p
+# SPDX-License-Identifier: CDDL-1.0
+#
+# 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) 2026 by Delphix. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+#	Setting the "path" vdev property completes and takes effect.
+#
+#	Like "allocating", the "path" property re-takes SCL_CONFIG as a
+#	writer underneath vdev_prop_set(): it calls spa_vdev_setpath() ->
+#	spa_vdev_set_common() -> spa_vdev_state_enter(spa, SCL_ALL) ->
+#	spa_config_enter(spa, SCL_ALL, RW_WRITER).  When
+#	zfs_ioc_vdev_set_props() holds SCL_CONFIG as a reader across
+#	vdev_prop_set(), taking it as a writer while the same thread already
+#	holds it as a reader self-deadlocks the calling thread, so "zpool set
+#	path=..." hangs in spa_config_enter().
+#
+# STRATEGY:
+#	1. Create a pool on our own file vdev (so we don't collide with the
+#	   pool the zpool_set setup creates on $DISKS).
+#	2. Point a /dev symlink at the leaf and set path= to it; verify the
+#	   command returns and the property reads back the new path.
+#
+#	"path" only records the string and requires a /dev/ prefix; it does
+#	not reopen the device, so a /dev symlink standing in for the file
+#	vdev is sufficient to exercise the set path.
+#
+
+verify_runnable "global"
+
+typeset POOL=path_testpool
+typeset VDEV=$TEST_BASE_DIR/vdev_path.$$
+typeset SYMLINK=/dev/vdev_set_path_$$
+
+function cleanup
+{
+	poolexists $POOL && destroy_pool $POOL
+	rm -f $VDEV $SYMLINK
+}
+
+log_onexit cleanup
+
+log_assert "setting the path vdev property completes and takes effect"
+
+log_must truncate -s $MINVDEVSIZE $VDEV
+log_must zpool create $POOL $VDEV
+
+# The stored leaf path is the file vdev we created.
+typeset OLDPATH=$(zpool get -H -o value path $POOL $VDEV)
+log_must test -n "$OLDPATH"
+
+# A new /dev path for the leaf; "path" must start with /dev/.
+log_must ln -s $OLDPATH $SYMLINK
+
+# The operation that previously deadlocked.
+log_must zpool set path=$SYMLINK $POOL $OLDPATH
+
+# The single leaf is the only vdev with a path; confirm it now reads back
+# the new value.  (Address by "all-vdevs" rather than by the new path, to
+# avoid depending on how the CLI re-resolves a just-renamed vdev.)
+log_must test \
+    "$(zpool get -H -o value path $POOL all-vdevs | grep '^/dev/')" = \
+    "$SYMLINK"
+
+log_pass "setting the path vdev property completes and takes effect"