Skip to content

Channel programs: add zfs.sync.clone() #17426

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

Closed
wants to merge 4 commits into from
Closed

Conversation

robn
Copy link
Member

@robn robn commented Jun 5, 2025

Motivation and Context

A popular request on the OpenZFS Production Users call is the ability for channel programs to create clones. I was catching up on this week's call at lunch today and heard it again and, in need of a palate cleanser after far too much correctness work this week, I took a swing at it. A pleasant diversion!

Description

Adds zfs.sync.clone(snapshot, newdataset). Since clone is already implemented with a standard check/sync pair of functions, it's all really just wiring.

How Has This Been Tested?

New tests included to test the new call, based on the existing zfs.sync.snapshot() tests. Since I've renamed and moved things around, I ran the zfs_clone tests also. All is well!

Types of changes

  • 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, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@robn robn changed the title Channel program: add zfs.sync.clone() Channel programs: add zfs.sync.clone() Jun 5, 2025
@Crest
Copy link

Crest commented Jun 5, 2025

Would creating new (empty) filesystems be just as easy? How would mounting the filesystems be handled? Would the channel program be expected to return the list of filesystems to be mounted?

@Crest
Copy link

Crest commented Jun 5, 2025

Would it be possible to also set (non-inheritable) properties on the clone e.g. canmount=noauto?

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jun 5, 2025
@robn
Copy link
Member Author

robn commented Jun 6, 2025

Would creating new (empty) filesystems be just as easy?

@Crest Quick glance says a bit harder, but maybe not very hard, at least for the simple cases.

How would mounting the filesystems be handled? Would the channel program be expected to return the list of filesystems to be mounted?

Unclear to me. I would expect the program itself wouldn't need to do anything; there's already a list of "datasets that were created" on the channel program context that are considered for follow-up work after the fact; this is how device nodes for new zvols are created (this functionality already existed to support zvol snapshots with snapdev=visible).

The tricky part for mounts is that we don't currently have a clean, cross-platform way to set up a mount from inside the kernel. Most of mounting is handled entirely by zfs mount, and on Linux we don't even do snapdir mounts inside the kernel - we create a new userspace process and execute mount.

It would take some actual hard thinking to work out what the right thing to do here is. I won't write it all out, but I have a bunch of questions about permissions, and channel program context, and the calling program, and containers/jails, etc.

Would it be possible to also set (non-inheritable) properties on the clone e.g. canmount=noauto?

I think that's not hard to do. When user properties were added in #9950 it even said that its limited to those because that's all they needed. There might be some that need a little more thought, but constraints like "non-inheritable" and perhaps "no side effects" should be an easy starting point.

These are all good avenues for future exploration, not for this PR. Keep banging on about it, and I'll probably get there ;)

And make its check and sync functions visible, so I can hook them up to
zcp_synctask. Rename not strictly necessary, but it definitely looks
more like a dsl_dataset thing than a dmu_objset thing, to the extent
that those things even have a meaningful distinction.

Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: https://despairlabs.com/sponsor/
Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: https://despairlabs.com/sponsor/
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 10, 2025
behlendorf pushed a commit that referenced this pull request Jun 10, 2025
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: https://despairlabs.com/sponsor/
Closes #17426
behlendorf pushed a commit that referenced this pull request Jun 10, 2025
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: https://despairlabs.com/sponsor/
Closes #17426
behlendorf pushed a commit that referenced this pull request Jun 10, 2025
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: https://despairlabs.com/sponsor/
Closes #17426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants