zpool iostat: refresh pool list every interval

cmd/zpool/zpool_iter.c

@@ -26,6 +26,7 @@
 
 /*
  * Copyright 2016 Igor Kozhukhov <[email protected]>.
+ * Copyright (c) 2025, Klara, Inc.
  */
 
 #include <libintl.h>
@@ -52,7 +53,7 @@
 typedef struct zpool_node {
 	zpool_handle_t	*zn_handle;
 	uu_avl_node_t	zn_avlnode;
-	int		zn_mark;
+	hrtime_t	zn_last_refresh;
 } zpool_node_t;
 
 struct zpool_list {
@@ -62,6 +63,7 @@ struct zpool_list {
 	uu_avl_pool_t	*zl_pool;
 	zprop_list_t	**zl_proplist;
 	zfs_type_t	zl_type;
+	hrtime_t	zl_last_refresh;
 };
 
 static int
@@ -81,32 +83,47 @@ zpool_compare(const void *larg, const void *rarg, void *unused)
  * of known pools.
  */
 static int
-add_pool(zpool_handle_t *zhp, void *data)
+add_pool(zpool_handle_t *zhp, zpool_list_t *zlp)
 {
-	zpool_list_t *zlp = data;
-	zpool_node_t *node = safe_malloc(sizeof (zpool_node_t));
+	zpool_node_t *node, *new = safe_malloc(sizeof (zpool_node_t));
 	uu_avl_index_t idx;
 
-	node->zn_handle = zhp;
-	uu_avl_node_init(node, &node->zn_avlnode, zlp->zl_pool);
-	if (uu_avl_find(zlp->zl_avl, node, NULL, &idx) == NULL) {
+	new->zn_handle = zhp;
+	uu_avl_node_init(new, &new->zn_avlnode, zlp->zl_pool);
+
+	node = uu_avl_find(zlp->zl_avl, new, NULL, &idx);
+	if (node == NULL) {
 		if (zlp->zl_proplist &&
 		    zpool_expand_proplist(zhp, zlp->zl_proplist,
 		    zlp->zl_type, zlp->zl_literal) != 0) {
 			zpool_close(zhp);
-			free(node);
+			free(new);
 			return (-1);
 		}
-		uu_avl_insert(zlp->zl_avl, node, idx);
+		new->zn_last_refresh = zlp->zl_last_refresh;
+		uu_avl_insert(zlp->zl_avl, new, idx);
 	} else {
+		node->zn_last_refresh = zlp->zl_last_refresh;
 		zpool_close(zhp);
-		free(node);
+		free(new);
 		return (-1);
 	}
 
 	return (0);
 }
 
+/*
+ * add_pool(), but always returns 0. This allows zpool_iter() to continue
+ * even if a pool exists in the tree, or we fail to get the properties for
+ * a new one.
+ */
+static int
+add_pool_cb(zpool_handle_t *zhp, void *data)
+{
+	(void) add_pool(zhp, data);
+	return (0);
+}
+
 /*
  * Create a list of pools based on the given arguments.  If we're given no
  * arguments, then iterate over all pools in the system and add them to the AVL
@@ -135,9 +152,10 @@ pool_list_get(int argc, char **argv, zprop_list_t **proplist, zfs_type_t type,
 	zlp->zl_type = type;
 
 	zlp->zl_literal = literal;
+	zlp->zl_last_refresh = gethrtime();
 
 	if (argc == 0) {
-		(void) zpool_iter(g_zfs, add_pool, zlp);
+		(void) zpool_iter(g_zfs, add_pool_cb, zlp);
 		zlp->zl_findall = B_TRUE;
 	} else {
 		int i;
@@ -159,15 +177,69 @@ pool_list_get(int argc, char **argv, zprop_list_t **proplist, zfs_type_t type,
 }
 
 /*
- * Search for any new pools, adding them to the list.  We only add pools when no
- * options were given on the command line.  Otherwise, we keep the list fixed as
- * those that were explicitly specified.
+ * Refresh the state of all pools on the list. Additionally, if no options were
+ * given on the command line, add any new pools and remove any that are no
+ * longer available.
  */
-void
-pool_list_update(zpool_list_t *zlp)
+int
+pool_list_refresh(zpool_list_t *zlp)
 {
-	if (zlp->zl_findall)
-		(void) zpool_iter(g_zfs, add_pool, zlp);
+	zlp->zl_last_refresh = gethrtime();
+
+	if (!zlp->zl_findall) {
+		/*
+		 * This list is a fixed list of pools, so we must not add
+		 * or remove any. Just walk over them and refresh their
+		 * state.
+		 */
+		int navail = 0;
+		for (zpool_node_t *node = uu_avl_first(zlp->zl_avl);
+		    node != NULL; node = uu_avl_next(zlp->zl_avl, node)) {
+			boolean_t missing;
+			zpool_refresh_stats(node->zn_handle, &missing);
+			navail += !missing;
+			node->zn_last_refresh = zlp->zl_last_refresh;
+		}
+		return (navail);
+	}
+
+	/*
+	 * Search for any new pools and add them to the list. zpool_iter()
+	 * will call zpool_refresh_stats() as part of its work, so this has
+	 * the side effect of updating all active handles.
+	 */
+	(void) zpool_iter(g_zfs, add_pool_cb, zlp);
+
+	/*
+	 * Walk the list for any that weren't refreshed, and update and remove
+	 * them. It's not enough to just skip available ones, as zpool_iter()
+	 * won't update them, so they'll still appear active in our list.
+	 */
+	zpool_node_t *node, *next;
+	for (node = uu_avl_first(zlp->zl_avl); node != NULL; node = next) {
+		next = uu_avl_next(zlp->zl_avl, node);
+
+		/*
+		 * Skip any that were refreshed and are online; they're already
+		 * handled.
+		 */
+		if (node->zn_last_refresh == zlp->zl_last_refresh &&
+		    zpool_get_state(node->zn_handle) != POOL_STATE_UNAVAIL)
+			continue;
+
+		/* Do the refresh ourselves, just in case. */
+		boolean_t missing;
+		zpool_refresh_stats(node->zn_handle, &missing);
+		if (missing) {
+			uu_avl_remove(zlp->zl_avl, node);
+			zpool_close(node->zn_handle);
+			free(node);
+		} else {
+			node->zn_last_refresh = zlp->zl_last_refresh;
+		}
+	}
+
+	return (uu_avl_numnodes(zlp->zl_avl));
 }
 
 /*
@@ -190,23 +262,6 @@ pool_list_iter(zpool_list_t *zlp, int unavail, zpool_iter_f func,
 	return (ret);
 }
 
-/*
- * Remove the given pool from the list.  When running iostat, we want to remove
- * those pools that no longer exist.
- */
-void
-pool_list_remove(zpool_list_t *zlp, zpool_handle_t *zhp)
-{
-	zpool_node_t search, *node;
-
-	search.zn_handle = zhp;
-	if ((node = uu_avl_find(zlp->zl_avl, &search, NULL, NULL)) != NULL) {
-		uu_avl_remove(zlp->zl_avl, node);
-		zpool_close(node->zn_handle);
-		free(node);
-	}
-}
-
 /*
  * Free all the handles associated with this list.
  */

cmd/zpool/zpool_main.c

@@ -33,7 +33,7 @@
  * Copyright (c) 2017, Intel Corporation.
  * Copyright (c) 2019, loli10K <[email protected]>
  * Copyright (c) 2021, Colm Buckley <[email protected]>
- * Copyright (c) 2021, 2023, Klara Inc.
+ * Copyright (c) 2021, 2023, 2025, Klara, Inc.
  * Copyright (c) 2021, 2025 Hewlett Packard Enterprise Development LP.
  */
 
@@ -5761,24 +5761,6 @@ print_vdev_stats(zpool_handle_t *zhp, const char *name, nvlist_t *oldnv,
 	return (ret);
 }
 
-static int
-refresh_iostat(zpool_handle_t *zhp, void *data)
-{
-	iostat_cbdata_t *cb = data;
-	boolean_t missing;
-
-	/*
-	 * If the pool has disappeared, remove it from the list and continue.
-	 */
-	if (zpool_refresh_stats(zhp, &missing) != 0)
-		return (-1);
-
-	if (missing)
-		pool_list_remove(cb->cb_list, zhp);
-
-	return (0);
-}
-
 /*
  * Callback to print out the iostats for the given pool.
  */
@@ -6359,15 +6341,14 @@ get_namewidth_iostat(zpool_handle_t *zhp, void *data)
  * This command can be tricky because we want to be able to deal with pool
  * creation/destruction as well as vdev configuration changes.  The bulk of this
  * processing is handled by the pool_list_* routines in zpool_iter.c.  We rely
- * on pool_list_update() to detect the addition of new pools.  Configuration
- * changes are all handled within libzfs.
+ * on pool_list_refresh() to detect the addition and removal of pools.
+ * Configuration changes are all handled within libzfs.
  */
 int
 zpool_do_iostat(int argc, char **argv)
 {
 	int c;
 	int ret;
-	int npools;
 	float interval = 0;
 	unsigned long count = 0;
 	zpool_list_t *list;
@@ -6618,26 +6599,31 @@ zpool_do_iostat(int argc, char **argv)
 		return (1);
 	}
 
+	int last_npools = 0;
 	for (;;) {
-		if ((npools = pool_list_count(list)) == 0)
+		/*
+		 * Refresh all pools in list, adding or removing pools as
+		 * necessary.
+		 */
+		int npools = pool_list_refresh(list);
+		if (npools == 0) {
 			(void) fprintf(stderr, gettext("no pools available\n"));
-		else {
+		} else {
+			/*
+			 * If the list of pools has changed since last time
+			 * around, reset the iteration count to force the
+			 * header to be redisplayed.
+			 */
+			if (last_npools != npools)
+				cb.cb_iteration = 0;
+
 			/*
 			 * If this is the first iteration and -y was supplied
 			 * we skip any printing.
 			 */
 			boolean_t skip = (omit_since_boot &&
 			    cb.cb_iteration == 0);
 
-			/*
-			 * Refresh all statistics.  This is done as an
-			 * explicit step before calculating the maximum name
-			 * width, so that any * configuration changes are
-			 * properly accounted for.
-			 */
-			(void) pool_list_iter(list, B_FALSE, refresh_iostat,
-			    &cb);
-
 			/*
 			 * Iterate over all pools to determine the maximum width
 			 * for the pool / device name column across all pools.
@@ -6728,6 +6714,8 @@ zpool_do_iostat(int argc, char **argv)
 
 		(void) fflush(stdout);
 		(void) fsleep(interval);
+
+		last_npools = npools;
 	}
 
 	pool_list_free(list);

cmd/zpool/zpool_util.h

@@ -76,11 +76,10 @@ typedef struct zpool_list zpool_list_t;
 
 zpool_list_t *pool_list_get(int, char **, zprop_list_t **, zfs_type_t,
     boolean_t, int *);
-void pool_list_update(zpool_list_t *);
+int pool_list_refresh(zpool_list_t *);
 int pool_list_iter(zpool_list_t *, int unavail, zpool_iter_f, void *);
 void pool_list_free(zpool_list_t *);
 int pool_list_count(zpool_list_t *);
-void pool_list_remove(zpool_list_t *, zpool_handle_t *);
 
 extern libzfs_handle_t *g_zfs;
 

tests/runfiles/common.run

@@ -490,6 +490,10 @@ tests = ['zpool_import_001_pos', 'zpool_import_002_pos',
 tags = ['functional', 'cli_root', 'zpool_import']
 timeout = 1200
 
+[tests/functional/cli_root/zpool_iostat]
+tests = ['zpool_iostat_interval_all', 'zpool_iostat_interval_some']
+tags = ['functional', 'cli_root', 'zpool_iostat']
+
 [tests/functional/cli_root/zpool_labelclear]
 tests = ['zpool_labelclear_active', 'zpool_labelclear_exported',
     'zpool_labelclear_removed', 'zpool_labelclear_valid']

tests/zfs-tests/tests/Makefile.am

@@ -197,6 +197,7 @@ nobase_dist_datadir_zfs_tests_tests_DATA += \
 	functional/cli_root/zpool_import/blockfiles/unclean_export.dat.bz2 \
 	functional/cli_root/zpool_import/zpool_import.cfg \
 	functional/cli_root/zpool_import/zpool_import.kshlib \
+	functional/cli_root/zpool_iostat/zpool_iostat.kshlib \
 	functional/cli_root/zpool_initialize/zpool_initialize.kshlib \
 	functional/cli_root/zpool_labelclear/labelclear.cfg \
 	functional/cli_root/zpool_remove/zpool_remove.cfg \
@@ -1178,6 +1179,10 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/cli_root/zpool_import/zpool_import_parallel_admin.ksh \
 	functional/cli_root/zpool_import/zpool_import_parallel_neg.ksh \
 	functional/cli_root/zpool_import/zpool_import_parallel_pos.ksh \
+	functional/cli_root/zpool_iostat/setup.ksh \
+	functional/cli_root/zpool_iostat/cleanup.ksh \
+	functional/cli_root/zpool_iostat/zpool_iostat_interval_all.ksh \
+	functional/cli_root/zpool_iostat/zpool_iostat_interval_some.ksh \
 	functional/cli_root/zpool_initialize/cleanup.ksh \
 	functional/cli_root/zpool_initialize/zpool_initialize_attach_detach_add_remove.ksh \
 	functional/cli_root/zpool_initialize/zpool_initialize_fault_export_import_online.ksh \

tests/zfs-tests/tests/functional/cli_root/zpool_iostat/cleanup.ksh

@@ -0,0 +1,30 @@
+#!/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) 2025, Klara, Inc.
+#
+#
+. $STF_SUITE/include/libtest.shlib
+
+log_pass