Pool split leaks DTL spacemap objects

zpool split does not release DTL space map objects for
removed vdevs. Use the same approach as in spa_vdev_detach().
Mark vdev detached, and dirty DTL.

vdev_count_leaves() is implemented to set reader lock
to reliably count vdevs with private function. However,
while we are already holding spa config locks, we can not
count vdevs.

zpool split does clone original pool MOS for new pool byte-by-byte,
including DTL data for vdevs. As new pool does not have vdevs
left into old pool, this would result leaking those DTL's.

ZTS updates:
remove vdev_zaps_007_pos from zts-report.py.in
zpool_split_devices: use zdb -MC to test split pools

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Toomas Soome <[email protected]>
Closes #18808
Closes #5599

module/zfs/spa.c

@@ -9185,6 +9185,46 @@ spa_vdev_trim(spa_t *spa, nvlist_t *nv, uint64_t cmd_type, uint64_t rate,
 	return (total_errors);
 }
 
+typedef struct spa_split_dtl_arg {
+	spa_t		*ssda_spa;	/* the new pool */
+	uint64_t	*ssda_objs;	/* original DTL space map objects */
+	uint_t		ssda_count;	/* nitems in ssda_objs */
+} spa_split_dtl_arg_t;
+
+/*
+ * Record the DTL space map object of every leaf that has one into objs[],
+ * advancing *idxp. These are the objects that will be carried, via the
+ * copied MOS, onto the split disks.
+ */
+static void
+spa_split_collect_dtl(vdev_t *vd, uint64_t *objs, uint_t *idxp)
+{
+	if (vd->vdev_ops->vdev_op_leaf) {
+		if (vd->vdev_dtl_sm != NULL)
+			objs[(*idxp)++] = space_map_object(vd->vdev_dtl_sm);
+		return;
+	}
+	for (uint64_t c = 0; c < vd->vdev_children; c++)
+		spa_split_collect_dtl(vd->vdev_child[c], objs, idxp);
+}
+
+/*
+ * Callback that frees the inherited DTL space map objects from the new
+ * pool MOS. The new pool MOS is a byte copy of the original pool, so it
+ * contains a DTL space map object for every leaf of the original pool.
+ * The new pool references none of them because split leaves
+ * start with an empty DTL and allocate their own on demand.
+ */
+static void
+spa_split_dtl_free_sync(void *arg, dmu_tx_t *tx)
+{
+	spa_split_dtl_arg_t *ssda = arg;
+	objset_t *mos = ssda->ssda_spa->spa_meta_objset;
+
+	for (uint_t i = 0; i < ssda->ssda_count; i++)
+		space_map_free_obj(mos, ssda->ssda_objs[i], tx);
+}
+
 /*
  * Split a set of devices from their mirrors, and create a new pool from them.
  */
@@ -9199,7 +9239,9 @@ spa_vdev_split_mirror(spa_t *spa, const char *newname, nvlist_t *config,
 	nvlist_t **child, *nvl, *tmp;
 	dmu_tx_t *tx;
 	const char *altroot = NULL;
-	vdev_t *rvd, **vml = NULL;			/* vdev modify list */
+	vdev_t *rvd, **vml = NULL;	/* vdev modify list */
+	uint64_t *dtl_objs = NULL;	/* DTL objs from original pool */
+	uint_t ndtl = 0, nleaves;
 	boolean_t activate_slog;
 
 	ASSERT(spa_writeable(spa));
@@ -9347,6 +9389,11 @@ spa_vdev_split_mirror(spa_t *spa, const char *newname, nvlist_t *config,
 		return (spa_vdev_exit(spa, NULL, txg, error));
 	}
 
+	/* Create array of DTL objects. */
+	nleaves = vdev_count_leaves(spa);
+	dtl_objs = kmem_zalloc(nleaves * sizeof (uint64_t), KM_SLEEP);
+	spa_split_collect_dtl(spa->spa_root_vdev, dtl_objs, &ndtl);
+
 	/* stop writers from using the disks */
 	for (c = 0; c < children; c++) {
 		if (vml[c] != NULL)
@@ -9444,6 +9491,20 @@ spa_vdev_split_mirror(spa_t *spa, const char *newname, nvlist_t *config,
 		    B_TRUE));
 	}
 
+	/*
+	 * Free the DTL space map objects inherited from the original pool
+	 * MOS so we won't leak them.
+	 */
+	if (ndtl != 0) {
+		spa_split_dtl_arg_t ssda;
+
+		ssda.ssda_spa = newspa;
+		ssda.ssda_objs = dtl_objs;
+		ssda.ssda_count = ndtl;
+		VERIFY0(dsl_sync_task(spa_name(newspa), NULL,
+		    spa_split_dtl_free_sync, &ssda, 0, ZFS_SPACE_CHECK_NONE));
+	}
+
 	/* set the props */
 	if (props != NULL) {
 		spa_configfile_set(newspa, props, B_FALSE);
@@ -9483,11 +9544,33 @@ spa_vdev_split_mirror(spa_t *spa, const char *newname, nvlist_t *config,
 			}
 
 			vdev_split(vml[c]);
+
+			/*
+			 * As in spa_vdev_detach(), mark the vdev detached
+			 * and dirty its DTL, so that vdev_dtl_sync() frees
+			 * the leaf's DTL space map object.
+			 */
+			vml[c]->vdev_detached = B_TRUE;
+
+			/*
+			 * The leaf ZAP was transferred to the new pool
+			 * and this pool's copy is destroyed by the AVZ
+			 * rebuild below, so clear it to keep
+			 * vdev_dtl_sync() from destroying it again.
+			 */
+			vml[c]->vdev_leaf_zap = 0;
+
+			/*
+			 * vml[c]->vdev_top may be stale; the
+			 * surviving top-level vdev is rvd->vdev_child[c].
+			 */
+			if (vml[c]->vdev_dtl_sm != NULL)
+				vdev_dirty(rvd->vdev_child[c], VDD_DTL,
+				    vml[c], txg);
+
 			if (error == 0)
 				spa_history_log_internal(spa, "detach", tx,
 				    "vdev=%s", vml[c]->vdev_path);
-
-			vdev_free(vml[c]);
 		}
 	}
 	spa->spa_avz_action = AVZ_ACTION_REBUILD;
