Repository navigation
fix: decide swap state from the kernel, and act on devices rather than mountpoints - #4865
Open
xander-lin wants to merge 2 commits into
Open
xander-lin wants to merge 2 commits into
xander-lin wants to merge 2 commits into
Conversation
`swapoff()` decided whether swap was active from lsblk, which reports a `[SWAP]` pseudo mountpoint for an active area. That marker is display state, not kernel state, and it is also what `umount()` had to filter out to avoid running `umount -R [SWAP]`. Read the active swap areas from /proc/swaps instead. `swapoff()` now takes the mountpoints of the device so a swapfile living on it is found too, and raises if an area survives the call. `swapon()` skips an area that is already active - swap is machine-wide state, so a re-run used to abort with "Device or resource busy" - and verifies that activation actually happened.
`Luks2.lock()` passed the mapper's mountpoints - directories - to `umount()`, which starts with `get_lsblk_info()`; lsblk refuses a directory, so a re-run that still had an unlocked and mounted LUKS partition crashed during cleanup. Read the children and act on them as devices: deactivate any swap area that lives on a child, unmount the child, then close it, and raise if a mapper survives the close. `umount_all_existing()` called `umount()` on swap partitions, which walks the `[SWAP]` marker; it now calls `swapoff()` for them.
Torxed
approved these changes
Oct 28, 2026
Member
|
It's most likely LLM generated code, perhaps #4857 is more reasonable to merge? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three things go wrong around swap, and they share one cause: the code decides what swap is doing
from a value lsblk prints for display, instead of from what the kernel reports.
1. Cleanup cannot unmount a filesystem that has an active swap file on it
umount_all_existing()walks the partitions of the target disk. A partition holding an active swapfile is not a swap partition - its
fs_typeis the filesystem - so it reachesumount(partition.path, recursive=True), which the kernel refuses because the swap file is stillactive:
The partition is still mounted afterwards, so the installation stops here.
2.
swapon()is not idempotent, and the mount stage only callsswapon()Swap is machine-wide state, not per-installation state. A previous run - or the live system - can
leave the very area this layout wants active, and then the mount stage aborts:
Re-running an installation is the common case after a failure, which makes this one of the more
likely ways to lose an hour.
3.
Luks2.lock()hands mountpoints toumount()umount()starts withget_lsblk_info(), which takes a device.lock()iterates a child'smountpoints and passes each one:
When the child is an active swap area, that mountpoint is the string
[SWAP]:Fix
Read the active set from
/proc/swaps, and act on devices rather than on displayed mountpoints.umount_all_existing()then dispatches on the filesystem type, and passes the mountpoints for thefilesystem case so a swap file on it is deactivated first:
And
lock()deactivates swap on the child, then unmounts the child by device path, so nomountpoint reaches lsblk:
lock()also raises if a mapper is still present aftercryptsetup close, instead of carrying on.from_existing_partition()no longer turns the marker into a mountpointThe same marker reached the layout model, because
from_existing_partition()took the first entryof
mountpointsas the mountpoint. A swap area has no mountpoint, so it now takes the firstabsolute path:
Measurements
Four scenarios, run on loop devices with a real swap partition and a real swap file:
swapon()on an already-active area[SWAP]reachlock()Row 4 differs in mechanism rather than in outcome: #4745 filters the marker at the
LsblkInfoboundary, this PR stops
lock()from passing mountpoints at all. Both work.What this PR does not fix
Three things, so that it is not read as a complete answer:
A swap file is only deactivated through
umount_all_existing().swapoff()accepts themountpoints of a path, but nothing else in the tree passes them yet.
add_swapfile()is untouched. It still writesdd bs=<size> count=1, which allocates a bufferof that size, and it does not set
nodatacowon btrfs. It has no caller in this tree today, sothis PR leaves it alone rather than changing code nothing reaches.
Luks2.lock()still looks one level below the mapper. On LUKS-on-LVM the mountpoints live onthe logical volumes, which are grandchildren, so a re-run can still fail to close that mapper. This
is a separate change (#4864) because it needs a subtree walk, and I did not want to bundle it here.
Relation to the two open PRs
#4745 and #4857 already fix row 1, and both are correct for that row - the
umount_all_existing()change they carry is byte-identical. This PR overlaps them there.
The reason it exists is rows 2 and 3, which neither covers, and which are not about the same line:
swapon()is in the mount stage and a swap file is not a swap partition, so neither of them comesnear it. If either of those PRs is merged first, this one wants to shrink to rows 2 and 3 - there
would be nothing left to do for row 1.
I am not proposing that this replaces either of them, and if this PR cannot be merged on its own
that is a real objection to it rather than something to work around: shipping two overlapping
cleanups is worse than shipping one, and the row 1 change is not the part of this PR worth
defending.
One conflict, if #4864 lands first
#4864 also rewrites the
for child in lsblk_info.children:body inLuks2.lock(), so the twotouches overlap there. Neither side is a superset: #4864 walks the subtree, this PR deactivates swap
and unmounts by device path. The merged body is both, and it is what I ran while checking the two
together:
_lsblk_subtree()comes from #4864; theswapoff()line and the trailing check are from this PR.One thing to watch when resolving by hand: #4864's
tests/test_luks_lock_subtree.pypatchesget_lsblk_info()with a fixedreturn_value, so the re-read added here still sees the mapper andthe new check raises. That test needs
children=[] if closed else [...]- the product code isfine, the stub just has to model the close. I hit exactly this while checking the combination.
Tests
tests/test_swapoff_existing.pycovers:swapoff()as a no-op when nothing is active, deactivatingboth a device and a swap file found below a mountpoint, raising when an area survives the call, and
the
umount_all_existing()dispatch (which path getsswapoff(), which getsumount(), and thatthe filesystem case passes its mountpoints).
lock()is covered for the same reason: swap isdeactivated with the mountpoints and unmounting uses the device path.