Pool split leaks DTL spacemap objects by tsoome · Pull Request #18808 · openzfs/zfs

tsoome · 2026-07-15T16:31:28Z

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.

Closes #5599

Motivation and Context

End leak of space map objects in original pool resulting from 'zpool split' command.

#5599

Description

mark new pool vdevs detached and DTL dirty. This would trigger release of respective objects on TXG sync. Once sync is done, free vdevs.

How Has This Been Tested?

zpool create testpool mirror disk1 disk2
zpool split testpool testpool2 disk2
zdb -MC testpool # to verify space map refcounts - there is refcount mismatch before update and no mismatch after update.

Types of Changes

[ x] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Quality assurance (non-breaking change which makes the code more robust against bugs)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist

[ x] My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
[x ] All commit messages are properly formatted and contain Signed-off-by.

Copilot

Pull request overview

This PR fixes a long-standing resource leak during zpool split by ensuring that removed mirror leaf vdevs have their DTL spacemap objects released (matching the cleanup behavior used by spa_vdev_detach()), preventing leaked MOS objects and zdb spacemap refcount mismatches in the original pool.

Changes:

Mark split-off leaf vdevs as detached and dirty their DTL so vdev_dtl_sync() frees the leaf DTL spacemap object in syncing context.
Clear the leaf ZAP pointer on the detached leaf to avoid double-destruction during subsequent AVZ rebuild.
Delay vdev_free() of removed leaf vdevs until after the relevant TXG has synced.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+			/*
+			 * 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].
+			 */
+			vdev_dirty(rvd->vdev_child[c], VDD_DTL, vml[c], txg);
+


behlendorf

Let's make sure this stays fixed. Can you add a log_must zdb -MC $TESTPOOL to the while loop in cli_root/zpool_split/zpool_split_devices.ksh. It wouldn't hurt to check $TESTPOOL2 as well.

tsoome · 2026-07-15T20:03:54Z

Let's make sure this stays fixed. Can you add a log_must zdb -MC $TESTPOOL to the while loop in cli_root/zpool_split/zpool_split_devices.ksh. It wouldn't hurt to check $TESTPOOL2 as well.

Sure. I actually did step on this while working on vdev_zaps_007 (on illumos), havent yet investigated OpenZFS tests (illumos is missing some of there).

tsoome · 2026-07-15T20:05:12Z

ah, I forgot to add. This work is addressing original pool. There is also new pool....

behlendorf · 2026-07-15T20:18:38Z

Oh, I'm glad you mentioned vdev_zaps_007. I was trying to remember where I saw this failure before. If this change is sufficient to sort out vdev_zaps_007 can you also remove it from from known failure list in tests/test-runner/bin/zts-report.py.in.

behlendorf · 2026-07-21T21:48:58Z

@tsoome are you planning to add the fixes for the new pool case to this PR as well?

tsoome · 2026-07-21T22:06:39Z

@tsoome are you planning to add the fixes for the new pool case to this PR as well?

I think, it would need one for its own. I got the hints what we do leak, but I havent yet got to see how to clear those leaks - and how to actually test the presence. Also still need to update the tests here - but that has to wait for tomorrow (its 1am here and I should get some sleep:D)

behlendorf · 2026-07-22T21:50:49Z

Got it. I'm fine with merging this in two parts. Could you just add the zdb -MC check to this PR to verify the original pool. Then follow up with the other half when you're ready.

Copilot

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

The new “Force creating DTL objects” sequence doesn’t guarantee that any TXG actually syncs while the leaf is offline. If the TXG sync happens only after zpool online, the DTL spacemap may never be created, and the test may not reliably exercise the leak (potential flakiness). Sync the pool once while the device is still offline, and wait for the resilver triggered by zpool online to complete before splitting.

	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

behlendorf · 2026-07-30T21:13:09Z

Pending the CI results I think this looks like it's ready.

tsoome · 2026-07-31T07:12:06Z

Pending the CI results I think this looks like it's ready.

I actually have code now for new pool fix too. The zpool_split_device test update currently does receive FAIL status, because with DTL creation, the zdb -MC will detect space map leak from new pool, therefore it may be good idea to incorporate the second half here after all.

Copilot

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

module/zfs/spa.c:9447

Typo in the new comment: "wont" -> "will not" (or "won't").

	/*
	 * Free the DTL space map objects inherited from the original pool
	 * MOS so we wont leak them.
	 */

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

This block claims to "Force creating DTL objects on vdevs" but only does so for the first device in gooddevs[$i]. For configurations where multiple devices are split, the test may not create a DTL object for every split leaf, which can miss regressions that only manifest on some removed vdevs. Consider looping over all split devices (and quote the output path).

	# 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

+	held =
+	    spa_config_held(spa, SCL_STATE, RW_READER) == SCL_STATE ||
+	    spa_config_held(spa, SCL_STATE, RW_WRITER) == SCL_STATE;
+
+	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);


Copilot

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

module/zfs/spa.c:9447

Typo in comment: "wont" should be "won't".

	/*
	 * Free the DTL space map objects inherited from the original pool
	 * MOS so we wont leak them.
	 */

