Add support for devid and phys_path keys in vdev disk labels. #4416
Conversation
|
As sort of expected, the build fails since the build VMs don't have libudev-devel package installed. What is the best way to get libudev-devel as part of the VM provisioning step? |
|
Thanks for opening this PR. I can add the missing dependency to the provisioning script or you could open a PR against the zfs-buildbot to do it, it's located here. https://github.com/zfsonlinux/zfs-buildbot/blob/master/scripts/bb-dependencies.sh Adding a dependency is fairly rare so it really hasn't been automated. |
|
I've updated the dependencies. If you force update this PR it should build. |
|
Looks like some of the older distros have an older version of libudev that doesn't have |
|
@don-brady I'd suggest adding a check for that function to |
|
something like this? |
|
Yes, exactly. Then you can add a |
|
There is a small subset of the community that does not use udev. Most of them use Alpine Linux (many), Gentoo Linux (few) or Funtoo Linux (slightly more than Gentoo), although there could be others that I do not know about. We should add a configure script option that allows those people to disable this functionality so that we do not add a hard dependency. |
|
@ryao good point, we can't make this mandatory since not all distributions provide this library. Is there an alternative on Alpine / Gentoo / Funtoo Linux? |
|
@behlendorf Alpine prefers mdev. which has no library for this. On Gentoo and Funtoo (which are 99% identical), the user is free to configure /dev however he wants, including via static nodes. I do not think many people do that though. It should be possible to use libblkid for some of the information that we are getting out of udev, although each libudev call will need to be examined on a case by case basis. I am heading out to a dental appointment, so I will need to look at this more later. |
|
@ryao @behlendorf I can certainly make this libudev dependent code optional and only available if HAVE_LIBUDEV is defined. But pending features like auto-online and auto-replace are based on a libudev monitor (to get disk events). We may end up requiring alternate code paths for sans-libudev platforms. Is libblkid ubiquitous? |
|
That's Linux. Every distribution makes their own choices about what packages to provide. All we can do is lay the ground work and integrate it with the most common packages which provide the required functionality. Hopefully, developers for those distributions will be able to extend the current functionality for their environment and push those patches back upstream. As for libblkid that should be ubiquitous. It was made mandatory for ZFS on Linux a few weeks back and so far I haven't seen any issues reported. |
|
@behlendorf A TBD. If we make libudev optional here, how do we communicate to developers to pick up the libudev-devel package before building ZFS? Is there a canonical list of recommended xxxx-devel packages for zfs? Thanks. |
|
@don-brady we do provide a recommended list of development packages which works for many, but not all, of the common distributions. We'll need to add http://zfsonlinux.org/generic-deb.html |
|
@don-brady It is possible to write a mdev rule that would provide notification of disk events. I wrote some code to do this in genkernel to setup /dev in the initramfs environment. It was only for add events (although mdev supports remove events too) and I need to do more before it is not a full replacement for udev's setup of That said, I do not expect you to write hooks for mdev though. People who care about this will either work on it or ping me until I work on it. |
| return (udev_device_get_is_initialized(dev)); | ||
| #else | ||
| /* wait for DEVLINKS property to be initialized */ | ||
| return (udev_device_get_property_value(dev, "DEVLINKS") != NULL); |
There was a problem hiding this comment.
What would be the downside to using udev_device_get_property_value() even if udev_device_get_is_initialized() is available? If there really isn't one it might make sense to always use that interface to keep the code simple.
There was a problem hiding this comment.
If the device is initialized, udev_device_get_is_initialized() is O(1) while udev_list_entry_get_by_name() is at least O(log(N)) as it does a couple binary searches in a sorted array. If it is not initialized, then it just looks in the database while udev_device_get_property_value will go to the uevent file before the database.
The optimization is probably not that useful given that we are running this in a loop with usleep().
There was a problem hiding this comment.
OK, thanks for the explanation. Since this bit of code is pretty clean either way I don't feel too strongly about this. Since @don-brady effectively already did the work to optimize this case when udev_device_get_is_initialized() is available I suggest we leave it as is. I don't see it causes us any long term maintainability issues which is my only real concern.
There was a problem hiding this comment.
@behlendorf The long term solution might be to improve the zpool_label_disk_wait() to be based on udev monitoring instead of polling a symlink and could eliminate the need for update_vdev_config_dev_strs() to check or wait for udev dev initialization. Perhaps once ZED has the udev monitoring merged we can revisit?
There was a problem hiding this comment.
Yup, sounds good we can definitely revisit this. Although we're always going to need to keep the polling behavior as a fallback for compatibility reasons since we can't assume every system has udev monitoring and we need to provide at least basic functionality.
|
@behlendorf So latest commit has this libudev based feature being optional. Do we have any configs in the build bot fleet that don't have libudev? Might be good idea to validate the non-libudev case (against future regressions). Thanks. |
|
@don-brady unfortunately we don't, but it would be a good idea to add one. @ryao adding a Gentoo builder might be a good way to address that. |
|
@don-brady is there anything you're planning to add to this PR. If not can you rebase it on master, squash it in to a single commit, and force update the branch. Then we can get one last round of testing and review comments before merging it. |
|
@behlendorf no additional changes planned. I will rebase/squash and update the branch. |
| AC_CHECK_HEADER([libudev.h], [AC_SUBST([LIBUDEV], ["-ludev"]) | ||
| AC_DEFINE([HAVE_LIBUDEV], 1, [Define if you have libudev])], []) | ||
|
|
||
| AC_CHECK_LIB([udev], [udev_device_get_is_initialized], [ |
There was a problem hiding this comment.
Understood. I think the value is in having .m4 files be good examples (admittedly I borrowed from another one). In theory I can just drop in AC_SEARCH_LIBS macro and swap the first two arguments.
|
Looks like all recent builds on Amazon 2015.09 x86_64 SIMD (TEST) are failing in zfs test suite for |
|
I've opened #4450 for the scrub failures. It looks to me like the test case is just racy and the much more capable Amazon SIMD TEST builder is exposing that. We'll either way to fix the test cases or disable them until we do. Thanks for squashing this, I started looking it over again yesterday but had to run. I'll go over it again. |
| LIBUDEV= | ||
|
|
||
| AC_CHECK_HEADER([libudev.h], [AC_SUBST([LIBUDEV], ["-ludev"]) | ||
| AC_DEFINE([HAVE_LIBUDEV], 1, [Define if you have libudev])], []) |
There was a problem hiding this comment.
Please fix the indentation on these two checks as well. The first two times I read this it wasn't clear to me the AC_DEFAULT line was inside AC_CHECK_HEADER. Same for AC_CHECK_LIB below. Use the normal cstyle indentation rules here as much as possible.
|
|
||
| /* | ||
| * For the benefit of legacy ZFS implementations, allow | ||
| * for opting out of devid strings in the vdev label. |
There was a problem hiding this comment.
It would be good to add why you might want/need to do this to the comment (or somewhere if I missed it)
|
The patch works well for me and build properly with and without libudev. Just a few small recommended improvements. I'd also suggest looking to to updating |
|
@behlendorf Thanks for the review feedback! I'll make the recommended changes and take a look at devname2devid |
|
Looks like the bash shell on Debian and Ubuntu builbots is not happy with zloop.sh (a script syntax error encountered). |
|
@don-brady yes the updated zloop.sh script really requires bash. I applied a fix to master yesterday so if you rebase against master and update this it should be fine. Sorry about that. |
|
Related to #2856 |
…ement devname2devid cmd.
|
@don-brady thanks for refreshing this. The latest version LGTM and works well in my manual testing. Just let me know if you have any more improvements planned or if I should go ahead and merged it. It looks like you got pretty unlucky in the automated testing, so I'm resubmitted those test runs. |
|
@behlendorf No further changes planned. Thanks for the reviews. |
|
@ryao can I add your signed off to this? |
|
@don-brady whoops, we accidentally broke the build on master when I merged this. The updated |
Accidentally introduced by commit 39fc0cb. The devname2devid utility which depends on libudev must only be built when libudev headers are available. This is accomplished through an AM_CONDITIONAL. Signed-off-by: Brian Behlendorf <[email protected]> Issue #4416
|
Rather than revert this commit from master I committed a small follow up patch which causes devname2devid to only get built when libudev is available. I should mention this happened to get missed because as @don-brady rightly point out we don't actually have a builder where the libudev headers are missing. e4023e4 Only build devname2devid when libudev headers are available |
|
I am now getting an error building zfs-kmod-9999 in gentoo related to WANT_DEVNAME2DEVID:
|
|
Have you rerun |
|
This is from an ebuild that builds the latest git. It will create a clean work directory each time. |
|
Yes I'm seeing this as well. For the moment you can revert the last two patches. I can't look in to this until tomorrow, but I'll sort it out then. Unless someone beats me to it. |
|
Thanks. I'll hold off the update until this is fixed. I have a recent enough build anyways. |
Accidentally introduced by commit e4023e4. The AM_CONDITIONAL cannot be located where it can be invoked conditionally, as in the `--with-config=user` case. Relocate it to the top level ZFS_AC_CONFIG macro along with the other AM_CONDITIONALs. Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#4416
|
The latest commit fixed this issue for me. Thanks. |
|
@drescherjm great, thanks for confirming the fix. |
Add support for devid and phys_path keys in vdev disk labels.
This is foundational work for ZED.
Updates a leaf vdev's persistent device strings on Linux platform
ZFS_VDEV_DEVID_OPT_OUT=YESSee details in http://github.com/zfsonlinux/zfs/issues/3978
Some examples: