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.

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

Cleanup systemd dependencies #6822

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

aerusso
Copy link
Contributor

@aerusso aerusso commented Nov 4, 2017

Some redundancy is present in the systemd dependencies, as
noticed in PR#6764. Existing setups might rely on these quirks,
so these cleanups have been moved to the development branch.

Description

Enable zfs-import.target by default, move dracut-mount and zfs-zed dependencies from zfs-import-*.unit to zfs-import.target, cleanup zfs-share dependency by relying on systemd default dependencies. Relax zfs-mount dependency on zfs-import to help for pools mounted before systemd, e.g., by initrd.

How Has This Been Tested?

I don't use dracut, zed, use zfs-share or have zfs on root, so I cannot test most of this. This will require eyes, and especially other users to test.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

Some redundancy is present in the systemd dependencies, as
noticed in PR#6764. Existing setups might rely on these quirks,
so these cleanups have been moved to the development branch.

Signed-off-by: Antonio Russo <[email protected]>
@rugubara
Copy link
Contributor

rugubara commented Nov 5, 2017

I do use ZFS root and trying to switch to dracut for initramfs. I confirm that this (and PR#6764) boots fine with genkernel generated initramfs. However I can't make it boot with dracut-generated initramfs. I suspect that mount of ZFS root dataset is attempted before the Import ZFS pools by cache file is completed. I'm dropped to an emergency shell, where I can see imported pools and I can mount my ZFS root dataset manually.
I use systemd for both dracut and main system.

I attach the rdsosreport.txt from the dracut boot.

@aerusso
Copy link
Contributor Author

aerusso commented Nov 6, 2017

@rugubara Thanks for testing this out! I'm really confused about what's going on here, because it's looking like the pools managed to get mounted before sysroot is attempted:

[ 5.654953] localhost systemd[1]: Started Import ZFS pools by cache file.
[ 5.659385] localhost systemd[1]: Mounting /sysroot...

Did you have dracut working before, and then this patch broke it? It sounds like your cache file is properly present in your initramfs, because you have said the pools were indeed already imported, correct?

@rugubara
Copy link
Contributor

rugubara commented Nov 7, 2017

@aerusso, I was never able to get booted from zfs root with initramfs built by dracut. I'm desperately looking for the information how dracut boot works and why my rig refuses to boot this way, while it works ok with initramfs by genkernel.
Answering your suggestion about zfs datasets being mounted before sysroot.mount - I don't see an evidence. The mount list is empty when I'm dropped to an emergency shell.

# zfs mount 
#

I also found out that I have 2 sysroot.mount units:

/# systemctl cat sysroot.mount
# /run/systemd/generator/sysroot.mount
# Automatically generated by systemd-fstab-generator

[Unit]
SourcePath=/proc/cmdline
Documentation=man:fstab(5) man:systemd-fstab-generator(8)
Before=initrd-root-fs.target

[Mount]
Where=/sysroot
What=zfs:main
Options=ro

# /run/systemd/generator/sysroot.mount.d/zfs-enhancement.conf
[Unit]
Before=initrd-root-fs.target
After=zfs-import-scan.service
After=zfs-import-cache.service
[Mount]
What=main
Type=zfs
Options=zfsutil
:/# 

and apparently systemd tries to start the 1st one and fails.

@aerusso
Copy link
Contributor Author

aerusso commented Nov 7, 2017

If my understanding of systemd is correct, both of those should be processed. This can be confirmed with, inside your emergency shell, systemctl show sysroot.mount (which I'd appreciate you attaching, if possible). What is the origin of this zfs-enhancement.conf file? It should ideally be updated in lock-step with the changes of #6764 and here.

Also, since this setup wasn't running before, we're still going to need other input from people to see how exactly this interacts with the dependencies.

@rugubara
Copy link
Contributor

rugubara commented Nov 7, 2017

I attach systemctl show sysroot.mount:

# systemctl show sysroot.mount       
Where=/sysroot
What=main
Options=zfsutil
Type=zfs
TimeoutUSec=1min 30s
ControlPID=0
DirectoryMode=0755
SloppyOptions=no
LazyUnmount=no
ForceUnmount=no
Result=exit-code
UID=[not set]
GID=[not set]
ExecMount={ path=/bin/mount ; argv[]=/bin/mount main /sysroot -t zfs -o zfsutil>
Slice=system.slice
MemoryCurrent=[not set]
CPUUsageNSec=[not set]
TasksCurrent=[not set]
IPIngressBytes=18446744073709551615
IPIngressPackets=18446744073709551615
IPEgressBytes=18446744073709551615
IPEgressPackets=18446744073709551615
Delegate=no
CPUAccounting=no
CPUWeight=[not set]
StartupCPUWeight=[not set]
CPUShares=[not set]
StartupCPUShares=[not set]
CPUQuotaPerSecUSec=infinity
IOAccounting=no
IOWeight=[not set]
StartupIOWeight=[not set]
BlockIOAccounting=no
BlockIOWeight=[not set]
StartupBlockIOWeight=[not set]
MemoryAccounting=no
MemoryLow=0
MemoryHigh=infinity
MemoryMax=infinity
MemorySwapMax=infinity
MemoryLimit=infinity
DevicePolicy=auto
TasksAccounting=yes
TasksMax=4915
IPAccounting=no
UMask=0022
LimitCPU=infinity
LimitCPUSoft=infinity
LimitFSIZE=infinity
LimitFSIZESoft=infinity
LimitDATA=infinity
LimitDATASoft=infinity
LimitSTACK=infinity
LimitSTACKSoft=8388608
LimitCORE=infinity
LimitCORESoft=infinity
LimitRSS=infinity
LimitRSSSoft=infinity
LimitNOFILE=4096
LimitNOFILESoft=1024
LimitAS=infinity
LimitASSoft=infinity
LimitNPROC=15644
LimitNPROCSoft=15644
LimitMEMLOCK=16777216
LimitMEMLOCKSoft=16777216
LimitLOCKS=infinity
LimitLOCKSSoft=infinity
LimitSIGPENDING=15644
LimitSIGPENDINGSoft=15644
LimitMSGQUEUE=819200
LimitMSGQUEUESoft=819200
LimitNICE=0
LimitNICESoft=0
LimitRTPRIO=0
LimitRTPRIOSoft=0
LimitRTTIME=infinity
LimitRTTIMESoft=infinity
OOMScoreAdjust=0
Nice=0
IOSchedulingClass=0
IOSchedulingPriority=4
CPUSchedulingPolicy=0
CPUSchedulingPriority=0
TimerSlackNSec=50000
CPUSchedulingResetOnFork=no
NonBlocking=no
StandardInput=null
StandardOutput=inherit
StandardError=inherit
TTYReset=no
TTYVHangup=no
TTYVTDisallocate=no
SyslogPriority=30
SyslogLevelPrefix=yes
SyslogLevel=6
SyslogFacility=3
SecureBits=0
CapabilityBoundingSet=cap_chown cap_dac_override cap_dac_read_search cap_fowner>
AmbientCapabilities=
DynamicUser=no
RemoveIPC=no
MountFlags=
PrivateTmp=no
PrivateDevices=no
ProtectKernelTunables=no
ProtectKernelModules=no
ProtectControlGroups=no
PrivateNetwork=no
PrivateUsers=no
ProtectHome=no
ProtectSystem=no
SameProcessGroup=yes
UtmpMode=init
IgnoreSIGPIPE=yes
NoNewPrivileges=no
SystemCallErrorNumber=0
LockPersonality=no
RuntimeDirectoryPreserve=no
RuntimeDirectoryMode=0755
StateDirectoryMode=0755
CacheDirectoryMode=0755
LogsDirectoryMode=0755
ConfigurationDirectoryMode=0755
MemoryDenyWriteExecute=no
RestrictRealtime=no
RestrictNamespaces=no
MountAPIVFS=no
KeyringMode=private
KillMode=control-group
KillSignal=15
SendSIGKILL=yes
SendSIGHUP=no
Id=sysroot.mount
Names=sysroot.mount
Requires=system.slice
RequiredBy=initrd-root-fs.target
Conflicts=umount.target
Before=umount.target initrd-root-fs.target dracut-pre-pivot.service
After=system.slice zfs-import-scan.service -.mount dracut-pre-mount.service loc>
RequiresMountsFor=/
Documentation=man:fstab(5) man:systemd-fstab-generator(8)
Description=/sysroot
LoadState=loaded
ActiveState=failed
SubState=failed
FragmentPath=/run/systemd/generator/sysroot.mount
SourcePath=/proc/cmdline
DropInPaths=/run/systemd/generator/sysroot.mount.d/zfs-enhancement.conf
UnitFileState=generated
UnitFilePreset=enabled
StateChangeTimestamp=Tue 2017-11-07 12:58:50 UTC
StateChangeTimestampMonotonic=5786228
InactiveExitTimestamp=Tue 2017-11-07 12:58:50 UTC
InactiveExitTimestampMonotonic=5782217
ActiveEnterTimestampMonotonic=0
ActiveExitTimestampMonotonic=0
InactiveEnterTimestamp=Tue 2017-11-07 12:58:50 UTC
InactiveEnterTimestampMonotonic=5786228
CanStart=yes
CanStop=yes
CanReload=yes
CanIsolate=no
StopWhenUnneeded=no
RefuseManualStart=no
RefuseManualStop=no
AllowIsolate=no
DefaultDependencies=yes
OnFailureJobMode=replace
IgnoreOnIsolate=yes
NeedDaemonReload=no
JobTimeoutUSec=infinity
JobRunningTimeoutUSec=infinity
JobTimeoutAction=none
ConditionResult=yes
AssertResult=yes
ConditionTimestamp=Tue 2017-11-07 12:58:50 UTC
ConditionTimestampMonotonic=5781557
AssertTimestamp=Tue 2017-11-07 12:58:50 UTC
AssertTimestampMonotonic=5781558
Transient=no
Perpetual=no
StartLimitIntervalSec=10000000
StartLimitBurst=5
StartLimitAction=none
InvocationID=3d524d19e64c48aea5f016250b3f1609

@rugubara
Copy link
Contributor

rugubara commented Nov 7, 2017

I tried to execute mount in the exact form as sysroot.mount attempts to do:

:/# mount main /sysroot -t zfs -o zfsutil
filesystem 'main' cannot be mounted using 'zfs mount'.
Use 'zfs set mountpoint=/sysroot' or 'mount -t zfs main /sysroot'.
See zfs(8) for more information.
:/# 

mount -t zfs main /sysroot succedes.

@rugubara
Copy link
Contributor

rugubara commented Nov 7, 2017

it seems that -o zfsutil is the culprit

@aerusso
Copy link
Contributor Author

aerusso commented Nov 7, 2017

@rugubara This doesn't look to be caused by this pull request. If I had to guess, it would be that your mount is failing because you have it set to legacy, and dracut is working under that assumption.

That said, this PR isn't the right place for this discussion.

@codecov
Copy link

codecov bot commented Nov 8, 2017

Codecov Report

Merging #6822 into master will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6822     +/-   ##
=========================================
- Coverage    75.3%   75.11%   -0.2%     
=========================================
  Files         297      297             
  Lines       94441    94441             
=========================================
- Hits        71116    70936    -180     
- Misses      23325    23505    +180
Flag Coverage Δ
#kernel 74.52% <ø> (ø) ⬆️
#user 67.39% <ø> (-0.24%) ⬇️
Impacted Files Coverage Δ
module/zfs/vdev_missing.c 60% <0%> (-30%) ⬇️
cmd/zed/agents/zfs_agents.c 75.78% <0%> (-15.63%) ⬇️
module/zfs/dmu_tx.c 81.67% <0%> (-4.59%) ⬇️
cmd/zed/agents/zfs_retire.c 77.01% <0%> (-2.49%) ⬇️
module/zcommon/zfs_uio.c 90.69% <0%> (-2.33%) ⬇️
module/zfs/arc.c 86.64% <0%> (-2.22%) ⬇️
cmd/zed/zed_disk_event.c 81.15% <0%> (-2.18%) ⬇️
module/zfs/dsl_scan.c 81.71% <0%> (-1.92%) ⬇️
module/zfs/vdev_disk.c 70.46% <0%> (-1.43%) ⬇️
module/zfs/spa_misc.c 91.43% <0%> (-1.41%) ⬇️
... and 32 more

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 df1f129...aeb8c88. Read the comment docs.

@rugubara
Copy link
Contributor

rugubara commented Nov 8, 2017

@aerusso, I agree with you. I'm sorry for polluting your PR. After changing the root dataset mountpoint to / I could boot ok. Thank you for your explanation and help.

@behlendorf behlendorf merged commit 80b4852 into openzfs:master Nov 8, 2017
aerusso added a commit to aerusso/zfs that referenced this pull request Dec 15, 2017
Cherry picked line from PR openzfs#6822, this enables the new
target introduced in PR openzfs#6764.

Signed-off-by: Antonio Russo <[email protected]>
tonyhutter pushed a commit that referenced this pull request Dec 18, 2017
Cherry picked line from PR #6822, this enables the new
target introduced in PR #6764.

Signed-off-by: Antonio Russo <[email protected]>
@Bronek
Copy link

Bronek commented Dec 18, 2017

Can we please add this to a minor release on 0.7 branch? I just had a problem after upgrade to linux 4.14 and systemd 235 where the system would no longer mount ZFS filesystems, due to zfs-mount not waiting for the completion of zfs-import-cache. Patch 80b4852 fixed the problem for me.

@aerusso
Copy link
Contributor Author

aerusso commented Dec 18, 2017

@Bronek Does PR #6968 resolve your issue (I think it should)? If not, please open a new issue with more information (i.e., distribution, are you using dracut, etc.) so that we can figure out what went wrong.

@Bronek
Copy link

Bronek commented Dec 18, 2017

@aerusso I am confused. I applied the patch 80b4852 on my ZFS package and it fixes the problem for me. Should I now revert this patch and try enabling zfs-import.target instead? Does that mean there is something wrong with this patch? To me it seems just right ...

FWIW my distribution is ArchLinux, I build both kernel and ZFS myself, I use mkinitcpio, I do not use dracut, and systemd presets are ignored in this distribution (at least for ZFS). Setting up targets and services manually are documented part of installing ZFS on ArchLinux.

@aerusso
Copy link
Contributor Author

aerusso commented Dec 19, 2017

@Bronek da16fc5 (due to #6968, and which is included in 0.7.5) contains the portion of 80b4852 which I believe resolves your issue. 80b4852 also includes additional cleanup that may be more intrusive (esp. for dracut users).

I hadn't realized 0.7.5 was released---I would have just asked you to try with that. Can you check if that resolves your issue?

@Bronek
Copy link

Bronek commented Dec 20, 2017

@aerusso thank you, it does. I installed 0.7.5 , verified that zfs-import.target is enabled and now ZFS is mounted correctly, i.e. after import has completed. All is good now, thanks again!

FWIW this is what systemd ZFS dependencies look like on my system now:

root@gdansk ~ # systemctl list-dependencies | tail -5
●   └─zfs.target
●     ├─zfs-mount.service
●     ├─zfs-zed.service
●     └─zfs-import.target
●       └─zfs-import-cache.service
root@gdansk ~ # systemctl show zfs-mount | grep After=
After=systemd-journald.socket system.slice systemd-remount-fs.service systemd-udev-settle.service zfs-import.target
root@gdansk ~ # systemctl show zfs-import.target | grep After=
After=zfs-import-cache.service zfs-import-scan.service
root@gdansk ~ # systemctl show zfs-import-cache | grep Before=
Before=zfs-zed.service dracut-mount.service zfs-import.target

PS. in ArchLinux , presets are ignored. You can read more about it here archzfs/archzfs#72 (this issue is brought to attention during the installation of ZFS, so users know to enable services)

@aerusso aerusso deleted the pull/systemd-cleanup branch January 22, 2018 21:07
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Jan 29, 2018
Some redundancy is present in the systemd dependencies, as
noticed in PR#6764. Existing setups might rely on these quirks,
so these cleanups have been moved to the development branch.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes openzfs#6822
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Feb 13, 2018
Some redundancy is present in the systemd dependencies, as
noticed in PR#6764. Existing setups might rely on these quirks,
so these cleanups have been moved to the development branch.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes openzfs#6822
FransUrbo pushed a commit to FransUrbo/zfs that referenced this pull request Apr 28, 2019
Some redundancy is present in the systemd dependencies, as
noticed in PR#6764. Existing setups might rely on these quirks,
so these cleanups have been moved to the development branch.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes openzfs#6822
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants