Skip to content
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

Allow modprobe to fail when called within systemd #7174

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Allow modprobe to fail when called within systemd #7174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion etc/systemd/system/zfs-import-cache.service.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ ConditionPathExists=@sysconfdir@/zfs/zpool.cache
[Service]
Type=oneshot
RemainAfterExit=yes
ExecStartPre=/sbin/modprobe zfs
ExecStartPre=-/sbin/modprobe zfs
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to address @loli10K's concerns why don't we leverage the existing --enable-linux-builtin configure option. When --enable-linux-builtin=yes then the entire ExecStartPre line should be removed from the service file. This can be done relatively easily in the Makefile.am in a similar fashion to the existing substitution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that'd work, ya

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as long as we are fine with more moving parts

Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK by me. Since we're already dynamically generating the service file we might as well generate what we want based off the provided configure options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'm willing to do it, but I have almost no experience with makefile writing. Wasn't able to find something like it in the other makefiles either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no memory of modprobe failing to load builtins: i could only find a bug reported for busybox's modprobe applet (https://bugs.busybox.net/show_bug.cgi?id=5270) which has since been fixed, according to my Debian8 (which is already the old stable):

root@linux:~# grep zfs /proc/modules 
root@linux:~# modprobe zfs
root@linux:~# echo $?
0
root@linux:~# busybox modprobe zfs
root@linux:~# echo $?
0
root@linux:~# cat /sys/module/zfs/version 
0.7.0-311_g9c5167d
root@linux:~# lsb_release -a
No LSB modules are available.
Distributor ID:	Debian
Description:	Debian GNU/Linux 8.0 (jessie)
Release:	8.0
Codename:	jessie
root@linux:~#

Please also consider the use case where we have different boot options, both builtin (usually used for recovery i guess?) and with external modules, sharing the same rootfs: unfortunately, if we have to support both options, i don't think we can conditionally remove ExecStartPre at build time.

With that said, i dismissed my review because i don't want to keep this from being merged, i just didn't (and still don't) understand why we would need this change and wanted to express my opinion on the matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the only case to allow it to fail would be for older systems with broken busyboxes (where I probably saw it). I'm fine dropping it if desired, just know that dracut itself merged a change just like the current one (whether they should or not).

Your point about building generic userland components makes sense to me as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to my Fedora27, busybox (busybox-1.26.2-3.fc27) is still broken, a patch was posted 2012-08-12 on busybox's issue tracker but was only merged recently (mirror/busybox@803c85a). Dracut doesn't seem to be using busybox's "modprobe" applet, this may be the reason i could not find any bug reported on the Fedora issue tracker.

Debian is not affected because they ship busybox with said patch backported to older versions.

ExecStart=@sbindir@/zpool import -c @sysconfdir@/zfs/zpool.cache -aN
ExecStartPost=/bin/bash -c "/bin/systemctl set-environment BOOTFS=$(@sbindir@/zpool list -H -o bootfs | /bin/awk '$1 != \"-\" {print; exit}')"

Expand Down
2 changes: 1 addition & 1 deletion etc/systemd/system/zfs-import-scan.service.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ ConditionPathExists=!@sysconfdir@/zfs/zpool.cache
[Service]
Type=oneshot
RemainAfterExit=yes
ExecStartPre=/sbin/modprobe zfs
ExecStartPre=-/sbin/modprobe zfs
ExecStart=@sbindir@/zpool import -aN -o cachefile=none
ExecStartPost=/bin/bash -c "/bin/systemctl set-environment BOOTFS=$(@sbindir@/zpool list -H -o bootfs | /bin/awk '$1 != \"-\" {print; exit}')"

Expand Down