Fix a dependency loop #10388
Fix a dependency loop #10388
Conversation
|
@didrocks @aerusso @InsanePrawn Can you take a look at this? |
rlaager
force-pushed
the
fix-dependency-loop
branch
from
99df06c to
dc4242b
Compare
|
I've pushed another commit that I think will fix #10356. zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Always setting StandardOutput=null and StandardError=null was the simplest fix I could think of. There are other options that are more complicated. We could do this only for specifically named datasets to account for the Root-on-ZFS HOWTO / Ubuntu installer layout. But that wouldn't fix #10356 where the user is using a slightly different encryption root (rpool/ROOT instead of rpool). It would also be needlessly restrictive on the pool name. So that's probably out. We could check if the key is currently loaded and set them in that case. This would be a bit of a hack, but would work okay on boot. It would lead to (probably harmless in practice) different behavior after a We could check to see if the mountpoint of the encryption root is /, but there again, that wouldn't work for the case in #10356. If we allowed / or none, then it would. We could check for datasets with a mountpoint of / and then write a drop-in unit for their encryption root's zfs-load-key-*.service. That also seems a little goofy, and I'm not 100% sure if generators writing drop-in units is supported, but it would probably work. If someone would prefer one of these alternatives or something else, please let me know. Otherwise, I'm fine with the simple solution. I don't see much value in the output of the zfs-load-key-* services anyway. |
Codecov Report
@@ Coverage Diff @@
## master #10388 +/- ##
==========================================
- Coverage 79.75% 79.48% -0.27%
==========================================
Files 394 394
Lines 124662 124662
==========================================
- Hits 99422 99092 -330
- Misses 25240 25570 +330
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I'll have to test this on my fringe Arch setup tomorrow-ish when I return home. Does nulling stdout in the unit affect/break systemd-ask-password during init/from a tty/from an X terminal? Note to future self: Master:
|
|
Thanks Richard for working on this! Some notes though: when the zfs mount generator was written, there has been quite some discussions on avoiding calling any However, as you saw in our distro ubuntu patch with the addition of zsys, this is unavoidable in some cases in practice. For instance, if you boot on a snapshot which will then clone in the initramfs the dataset (and not rollback), this doesn’t work properly: 1.0 You boot on There are in that situation 2 cases with one which accidently work, even if the state is incorrect: 2.a. The initiramfs mounts both
The main goal is to illustrate that I think in general, you are right to call zfs commands there (we should just try to minimize them as much as possible). I can file a bug for this separate issue and trigger this discussions, while upstreaming a first pass to get a better situation for the above (basically, refreshing the cache if we boot on a snapshot). |
|
@didrocks That sounds like a separate issue. |
Oh, it completely is, I’m just adding arguments on the "anyway, you need to call external commands like zpool or zfs" which is what you could get during review. |
behlendorf
added
Status: Accepted
|
So, I tried this patch on ubuntu (groovy). Note that the number of those warnings in the journal isn’t stable (sometimes, we have more or less of them, only systemd-random-seed.service is always there). For reference, I tried with and without this distro-patch (no bug referenced, unsure if we should drop it?): Description: Start zfs-mount.service after zfs-load-module.service, and if zfs
module was actually loaded. This should allow installation of probert in
containers, which pulls in zfs-linux utilities.
Author: Dimitri John Ledkov <[email protected]>
--- zfs-linux-0.8.3.orig/etc/systemd/system/zfs-mount.service.in
+++ zfs-linux-0.8.3/etc/systemd/system/zfs-mount.service.in
@@ -7,6 +7,8 @@ After=zfs-import.target
After=systemd-remount-fs.service
Before=local-fs.target
+After=zfs-load-module.service
+ConditionPathExists=/sys/module/zfs
[Service]
Type=oneshotSo, I’m unsure this fixes it all or not. |
rlaager
added
Status: Code Review Needed
|
@didrocks I see you merged this into the Ubuntu package already. Did you find more info? Otherwise, can you provide the output from something like Can you also try disabling the cryptoswap to see if that works around the issue? That is, does the problem only occur (with the patch) if encrypted swap is in use? |
|
I merged the patch in ubuntu as we have less warnings with it, jibel has a vm ready and will post the results shortly so that we can debug that and have a better idea of the loop. |
|
Here are the logs with encrypted swap with @rlaager 's patch and without dimitri's ones. And here is the output of the command you requested: Disabling encrypted swap doesn't work around the issue. Note that there is still another encrypted partition mounted from initrd. |
rlaager
force-pushed
the
fix-dependency-loop
branch
from
dc4242b to
13dd589
Compare
|
I've uploaded the simple fix from @aerusso posted at: I still need to get to reviewing the last few comments here. |
|
Sorry @rlaager for the delay looking at this, I completely lost track of this MR. Apparently getting it into Debian was the quickest way to get me to test it! With the change, I see no harm in the patch. However---and I mentioned this before---I still think these key load dependencies are suspicious, probably contributing to some of these dependency loops. |
|
@rlaager just let me know when your happy with this and we can get it merged. |
Ha! We (upstream) have now independently arrived at that same idea (though I suggested |
rlaager
force-pushed
the
fix-dependency-loop
branch
from
13dd589 to
93f6da4
Compare
|
I finally got a chance to look at this in detail again. I believe I've gotten to the bottom of the problem.
With these two changes, I have pushed the fix. At this point, I consider this ready to merge. But ideally, someone else should sanity check this. |
|
The correct way to do this is to save I have never seen un-setting |
rlaager
force-pushed
the
fix-dependency-loop
branch
from
93f6da4 to
00e5e69
Compare
An unset $IFS is supposed to behave as the default: However, in practice maybe that is a problem: So I've made the change you suggested. |
rlaager
force-pushed
the
fix-dependency-loop
branch
2 times, most recently
from
9ad70d0 to
b3b3222
Compare
rlaager
force-pushed
the
fix-dependency-loop
branch
from
b3b3222 to
352f55a
Compare
When generating units with zfs-mount-generator, if the pool is already
imported, zfs-import.target is not needed. This avoids a dependency
loop on root-on-ZFS systems:
systemd-random-seed.service After (via RequiresMountsFor)
var-lib.mount After
zfs-import.target After
zfs-import-{cache,scan}.service After
cryptsetup.service After
systemd-random-seed.service
Signed-off-by: Richard Laager <[email protected]>
zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Signed-off-by: Richard Laager <[email protected]> Closes: openzfs#10356
rlaager
force-pushed
the
fix-dependency-loop
branch
from
352f55a to
1e6ae42
Compare
|
@aerusso @InsanePrawn I rebased this. What do you think? |
|
@aerusso @InsanePrawn any feedback. How would you like to proceed with this? |
|
@behlendorf @rlaager I've tested this on two machines (one zfs on root), and they boot no problem. LGTM---there may still be things to improve in the generator, but this MR is absolutely motion in the right direction. |
behlendorf
added
Status: Accepted
zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Reviewed-by: Antonio Russo <[email protected]> Reviewed-by: InsanePrawn <[email protected]> Signed-off-by: Richard Laager <[email protected]> Closes #10356 Closes #10388
When generating units with zfs-mount-generator, if the pool is already
imported, zfs-import.target is not needed. This avoids a dependency
loop on root-on-ZFS systems:
systemd-random-seed.service After (via RequiresMountsFor)
var-lib.mount After
zfs-import.target After
zfs-import-{cache,scan}.service After
cryptsetup.service After
systemd-random-seed.service
Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: InsanePrawn <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes #10388
zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Reviewed-by: Antonio Russo <[email protected]> Reviewed-by: InsanePrawn <[email protected]> Signed-off-by: Richard Laager <[email protected]> Closes #10356 Closes #10388
When generating units with zfs-mount-generator, if the pool is already
imported, zfs-import.target is not needed. This avoids a dependency
loop on root-on-ZFS systems:
systemd-random-seed.service After (via RequiresMountsFor)
var-lib.mount After
zfs-import.target After
zfs-import-{cache,scan}.service After
cryptsetup.service After
systemd-random-seed.service
Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: InsanePrawn <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#10388
zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Reviewed-by: Antonio Russo <[email protected]> Reviewed-by: InsanePrawn <[email protected]> Signed-off-by: Richard Laager <[email protected]> Closes openzfs#10356 Closes openzfs#10388
Hi, I'm not sure if this is correct or not - but this change removes section 4.15, "Patch a dependency loop", as it seems that this is no longer required now that openzfs/zfs#10388 has been merged? Following the current instructions in 4.15 generates the following console output: ``` root@rescue ~ # curl https://launchpadlibrarian.net/478315221/2150-fix-systemd-dependency-loops.patch | \ > sed "s|/etc|/lib|;s|\.in$||" | (cd / ; sudo patch -p1) % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 1487 100 1487 0 0 4260 0 --:--:-- --:--:-- --:--:-- 4260 patching file lib/systemd/system-generators/zfs-mount-generator Hunk openzfs#1 FAILED at 42. Hunk openzfs#2 FAILED at 62. Hunk openzfs#3 succeeded at 157 with fuzz 2 (offset 80 lines). 2 out of 3 hunks FAILED -- saving rejects to file lib/systemd/system-generators/zfs-mount-generator.rej patching file lib/systemd/system/zfs-mount.service Hunk openzfs#1 FAILED at 6. 1 out of 1 hunk FAILED -- saving rejects to file lib/systemd/system/zfs-mount.service.rej ``` /lib/systemd/system/zfs-mount.service.rej contains the following: ``` root@rescue ~ # cat /lib/systemd/system-generators/zfs-mount-generator.rej --- lib/systemd/system-generators/zfs-mount-generator +++ lib/systemd/system-generators/zfs-mount-generator @@ -42,6 +42,8 @@ do_fail "zero or three arguments required" fi +pools=$(zpool list -H -o name) + # For ZFSs marked "auto", a dependency is created for local-fs.target. To # avoid regressions, this dependency is reduced to "wants" rather than # "requires". **THIS MAY CHANGE** @@ -62,6 +64,7 @@ set -f set -- $1 dataset="${1}" + pool="${dataset%%/*}" p_mountpoint="${2}" p_canmount="${3}" p_atime="${4}" ``` /lib/systemd/system-generators/zfs-mount-generator seems to already contain equivalent code to the rejected hunks, and the accepted hunk seems to generate a code block starting line 160, that seems like a partial and older version of the code block at line 121? ``` 121 # If the pool is already imported, zfs-import.target is not needed. This 122 # avoids a dependency loop on root-on-ZFS systems: 123 # systemd-random-seed.service After (via RequiresMountsFor) var-lib.mount 124 # After zfs-import.target After zfs-import-{cache,scan}.service After 125 # cryptsetup.service After systemd-random-seed.service. 126 # 127 # Pools are newline-separated and may contain spaces in their names. 128 # There is no better portable way to set IFS to just a newline. Using 129 # $(printf '\n') doesn't work because $(...) strips trailing newlines. 130 IFS=" 131 " 132 for p in $pools ; do 133 if [ "$p" = "$pool" ] ; then 134 after="" 135 wants="" 136 break 137 fi 138 done . . . . . 160 # If the pool is already imported, zfs-import.target is not needed. This 161 # avoids a dependency loop on root-on-ZFS systems: 162 # systemd-random-seed.service After (via RequiresMountsFor) var-lib.mount 163 # After zfs-import.target After zfs-import-{cache,scan}.service After 164 # cryptsetup.service After systemd-random-seed.service. 165 for p in $pools ; do 166 if [ "$p" = "$pool" ] ; then 167 wants="" 168 break 169 fi 170 done ``` /lib/systemd/system/zfs-mount.service.rej contains the following: ``` --- lib/systemd/system/zfs-mount.service +++ lib/systemd/system/zfs-mount.service @@ -6,7 +6,6 @@ After=zfs-import.target After=systemd-remount-fs.service Before=local-fs.target -Before=systemd-random-seed.service After=zfs-load-module.service ConditionPathExists=/sys/module/zfs ``` And /lib/systemd/system/zfs-mount.service does not contain the string "Before=systemd-random-seed.service" ...Is it therefore right to remove section 4.15 from this manual?
When generating units with zfs-mount-generator, if the pool is already
imported, zfs-import.target is not needed. This avoids a dependency
loop on root-on-ZFS systems:
systemd-random-seed.service After (via RequiresMountsFor)
var-lib.mount After
zfs-import.target After
zfs-import-{cache,scan}.service After
cryptsetup.service After
systemd-random-seed.service
Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: InsanePrawn <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#10388
zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Reviewed-by: Antonio Russo <[email protected]> Reviewed-by: InsanePrawn <[email protected]> Signed-off-by: Richard Laager <[email protected]> Closes openzfs#10356 Closes openzfs#10388
When generating units with zfs-mount-generator, if the pool is already
imported, zfs-import.target is not needed. This avoids a dependency
loop on root-on-ZFS systems:
systemd-random-seed.service After (via RequiresMountsFor)
var-lib.mount After
zfs-import.target After
zfs-import-{cache,scan}.service After
cryptsetup.service After
systemd-random-seed.service
Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: InsanePrawn <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#10388
zfs-load-key-DATASET.service was gaining an After=systemd-journald.socket due to its stdout/stderr going to the journal (which is the default). systemd-journald.socket has an After (via RequiresMountsFor=/run/systemd/journal) on -.mount. If the root filesystem is encrypted, -.mount gets an After zfs-load-key-DATASET.service. By setting stdout and stderr to null on the key load services, we avoid this loop. Reviewed-by: Antonio Russo <[email protected]> Reviewed-by: InsanePrawn <[email protected]> Signed-off-by: Richard Laager <[email protected]> Closes openzfs#10356 Closes openzfs#10388
Motivation and Context
When generating units with zfs-mount-generator, if the pool is already
imported, zfs-import.target is not needed. This avoids a dependency
loop on root-on-ZFS systems:
Description
This changes zfs-mount-generator to omit the dependency and ordering on zfs-import.target if the pool is already imported (which is the case for the root pool, as it is imported by the initrd).
This fixes the issue reported to Ubuntu here:
https://bugs.launchpad.net/ubuntu/+source/zfs-linux/+bug/1875577
This change reverts 8ae8b2a from #9360. The author of that (didrocks) is aware of this. See my comment: https://bugs.launchpad.net/ubuntu/+source/zfs-linux/+bug/1875577/comments/5 and his reply: https://bugs.launchpad.net/ubuntu/+source/zfs-linux/+bug/1875577/comments/9
How Has This Been Tested?
I have tested this (as adjusted for ZFS 0.8.3 in 20.04) on test VMs as part of the Root on ZFS HOWTO updates. Prior to that, it was tested by the original reporter here:
https://bugs.launchpad.net/ubuntu/+source/zfs-linux/+bug/1875577/comments/13
This could use a little testing in master.
Types of changes
Checklist:
Signed-off-by.