systemd: order zfs-import-* before local-fs-pre #6764

aerusso · 2017-10-16T22:28:18Z

Description

A new zfs-import.target is created, which should be After any zpool import target, and should be Before zfs-mount.service.

The zfs(8) man page is updated to suggest an explicit x-systemd.after=zfs-import.target dependency (thanks @Fabian-Gruenbichler for pointing to this systemd directive) for legacy zfs mounts.

Motivation and Context

zfs-import-{cache,scan}.service must complete before any mounting of legacy fstab ZFS filesystems can occur, creating a race between each legacy zfs .mount and zfs-import-*.service

This patch introduces documentation and framework to simplify ordering for legacy mounts.

Ideally, only the ZFS legacy mounts would be ordered after zfs-import-*, but that level of fine-grained tuning will have to wait for #4943, but that PR is far more comprehensive and sophisticated. This should work now.

If an installation doesn't have zpool in its root partition, this will presumably cause a dependency loop or zfs-import-* to fail. I don't know how significant the number of people who have the zpool/zfs on non-root filesystems. I also was not brave enough to try booting my system without this patch, so I don't know how critical it is.

By separating out a uniform zfs-import.target, a great deal of flexibility is allowed to system administrators. Unlike the earlier revision of this change, there should be no backwards incompatibility.

How Has This Been Tested?

I am running this on a machine that mounts several system directories via legacy fstab.

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.

Fabian-Gruenbichler · 2017-10-17T08:28:25Z

this can also be solved by adding the proper x-systemd.* options (see "man systemd.mount" -> FSTAB) to the legacy mounts in /etc/fstab, without moving the ZFS services around..

aerusso · 2017-10-17T12:01:34Z

@Fabian-Gruenbichler That is absolutely the right way to do this. We may want to introduce another target zfs-import.target that is After each of the zfs-import-* services so that there can be a blanket recommendation in the documentation to just put something like x-systemd.after=zfs-import.target.

If this sounds like a good idea to everyone, I can code something like that up.

codecov · 2017-10-17T12:40:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@867959b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #6764   +/-   ##
=========================================
  Coverage          ?   74.62%           
=========================================
  Files             ?      297           
  Lines             ?    94357           
  Branches          ?        0           
=========================================
  Hits              ?    70414           
  Misses            ?    23943           
  Partials          ?        0

Flag	Coverage Δ
#kernel	`74.59% <ø> (?)`
#user	`66.93% <ø> (?)`

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 867959b...af0dbee. Read the comment docs.

behlendorf · 2017-10-27T00:06:55Z

@Fabian-Gruenbichler would you mind reviewed the updated patch which implements your suggestion.

Fabian-Gruenbichler

looks mostly good to me, some minor nit picks only

Fabian-Gruenbichler · 2017-10-27T07:23:00Z

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

@@ -6,6 +6,7 @@ Requires=systemd-udev-settle.service
 After=systemd-udev-settle.service
 After=cryptsetup.target
 Before=dracut-mount.service
+Before=zfs-import.target


AFAICT, the "WantedBy" below already implies a "Before", so this is probably redundant?

see "man systemd.unit":

A number of unit dependencies are automatically established, depending on unit configuration. On top of that, for units with DefaultDependencies=yes (the default) a couple of additional dependencies are added. The precise effect of DefaultDependencies=yes depends on the unit type (see below). If DefaultDependencies=yes is set, units that are referenced by other units of type .target via a Wants= or Requires= dependency might automatically gain an Before= dependency too. See systemd.target(5) for details.

Isn't DefaultDependencies=no here, though?

yes, sorry I missed it. still not really used to the GH diff view..

Fabian-Gruenbichler · 2017-10-27T07:37:47Z

man/man8/zfs.8

+finishes at boot time. For example, on machines using systemd, the mount
+option
+.Pp
+.Nm x-systemd.after=zfs-import.target


is only available since systemd 233, which excludes current Debian stable and Ubuntu LTS (I have not idea about RH and derived distros).

maybe use the (better fitting as well) x.systemd.requires, which adds a Requires and After? a reference to man systemd.mount might also be helpful for further reading.

I'll do this.

Fabian-Gruenbichler · 2017-10-27T07:37:50Z

etc/systemd/system/zfs-import.target.in

+Description=ZFS pool import target
+
+[Install]
+WantedBy=zfs-mount.service


IMHO also redundant, zfs-mount.service already has an After=, and the main zfs.target already ensures that it is enabled by default

zfs-mount.service also has DefaultDependencies=no.

I might be missing something though.

yes, but we already have a default chain of
multi-user.target wants zfs.target wants zfs-FOO.target/service that makes sure all the services and targets are enabled. so we only need ordering between mount and import, if both are enabled.

zfs-mount.service works independently of zfs-import.target and the zfs-import-FOO services (e.g., if your rpool has been imported but only the bootfs mounted in initramfs/dracut), it should just run after them if they are also enabled.

it's just a minor nitpick though, and the old units had that WantedBy as well, so feel free to ignore this ;) the same would IMHO also apply to zfs-share -> zfs-mount, which is entirely unrelated to this PR

Yes, you are absolutely right. I'm pretty sure I was mirroring the other units dependencies, and worried that someone might enable zfs-mount without zfs.target. Or something quirky that the other units seemed to allow for.

I'll probably leave this as-is so as to not break anyone's (weird) existing setup, unless there's a strong opinion.

This would be a good time to cleanup the other unit dependencies that don't make sense. Although it should be done with two patches so we have the option of cherry-picking the minimal change back to the 0.7 releases.

