Use a single creation time for a recursive snapshot

All snapshots created by one "zfs snapshot -r" are taken atomically in a
single txg, but dsl_dataset_snapshot_sync_impl() sampled the wall clock
once per snapshot inside the per-dataset loop.  If that loop crossed a
one-second boundary, snapshots of the same txg were stamped with
different "creation" times, which confuses tools that order snapshots by
creation time (createtxg was already coherent).

Sample the wall clock once in dsl_dataset_snapshot_sync(), before the
loop, and pass it to each dsl_dataset_snapshot_sync_impl() call.  The
other callers (origin, temporary snapshot, and receive) each create a
single snapshot per sync task and keep their current behavior by passing
gethrestime_sec() directly.

Add a test that takes a recursive snapshot of a 50-child hierarchy and
verifies every snapshot shares one createtxg and one creation time.  The
test locks the fixed invariant rather than reproducing the timing race,
which the fast in-kernel sync loop hit only rarely.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: MorganaFuture <[email protected]>
Closes #18220
Closes #18754

include/sys/dsl_dataset.h

@@ -468,7 +468,7 @@ void dsl_dataset_clone_swap_sync_impl(dsl_dataset_t *clone,
 int dsl_dataset_snapshot_check_impl(dsl_dataset_t *ds, const char *snapname,
     dmu_tx_t *tx, boolean_t recv, uint64_t cnt, cred_t *cr);
 void dsl_dataset_snapshot_sync_impl(dsl_dataset_t *ds, const char *snapname,
-    dmu_tx_t *tx);
+    uint64_t crtime, dmu_tx_t *tx);
 
 void dsl_dataset_remove_from_next_clones(dsl_dataset_t *ds, uint64_t obj,
     dmu_tx_t *tx);

module/zfs/dmu_recv.c

@@ -3729,12 +3729,10 @@ dmu_recv_end_sync(void *arg, dmu_tx_t *tx)
 		drc->drc_os = NULL;
 
 		dsl_dataset_snapshot_sync_impl(origin_head,
-		    drc->drc_tosnap, tx);
+		    drc->drc_tosnap, drc->drc_drrb->drr_creation_time, tx);
 
-		/* set snapshot's creation time and guid */
+		/* set snapshot's guid */
 		dmu_buf_will_dirty(origin_head->ds_prev->ds_dbuf, tx);
-		dsl_dataset_phys(origin_head->ds_prev)->ds_creation_time =
-		    drc->drc_drrb->drr_creation_time;
 		dsl_dataset_phys(origin_head->ds_prev)->ds_guid =
 		    drc->drc_drrb->drr_toguid;
 		dsl_dataset_phys(origin_head->ds_prev)->ds_flags &=
