Wait in libzfs_init() for the /dev/zfs device #3431

behlendorf · 2015-05-19T20:50:04Z

While module loading itself is synchronous the creation of the /dev/zfs
device is not. This is because /dev/zfs is typically created by a udev
rule after the module is registered and presented to user space through
sysfs. This small window between module loading and device creation
can result in spurious failures of libzfs_init().

This patch closes that race by extending libzfs_init() so it can detect
that the modules are loaded and only if required wait for the /dev/zfs
device to be created. This allows scripts to reliably use the following
shell construct without the need for additional error handling.

$ /sbin/modprobe zfs && /sbin/zpool import -a

To minimize the potential time waiting in libzfs_init() a strategy
similar to adaptive mutexes is employed. The function will busy-wait
for up to 10ms based on the expectation that the modules were just
loaded and therefore the /dev/zfs will be created imminently. If it
takes longer than this it will fall back to polling for up to 10 seconds.

This behavior can be customized to some degree by setting the following
new environment variables. This functionality is provided for backwards
compatibility with existing scripts which depend on the module auto-load
behavior. By default module auto-loading is now disabled.

ZFS_MODULE_LOADING="YES|yes|ON|on" - Attempt to load modules.
ZFS_MODULE_TIMEOUT="" - Seconds to wait for /dev/zfs

The additional small changes were also made.

In libzfs_run_process() 'rc' variables was renamed to 'error' for
consistency with the rest of the code base.
All fprintf() error messages were moved out of the libzfs_init()
library function where they never belonged in the first place. A
libzfs_error_init() function was added to provide useful error
messages for the most common causes of failure.
The zfs-import-* systemd service files have been updated to call
'/sbin/modprobe zfs' so they no longer rely on the legacy auto-loading
behavior.

Signed-off-by: Brian Behlendorf [email protected]
Issue #2556

ryao · 2015-05-19T21:10:40Z

lib/libzfs/libzfs_util.c