zfs-import-{cache,scan}.service must complete before any mounting of filesystems can occur. To simplify this dependency, create a target that is reached After (in the systemd sense) the pool is imported. Additionally, recommend that legacy zfs mounts use the option x-systemd.requires=zfs-import.target to codify this requirement. Signed-off-by: Antonio Russo <[email protected]>

aerusso · 2017-10-27T22:45:44Z

I've implemented @Fabian-Gruenbichler's suggestion to the documentation. Once this is merged, I'll clean up the dependencies as suggested, and open a new pull request for discussion.

zfs-import-{cache,scan}.service must complete before any mounting of filesystems can occur. To simplify this dependency, create a target that is reached After (in the systemd sense) the pool is imported. Additionally, recommend that legacy zfs mounts use the option x-systemd.requires=zfs-import.target to codify this requirement. Reviewed-by: Fabian Grünbichler <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Giuseppe Di Natale <[email protected]> Signed-off-by: Antonio Russo <[email protected]> Closes openzfs#6764

Cherry picked line from PR openzfs#6822, this enables the new target introduced in PR openzfs#6764. Signed-off-by: Antonio Russo <[email protected]>

Cherry picked line from PR #6822, this enables the new target introduced in PR #6764. Signed-off-by: Antonio Russo <[email protected]>

zfs-import-{cache,scan}.service must complete before any mounting of filesystems can occur. To simplify this dependency, create a target that is reached After (in the systemd sense) the pool is imported. Additionally, recommend that legacy zfs mounts use the option x-systemd.requires=zfs-import.target to codify this requirement. Reviewed-by: Fabian Grünbichler <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Giuseppe Di Natale <[email protected]> Signed-off-by: Antonio Russo <[email protected]> Closes openzfs#6764

behlendorf added the Status: Work in Progress Not yet ready for general review label Oct 18, 2017

aerusso force-pushed the systemd-ordering branch 2 times, most recently from b036142 to 7d28b6e Compare October 18, 2017 22:36

behlendorf approved these changes Oct 27, 2017

View reviewed changes

dinatale2 approved these changes Oct 27, 2017

View reviewed changes

Fabian-Gruenbichler suggested changes Oct 27, 2017

View reviewed changes

aerusso force-pushed the systemd-ordering branch from 7d28b6e to af0dbee Compare October 27, 2017 22:43

behlendorf removed the Status: Work in Progress Not yet ready for general review label Oct 30, 2017

behlendorf merged commit 5c2552c into openzfs:master Oct 30, 2017

aerusso mentioned this pull request Nov 7, 2017

Cleanup systemd dependencies #6822

Merged

13 tasks

Fabian-Gruenbichler mentioned this pull request Dec 13, 2017

Changed systemd unit ordering with 0.7.4 (zfs-mount/local-fs.target) #6953

Closed

aerusso mentioned this pull request Dec 14, 2017

Use zfs-import.target in contrib/dracut #6964

Merged

13 tasks

aerusso added a commit to aerusso/zfs that referenced this pull request Dec 15, 2017

Enable zfs-import.target in systemd preset

6069d43

Cherry picked line from PR openzfs#6822, this enables the new target introduced in PR openzfs#6764. Signed-off-by: Antonio Russo <[email protected]>

aerusso mentioned this pull request Dec 15, 2017

Enable zfs-import.target in systemd preset #6968

Merged

13 tasks

tonyhutter pushed a commit that referenced this pull request Dec 18, 2017

Enable zfs-import.target in systemd preset (#6968)

da16fc5

Cherry picked line from PR #6822, this enables the new target introduced in PR #6764. Signed-off-by: Antonio Russo <[email protected]>

aerusso deleted the systemd-ordering branch January 22, 2018 21:10

aerusso mentioned this pull request Jun 20, 2018

Correctly importing ZFS pools, and nixos-generate-config perl-ness NixOS/nixpkgs#42178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemd: order zfs-import-* before local-fs-pre #6764

systemd: order zfs-import-* before local-fs-pre #6764

aerusso commented Oct 16, 2017 •

edited

Loading

Fabian-Gruenbichler commented Oct 17, 2017

aerusso commented Oct 17, 2017

codecov bot commented Oct 17, 2017 •

edited

Loading

behlendorf commented Oct 27, 2017

Fabian-Gruenbichler left a comment

Fabian-Gruenbichler Oct 27, 2017

aerusso Oct 27, 2017 •

edited

Loading

Fabian-Gruenbichler Oct 27, 2017

Fabian-Gruenbichler Oct 27, 2017

aerusso Oct 27, 2017

Fabian-Gruenbichler Oct 27, 2017

aerusso Oct 27, 2017 •

edited

Loading

Fabian-Gruenbichler Oct 27, 2017

aerusso Oct 27, 2017

behlendorf Oct 27, 2017

aerusso commented Oct 27, 2017

systemd: order zfs-import-* before local-fs-pre #6764

systemd: order zfs-import-* before local-fs-pre #6764

Conversation

aerusso commented Oct 16, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Fabian-Gruenbichler commented Oct 17, 2017

aerusso commented Oct 17, 2017

codecov bot commented Oct 17, 2017 • edited Loading

Codecov Report

behlendorf commented Oct 27, 2017

Fabian-Gruenbichler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerusso Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerusso Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerusso commented Oct 27, 2017

aerusso commented Oct 16, 2017 •

edited

Loading

codecov bot commented Oct 17, 2017 •

edited

Loading

aerusso Oct 27, 2017 •

edited

Loading

aerusso Oct 27, 2017 •

edited

Loading