+			/*
+			 * vml[c]->vdev_top may be stale; the
+			 * surviving top-level vdev is rvd->vdev_child[c].
+			 */
+			vdev_dirty(rvd->vdev_child[c], VDD_DTL, vml[c], txg);


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. Signed-off-by: Toomas Soome <tsoome@me.com> Closes openzfs#5599

Signed-off-by: Toomas Soome <tsoome@me.com>

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. Signed-off-by: Toomas Soome <tsoome@me.com>

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. Signed-off-by: Toomas Soome <tsoome@me.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

module/zfs/spa.c:9447

Typo in the comment: "wont" should be "won't".

	/*
	 * Free the DTL space map objects inherited from the original pool
	 * MOS so we wont leak them.
	 */

behlendorf · 2026-07-31T18:53:11Z

Ah okay. Just let me know when this is ready for another look.

tsoome · 2026-08-03T06:47:41Z

Ah okay. Just let me know when this is ready for another look.

I think, it should be good.

behlendorf · 2026-08-04T23:25:49Z

+
+	# check space maps
+	log_must zdb -MC $TESTPOOL
+	log_must zdb -MC $TESTPOOL2


Interesting, this check is reliably failing in the CI. If we trust the error message it seems the split pool doesn't get added to the cache file. That sure seems like another bug which has been hiding.

11:41:07.28 failed to find config for pool 'testpool2': no matching pools 11:41:07.28 zdb: can't open 'testpool2': No such file or directory 11:41:07.28 ERROR: zdb -MC testpool2 exited 1

in my test machine, the cause was left around pool traces - like exported pool once created from multiple disks, then new pool was created on some of those disks and zdb got confused from those. When I made sure all disks had destroyed pools, the error went away... I did that test on illumos, havent checked with OpenZFS yet.

Copilot AI review requested due to automatic review settings July 15, 2026 16:31

Copilot started reviewing on behalf of tsoome July 15, 2026 16:31 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

behlendorf reviewed Jul 15, 2026

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 15, 2026

Copilot AI review requested due to automatic review settings July 23, 2026 23:06

tsoome force-pushed the split branch from f56887a to 74a92d2 Compare July 23, 2026 23:06

Copilot started reviewing on behalf of tsoome July 23, 2026 23:07 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

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

Copilot AI review requested due to automatic review settings July 30, 2026 20:47

tsoome force-pushed the split branch from 74a92d2 to fecc591 Compare July 30, 2026 20:47

Copilot started reviewing on behalf of tsoome July 30, 2026 20:48 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

behlendorf approved these changes Jul 30, 2026

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 30, 2026

Copilot AI review requested due to automatic review settings July 31, 2026 13:46

github-actions Bot removed the Status: Accepted Ready to integrate (reviewed, tested) label Jul 31, 2026

Copilot started reviewing on behalf of tsoome July 31, 2026 13:47 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 31, 2026 14:07

tsoome force-pushed the split branch from 0b6346a to 0349c97 Compare July 31, 2026 14:07

Copilot started reviewing on behalf of tsoome July 31, 2026 14:08 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Comment thread module/zfs/spa.c Outdated

Comment on lines +9513 to +9517

/*

* vml[c]->vdev_top may be stale; the

* surviving top-level vdev is rvd->vdev_child[c].

*/

vdev_dirty(rvd->vdev_child[c], VDD_DTL, vml[c], txg);

tsoome added 5 commits July 31, 2026 17:19

Pool split leaks DTL spacemap objects

d4844a6

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. Signed-off-by: Toomas Soome <tsoome@me.com> Closes openzfs#5599

remove vdev_zaps_007_pos from zts-report.py.in

413c35e

Signed-off-by: Toomas Soome <tsoome@me.com>

zpool_split_devices: use zdb -MC to test split pools

4e6348e

Signed-off-by: Toomas Soome <tsoome@me.com>

Allow vdev_count_leaves() to be called with lock held

653666c

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. Signed-off-by: Toomas Soome <tsoome@me.com>

zpool split does leak DTLs in split off pool

4a5d619

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. Signed-off-by: Toomas Soome <tsoome@me.com>

Copilot AI review requested due to automatic review settings July 31, 2026 14:20

tsoome force-pushed the split branch from 0349c97 to 4a5d619 Compare July 31, 2026 14:20

Copilot started reviewing on behalf of tsoome July 31, 2026 14:20 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

behlendorf self-requested a review July 31, 2026 18:52

behlendorf added the Status: Work in Progress Not yet ready for general review label Jul 31, 2026

behlendorf reviewed Aug 4, 2026

View reviewed changes

Uh oh!

Conversation

tsoome commented Jul 15, 2026

Motivation and Context

Description

How Has This Been Tested?

Types of Changes

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

tsoome commented Jul 15, 2026

Uh oh!

tsoome commented Jul 15, 2026

Uh oh!

behlendorf commented Jul 15, 2026

Uh oh!

behlendorf commented Jul 21, 2026

Uh oh!

tsoome commented Jul 21, 2026

Uh oh!

behlendorf commented Jul 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

behlendorf commented Jul 30, 2026

Uh oh!

tsoome commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

behlendorf commented Jul 31, 2026

Uh oh!

tsoome commented Aug 3, 2026

Uh oh!

behlendorf Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

tsoome Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants