From 891e379d0ff2e7285b009a0bdb108feb642daa98 Mon Sep 17 00:00:00 2001 From: Paul Dagnelie Date: Mon, 18 May 2026 09:12:09 -0700 Subject: [PATCH] Fix failfast default and usage The feature that added a failfast property to vdevs unfortunately did not correctly set the default at creation time, so many vdevs do not actually have the property set. In addition, when the property is used, the failfast flag is not checked correctly, resulting in the feature mostly not working as intended. Set the failfast property to the default value at vdev allocation time. The value will be read in from the ZAP as normal when the vdev metadata is loaded. Allow the property to be set on any vdev and have it be inherited from the root or top-level vdev. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Reviewed-by: Tony Hutter Signed-off-by: Paul Dagnelie Closes #18410 --- include/sys/fs/zfs.h | 1 + man/man7/vdevprops.7 | 5 +- module/os/linux/zfs/vdev_disk.c | 8 +- module/zcommon/zpool_prop.c | 12 +- module/zfs/vdev.c | 30 +++-- tests/runfiles/common.run | 4 +- tests/runfiles/sanity.run | 2 +- tests/zfs-tests/tests/Makefile.am | 1 + .../cli_root/zpool_set/zpool_set_inherit.ksh | 115 ++++++++++++++++++ 9 files changed, 161 insertions(+), 17 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_inherit.ksh diff --git a/include/sys/fs/zfs.h b/include/sys/fs/zfs.h index d9b6e7654b0..4c4d15f8ce0 100644 --- a/include/sys/fs/zfs.h +++ b/include/sys/fs/zfs.h @@ -363,6 +363,7 @@ typedef enum { /* Small enough to not hog a whole line of printout in zpool(8). */ #define ZPROP_MAX_COMMENT 32 #define ZPROP_BOOLEAN_NA 2 +#define ZPROP_BOOLEAN_INHERIT 2 #define ZPROP_VALUE "value" #define ZPROP_SOURCE "source" diff --git a/man/man7/vdevprops.7 b/man/man7/vdevprops.7 index 5f5e10723c1..da38acafeee 100644 --- a/man/man7/vdevprops.7 +++ b/man/man7/vdevprops.7 @@ -183,9 +183,12 @@ output. A text comment up to 8192 characters long .It Sy bootsize The amount of space to reserve for the EFI system partition -.It Sy failfast +.It Sy failfast Ns = Ns Sy inherit Ns | Ns Sy on Ns | Ns Sy off If this device should propagate BIO errors back to ZFS, used to disable failfast. +.Sy inherit +causes the vdev to adopt the behavior of its parent vdev, +recursively up the tree. .It Sy sit_out Only valid for .Sy RAIDZ diff --git a/module/os/linux/zfs/vdev_disk.c b/module/os/linux/zfs/vdev_disk.c index 66e10584ab5..7cc19fe5afb 100644 --- a/module/os/linux/zfs/vdev_disk.c +++ b/module/os/linux/zfs/vdev_disk.c @@ -931,8 +931,14 @@ vdev_disk_io_rw(zio_t *zio) return (SET_ERROR(EIO)); } + vdev_t *iter = v; + while (iter != NULL && iter->vdev_failfast == ZPROP_BOOLEAN_INHERIT) + iter = iter->vdev_parent; + + boolean_t failfast = iter ? iter->vdev_failfast == 1 : + vdev_prop_default_numeric(VDEV_PROP_FAILFAST); if (!(zio->io_flags & (ZIO_FLAG_IO_RETRY | ZIO_FLAG_TRYHARD)) && - v->vdev_failfast == B_TRUE) { + failfast) { bio_set_flags_failfast(bdev, &flags, zfs_vdev_failfast_mask & 1, zfs_vdev_failfast_mask & 2, zfs_vdev_failfast_mask & 4); } diff --git a/module/zcommon/zpool_prop.c b/module/zcommon/zpool_prop.c index 13a1390d1e1..ccd9f3854f5 100644 --- a/module/zcommon/zpool_prop.c +++ b/module/zcommon/zpool_prop.c @@ -374,10 +374,16 @@ vdev_prop_init(void) { "on", 1}, { NULL } }; + static const zprop_index_t boolean_inherit_table[] = { + { "off", 0}, + { "on", 1}, + { "inherit", ZPROP_BOOLEAN_INHERIT}, + { NULL } + }; static const zprop_index_t boolean_na_table[] = { { "off", 0}, { "on", 1}, - { "-", 2}, /* ZPROP_BOOLEAN_NA */ + { "-", ZPROP_BOOLEAN_NA}, { NULL } }; @@ -555,8 +561,8 @@ vdev_prop_init(void) /* default index properties */ zprop_register_index(VDEV_PROP_FAILFAST, "failfast", B_TRUE, - PROP_DEFAULT, ZFS_TYPE_VDEV, "on | off", "FAILFAST", boolean_table, - sfeatures); + PROP_DEFAULT, ZFS_TYPE_VDEV, "on | off | inherit", "FAILFAST", + boolean_inherit_table, sfeatures); zprop_register_index(VDEV_PROP_SLOW_IO_EVENTS, "slow_io_events", B_TRUE, PROP_DEFAULT, ZFS_TYPE_VDEV, "on | off", "SLOW_IO_EVENTS", boolean_table, sfeatures); diff --git a/module/zfs/vdev.c b/module/zfs/vdev.c index 9f083cd510f..e4dc9e97af7 100644 --- a/module/zfs/vdev.c +++ b/module/zfs/vdev.c @@ -1117,6 +1117,11 @@ vdev_alloc(spa_t *spa, vdev_t **vdp, nvlist_t *nv, vdev_t *parent, uint_t id, if (top_level && (ops == &vdev_raidz_ops || ops == &vdev_draid_ops)) vd->vdev_autosit = vdev_prop_default_numeric(VDEV_PROP_AUTOSIT); + if (ops == &vdev_root_ops) + vd->vdev_failfast = + vdev_prop_default_numeric(VDEV_PROP_FAILFAST); + else + vd->vdev_failfast = ZPROP_BOOLEAN_INHERIT; /* * Add ourselves to the parent's list of children. @@ -3912,10 +3917,9 @@ vdev_load(vdev_t *vd) vdev_prop_to_name(VDEV_PROP_FAILFAST), sizeof (failfast), 1, &failfast); if (error == 0) { - vd->vdev_failfast = failfast & 1; + vd->vdev_failfast = failfast; } else if (error == ENOENT) { - vd->vdev_failfast = vdev_prop_default_numeric( - VDEV_PROP_FAILFAST); + vd->vdev_failfast = ZPROP_BOOLEAN_INHERIT; } else { vdev_dbgmsg(vd, "vdev_load: zap_lookup(top_zap=%llu) " @@ -6230,11 +6234,14 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl) error = spa_vdev_alloc(spa, vdev_guid); break; case VDEV_PROP_FAILFAST: - if (nvpair_value_uint64(elem, &intval) != 0) { + if (nvpair_value_uint64(elem, &intval) != 0 || + intval > ZPROP_BOOLEAN_INHERIT || + (intval == ZPROP_BOOLEAN_INHERIT && + vd->vdev_ops == &vdev_root_ops)) { error = EINVAL; break; } - vd->vdev_failfast = intval & 1; + vd->vdev_failfast = intval; break; case VDEV_PROP_SIT_OUT: /* Only expose this for a draid or raidz leaf */ @@ -6764,18 +6771,23 @@ vdev_prop_get(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl) break; case VDEV_PROP_FAILFAST: src = ZPROP_SRC_LOCAL; - strval = NULL; err = zap_lookup(mos, objid, nvpair_name(elem), sizeof (uint64_t), 1, &intval); if (err == ENOENT) { - intval = vdev_prop_default_numeric( - prop); + if (vd->vdev_ops == &vdev_root_ops) + intval = + vdev_prop_default_numeric( + prop); + else + intval = ZPROP_BOOLEAN_INHERIT; err = 0; } else if (err) { break; } - if (intval == vdev_prop_default_numeric(prop)) + if (intval == ZPROP_BOOLEAN_INHERIT || + (vd->vdev_ops == &vdev_root_ops && + intval == 1)) src = ZPROP_SRC_DEFAULT; vdev_prop_add_list(outnvl, propname, strval, diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index 003e1c35495..82a2d1e815e 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -573,8 +573,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', + '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'] tags = ['functional', 'cli_root', 'zpool_set'] diff --git a/tests/runfiles/sanity.run b/tests/runfiles/sanity.run index 936f2bcc32b..d62f8f3fb16 100644 --- a/tests/runfiles/sanity.run +++ b/tests/runfiles/sanity.run @@ -353,7 +353,7 @@ 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'] + 'zpool_set_ashift', 'zpool_set_features', 'zpool_set_inherit'] 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 75b53c6ddd0..85f00f28b0f 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -1303,6 +1303,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/cli_root/zpool_set/zpool_set_002_neg.ksh \ functional/cli_root/zpool_set/zpool_set_003_neg.ksh \ functional/cli_root/zpool_set/zpool_set_ashift.ksh \ + functional/cli_root/zpool_set/zpool_set_inherit.ksh \ 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 \ diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_inherit.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_inherit.ksh new file mode 100755 index 00000000000..2694e3278d9 --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_set/zpool_set_inherit.ksh @@ -0,0 +1,115 @@ +#!/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 2026, Klara, Inc. All rights reserved. +# + +. $STF_SUITE/include/libtest.shlib + +# +# DESCRIPTION: +# +# zpool set can set the failfast property to 'inherit' +# +# STRATEGY: +# 1. Create a pool +# 2. Verify that we can set 'failfast' to various values, including inherit +# 3. Verify that the root vdev cannot be set to inherit +# + +verify_runnable "global" + +function cleanup +{ + destroy_pool $TESTPOOL1 + rm -f $FILEVDEV1 $FILEVDEV2 $FILEVDEV3 +} + +function get_failfast +{ + zpool get -H -o value failfast $TESTPOOL1 $@ +} + +log_onexit cleanup + +log_assert "zpool set can configure 'failfast' property to inherit" +FILEVDEV1="$TEST_BASE_DIR/zpool_set_inherit1.$$.dat" +FILEVDEV2="$TEST_BASE_DIR/zpool_set_inherit2.$$.dat" +FILEVDEV3="$TEST_BASE_DIR/zpool_set_inherit3.$$.dat" + +log_must truncate -s $MINVDEVSIZE $FILEVDEV1 +log_must truncate -s $MINVDEVSIZE $FILEVDEV2 +log_must truncate -s $MINVDEVSIZE $FILEVDEV3 + +log_must zpool create -f $TESTPOOL1 $FILEVDEV1 mirror $FILEVDEV2 $FILEVDEV3 +failfast=$(get_failfast $FILEVDEV1) +[[ "$failfast" == "inherit" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=on $TESTPOOL1 $FILEVDEV1 +failfast=$(get_failfast $FILEVDEV1) +[[ "$failfast" == "on" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=off $TESTPOOL1 $FILEVDEV1 +failfast=$(get_failfast $FILEVDEV1) +[[ "$failfast" == "off" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=inherit $TESTPOOL1 $FILEVDEV1 + +failfast=$(get_failfast $FILEVDEV2) +[[ "$failfast" == "inherit" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=on $TESTPOOL1 $FILEVDEV2 +failfast=$(get_failfast $FILEVDEV2) +[[ "$failfast" == "on" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=off $TESTPOOL1 $FILEVDEV2 +failfast=$(get_failfast $FILEVDEV2) +[[ "$failfast" == "off" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=inherit $TESTPOOL1 $FILEVDEV2 + +failfast=$(get_failfast mirror-1) +[[ "$failfast" == "inherit" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=on $TESTPOOL1 mirror-1 +failfast=$(get_failfast mirror-1) +[[ "$failfast" == "on" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=off $TESTPOOL1 mirror-1 +failfast=$(get_failfast mirror-1) +[[ "$failfast" == "off" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=inherit $TESTPOOL1 mirror-1 + +failfast=$(get_failfast root) +[[ "$failfast" == "on" ]] || log_fail "incorrect failfast value: $failfast" + +log_must zpool set failfast=off $TESTPOOL1 root +failfast=$(get_failfast root) +[[ "$failfast" == "off" ]] || log_fail "incorrect failfast value: $failfast" + +log_mustnot zpool set failfast=inherit $TESTPOOL1 root + + +log_pass "zpool set can configure 'failfast' property to inherit"