zpool iostat: refresh pool list every interval

When running zpool iostat in interval mode, it would not notice any new
pools created or imported, and would forget any destroyed or exported,
so would not notice if they came back. This leads to outputting "no
pools available" every interval until killed.

It looks like this was at least intended to work; the comment above
zpool_do_iostat() indicates that it is expected to "deal with pool
creation/destruction" and that pool_list_update() would detect new
pools. That call however was removed in 3e43edd, though its unclear
if that broke this behaviour and it wasn't noticed, or if it never
worked, or if something later broke it. That said, the lack of
pool_list_update() is only part of the reason it doesn't work properly.

The fundamental problem is that the various things involved in
refreshing or updating the list of pools would aggressively ignore,
remove, skip or fail on pools that stop existing, or that already exist.
Mostly this meant that once a pool is removed from the list, it will
never be seen again. Restoring pool_list_update() to the
zpool_do_iostat() loop only partially fixes this - it would find "new"
pools again, but only in the "all pools" (no args) mode, and because its
iterator callback add_pool() would abort the iterator if it already has
a pool listed, it would only add pools if there weren't any already.

So, this commit reworks the structure somewhat. pool_list_update()
becomes pool_list_refresh(), and will ensure the state of all pools in
the list are updated. In the "all pools" mode, it will also add new
pools and remove pools that disappear, but when a fixed list of pools is
used, the list doesn't change, only the state of the pools within it.

The rest of the commit is adjusting things for this much simpler
structure. Regardless of the mode in use, pool_list_refresh() will
always do the right thing, so the driver code can just get on with the
display.

Now that pools can appear and disappear, I've made it so the header (if
enabled) is re-printed when the list changes, so that its easier to see
what's happening if the column widths change.

Since this is all rather complicated, I've included tests for the "all
pools" and "set of pools" modes.

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

Author: robn

cmd/zpool/zpool_iter.c

@@ -81,9 +81,8 @@ 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));
 	uu_avl_index_t idx;
 
@@ -107,6 +106,18 @@ add_pool(zpool_handle_t *zhp, void *data)
 	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
@@ -137,7 +148,7 @@ pool_list_get(int argc, char **argv, zprop_list_t **proplist, zfs_type_t type,
 	zlp->zl_literal = literal;
 
 	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 +170,51 @@ 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);
+	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;
+		}
+		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);
+
+	/* Now walk the list and remove any that are no longer available. */
+	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);
+
+		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);
+		}
+	}
+
+	return (uu_avl_numnodes(zlp->zl_avl));
 }
 
 /*
@@ -190,23 +237,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

@@ -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

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

@@ -0,0 +1,32 @@
+#!/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
+
+verify_runnable "global"
+
+log_pass