Skip to content

New stable API (For Initial Review) #3299

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 17 commits into from
Closed

New stable API (For Initial Review) #3299

wants to merge 17 commits into from

Conversation

ryao
Copy link
Contributor

@ryao ryao commented Apr 15, 2015

This is the foundation for a stable /dev/zfs API for management that moves the kernel-userland interface away from binary data structures and numerical values to XDR-encoded nvlists and string values to enable ease of programming and long term interface stability. Most of the existing libzfs_core API functions have been migrated to the proposed API and the following new ones have been implemented for use by flocker and others:

  1. lzc_send_ext()
  2. lzc_promote()
  3. lzc_send_progress()
  4. lzc_set_props()
  5. lzc_list()
  6. lzc_rename()
  7. lzc_inherit()
  8. lzc_destroy_one()

In most cases, the userland tools have been modified to use them. A few open questions that need to be answered are:

  1. Is our documentation of these functions sufficient?
  2. Do we want lzc_inherit() to only process 1 dataset at a time?
  3. Are there any other functions that we should include in the initial implementation?
  4. Do we want to pass the meta-nvlist as an input to the history logging?
  5. Do we want to continue using the legacy zfs_cmd_t structure?
  6. The zfs list command fails to list an individual bookmark. This is a pre-existing userland bug. Should we address it here?

Lastly, this depends on openzfs/spl#449.

ryao and others added 17 commits April 10, 2015 11:37
@ryao
dmu_objset_find_dp refactor for recursion limit
c636dd5 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Refactor zfs_ioc_dataset_list_next() to stop passing around zfs_comma…
0d12a41 
…nd_t

Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Support string values for index properties
e14955b 
We retain the ability to pass index values for backward compatibility
with older tools, but this functionality is depreciated.

Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Pass index property values as strings
c9532be 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Implement kernel side of stable ioctl interface
96d0853 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Make libzfs_core hook into stable API
e4d4deb 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@avg-I @ryao
add extended version lzc_send
34cc4b9 
lzc_send_ext() accepts an nvlist of additional options that modify
send behavior.
Right now only one such additional option is supported: "fromorigin", it
instructs to produce an incremental stream for a cloned filesystem
starting from its origin snapshot.

To do: consider passing the options via 'opts' nvlist rather than 'innvl'
@avg-I @ryao
add lzc_promote
90fa5d5
@avg-I @ryao
add lzc_send_progress
22a02fb 
this is libzfs_core counterpart for ZFS_IOC_SEND_PROGRESS
@avg-I @ryao
add lzc_set_props to set multiple properties of a single dataset
8f631c6
@ryao
Fix build error in zfs promote
5530ee9 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Fix build failure in zfs set props
667b9ce 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Fix zfs send build failure
1aef69b 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Implement lzc_list()
acb258e 
This implements a stable zfs list API. inspired by the CLI `zfs list`.
It unifies several existing API calls into a single call that provides
an almost complete subset of the functionality provided by `zfs list`.
The existing API calls that this supercedes are:

	- ZFS_IOC_OBJSET_STATS
	- ZFS_IOC_OBJSET_ZPLPROPS
	- ZFS_IOC_DATASET_LIST_NEXT
	- ZFS_IOC_SNAPSHOT_LIST_NEXT
	- ZFS_IOC_GET_BOOKMARKS (stable lzc API)

Since ZFS_IOC_GET_BOOKMARKS is a stable API, it has been left untouched.

The functionality absent from the API that is present in `zfs list` is
the ability to sort and request data on specific properties. Under this
API, userland is (still) expected to sort and all property data is
returned.

Programming documentation on the API is included a comment in
libzfs_core.c.

Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Implement lzc_rename()
3cf1088 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Implement lzc_inherit()
6167f84 
Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Implement lzc_destroy_one()
87619fa 
This was originally intended to be `lzc_destroy()`, but recent versions
of the Linux kernel appear to have removed the information we need to
unmount datasets from the kernel driver without resorting to symbol
table hacks. Consequently, unmounting requires doing upcalls to userland
and error handling becomes easier if we delegate unmounting to the
userland caller.

Signed-off-by: Richard Yao <ryao@gentoo.org>
@ryao
Copy link
Contributor Author

ryao commented Apr 15, 2015

@ahrens @lundman Any feedback that you have would be appreciated. I would like to have some dialogue on the design before any platform commits to a stable ioctl interface for any subset of functionality.

ryao
ryao reviewed Apr 15, 2015
View reviewed changes
module/zfs/zfs_ioctl.c
dsl_pool_config_exit(dp, FTAG);
if (error)
return (error);
dd_obj = ds->ds_dir->dd_object;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a race here between zfs list and zfs destroy that creates an edge case where zfs list says there is something and then the pipe says otherwise. An attempt to take the lock here and transfer ownership of the lock to another thread was messy, although it would close this race. Thoughts on whether this race should be closed or documented would be appreciated.

@avg-I
Copy link
Contributor

avg-I commented May 6, 2015

I also have a general comment about this pull request.
I think that it would be beneficial to discuss the proposed API additions (function signatures and their logic) on the OpenZFS mailing list. The pull request does not seem to have enough attention from all potentially interested parties. Also, github is quite good for discussing the code changes (implementation), but, IMO, not so good for discussing API design.

@ryao
Copy link
Contributor Author

ryao commented May 6, 2015

I should have this ported to Illumos soon. When I have that done and have addressed these comments, I will submit it to Illumos. The Illumos submission process requires that I email the list, so this would be done as part of it.

That said, I had some feedback in IRC (people generally liked the API), but the review was fairly superficial.

@trisk
Copy link
Contributor

trisk commented May 6, 2015

I think an additional document describing the usage and semantics of new interfaces (which eventually could become a man page) would be helpful for discussing their design. We don't have to wait for the code to be ported.

@avg-I
Copy link
Contributor

avg-I commented May 7, 2015

I like the idea of having the document for the usage and semantics.

@ryao
Copy link
Contributor Author

ryao commented May 7, 2015

Are there any style guides or existing examples of good API documentation that I could use as a template?

@ryao
Copy link
Contributor Author

ryao commented Oct 10, 2015

Closed in favor of #3907.

@ryao ryao closed this Oct 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryao @avg-I @trisk @ahrens