fix(core): make transfer source mounts read-only - #3841
Conversation
helix-nine
left a comment
There was a problem hiding this comment.
Reviewed git diff origin/master...26818797 — 5 files, +49/−8. Fanned out across subagents: seven angles, each candidate judged by a separate verifier against the code. CI is green on all five build targets, so I didn't re-run the type-check; everything below is from reading.
The change is right and the call-site split is complete. import gained a required parameter, so the compiler proves all five sites were considered, and Convert is the correct variant at each of the four. The claim that nothing on the transfer path needs btrfs holds: dir_copy/dir_size are plain byte-copy recursive walks (util/io.rs:661), BlockDev overrides no mount_type() so the mount carries no -t, and RepairStrategy::fsck dispatches on the detected filesystem (fsck/mod.rs:49-51) — the preserved ext4 source mounts and copies fine. git log -S btrfs-convert puts the conversion in released start-os/v0.4.0, so the changelog is describing behaviour real users had, and no start-os/v0.4.0.2 tag exists.
Six findings.
1. shared-libs/crates/start-core/src/disk/main.rs:320 — CONFIRMED — the e2fsck abort leaves along with the conversion block, and nothing downstream replaces it.
The block you gated at :320 opened with e2fsck -fy (:330) and if e2fsck_exit >= 4 { return Err(...) } (:340). Under Preserve the only remaining check is repair.fsck at :380, which for ext2 lands in e2fsck_preen — and e2fsck_runner treats exit 4 as success: it logs one tracing::error! (fsck/ext4.rs:70) and returns Ok(RequiresReboot(false)) because code < 8 (fsck/ext4.rs:89).
Exit 4 is preen's "unexpected inconsistency; run fsck manually" — the state a crashed 0.3.x server leaves. On master that aborted the import with e2fsck's own message. Now mount_fs continues to BlockDev::mount(..., ReadWrite) (:402) and dir_copy streams whatever the damaged tree yields onto the new drive, with the wizard reporting the transfer complete.
I don't think the answer is to put -fy back — auto-repairing the drive you promised to leave alone is the bug this PR exists to fix. But the abort was riding along for free and keeping it needs no write to the source: surface code & 4 out of e2fsck_runner and have migrate refuse rather than mount. Worth scoping to the transfer specifically — boot (bins/start_init.rs:149) continues past bit 4 today and I wouldn't change that here.
2. CONFIRMED — Preserve is a format guarantee, and four places describe it as a read-only one.
conversion is read at exactly one site (:320), so Preserve suppresses the conversion and nothing else. The transfer still writes to the source three ways: e2fsck -p repairs in place (:380 → fsck/mod.rs:50 → fsck/ext4.rs:15), the mount is ReadWrite (:402-403), and migrate deletes /media/startos/migrate/package-data/tmp from it (setup.rs:1078). Your "deliberately not in scope" section names both, and I agree with the scoping — but the prose landed as if they weren't there:
setup.rs:1056—// A transfer only reads the source drive, so it stays as the user's fallback.Falsified 22 lines below by thedelete_dir.CHANGELOG.md:241— "no longer rewrites the drive you are transferring from", and :249 "A transfer now reads the source drive as it finds it". A user reads that, keeps the old drive as an untouched fallback, and skips a backup.disk/main.rs:24-25— "a caller that only reads a drive it is leaving behind must passPreserve" describes no caller in the tree, and never says whenConvertis right.import's signature carries no read-only intent, so the next write added insidemount_fsreaches the transfer source by construction — and a reviewer seeingExt4Conversion::Preserveplus the comment at :1056 waves it through.
The format guarantee is real and worth shipping; it's the wording that overreaches. docs/src/initial-setup.md already gets this right — "left in the format your server wrote it in" — so matching the changelog, the comment and the doc-comment to it closes the gap. If you'd rather make the stronger claim true instead, MountType::ReadOnly already exists and already threads through FileSystem::mount; that forces dealing with the tmp deletion, which is the larger change you flagged.
3. setup.rs:1122 — CONFIRMED — the source is unmounted and exported only on the success path (pre-existing).
export(&old_guid, "/media/startos/migrate") sits after the copy, so every ? between :1069 and :1113 leaves the source mounted read-write, its LUKS mapping open, its VG imported, and the tmp deletion unflushed. recover() uses TmpMountGuard (:1023) for exactly this reason. Not introduced here, but this PR raises the stakes on it: "your old drive is still your fallback" is the promise, and a failed transfer is precisely when a user pulls the drive.
4. setup.rs:1062 — CONFIRMED — guid should be old_guid (pre-existing, inert today).
The import of the source pool picks its LUKS passphrase by testing the target pool's guid. It's harmless only because mount_fs re-derives the decision from the guid it was handed and falls back to the same constant (main.rs:295-296) — the moment import's Option<&str> carries a real per-drive passphrase, the transfer's luksOpen fails on the drive the user is trying to rescue. One word, and it's the line directly under the one you added.
5. setup.rs:1057 — PLAUSIBLE — let _ = now discards a value that can be true.
Pre-PR this was structurally always RequiresReboot(false) here: conversion made the device btrfs, and Preen on btrfs is a hardcoded Ok(RequiresReboot(false)) (fsck/mod.rs:74). With Preserve, e2fsck_preen can set bit 2 and return true (fsck/ext4.rs:90-92), which attach (:293/:315), execute_inner (:860/:879) and boot (start_init.rs:174) all treat as fatal. Whether e2fsck sets FSCK_REBOOT for a non-root LV under /media/startos/migrate isn't decidable from this repo, so I'm holding it at PLAUSIBLE — but either handling it or one line saying why a transfer source is exempt would settle it.
6. disk/main.rs:319 — CONFIRMED — one dead grub-probe per LV (nit).
fs_type is bound unconditionally and used only at :320, and repair.fsck probes again independently at fsck/mod.rs:49. Under Preserve both probes in mount_all_fs are dead work whose answer cannot change anything. if conversion == Ext4Conversion::Convert && detect_filesystem(&blockdev_path).await? == "ext2" { is behaviour-identical under Convert, drops the binding, and reads the way the logic actually works — the caller's flag is the gate, not the filesystem type.
Comments:
setup.rs:1056— currently// A transfer only reads the source drive, so it stays as the user's fallback.Rewrite to what the code does:// The source keeps its existing filesystem, so it stays readable as the user's fallback.disk/main.rs:24-25— replace the criterion with the positive one your changelog already states:/// Whether mounting may convert an ext4 data partition to btrfs in place. /// Conversion rewrites the filesystem and cannot be undone, so pass `Convert` /// only when this server is taking ownership of the drive.
disk/main.rs:383—// Backup LUKS header if e2fsck succeeded— delete. The block's only guard isif !guid.ends_with("_UNENC"); nothing between :380 and theluksHeaderBackuptestsreboot,e2fsck_runnerreturnsOkfor exit 4-7, and on a btrfs pool no e2fsck runs at all. You already delete its sibling five lines above.disk/main.rs:355—// Delete ext2_saved subvolume and defragment after conversion— delete; it names the two subcommands directly beneath it. If a line stays, make it the why:// Deleting ext2_saved discards btrfs-convert's rollback image, so the conversion cannot be undone.docs/src/initial-setup.md:21— the second clause restates the first, and the prohibition arrives without the reason.transfer.page.ts:114already gives it ("may result in services malfunctioning, data corruption, or loss of funds"). Something like: "Your existing drive is left in the format your server wrote it in, so it remains a usable copy if the transfer fails. Once the new drive is running, do not boot the old one as a server again."
One logistical note: the PR is DIRTY against master — the sole conflict is projects/start-os/CHANGELOG.md, master having appended to the same 0.4.0.2 section. Since a rebase push is needed anyway, the nits above are free to fold in.
Untested here too: I have no 0.3.x ext4 drive to exercise, and the ext4-source path is the one that changes. Your note asking for a hardware pass before this ships is the right call, and finding 1 is the specific case I'd want covered — a source with damage e2fsck -p declines to fix.
`migrate` imported the source drive through the same `disk::main::import` every other flow uses, and `mount_fs` converts an ext4 data partition to btrfs in place whenever it mounts one. So a transfer off a 0.3.x drive ran e2fsck, `btrfs-convert`, `btrfs subvolume delete ext2_saved` and a full recursive defragment against the drive the user is keeping as their fallback — before it read a byte of their data — and a failure partway through could have taken the original with it. Conversion is now a decision the caller makes: `Ext4Conversion::Convert` for a drive the server takes ownership of (boot, attach, a new data drive, the setup target) and `Preserve` for the transfer source. The source is still fsck'd (that is what makes the mount safe) and still has its `package-data/tmp` removed so transient data isn't copied across. A fully read-only source mount would need that tmp handling reworked.
Mount transfer-source PVs, logical volumes, encrypted mappings, and filesystems read-only. Dirty ext4 and btrfs sources retry with journal or log replay disabled rather than repairing the source.\n\nExclude transient package data without deleting it, and always attempt full source teardown while preserving the transfer error as primary. The import mode now owns repair and mount policy, eliminating invalid combinations and the redundant filesystem probe.
2681879 to
c6d3d99
Compare
|
Updated the transfer implementation after rebase and review:
|
|
Thanks for the review. No further code changes are needed from the approval; CI is still running on the current head. |
What
The setup wizard's Transfer flow now preserves the source filesystem format and mounts source filesystems read-only while copying data. This keeps the old drive usable as a fallback instead of converting ext4 to btrfs in place.
How
ImportMode::{ReadOnly, ReadWrite(RepairStrategy)}so conversion, repair, encryption, and mount policy cannot be combined unsafely.e2fsck -ponly if the protected mount fails, then returns to read-only mode before copying.cryptsetup --readonly; LUKS keys are passed over stdin rather than written to a temporary file.package-data/tmpfrom sizing and copying instead of deleting it from the source.The source filesystem can still be modified by ext4 repair or Btrfs tree-log replay when a protected mount fails; those recovery steps run only when mounting requires them. Copying itself always uses a block-protected read-only view.
Docs and cleanup
AGENTS.mdand repoints its inbound links, per the repository's documentation policy.Testing
make start-core-test— 445 passed, 3 ignoredcargo check -p start-coremake start-core-format-checkmdbook buildinprojects/start-os/docsgit diff --check origin/master...HEADA complete setup-wizard transfer with physical old/new drives was not available in this environment.