Initial stable API + ClusterHQ libzfs_core extensions + various fixes #3907

ryao · 2015-10-10T00:12:18Z

This contains the initial stable API, the libzfs_core extensions and various fixes that we have developed at ClusterHQ for use with flocker. It also cherry-picks the proposed fix for Illumos 6267 from #3874. This was needed to fix test failures at ClusterHQ.

The current code passes the functional tests in clusterhq/pyzfs and the flocker acceptance tests. There are no known regressions in ClusterHQ's tracker and the code is believed to be production quality.

adamtheturtle · 2015-10-10T14:39:15Z

lib/libzfs_core/libzfs_core.c

+ * Destroying snapshots and bookmarks is not currently supported. Call
+ * lzc_destroy_snaps and lzc_destroy_bookmarks for those respectively.
+ *
+ * The only currently valud property is the boolean "defer". It makes


nit: typo "valud" > "valid"

ryao · 2015-10-10T15:37:00Z

Some minor changes have been made.

The buildbot revealed cstyle issues in the header files. Those are now fixed.
@adamtheturtle's comment about a comment containing "valud" rather than "valid" is addressed. The manpage had the same mistake (as I copy and pasted the text), which has also been fixed.
Some minor corrections have been made to comments on the zfs_ioc_dataset_list_next().
Also, zfs_stable_ioc_set_props was developed against 0.6.4 where the old-style filesystem_limit and snapshot_limit properties did not exist. This was missed when we rebased against 0.6.5/HEAD causing attempts to atomically set them to be performed in a non-atomic manner. It now properly fails when asked to set filesystem and snapshot limits atomically. This is also now documented in the man page.

ahrens · 2015-10-25T23:02:07Z

lib/libzfs_core/libzfs_core.c

@@ -169,6 +171,29 @@ lzc_ioctl(zfs_ioc_t ioc, const char *name,
 	return (error);
 }

