Repository navigation
Conversation
lsblk reports an active swap area by printing `[SWAP]` where a mountpoint
would go, in both `mountpoint` and `mountpoints`. `LsblkInfo` parsed that
straight into `Path('[SWAP]')`, so a swap partition looked like it was
mounted at a folder of that name. That is why `[SWAP]` shows up as a
mountpoint in the partition list, and it is what later gets handed to
`umount`.
`[SWAP]` is the only bracketed value lsblk emits, so match it exactly rather
than stripping anything bracketed: a real mountpoint may legitimately contain
brackets.
`umount_all_existing()` ran `umount` against every partition that was not LUKS, including swap. Swap is not mounted at a folder, so the call failed and the partition stayed busy, which is what breaks opening the installer a second time on a disk with a legacy swap partition. Route swap partitions to a new `swapoff()` helper alongside `swapon()`. swapoff fails if it is pointed at something that is not currently in use as swap, so the helper asks `swapon --show` first and does nothing if the path is not there. Both sides are resolved because swap can be switched on through a link such as /dev/disk/by-uuid/... A real failure raises `DiskError`, matching `swapon()`. Taking that list from swapon rather than from lsblk keeps the helper usable for anything that can be swap, including encrypted and LVM devices and swap files. lsblk cannot describe a swap file at all.
|
This seems to be a purely AI generated PR, we don't have a AI policy (yet) so I'll leave these comments for it
|
|
Replied on #4747. |
xander-lin
left a comment
There was a problem hiding this comment.
Review: swap is machine-wide state, and two of the three helpers still assume it is not
The three pieces this PR adds are the right shape, and each is correct on its own:
- filtering
[SWAP]inLsblkInfo— this is what stopsfrom_existing_partition()from producing
aPath('[SWAP]')mountpoint, which is the display problem reported in #4235. Confirmed:
a partition whose lsblk output carries[SWAP]now validates tomountpoint=None,
mountpoints=[], while a real directory named/mnt/[SWAP]is still kept; swapoff()before unmounting a swap partition inumount_all_existing();- reading the active set from
swapon --showrather than inferring it from the lsblk marker.
What follows is what the PR still leaves open, measured on this tree. None of it is a regression -
master behaves identically in these cases - so this is about how much of the reported problem the
PR closes, not about the patch being harmful.
1. A swap file on the partition is not deactivated, so the unmount still fails
swapoff(partition.path) matches the device against the active set. swapon --show reports a swap
file under the file's own path, never under the partition it lives on, so the match fails and the
call returns without doing anything:
scenario 1: swap file on a partition
/proc/swaps names it: /tmp/review_sfmnt/swapfile
active set : ['/dev/nvme0n1p3', '/tmp/review_sfmnt/swapfile']
partition.path : /dev/loop1p1
partition in set? : False <- swapoff() would skip
That partition is ext4, so umount_all_existing() takes the else branch and calls
umount(partition.path, recursive=True), which the live swap file blocks:
umount_all_existing(): SysCallError: ['/usr/bin/umount', '-R', '/tmp/review_sfmnt'] exited with
abnormal exit code [32]: umount: /tmp/review_sfmnt: target is busy.
partition still mounted: /dev/loop1p1 on /tmp/review_sfmnt -> True
So the teardown that this PR is meant to make work still stops here, with the difference that the
failure now happens in umount rather than in the swap branch. Closing it needs the mountpoints of
the partition handed in as well, so an area living below it is found: a call like
swapoff(partition.path, get_lsblk_info(partition.path).mountpoints) and a match against those
mountpoints, not only against partition.path.
2. swapon() is still not idempotent, and the mount stage only calls swapon()
umount_all_existing() now deactivates swap, but nothing re-checks before activating. The mount
stage does:
# archinstall/lib/installer.py:395
elif part_mod.is_swap():
swapon(part_mod.dev_path)and that function runs swapon unconditionally. Swap is machine-wide, so a second attempt at the
same layout - after a failure, or a re-run - finds the area already active and stops the
installation:
scenario 2: swapon() on an area that is already active
first swapon rc=0
swapon() again: DiskError: ['/usr/bin/swapon', '/dev/loop1p1'] exited with abnormal exit code
[255]: swapon: /dev/loop1p1: swapon failed: Device or resource busy
The new swapoff() already knows how to ask the kernel what is active; the same check makes
swapon() a no-op when the area is on, which is what lets a re-run succeed. Point 1 and this point
are the same shape as #4235 itself: the code acts on an assumption about swap state instead of on
the state the kernel reports.
3. The active set is keyed by what swapon --show prints, which is not always the partition
swapoff() resolves both sides, which correctly handles /dev/mapper/root vs /dev/dm-0. But the
active set contains whatever name the kernel uses - a mapper device, a regular file, a symlinked
path - and the lookup only asks whether one specific path is in it. The cases where those differ are
exactly the ones above.
4. Unrelated commit in the same PR
67cb6b71 changes the Urdu translation, msgstr "Greeter" -> msgstr "{} Greeter". That is a real
fix - the {} placeholder had been dropped - but it has nothing to do with swap, and it also
rebuilds base.mo, so it puts a binary file in this diff. It would be easier to review (and to
backport) as its own commit.
Tests
tests/test_lsblk_swap.py covers the parsing and the matching well. To check that the core change
is not riding on an untested path, replacing
elif partition.fs_type == FilesystemType.LINUX_SWAP:
swapoff(partition.path)with umount(partition.path, recursive=True) makes
test_existing_partitions_disable_swap_and_unmount_filesystems fail, so that behaviour is pinned.
Not covered: the file-vs-device case in point 1 and the second-swapon case in point 2. Both need a
fixture whose swapon --show name differs from the partition path.
Everything above was run on a loop device with a real swap partition and a real swap file; no extra
tooling was needed beyond losetup, sfdisk, mkswap and fallocate.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The installer is trying to unmount a directory called
[SWAP]. That isn't a real mountpoint; it's howlsblkmarks active swap.This filters that marker out and turns off active swap partitions on the selected disk before repartitioning.
Fixes #4235.