Respond to review feedback

Update man pages, properly handle sit-outs of non-direct children, add
efficient low-res time function, and update stats every time

Signed-off-by: Paul Dagnelie <[email protected]>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

Author: Paul Dagnelie

include/os/freebsd/spl/sys/time.h

@@ -62,6 +62,23 @@ typedef longlong_t	hrtime_t;
 #define	SEC_TO_TICK(sec)	((sec) * hz)
 #define	NSEC_TO_TICK(nsec)	((nsec) / (NANOSEC / hz))
 
+static __inline hrtime_t
+getlrtime(void)
+{
+	struct timespec ts;
+	hrtime_t nsec;
+
+	getnanotime(&ts);
+	nsec = ((hrtime_t)ts.tv_sec * NANOSEC) + ts.tv_nsec;
+	return (nsec);
+}
+
+static __inline void
+nanouptime(struct timespec *ts)
+{
+	clock(&ts);
+}
+
 static __inline hrtime_t
 gethrtime(void)
 {

include/os/linux/spl/sys/time.h

@@ -79,6 +79,14 @@ gethrestime_sec(void)
 	return (ts.tv_sec);
 }
 
+static inline hrtime_t
+getlrtime(void)
+{
+	inode_timespec_t ts;
+	ktime_get_coarse_real_ts64(&ts);
+	return (((hrtime_t)ts.tv_sec * NSEC_PER_SEC) + ts.tv_nsec);
+}
+
 static inline hrtime_t
 gethrtime(void)
 {

include/sys/vdev_raidz.h

@@ -63,6 +63,7 @@ void vdev_raidz_reflow_copy_scratch(spa_t *);
 void raidz_dtl_reassessed(vdev_t *);
 boolean_t vdev_sit_out_reads(vdev_t *, zio_flag_t);
 extern void vdev_raidz_sit_child(vdev_t *svd);
+extern void vdev_raidz_unsit_child(vdev_t *vd);
 
 extern const zio_vsd_ops_t vdev_raidz_vsd_ops;
 

lib/libspl/include/sys/time.h

@@ -97,6 +97,15 @@ gethrestime_sec(void)
 	return (tv.tv_sec);
 }
 
+static inline hrtime_t
+getlrtime(void)
+{
+	struct timeval tv;
+	(void) gettimeofday(&tv, NULL);
+	return ((((uint64_t)tv.tv_sec) * NANOSEC) +
+	    ((uint64_t)tv.tv_usec * NSEC_PER_USEC));
+}
+
 static inline hrtime_t
 gethrtime(void)
 {

lib/libzfs/libzfs_pool.c

@@ -5712,8 +5712,16 @@ zpool_set_vdev_prop(zpool_handle_t *zhp, const char *vdevname,
 	nvlist_free(nvl);
 	nvlist_free(outnvl);
 
-	if (ret)
-		(void) zpool_standard_error(zhp->zpool_hdl, errno, errbuf);
+	if (ret) {
+		if (errno == ENOTSUP) {
+			zfs_error_aux(zhp->zpool_hdl, dgettext(TEXT_DOMAIN,
+			    "property not supported for this vdev"));
+			(void) zfs_error(zhp->zpool_hdl, EZFS_PROPTYPE, errbuf);
+		} else {
+			(void) zpool_standard_error(zhp->zpool_hdl, errno,
+			    errbuf);
+		}
+	}
 
 	return (ret);
 }

man/man7/vdevprops.7

@@ -105,11 +105,6 @@ Comma separated list of children of this vdev
 The number of children belonging to this vdev
 .It Sy read_errors , write_errors , checksum_errors , initialize_errors , trim_errors
 The number of errors of each type encountered by this vdev
-.It Sy sit_out
-True when a slow disk outlier was detected and the vdev is currently in a sit
-out state.
-While sitting out, the vdev will not participate in normal reads, instead its
-data will be reconstructed as needed from parity.
 .It Sy slow_ios
 This indicates the number of slow I/O operations encountered by this vdev.
 A slow I/O is defined as an operation that did not complete within the
@@ -161,6 +156,26 @@ The amount of space to reserve for the EFI system partition
 .It Sy failfast
 If this device should propagate BIO errors back to ZFS, used to disable
 failfast.
+.It Sy sit_out
+True when a slow disk outlier was detected and the vdev is currently in a sit
+out state.
+This property can be manually set to cause vdevs to sit out.
+It will also be automatically set by the
+.Sy autosit
+logic if that is enabled.
+While sitting out, the vdev will not participate in normal reads, instead its
+data will be reconstructed as needed from parity.
+.It Sy autosit
+Only valid for
+.Sy RAIDZ
+and
+.Sy DRAID
+vdevs.
+If set, this enables the kernel-level slow disk detection logic.
+This logic automatically causes any vdevs that are significant negative
+performance outliers to sit out, as described in the
+.Sy sit_out
+property.
 .It Sy path
 The path to the device for this vdev
 .It Sy allocating

module/zfs/vdev.c

@@ -6150,10 +6150,13 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 				break;
 			}
 			if (intval == 1) {
+				vdev_t *ancestor = vd;
+				while (ancestor->vdev_parent != vd->vdev_top)
+					ancestor = ancestor->vdev_parent;
 				vdev_t *pvd = vd->vdev_top;
 				uint_t sitouts = 0;
 				for (int i = 0; i < pvd->vdev_children; i++) {
-					if (pvd->vdev_child[i] == vd)
+					if (pvd->vdev_child[i] == ancestor)
 						continue;
 					if (vdev_sit_out_reads(
 					    pvd->vdev_child[i], 0)) {
@@ -6167,7 +6170,7 @@ vdev_prop_set(vdev_t *vd, nvlist_t *innvl, nvlist_t *outnvl)
 				if (error == 0)
 					vdev_raidz_sit_child(vd);
 			} else {
-				vd->vdev_read_sit_out_expire = 0;
+				vdev_raidz_unsit_child(vd);
 			}
 			break;
 		case VDEV_PROP_AUTOSIT:

module/zfs/vdev_raidz.c

@@ -2340,6 +2340,15 @@ vdev_sit_out_reads(vdev_t *vd, zio_flag_t io_flags)
 	/* Avoid skipping a data column read when scrubbing */
 	if (io_flags & ZIO_FLAG_SCRUB)
 		return (B_FALSE);
+
+	if (!vd->vdev_ops->vdev_op_leaf) {
+		boolean_t sitting = B_FALSE;
+		for (int c = 0; c < vd->vdev_children; c++) {
+			sitting |= vdev_sit_out_reads(vd->vdev_child[c],
+			    io_flags);
+		}
+		return (sitting);
+	}
 	if (vd->vdev_read_sit_out_expire >= gethrestime_sec())
 		return (B_TRUE);
 	vd->vdev_read_sit_out_expire = 0;
@@ -2904,6 +2913,13 @@ latency_compare(const void *arg1, const void *arg2)
 void
 vdev_raidz_sit_child(vdev_t *svd)
 {
+	for (int c = 0; c < svd->vdev_children; c++) {
+		vdev_raidz_sit_child(svd->vdev_child[c]);
+	}
+
+	if (!svd->vdev_ops->vdev_op_leaf)
+		return;
+
 	/*
 	 * Begin a sit out period for this slow drive
 	 */
@@ -2915,6 +2931,19 @@ vdev_raidz_sit_child(vdev_t *svd)
 	mutex_exit(&svd->vdev_stat_lock);
 }
 
+void
+vdev_raidz_unsit_child(vdev_t *vd)
+{
+	for (int c = 0; c < vd->vdev_children; c++) {
+		vdev_raidz_sit_child(vd->vdev_child[c]);
+	}
+
+	if (!vd->vdev_ops->vdev_op_leaf)
+		return;
+
+	vd->vdev_read_sit_out_expire = 0;
+}
+
 /*
  * Check for any latency outlier from latest set of child reads.
  *
@@ -2943,7 +2972,7 @@ vdev_child_slow_outlier(zio_t *zio)
 	    vd->vdev_children < LAT_SAMPLES_MIN)
 		return;
 
-	hrtime_t now = gethrtime();
+	hrtime_t now = getlrtime();
 	uint64_t last = atomic_load_64(&vd->vdev_last_latency_check);
 
 	if ((now - last) < MSEC2NSEC(vdev_raidz_outlier_check_interval_ms) ||
@@ -2976,64 +3005,64 @@ vdev_child_slow_outlier(zio_t *zio)
 	uint64_t max = 0;
 	vdev_t *svd = NULL; /* suspect vdev */
 	uint_t sitouts = 0;
+	boolean_t skip = B_FALSE, svd_sitting = B_FALSE;
 	for (int c = 0; c < samples; c++) {
 		vdev_t *cvd = vd->vdev_child[c];
+		boolean_t sitting = vdev_sit_out_reads(cvd, 0);
 
-		if (cvd->vdev_read_sit_out_expire != 0) {
-			if (cvd->vdev_read_sit_out_expire < gethrestime_sec()) {
-				/*
-				 * Done with our sit out, wait for new outlier
-				 * to emerge.
-				 */
-				cvd->vdev_read_sit_out_expire = 0;
-			} else if (sitouts++ >= vdev_get_nparity(vd)) {
-				/*
-				 * We can't sit out more disks than we have
-				 * parity
-				 */
-				goto out;
-			}
+		if (sitting && sitouts++ >= vdev_get_nparity(vd)) {
+			/*
+			 * We can't sit out more disks than we have
+			 * parity
+			 */
+				skip = B_TRUE;
 		}
 		mutex_enter(&cvd->vdev_stat_lock);
+
+		uint64_t *prev_histo = cvd->vdev_prev_histo;
 		uint64_t *histo =
 		    cvd->vdev_stat_ex.vsx_disk_histo[ZIO_TYPE_READ];
-		uint64_t *prev_histo = cvd->vdev_prev_histo;
+		if (skip) {
+			size_t size =
+			    sizeof (cvd->vdev_stat_ex.vsx_disk_histo[0]);
+			memcpy(prev_histo, histo, size);
+			mutex_exit(&cvd->vdev_stat_lock);
+			continue;
+		}
 		uint64_t count = 0;
 		lat_data[c] = 0;
 		for (int i = 0; i < VDEV_L_HISTO_BUCKETS; i++) {
 			uint64_t this_count = histo[i] - prev_histo[i];
 			lat_data[c] += (1ULL << i) * this_count;
 			count += this_count;
 		}
+		size_t size = sizeof (cvd->vdev_stat_ex.vsx_disk_histo[0]);
+		memcpy(prev_histo, histo, size);
 		mutex_exit(&cvd->vdev_stat_lock);
 		lat_data[c] /= MAX(1, count);
 
 		/* wait until all disks have been read from */
-		if (lat_data[c] == 0 && cvd->vdev_read_sit_out_expire == 0)
-			goto out;
+		if (lat_data[c] == 0 && !sitting) {
+			skip = B_TRUE;
+			continue;
+		}
 
 		/* keep track of the vdev with largest value */
 		if (lat_data[c] > max) {
 			max = lat_data[c];
 			svd = cvd;
+			svd_sitting = sitting;
 		}
 	}
-
-	for (int c = 0; c < samples; c++) {
-		vdev_t *cvd = vd->vdev_child[c];
-		mutex_enter(&cvd->vdev_stat_lock);
-		size_t size = sizeof (cvd->vdev_stat_ex.vsx_disk_histo[0]);
-		memcpy(cvd->vdev_prev_histo,
-		    cvd->vdev_stat_ex.vsx_disk_histo[ZIO_TYPE_READ], size);
-		mutex_exit(&cvd->vdev_stat_lock);
-	}
+	if (skip)
+		goto out;
 
 	qsort((void *)lat_data, samples, sizeof (uint64_t), latency_compare);
 	uint64_t iqr = 0;
 	uint64_t fence = latency_quartiles_fence(lat_data, samples, &iqr);
 
 	ASSERT3U(lat_data[samples - 1], ==, max);
-	if (max > fence && svd->vdev_read_sit_out_expire == 0) {
+	if (max > fence && !svd_sitting) {
 		uint64_t incr = MAX(1, (max - fence) / iqr);
 		vd->vdev_outlier_count += incr;
 		if (vd->vdev_outlier_count >= samples) {