+static int
+lzc_ioctl(const char *cmd, const char *name, nvlist_t *source,


what's the difference between source (innvl) and opts? why separate them out?

I asked during an earlier review; the answer was this was to let "options remain separate from actual data (e.g. a list of properties to set) so that we don't get pesky namespace collisions or inconsistencies from future extensions"

It also exists as an nvlist rather than a bitmask for said extensions. I'm still not entirely convinced this is necessary (worst case, nested lists would work).

@ahrens My apologies for missing this comment. As @trisk said, the idea is to avoid mixing configuration from data.

mmlb · 2015-11-18T18:15:27Z

@ryao commit 66a2da lzc_rename seems to have broken recursive renames. Following script from https://docs.oracle.com/cd/E19082-01/817-2271/gevnp/index.html demonstrates:

truncate -s1G disk{1,2,3}.img
zpool create users $(realpath disk{1,2,3}.img)
zfs create users/home
for user in markm marks neil; do
    zfs create users/home/$user
done

zfs snapshot -r users/home@today
zfs list -t all

zfs rename -r users/home@today @yesterday
zfs list -t all

and errors with cannot rename 'users/home@marks': dataset does not exist

ryao · 2015-11-25T02:22:35Z

@mmlb Thanks for catching that. This is fixed in my local branch. I will clean it up and push it on either Thursday and Friday.

ryao · 2015-12-10T22:00:56Z

@behlendorf @ahrens @mmlb This has been refreshed. There have been several changes from the last push:

Several fixes, including a fix for the lzc_rename issue that @mmlb highlighted
The proposed lzc_promote() has had outnvl added (backward compatibility break with previous proposed version) to improve error handling.
The logic for traversing bookmarks in the kernel side of lzc_list has moved into dmu_objset_find_dp() and dsl_bookmark_lookup_ds has been made avaliable for use with it. This makes it significantly more maintainable.
lzc_list() has gained minimum depth traversal, which was needed for the next item.
Several lzc_pool_* functions have been added to stabilize the boot process for / on ZFS.
lzc_list_iter() has been added following feedback from Robert Mustacchi and the zfs_iter_generic() function previously used to hook into lzc_list() has been refactored to use it.
The int zc_pad2 in zfs_cmd_t has been changed to uint64_t zc_real_err to store the error code before userland copying.
All of the existing code has been refactored to use the new lzc_* functions, which @ahrens suggested.
The kernel code for ZFS_IOC_POOL_STATS has been altered to stop processing a zfs_cmd_t member that userland never touches.
Various functions have been constified for const-correctness.
Robert Mustacchi pointed out that lzc_list() could violate resource restrictions. This is a problem on Illumos and other platforms. Code has been added that should create a userland thread whose kernel stack never switches to userspace so that resource constraints are observed. An attempt was made to do something similar on Linux, but Linux's kernel API makes this incredibly difficult, so it is not fully fleshed out, but I believe this is solveable. SInce Linux restricts /dev/zfs to root, this does not seem like it should be a priority.

There are a few minor outstanding things, but they should not impede review of this code:

@mmlb pointed out that we are copying the full zfs_cmd_t into the kernel, which is not necessary. This is an existing issue in the libzfs_core code.
@mmlb pointed out that lzc_receive() has not been converted to use the libzfs_core stable ioctl.
Some minor man page fixes were suggested by @mmlb.
My collegue wrote another lzc_* function, which I plan to add, but it wasn't included in Gentoo and really should not block review of these changes.
The lzc_pool_* and lzc_list_iter() functions lack man pages. I plan to write these, but given how simple these functions are, they can be written asynchronously to review.
The man pages need to be updated to reflect that the true error code is in errno after a lzc_* function returns while the function return code is the error after copying.
Imposing resource limits on lzc_list()'s worker thread remains to be done.

An almost identical version was put into production in Gentoo Linux 1 week ago. The only difference is that lzc_list_iter() was not there.

So far, the only reported problem has been grub2-probe breaking when linked against the new libzfs.so on a system where an older kernel module is used. GRUB2 links to libzfs.so so that it can (ab)use it to read the cachefile and fallback to getting the cache file contents from the kernel if it is missing, which means it breaks every time that functionality changes in a backward incompatible way. The GRUB2 developers have requested functionality like lzc_pool_configs() in conversations about fixing this. I believe this is the last time GRUB2 will break after both this is merged and GRUB2 adopts it.

ryao · 2015-12-11T23:26:28Z

Clang's Address Sanitizer found a few issues in lzc_list_iter() and others that I had missed. I have revised lzc_list_iter() and pushed another patch for the other bits. The changes are rather small though.

behlendorf · 2015-12-12T01:50:20Z

@ryao can these patches be structured such that intermediary patches in the stack don't break the build.

ryao · 2015-12-14T19:03:00Z

@behlendorf Yes. I am working on that now.

This is unused functionality that could bite us in the future. Signed-off-by: Richard Yao <ryao@gentoo.org>

Signed-off-by: Richard Yao <ryao@gentoo.org>

lzc_snaprange_space has a couple of bugs. One is that `usedp == NULL` will cause a segmentation fault. Another is that passing snapshots from different datasets will return ENOENT rather than EXDEV, which is inconsistent. This corrects that to make this more predictable. Signed-off-by: Richard Yao <ryao@gentoo.org>

Some non-essential properties like mountpoint are lost during the creation of a temporary pool because will try to set them on the pool's permanent name, which does not exist inside the sPA namespace during pool creation. Most notably, the mountpoint property would be lost. Signed-off-by: Richard Yao <ryao@gentoo.org>

This introduces a new ZFS_IOC_LIBZFS_CORE ioctl and replaces the ioctl calls in all but 1 function in libzfs_core.so and adds many more. It is intended to be as stable kernel/userland interface that allows basic userland functionality to continue working when an older userland is used with newer kernel modules or vice versa by mistake. At this time, it is a subset of the legacy functionality, but enough API functions have been implemented that mismatched kernel/userland code will not break the Gentoo's boot process (excluding zfs share and ZED, which are excluded at this time) and ClusterHQ's flocker. This sufficiently addresses the long standing /dev/zfs API instability that it opens the possibility of stabilization of the ZFS packaging in Gentoo on amd64 and other architectures, which makes this an improvement in more ways than one. To ease the transition to the new ABI, we include a shim library that implements the new C library extensions using the old kernel ABI. The --with-libzfs_core=default|all|legacy|stable configure switch determines which is used and presently, the default is an alias for legacy that can be changed in a future release. --with-libzfs_core=all will install both, with libzfs_core.so.1 using the new ABI while a libzfs_core-legacy.so.0 that can be loaded with LD_PRELOAD will be installed containing the old one. Signed-off-by: Richard Yao <ryao@gentoo.org>

behlendorf · 2017-04-10T17:43:38Z

@ryao I'd like to close this PR until it's being actively worked on again. We can always open a new PR in the future.

ryao · 2017-04-28T16:21:34Z

@behlendorf Sure thing.

ryao mentioned this pull request Oct 10, 2015

New stable API (For Initial Review) #3299

Closed

ryao force-pushed the libzfs_core-HEAD branch 3 times, most recently from 780f6f0 to 47eb5bb Compare October 10, 2015 13:51

adamtheturtle reviewed Oct 10, 2015
View reviewed changes

ryao force-pushed the libzfs_core-HEAD branch 3 times, most recently from b141e16 to b23828a Compare October 10, 2015 15:35

behlendorf added the Type: Feature Feature request or new feature label Oct 13, 2015

behlendorf added the OpenZFS Review label Oct 20, 2015

ahrens reviewed Oct 25, 2015
View reviewed changes

This was referenced Dec 3, 2015

Fix race between getf() and areleasef() openzfs/spl#492

Closed

Remove kobj and vnode interfaces (WIP) openzfs/spl#497

Closed

Add minimal vnode interfaces (WIP) #4005

Closed

Make taskq_member() use ->journal_info openzfs/spl#505

Closed

ryao force-pushed the libzfs_core-HEAD branch 4 times, most recently from 9ea9c20 to 48de27d Compare December 10, 2015 22:00

behlendorf mentioned this pull request Dec 10, 2015

Accessing zfs information programatically is not working #2908

Closed

ryao force-pushed the libzfs_core-HEAD branch from 48de27d to 2deb68e Compare December 11, 2015 23:25

ryao force-pushed the libzfs_core-HEAD branch from 2deb68e to 99918d3 Compare December 16, 2015 14:27

ryao force-pushed the libzfs_core-HEAD branch 7 times, most recently from a3b3606 to b0fc5d2 Compare August 27, 2016 03:01

ryao force-pushed the libzfs_core-HEAD branch from b0fc5d2 to d50f471 Compare September 9, 2016 15:11

ryao and others added 9 commits September 9, 2016 11:30

ZFS_IOC_POOL_STATS should not pass zc_value to spa_get_stats

65fe6b2

This is unused functionality that could bite us in the future. Signed-off-by: Richard Yao <ryao@gentoo.org>

Constify various string arguments

9159e08

Signed-off-by: Richard Yao <ryao@gentoo.org>

Provide dsl_bookmark_lookup_ds() function

be4d8c0

Signed-off-by: Richard Yao <ryao@gentoo.org>

Want bookmark and depth traversal support in dmu_objset_find_dp

ac3ba81

Signed-off-by: Richard Yao <ryao@gentoo.org>

Fix memory leak in get_numeric

ff9427e

Signed-off-by: Richard Yao <ryao@gentoo.org>

dsl_dataset_rename_snapshot does not belong in sys/dmu.h

c8ce2b2

Signed-off-by: Richard Yao <ryao@gentoo.org>

lzc_receive: add a possibility to take the snapshot name from the stream

7d602fb

ryao force-pushed the libzfs_core-HEAD branch 3 times, most recently from 9eb3767 to c585ca8 Compare September 9, 2016 20:09

ryao force-pushed the libzfs_core-HEAD branch 3 times, most recently from d03d464 to 8dad8bf Compare September 20, 2016 19:26

ryao force-pushed the libzfs_core-HEAD branch from 8dad8bf to 32b87ca Compare September 20, 2016 19:41

behlendorf added the Status: Work in Progress Not yet ready for general review label Sep 30, 2016

behlendorf closed this Apr 28, 2017

georgyo mentioned this pull request Jul 9, 2019

pyzfs / libzfs_core.c has no way to list filesystems or snapshots. #9008

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Initial stable API + ClusterHQ libzfs_core extensions + various fixes #3907

Initial stable API + ClusterHQ libzfs_core extensions + various fixes #3907

Uh oh!

ryao commented Oct 10, 2015

Uh oh!

adamtheturtle Oct 10, 2015

Uh oh!

ryao commented Oct 10, 2015

Uh oh!

ahrens Oct 25, 2015

Uh oh!

trisk Oct 29, 2015

Uh oh!

ryao Oct 29, 2015

Uh oh!

mmlb commented Nov 18, 2015

Uh oh!

ryao commented Nov 25, 2015

Uh oh!

ryao commented Dec 10, 2015

Uh oh!

ryao commented Dec 11, 2015

Uh oh!

behlendorf commented Dec 12, 2015

Uh oh!

ryao commented Dec 14, 2015

Uh oh!

behlendorf commented Apr 10, 2017

Uh oh!

ryao commented Apr 28, 2017

Uh oh!

Uh oh!

Initial stable API + ClusterHQ libzfs_core extensions + various fixes #3907

Initial stable API + ClusterHQ libzfs_core extensions + various fixes #3907

Uh oh!

Conversation

ryao commented Oct 10, 2015

Uh oh!

adamtheturtle Oct 10, 2015

Choose a reason for hiding this comment

Uh oh!

ryao commented Oct 10, 2015

Uh oh!

ahrens Oct 25, 2015

Choose a reason for hiding this comment

Uh oh!

trisk Oct 29, 2015

Choose a reason for hiding this comment

Uh oh!

ryao Oct 29, 2015

Choose a reason for hiding this comment

Uh oh!

mmlb commented Nov 18, 2015

Uh oh!

ryao commented Nov 25, 2015

Uh oh!

ryao commented Dec 10, 2015

Uh oh!

ryao commented Dec 11, 2015

Uh oh!

behlendorf commented Dec 12, 2015

Uh oh!

ryao commented Dec 14, 2015

Uh oh!

behlendorf commented Apr 10, 2017

Uh oh!

ryao commented Apr 28, 2017

Uh oh!

Uh oh!