+			return (0);
+		} else if (errno == ENOENT) {
+			if (NSEC2MSEC(gethrtime() - start) > busy_timeout)
+				usleep(10 * MILLISEC);


There should be an else sched_yield(); here.

I actually intentionally dropped it because we're only going to be spinning briefly. But I can add it back, it does appear to be POSIX so there aren't compatibility concerns.

ryao · 2015-05-19T21:14:09Z

There is a pre-existing issue where zpool import in the systemd unit files will invoke a label update rather than doing a verbatim import. We should probably use zpool import -F there while we are touching it to fix that.

ryao · 2015-05-19T21:18:49Z

A fix for this issue is something that we would want to backport to 0.6.4, but the library changes and behavior changes are not something that are good to backport. Lets split it into two patches. One implements things without restructuring layering/functionality and another that does. Then we can backport the first patch.

behlendorf · 2015-05-19T21:34:56Z

That's amusing the test failures are due to parts of the test suite assuming the auto-loading behavior. When I refresh this I'll update the offending scripts accordingly.

systemd unit files will invoke a label update rather than doing a verbatim import.

Do you have the issue number? I'd like to see the full bug. We should not need to use -F when importing the pool under normal circumstances. If we do change that behavior I'd definitely keep it as a separate patch.

@ryao this doesn't strike me as a critical issue worth applying to a point release.

behlendorf · 2015-05-19T22:32:24Z

Refreshed:

Added a call to sched_yield() when busy-waiting.
Correctly handle the case when ZFS_MODULE_TIMEOUT=0

dun · 2015-05-19T22:42:49Z

cmd/zhack/zhack.c

-	ASSERT(g_zfs != NULL);
+	if ((g_zfs = libzfs_init()) == NULL) {
+		(void) fprintf(stderr, "%s", libzfs_error_init(errno));
+		return;


Shouldn't a libzfs_init() failure here be fatal, especially since you've removed the ASSERT?

Now if libzfs_init() fails, you return early from import_pool() . That function returns a void, so you can't return the error to the caller (zhack_spa_open()) which obliviously rumbles on.

Yes, yes it should be. I'll fix it by reverting this hunk so it matches upstream. I clearly just replaced all this calls without fully inspecting them.

behlendorf · 2015-05-19T23:38:57Z

Refreshed with the zhack chunk dropped.

dun · 2015-05-20T00:16:30Z

lib/libzfs/libzfs_util.c

+	timeout_str = getenv("ZFS_MODULE_TIMEOUT");
+	if (timeout_str) {
+		timeout = strtol(timeout_str, NULL, 0);
+		timeout = MIN(MAX(timeout, (10 * 60)), 0); /* 0 <= N <= 600 */


timeout here is always 0. I think you mean to swap MIN and MAX:

timeout = MAX(MIN(timeout, (10 * 60)), 0); /* 0 <= N <= 600 */

That's embarrassing, and exactly what I deserve for rushing this small tweak to the original patch. Originally it didn't have a minimum bound.

ryao · 2015-05-20T17:58:38Z

@behlendorf There is no issue number for the misuse of zpool import in the start scripts (yet). I noticed it when debugging this problem on Friday. When the pool is imported by the module reading zpool.cache, a verbatim import is done, but when the script does it, a non-verbatim import is done. This poses problems for txg rollback if something goes wrong and is only necessary when a change is done to the pool vdev configuration.

My employer considers backporting a fix for this to be important because this issue affects the flocker tutorial in anywhere from 1/3 to 2/3 of all runs. I spent yesterday backporting my original patch to 0.6.4.1 (as 0.6.4.1.flocker.0). I did it in a manner roughly equivalent to what you do when you tag releases, such that there are source tarballs that can be used to build packages using the generic rpm/deb instructions. They have autotools products included, so no ./autogen.sh is necessary. I pushed the branches to a public git repository earlier today:

https://github.com/ClusterHQ/spl/tree/flocker-backports
https://github.com/ClusterHQ/zfs/tree/flocker-backports

The tarballs are available at a temporary location. If anyone wants them, I can provide the link, but I am going to avoid posting the temporary link to the issue tracker so that people in the future do not encounter a broken link. If anyone asks for the tarballs, I'll post the official link after it is up.

behlendorf · 2015-05-20T18:13:46Z

Refreshed to include the restructured do-while loop and the MIN/MAX inversion.

I've also added an else clause to the ZFS_MODULE_LOADING environment variable handling. This way the patch can be cherry picked in to the 0.6.4-release without changing the default behavior by just inverting the default value for load in libzfs_load_module(). This should address @ryao's concerns about backporting this to the release version and we can apply it to 0.6.4.2.

dun · 2015-05-20T19:19:37Z

Looks good to me.

ryao · 2015-05-20T20:00:37Z

etc/systemd/system/zfs-import-scan.service.in

@@ -9,4 +9,4 @@ ConditionPathExists=!@sysconfdir@/zfs/zpool.cache
 [Service]
 Type=oneshot
 RemainAfterExit=yes
-ExecStart=@sbindir@/zpool import -d /dev/disk/by-id -aN
+ExecStart=/sbin/modprobe zfs && @sbindir@/zpool import -d /dev/disk/by-id -aN


I suggest adding a -F here to match the behavior when the module auto-imports the pool itself to fix #3434.

Rather than sneak that in here. Let's tackle that in a separate patch because why that's desirable is going to need a fair bit of explanation and it would be a change in existing behavior.

That is fine.

ryao · 2015-05-20T20:23:12Z

@behlendorf I am not a fan of backporting the changes to the libzfs API because they ideally shouldn't change across point releases, but it is your call. You will have no further complaints from me as long as you are aware of that concern.

As a side note, issues with library API changes are not an issue for Gentoo because Gentoo's packaging keeps the userland binaries in a single package. It is more likely to be an issue for other distributions because their packaging breaks the tools and libraries into discrete packages.

ryao · 2015-05-20T20:39:02Z

@behlendorf Those comments aside, this looks good to me.

behlendorf · 2015-05-20T21:45:06Z

I because they ideally shouldn't change across point releases,

That's a very good point. This patch does remove one public function and add another which could potentially cause breakage. I suspect the odds of this causing breakage are quite low but it's definitely possible. I've split this patch as you originally suggested in to two parts, we'll only cherry pick the first part to the release branch to avoid any API changes. Thanks for keeping me honest about this.

While module loading itself is synchronous the creation of the /dev/zfs device is not. This is because /dev/zfs is typically created by a udev rule after the module is registered and presented to user space through sysfs. This small window between module loading and device creation can result in spurious failures of libzfs_init(). This patch closes that race by extending libzfs_init() so it can detect that the modules are loaded and only if required wait for the /dev/zfs device to be created. This allows scripts to reliably use the following shell construct without the need for additional error handling. $ /sbin/modprobe zfs && /sbin/zpool import -a To minimize the potential time waiting in libzfs_init() a strategy similar to adaptive mutexes is employed. The function will busy-wait for up to 10ms based on the expectation that the modules were just loaded and therefore the /dev/zfs will be created imminently. If it takes longer than this it will fall back to polling for up to 10 seconds. This behavior can be customized to some degree by setting the following new environment variables. This functionality is provided for backwards compatibility with existing scripts which depend on the module auto-load behavior. By default module auto-loading is now disabled. * ZFS_MODULE_LOADING="YES|yes|ON|on" - Attempt to load modules. * ZFS_MODULE_TIMEOUT="<seconds>" - Seconds to wait for /dev/zfs The zfs-import-* systemd service files have been updated to call '/sbin/modprobe zfs' so they no longer rely on the legacy auto-loading behavior. Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#2556

All fprintf() error messages are moved out of the libzfs_init() library function where they never belonged in the first place. A libzfs_error_init() function is added to provide useful error messages for the most common causes of failure. Additionally, in libzfs_run_process() the 'rc' variable was renamed to 'error' for consistency with the rest of the code base. Signed-off-by: Brian Behlendorf <[email protected]>

behlendorf · 2015-05-22T19:14:55Z

The failures here were due to the test suite needing an update to explicitly load the zfs modules for ztest. I'll update the tests and get this merged today.

behlendorf · 2015-05-22T20:37:40Z

Merged as:

65037d9 Add libzfs_error_init() function
87abfcb Wait in libzfs_init() for the /dev/zfs device

This was referenced May 19, 2015

libzfs_init() should busy-wait on module initialization #3427

Closed

Complete rewrite of init script(s) - attempt 3 #3337

Closed

ryao reviewed May 19, 2015
View reviewed changes

behlendorf force-pushed the issue-2556 branch from 4e2ed90 to 61f9e88 Compare May 19, 2015 22:31

dun reviewed May 19, 2015
View reviewed changes

behlendorf force-pushed the issue-2556 branch from 61f9e88 to 8e4a629 Compare May 19, 2015 23:38

dun reviewed May 20, 2015
View reviewed changes

behlendorf force-pushed the issue-2556 branch from 8e4a629 to d107980 Compare May 20, 2015 18:05

ryao mentioned this pull request May 20, 2015

The start scripts dirty vdevs when importing from the cachefile #3434

Closed

behlendorf added the Bug - Point Release label May 20, 2015

ryao reviewed May 20, 2015
View reviewed changes

behlendorf force-pushed the issue-2556 branch from d107980 to ffc5486 Compare May 20, 2015 21:45

behlendorf added 2 commits May 21, 2015 15:55

behlendorf force-pushed the issue-2556 branch from ffc5486 to a7d3670 Compare May 21, 2015 22:58

behlendorf closed this May 22, 2015

behlendorf removed the Bug - Point Release label Sep 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait in libzfs_init() for the /dev/zfs device #3431

Wait in libzfs_init() for the /dev/zfs device #3431

behlendorf commented May 19, 2015

ryao May 19, 2015

behlendorf May 19, 2015

ryao commented May 19, 2015

ryao commented May 19, 2015

behlendorf commented May 19, 2015

behlendorf commented May 19, 2015

dun May 19, 2015

behlendorf May 19, 2015

behlendorf commented May 19, 2015

dun May 20, 2015

behlendorf May 20, 2015

ryao commented May 20, 2015

behlendorf commented May 20, 2015

dun commented May 20, 2015

ryao May 20, 2015

behlendorf May 20, 2015

ryao May 20, 2015

ryao commented May 20, 2015

ryao commented May 20, 2015

behlendorf commented May 20, 2015

behlendorf commented May 22, 2015

behlendorf commented May 22, 2015

Wait in libzfs_init() for the /dev/zfs device #3431

Wait in libzfs_init() for the /dev/zfs device #3431

Conversation

behlendorf commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryao commented May 19, 2015

ryao commented May 19, 2015

behlendorf commented May 19, 2015

behlendorf commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryao commented May 20, 2015

behlendorf commented May 20, 2015

dun commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryao commented May 20, 2015

ryao commented May 20, 2015

behlendorf commented May 20, 2015

behlendorf commented May 22, 2015

behlendorf commented May 22, 2015