-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add snapshots_changed_nsecs dataset property (#17998) #18031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add snapshots_changed_nsecs dataset property (#17998) #18031
Conversation
36e9976 to
1626909
Compare
|
While I understand your motivation, it still looks weird to me report time in two properties. Sure it may work as the change notification, but for any other use it would be quite useless, being potentially incoherent with the seconds part. |
It's a bit weird, yes, but it is consistent with the second part. The full nanosecond timestamp since the Unix epoch can be reconstructed as snapshots_changed * 1000000000 + snapshots_changed_nsecs It's also consistent with the existing design elsewhere in the ZFS codebase. FWIW, I figured I should design it so the change is as minimal as possible and "fits in" with the existing codebase, i.e. the ZFS event time split, which does the same thing - it reports ZEVENT_TIME_SECS and ZEVENT_TIME_NSECS with the same semantics: The full nanosecond timestamp = ZEVENT_TIME_SECS * 1000000000 + ZEVENT_TIME_NSECS @amotin Having said that, I'd be fine with changing snapshots_changed_nsecs to represent the full timestamp in nanoseconds in a uint64 if that's what you'd prefer (the number will overflow uint64 in the year 2554 but I'm not too worried about that yet :-) |
1626909 to
bf3d8da
Compare
It can't, if a new snapshot is created between reading two properties.
I can't say that I like having two properties for the same thing just with different precision, but this would make more sense to me. |
5606481 to
b23fbc8
Compare
@amotin Ok, I've now changed the code (and pushed it) such that snapshots_changed_nsecs represents the full timestamp in nanoseconds in a uint64. Let me know if you find any other issues! |
b23fbc8 to
98e725c
Compare
module/zfs/dsl_dataset.c
Outdated
| dsl_prop_nvlist_add_uint64(nv, ZFS_PROP_SNAPSHOTS_CHANGED, | ||
| dsl_dir_snap_cmtime(ds->ds_dir).tv_sec); | ||
| snap_cmtime.tv_sec); | ||
| if (snap_cmtime.tv_sec != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be conditional, if the original property is not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've now changed it in libzfs_dataset.c and dsl_dataset.c to use the same pattern as snapshots_changed. See the new commit I just pushed.
[The if (snap_cmtime.tv_sec != 0) branch amounted to the same effect wrt “report - until the first snapshot is created”, in a somewhat more compact but less consistent style.]
BTW, the new commit also changes visible = B_FALSE to visible = B_TRUE for snapshots_changed_nsecs in zfs_prop.c so it is now the same as the existing snapshots_changed pattern - both properties are visible by default in zfs get output.
98e725c to
7c4c6d9
Compare
Signed-off-by: Wolfgang Hoschek <[email protected]>
7c4c6d9 to
ee9e40e
Compare
This PR is in response to #17998
Motivation and Context
OpenZFS 2.2 introduced the
snapshots_changeddataset property as a cheap, persistent way to tell whether the snapshot set for a dataset has changed without having to runzfs list -t snapshoton every poll. Replication and monitoring tools use this as a cache key to avoid repeatedly walking large snapshot trees.Because
snapshots_changedis exposed only with integer-second granularity, multiple snapshot creations or deletions that occur within the same second share the same value. Tools that rely on it as a change detector can therefore miss snapshot updates that happen in the same second, causing cache-based fast paths to skipzfs list -t snapshotwhen they should refresh their snapshot metadata. This is particularly problematic for near real-time replication and monitoring systems that take frequent snapshots.This change adds a companion read-only dataset property,
snapshots_changed_nsecs, which exposes the nanosecond (tv_nsec) component of the same internal timestamp that backssnapshots_changed. Together,(snapshots_changed, snapshots_changed_nsecs)provide a reliable, nanosecond-granularity indicator of snapshot-set changes without requiring any on-disk format changes.The change is intentionally minimal and aligned with existing patterns:
inode_timespecwithtv_secandtv_nsec.ZEVENT_TIME_SECS/ZEVENT_TIME_NSECS).Description
snapshots_changed_nsecswith enumZFS_PROP_SNAPSHOTS_CHANGED_NSECSininclude/sys/fs/zfs.handlib/libzfs/libzfs.abi.module/zcommon/zfs_prop.cas a numeric, read-only dataset property for filesystems and volumes:snapshots_changed_nsecsPROP_TYPE_NUMBERSNAPSHOTS_CHANGED_NSECScolumn, no humanized formatting)dsl_dir_snap_cmtime()timestamp indsl_dataset_stats()(module/zfs/dsl_dataset.c):dsl_dir_snap_cmtime(ds->ds_dir)into a localinode_timespec_t snap_cmtime.snap_cmtime.tv_secassnapshots_changed.snap_cmtime.tv_sec != 0, exportsnap_cmtime.tv_nsecassnapshots_changed_nsecs.libzfs(lib/libzfs/libzfs_dataset.c):ZFS_PROP_SNAPSHOTS_CHANGED_NSECScase tozfs_prop_get()which retrieves the numeric value and formats it as a decimal string.module/zfs/zcp_get.c):get_special_prop()to returndsl_dir_snap_cmtime(ds->ds_dir).tv_nsecforZFS_PROP_SNAPSHOTS_CHANGED_NSECS.zfsprops(7)(man/man7/zfsprops.7):snapshots_changed_nsecsis the nanosecond component corresponding tosnapshots_changed.[0, 999999999]and is only meaningful whensnapshots_changedis not-.snapshots_changed * 1000000000 + snapshots_changed_nsecs.snapshot_018_pos(tests/zfs-tests/tests/functional/snapshot/snapshot_018_pos.ksh):snapshots_changedandsnapshots_changed_nsecsare-before any snapshots exist, via bothzfs getandzfs list -o.snapshots_changedis greater than or equal to the current time, and thatsnapshots_changed_nsecsis in[0, 1000000000).zfs getandzfs list -oreport identical values for both properties on the pool and filesystem.snapshots_changedandsnapshots_changed_nsecsand that the.zfs/snapshotdirectory mtime matchessnapshots_changed.How Has This Been Tested?
tests/zfs-tests/tests/functional/snapshot/snapshot_018_pos.kshto cover bothsnapshots_changedandsnapshots_changed_nsecsfor:snapshots_changedandsnapshots_changed_nsecsare-when no snapshots exist.snapshots_changed_nsecsis always in the range[0, 1000000000)whensnapshots_changedis set.zfs get -pandzfs list -p -oreturn consistent values for both properties..zfs/snapshotdirectory mtime remains consistent withsnapshots_changed.Types of changes
Checklist:
Signed-off-by.