Allow modprobe to fail when called within systemd

[prometheanfire]

Allows for built in zfs

should be backported to zfs-0.7-release

[loli10K]

@prometheanfire could you please specify if this fixes a specific issue and how this was tested? I just built the current SPL/ZFS master just fine and i don't understand why this change is needed.

[loli10K]

Please add your "Signed-off-by" to the commit message as per contributing guidelines/pull request template (which is somehow missing from this PR?) Thanks!

[prometheanfire]

updated, reasoning is also in an amended commit message.

[loli10K]

@prometheanfire thanks, but i wonder if this change is really needed?

This is from my Debian8 builder:

root@linux:~# systemctl restart zfs-import-scan.service
root@linux:~# systemctl status zfs-import-scan.service
● zfs-import-scan.service - Import ZFS pools by device scanning
   Loaded: loaded (/usr/lib/systemd/system/zfs-import-scan.service; disabled)
   Active: active (exited) since Thu 1970-01-01 00:03:30 UTC; 2s ago
     Docs: man:zpool(8)
  Process: 1742 ExecStartPost=/bin/bash -c /bin/systemctl set-environment BOOTFS=$(/sbin/zpool list -H -o bootfs | /bin/awk '$1 != "-" {print; exit}') (code=exited, status=0/SUCCESS)
  Process: 1738 ExecStart=/sbin/zpool import -aN -o cachefile=none (code=exited, status=0/SUCCESS)
  Process: 1735 ExecStartPre=/sbin/modprobe zfs (code=exited, status=0/SUCCESS)
 Main PID: 1738 (code=exited, status=0/SUCCESS)

Dec 45 00:03:30 linux zpool[1738]: no pools available to import
root@linux:~# /sbin/modprobe zfs
root@linux:~# echo $?
0
root@linux:~# grep zfs /proc/modules 
root@linux:~# zgrep -E 'CONFIG_(SPL|ZFS)=' /proc/config.gz 
CONFIG_SPL=y
CONFIG_ZFS=y
root@linux:~# modinfo zfs
modinfo: ERROR: Module zfs not found.
root@linux:~# 
root@linux:~# cat /sys/module/zfs/version 
0.7.0-311_g9c5167d

[prometheanfire]

hmm, I know modprobe used to fail if trying to modprobe a statically included module. Guess it doesn't anymore :| In any case it's best practice to use =- for things that can exit non-zero without causing an actual failure.

[codecov]

Codecov Report

Merging #7174 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7174      +/-   ##
==========================================
- Coverage   76.36%   76.36%   -0.01%     
==========================================
  Files         327      327              
  Lines      103765   103765              
==========================================
- Hits        79242    79235       -7     
- Misses      24523    24530       +7

Flag	Coverage Δ
#kernel	76.37% <ø> (+0.08%)	⬆️
#user	65.77% <ø> (+0.17%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4411de2...b03a367. Read the comment docs.

[loli10K]

In any case it's best practice to use =- for things that can exit non-zero without causing an actual failure.

I don't know about that, it's true modprobe can exit with non-zero status and we would still try to zpool import (and optionally error out with "The ZFS modules are not loaded. Try running '/sbin/modprobe zfs' as root to load them.") but that doesn't seem correct to me:

• /sbin/modprobe = ensure kernel module is found in the kernel (loaded or builtin) and then proceed with the next step

vs

• [minus]/sbin/modprobe = try loading kernel modules, if we fail for any reason don't care about the error and just keep going to the next step

I like the explicit, fatal failure (having ZFS into the kernel is a prerequisite, fail otherwise) approach here, and given this doesn't seem to be fixing any outstanding issue i don't see why we would need this change, unless modprobe returns an error on some distro when ZFS is builtin: when/where did you test this change?

I'm going to withdraw my review since you added the "Signed-off-by" to the commit message, though i cannot approve this change because of my previous points.

Requested change was applied

[behlendorf]

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.

[prometheanfire]

that'd work, ya

[prometheanfire]

as long as we are fine with more moving parts

[behlendorf]

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.

[prometheanfire]

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.

[loli10K]

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.

[prometheanfire]

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.

[loli10K]

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.

[loli10K]

All things considered this would probably affect embedded systems (which are more likely to use busybox's modprobe) using builtin ZFS, though if you're designing an embedded system with ZFS 0.7.x you shouldn't be using such an old busybox ...

[prometheanfire]

so... I'm gonna leave it as is unless someone tells me otherwise :D

[loli10K]

@prometheanfire @behlendorf i'm sorry for stalling this (though this wasn't my intention), i'm ok with merging this to support embedded systems using old busybox versions.

[behlendorf]

@loli10K that's not a problem, I'm glad you were able to identify where this might potentially be an issue. You're point about supporting both builtin and the kmod case is well taken, I hadn't considered that scenario. In which case this LGTM as well, will merge.