@@ -9498,6 +9581,18 @@ spa_vdev_split_mirror(spa_t *spa, const char *newname, nvlist_t *config,
 		dmu_tx_commit(tx);
 	(void) spa_vdev_exit(spa, NULL, txg, 0);
 
+	/*
+	 * txg is synced, free vdevs.
+	 */
+	spa_config_enter(spa, SCL_STATE_ALL, spa, RW_WRITER);
+	for (c = 0; c < children; c++) {
+		if (vml[c] != NULL && vml[c]->vdev_ops != &vdev_indirect_ops) {
+			ASSERT0P(vml[c]->vdev_dtl_sm);
+			vdev_free(vml[c]);
+		}
+	}
+	spa_config_exit(spa, SCL_STATE_ALL, spa);
+
 	if (zio_injection_enabled)
 		zio_handle_panic_injection(spa, FTAG, 3);
 
@@ -9507,6 +9602,8 @@ spa_vdev_split_mirror(spa_t *spa, const char *newname, nvlist_t *config,
 
 	newspa->spa_is_splitting = B_FALSE;
 	kmem_free(vml, children * sizeof (vdev_t *));
+	if (dtl_objs != NULL)
+		kmem_free(dtl_objs, nleaves * sizeof (uint64_t));
 
 	/* if we're not going to mount the filesystems in userland, export */
 	if (exp)
@@ -9540,6 +9637,9 @@ out:
 	(void) spa_vdev_exit(spa, NULL, txg, error);
 
 	kmem_free(vml, children * sizeof (vdev_t *));
+	if (dtl_objs != NULL)
+		kmem_free(dtl_objs, nleaves * sizeof (uint64_t));
+
 	return (error);
 }
 

module/zfs/vdev.c

@@ -555,10 +555,15 @@ int
 vdev_count_leaves(spa_t *spa)
 {
 	int rc;
+	boolean_t held;
 
-	spa_config_enter(spa, SCL_VDEV, FTAG, RW_READER);
+	held = (spa_config_held(spa, SCL_VDEV, RW_WRITER) == SCL_VDEV);
+
+	if (!held)
+		spa_config_enter(spa, SCL_VDEV, FTAG, RW_READER);
 	rc = vdev_count_leaves_impl(spa->spa_root_vdev);
-	spa_config_exit(spa, SCL_VDEV, FTAG);
+	if (!held)
+		spa_config_exit(spa, SCL_VDEV, FTAG);
 
 	return (rc);
 }

tests/test-runner/bin/zts-report.py.in

@@ -170,7 +170,6 @@ known = {
     'refreserv/refreserv_004_pos': ['FAIL', known_reason],
     'rootpool/setup': ['SKIP', na_reason],
     'rsend/rsend_008_pos': ['SKIP', 6066],
-    'vdev_zaps/vdev_zaps_007_pos': ['FAIL', known_reason],
 }
 
 if sys.platform.startswith('freebsd'):

tests/zfs-tests/tests/functional/cli_root/zpool_split/zpool_split_devices.ksh

@@ -89,7 +89,24 @@ do
 	typeset conf=${poolconfs[$i]}
 	setup_mirror $conf
 	log_mustnot zpool split $TESTPOOL $TESTPOOL2 ${baddevs[$i]}
-	log_must zpool split -R $altroot $TESTPOOL $TESTPOOL2 ${gooddevs[$i]}
+
+	# Force creating DTL objects on vdevs
+	typeset dtldev=${gooddevs[$i]%% *}
+	typeset mntpnt=$(get_prop mountpoint $TESTPOOL)
+	log_must zpool offline $TESTPOOL $dtldev
+	log_must dd if=/dev/zero of=$mntpnt/dtlfile bs=1k count=1
+	log_must zpool online $TESTPOOL $dtldev
+	sync_pool $TESTPOOL true
+
+	log_must zpool split -R $altroot -o cachefile=/etc/zfs/zpool.cache \
+	    $TESTPOOL $TESTPOOL2 ${gooddevs[$i]}
+	sync_pool $TESTPOOL true
+	sync_pool $TESTPOOL2 true
+
+	# check space maps
+	log_must zdb -MC $TESTPOOL
+	log_must zdb -MC $TESTPOOL2
+
 	# Verify "good" devices ended up in the new pool
 	log_must poolexists $TESTPOOL2
 	for filedev in ${gooddevs[$i]}; do