@@ -3755,12 +3753,11 @@ dmu_recv_end_sync(void *arg, dmu_tx_t *tx)
 	} else {
 		dsl_dataset_t *ds = drc->drc_ds;
 
-		dsl_dataset_snapshot_sync_impl(ds, drc->drc_tosnap, tx);
+		dsl_dataset_snapshot_sync_impl(ds, drc->drc_tosnap,
+		    drc->drc_drrb->drr_creation_time, tx);
 
-		/* set snapshot's creation time and guid */
+		/* set snapshot's guid */
 		dmu_buf_will_dirty(ds->ds_prev->ds_dbuf, tx);
-		dsl_dataset_phys(ds->ds_prev)->ds_creation_time =
-		    drc->drc_drrb->drr_creation_time;
 		dsl_dataset_phys(ds->ds_prev)->ds_guid =
 		    drc->drc_drrb->drr_toguid;
 		dsl_dataset_phys(ds->ds_prev)->ds_flags &=

module/zfs/dsl_dataset.c

@@ -1723,7 +1723,7 @@ dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 
 void
 dsl_dataset_snapshot_sync_impl(dsl_dataset_t *ds, const char *snapname,
-    dmu_tx_t *tx)
+    uint64_t crtime, dmu_tx_t *tx)
 {
 	dsl_pool_t *dp = ds->ds_dir->dd_pool;
 	dmu_buf_t *dbuf;
@@ -1771,7 +1771,7 @@ dsl_dataset_snapshot_sync_impl(dsl_dataset_t *ds, const char *snapname,
 	dsphys->ds_prev_snap_txg = dsl_dataset_phys(ds)->ds_prev_snap_txg;
 	dsphys->ds_next_snap_obj = ds->ds_object;
 	dsphys->ds_num_children = 1;
-	dsphys->ds_creation_time = gethrestime_sec();
+	dsphys->ds_creation_time = crtime;
 	dsphys->ds_creation_txg = crtxg;
 	dsphys->ds_deadlist_obj = dsl_dataset_phys(ds)->ds_deadlist_obj;
 	dsphys->ds_referenced_bytes = dsl_dataset_phys(ds)->ds_referenced_bytes;
@@ -1909,6 +1909,14 @@ dsl_dataset_snapshot_sync(void *arg, dmu_tx_t *tx)
 	dsl_pool_t *dp = dmu_tx_pool(tx);
 	nvpair_t *pair;
 
+	/*
+	 * All snapshots created by one request are taken atomically in a
+	 * single txg, so give them a single creation time; sampling the
+	 * clock per snapshot could otherwise straddle a second boundary and
+	 * report different creation times for snapshots of the same txg.
+	 */
+	uint64_t crtime = gethrestime_sec();
+
 	for (pair = nvlist_next_nvpair(ddsa->ddsa_snaps, NULL);
 	    pair != NULL; pair = nvlist_next_nvpair(ddsa->ddsa_snaps, pair)) {
 		dsl_dataset_t *ds;
@@ -1920,7 +1928,7 @@ dsl_dataset_snapshot_sync(void *arg, dmu_tx_t *tx)
 		(void) strlcpy(dsname, name, atp - name + 1);
 		VERIFY0(dsl_dataset_hold(dp, dsname, FTAG, &ds));
 
-		dsl_dataset_snapshot_sync_impl(ds, atp + 1, tx);
+		dsl_dataset_snapshot_sync_impl(ds, atp + 1, crtime, tx);
 		if (ddsa->ddsa_props != NULL) {
 			dsl_props_set_sync_impl(ds->ds_prev,
 			    ZPROP_SRC_LOCAL, ddsa->ddsa_props, tx);
@@ -2065,7 +2073,8 @@ dsl_dataset_snapshot_tmp_sync(void *arg, dmu_tx_t *tx)
 
 	VERIFY0(dsl_dataset_hold(dp, ddsta->ddsta_fsname, FTAG, &ds));
 
-	dsl_dataset_snapshot_sync_impl(ds, ddsta->ddsta_snapname, tx);
+	dsl_dataset_snapshot_sync_impl(ds, ddsta->ddsta_snapname,
+	    gethrestime_sec(), tx);
 	dsl_dataset_user_hold_sync_one(ds->ds_prev, ddsta->ddsta_htag,
 	    ddsta->ddsta_cleanup_minor, gethrestime_sec(), tx);
 	dsl_destroy_snapshot_sync_impl(ds->ds_prev, B_TRUE, tx);

module/zfs/dsl_pool.c

@@ -1180,7 +1180,8 @@ dsl_pool_create_origin(dsl_pool_t *dp, dmu_tx_t *tx)
 	dsobj = dsl_dataset_create_sync(dp->dp_root_dir, ORIGIN_DIR_NAME,
 	    NULL, 0, kcred, NULL, tx);
 	VERIFY0(dsl_dataset_hold_obj(dp, dsobj, FTAG, &ds));
-	dsl_dataset_snapshot_sync_impl(ds, ORIGIN_DIR_NAME, tx);
+	dsl_dataset_snapshot_sync_impl(ds, ORIGIN_DIR_NAME, gethrestime_sec(),
+	    tx);
 	VERIFY0(dsl_dataset_hold_obj(dp, dsl_dataset_phys(ds)->ds_prev_snap_obj,
 	    dp, &dp->dp_origin_snap));
 	dsl_dataset_rele(ds, FTAG);

tests/runfiles/common.run

@@ -359,7 +359,7 @@ tags = ['functional', 'cli_root', 'zfs_share']
 tests = ['zfs_snapshot_001_neg', 'zfs_snapshot_002_neg',
     'zfs_snapshot_003_neg', 'zfs_snapshot_004_neg', 'zfs_snapshot_005_neg',
     'zfs_snapshot_006_pos', 'zfs_snapshot_007_neg', 'zfs_snapshot_008_neg',
-    'zfs_snapshot_009_pos']
+    'zfs_snapshot_009_pos', 'zfs_snapshot_010_pos']
 tags = ['functional', 'cli_root', 'zfs_snapshot']
 
 [tests/functional/cli_root/zfs_unload-key]

tests/zfs-tests/tests/Makefile.am

@@ -1017,6 +1017,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/cli_root/zfs_snapshot/zfs_snapshot_007_neg.ksh \
 	functional/cli_root/zfs_snapshot/zfs_snapshot_008_neg.ksh \
 	functional/cli_root/zfs_snapshot/zfs_snapshot_009_pos.ksh \
+	functional/cli_root/zfs_snapshot/zfs_snapshot_010_pos.ksh \
 	functional/cli_root/zfs_sysfs/cleanup.ksh \
 	functional/cli_root/zfs_sysfs/setup.ksh \
 	functional/cli_root/zfs_sysfs/zfeature_set_unsupported.ksh \

tests/zfs-tests/tests/functional/cli_root/zfs_snapshot/zfs_snapshot_010_pos.ksh

@@ -0,0 +1,77 @@
+#!/bin/ksh -p
+# SPDX-License-Identifier: CDDL-1.0
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2026 by MorganaFuture. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+#	A recursive snapshot is taken atomically in a single txg, so every
+#	resulting snapshot must report the same 'creation' time.
+#
+# STRATEGY:
+#	1. Create a hierarchy with many child datasets.
+#	2. Take a recursive snapshot of the whole hierarchy.
+#	3. Verify every resulting snapshot shares one createtxg and one
+#	   creation time.
+#
+# This is an invariant check on the fixed behavior, not a reproducer of the
+# old per-dataset clock sampling.  That race only showed up when the in-kernel
+# snapshot sync loop happened to straddle a one-second boundary, which is rare
+# and not something a test can trigger reliably; here we just assert that a
+# single recursive request collapses to one creation time by construction.
+#
+
+verify_runnable "both"
+
+typeset PARENT=$TESTPOOL/crtime
+
+function cleanup
+{
+	datasetexists $PARENT && destroy_dataset $PARENT -r
+}
+
+log_assert "All snapshots from a recursive snapshot share one creation time"
+log_onexit cleanup
+
+log_must zfs create $PARENT
+
+# Several children so the recursive snapshot spans many datasets.
+for i in $(seq 1 50); do
+	log_must zfs create $PARENT/child$i
+done
+
+log_must zfs snapshot -r $PARENT@snap
+
+# The recursive snapshot must have covered the whole hierarchy: the parent
+# plus its 50 children.
+typeset nsnaps=$(zfs list -Hr -t snapshot -o name $PARENT | wc -l)
+log_must test "$nsnaps" -eq 51
+
+# The snapshots are atomic with respect to createtxg, so they already share
+# one transaction group; check that first as a sanity guard on the hierarchy.
+typeset txgs=$(zfs list -Hr -t snapshot -o createtxg -p $PARENT | sort -u |
+    wc -l)
+log_must test "$txgs" -eq 1
+
+# With the fix every snapshot in the one recursive request also reports the
+# same creation time, so the set of creation values collapses to exactly one.
+typeset count=$(zfs list -Hr -t snapshot -o creation -p $PARENT | sort -u |
+    wc -l)
+log_note "distinct creation times across the recursive snapshot: $count"
+log_must test "$count" -eq 1
+
+log_pass "All snapshots from a recursive snapshot share one creation time"