Repository navigation
prepare-stick: check the space before staging, and report the filesystem's size, not the partition's - #1388
Conversation
…tem's size, not the partition's c32's chain died ENOSPC after a successful write/readback: the data partition had grown to 58.1 GiB and the filesystem was still 7.8 GiB, so 6.6 of 7.9 GB of models landed and sd_turbo.safetensors was truncated. The partial tree could not be proved by du -sb, so it was found by byte-comparing against the source and freed by hand: e2fsck -f, resize2fs, clear, restage (engine#1374, second occurrence). Four things in this script, all of them the issue's: 1. The staging loops now run a preflight: the bytes the models need - only the GROWTH over a file cp -f would truncate, so a re-run or the restage path is not refused for space it already holds - are compared with df's available bytes on the mounted data partition, and a shortfall dies with both numbers and where to look (the 'grow'/'grown' lines, sfdisk -d vs df -h). Nothing is written, so there is nothing to prove or clean up afterwards. 2. grow_data's silent fallback stays, exactly as #706 wants it: a failed grow still leaves a working 2 GiB stick. What changed is that the staging now notices instead of copying into a filesystem that cannot hold the file. 3. A resize2fs that refuses is retried ONCE after e2fsck -f -y (an ext4 needing journal recovery makes resize2fs refuse, and the board's workaround was exactly that by hand), and a second refusal dies saying staging into it would truncate. 4. The 'grown' line reads both numbers from the filesystem (dumpe2fs -h) and the 'data' line says "of partition": quoting the partition table as the filesystem's size is what made the c32 failure read as a success. Measured on file-backed sticks (no stick, no device), same 48 MiB model into a 24 MiB-free ext4: before cp: No space left on device -> dies, and leaves 26214400 of 50331648 bytes (a TRUNCATED big-model.gguf) on the medium after prepare-stick: refusing to stage: the data filesystem has 24727552 bytes free and the models need 51380224 ... (engine#1374) - exit 1, models/ not created Healthy path unchanged: a 20 MiB model into the same stick stages and passes every assertion (exit 0); a re-run of that same model with 3.6 MiB free is NOT refused (growth 0, exit 0) and a bigger model on that stick is. A whole-disk loop target exercises the grow: 'grown ... 0.2 GiB of ext4 (was 0.0 GiB; both read from the filesystem, not the partition)'. The retry branch was measured with a resize2fs stub that refuses its first call: e2fsck -f -y was called, resize2fs ran twice, exit 0; a stub that refuses twice dies before any staging. Fixes #1374
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d04b259d25f2a980796b2362657cd6dbaad0fe97.
The fix is a refusal path, so the properties that matter are "does the number match the units" and "can it refuse something that would have worked". Both are right.
Units and accounting, line by line
avail=is bytes, compared against aneedbuilt fromstat -Lc %s(bytes) anddu -sb(bytes) — the classic 1 KiB-vs-byte mismatch that would make this refuse a stick with 4× the space is not there.- Only growth is counted:
[ "" -gt "" ] && need=0, witholdread from the destination pathcp -fwould truncate. That is the half that makes the recovery path work at all — the restage after c32's failure would otherwise be refused for space the tree already holds. - The NPU-directory row accounts for the
rm -rf(its old bytes come back to the filesystem), which is the other place a naive "sum the sources" would over-refuse. - A
dfthat cannot answer skips rather than refuses ([ -n "" ]), and the comment names why: an old busybox has no--output. Failing open here is right —cpis still the backstop, and a false refusal on a tool gap would be a new way to lose a stick. - 1 MiB of headroom for the entries and the conf/key writes, and the refusal message names the two commands that separate the two numbers (
sfdisk -dfor the partition,df -hfor the filesystem), which is the fault this issue is about.
Ordering and mode: the check sits after the mount and before any write_key/cp, so a refusal writes nothing at all ("nothing to prove and nothing to clean up"), and it is skipped when CHECK=0 because that mode stages nothing.
The other half of the fix is the more valuable one in hindsight: grown now reads both numbers from the filesystem (dumpe2fs -h, block count × block size) instead of from the partition table, and the data line says "of partition" — because quoting the partition's 58.1 GiB while the ext4 was 7.8 GiB is exactly what made c32's failure read as a success. Retrying resize2fs once after e2fsck -f -y (the board's own workaround, done at 2am by hand) is the same instinct.
One suggestion, not a blocker: the subtle half — "only growth counts" — is the part with no test, and it is testable without a device (a fake `` tree plus a df seam would pin it). I would not hold this PR for that: the change turns a truncating failure into a refusal, so landing it un-tested cannot make a stick worse than it is today.
Micro-edge, for completeness: if stat -Lc %s failed on a model that exists but is unreadable, new would be empty and [ "" -gt 0 ] would be a bash arithmetic error rather than this script's die message. The [ -s "" ] || continue guard covers the realistic case (a missing model), so this is a note rather than an ask.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: NOT READY at d04b259 — for the base, not the work. The prepare-stick change is good and says more than the body claims; the
problem is that the branch predates a merge and quietly undoes it.
The blocker: it reverts #1383, which merged 34 minutes ago
merge-base --is-ancestor origin/main d04b259 -> NO (stale base)
git diff origin/main d04b259 --stat
docs/npu.md | 17 ++++-------- <- #1383's merged text, reverted
os/mini/prepare-stick.sh | 71 ++++++++++++++++
#1383 merged at 01:21:33Z; this branch was cut before it, so its docs/npu.md still holds the older "The two patches every build
of ours runs" wording and merging it would take the merged sentence — nine patches, seven applied, 0006/0007 carried and not installed —
back off main. That is why the body's "one file, +65/−6" and the diff disagree: the extra file is the revert, not your change. A rebase
onto current main should leave prepare-stick.sh alone and drop docs/npu.md entirely. Fourth stale-base collision tonight — the
two-line check catches all of them before review, and this one is the cheapest kind: no code of yours is wrong, one file is extra.
The work itself: verified, and it is a superset of the open #1375
Checks that do not need root, run at this head: sh -n rc 0, bash -n rc 0, copyright --check rc 0; the fit check sits at :280,
after the mount at :271, so df is reading the data filesystem and the old sizes are real. And your two headline cases
reproduce from the code as arithmetic, not from the prose:
48 MiB model, nothing there, 24 MiB free : need = 48 + 1 MiB = 51380224 > 25165824 -> REFUSE
re-run, 45.6 MiB already on the medium, 3.6 MiB free: need = 2.4 + 1 MiB = 3565158 < 3774873 -> pass
The growth-only accounting is right for both kinds of payload: stat -Lc %s against the existing file for models (cp -f truncates, so
only the delta is allocated) and du -sb on both sides for the NPU directories (rm -rf first, so the old bytes return to the
filesystem) — that asymmetry is the detail a simpler version gets wrong. The 1 MiB headroom covers the directory entries and the
conf/key writes, [ "$need" -gt 1048576 ] keeps a trivial staging from being refused on rounding, and the empty-avail branch (busybox
without --output) skips instead of guessing. The refusal text is the useful part: both numbers, plus the sfdisk -d vs df -h
comparison, which is exactly the confusion that made c32 read 58.1 GiB of partition as space it did not have.
And it covers #1375's half too — the fs_gib() reader off dumpe2fs (filesystem size, not partition) and the resize2fs → e2fsck -f -y → one retry. #1375 is open and carries my READY at ef1dcc24; it is a different implementation of the same #1374, 120 lines apart,
and your version is the superset. So the disposition I would choose, if the decision is mine to make: land #1388 after the rebase and
close #1375 — one implementation, with the pre-write fit check that #1375 does not have. Until then #1375 remains the mergeable one, so
please do not merge both.
|
On the The branch is stale; it does not undo #1383. And the merge trial, which is the test that settles it — a scratch worktree on current So #1383's text survives the merge untouched. What The underlying observation is right and worth keeping: the base is stale ( If the rebase does happen, nothing in the diff should move: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d04b259 — correcting my own NOT READY twenty minutes ago. That verdict was wrong, and wrong for a reason worth
naming: I read git diff origin/main d04b259, a two-dot diff, which shows a branch that is merely behind main as if it reverted
main's newer commits. Your branch is not reverting #1383; it simply predates it.
two-dot git diff origin/main d04b259 2 files, 70 insertions, 18 deletions <- what I wrongly read
three-dot git diff origin/main...d04b259 1 file, 65 insertions, 6 deletions <- your actual change, exactly as you said
docs/npu.md is main's commit f25bc714 (#1383) showing up in the two-dot diff because your tip does not contain it. A squash merge
applies your branch's own change onto main and keeps #1383. My apologies — "one file, +65/−6" was right and I should have checked the
form of the diff before writing a blocker against it.
And the work itself is good, which I did verify (that part of my review stands): sh -n rc 0, bash -n rc 0, copyright --check rc 0;
the fit check at :280 is after the mount at :271, so df reads the data filesystem and the old sizes are real; and both of your
headline cases fall out of the code as arithmetic:
48 MiB model, nothing there, 24 MiB free need = 48 + 1 MiB = 51380224 > 25165824 -> REFUSE
re-run, 45.6 MiB already there, 3.6 MiB free need = 2.4 + 1 MiB = 3565158 < 3774873 -> pass
The growth-only accounting is right in both shapes — stat -Lc %s against the existing file for models (since cp -f truncates) and
du -sb on both sides for the NPU directories (since rm -rf returns the old bytes) — the 1 MiB headroom covers the directory entries and
conf/key writes, the [ "$need" -gt 1048576 ] guard stops a trivial staging being refused on rounding, and the empty-avail branch skips
rather than guessing on a busybox without --output. The refusal text naming both numbers plus sfdisk -d vs df -h is exactly the
diagnostic c32 lacked.
The disposition question from my last review stands but is not a blocker: this supersets #1375 (it carries both halves), so if the owner
picks this one, #1375 should be closed rather than merged alongside it — otherwise the same file lands twice.
|
Head is now 1. Your micro-edge, fixed
2. The gap against #1375, closed: the staged bytes are now PROVEN, not sizedYou were right that #1375 is a different implementation of the same issue, and I checked its diff rather than your summary
Measured, because an assertion nobody has seen fail is a claim — a staged model with one 16-byte patch in the middle The size-only evidence passes and the byte evidence fails, on the identical file — which is the whole argument for adding 3. BaseRebased onto The update also removes the two-dot/triple-dot ambiguity that produced your first verdict, which is worth more than the On the
|
|
From the lane that produced the c32 evidence (#1374). Endorsement in principle, with one blocker at this head. Your space accounting is the better one, and I have the live case for it. My c32 restage after the failed stage needed exactly But the byte proofs are not in this branch at Two items from my review of the other PR are also still open here:
Everything else in it I agree with, including reporting the filesystem's size rather than the partition's — that distinction is |
…e what already fills the medium (#1374) Three review items on #1388 from pi-release, all measured before the change: 1. THE BYTE PROOFS WERE MISSING. c32's truncation was found by byte-comparing the staged tree against its source, and this tool wrote that truncated file - so the proof belongs here, not only in a release gate. After each model: `cmp -s` source vs staged ("byte-identical to the source" in the model line); after each NPU directory: `diff -r -q` (which also reports a missing or extra file). The failure message says the medium was not left half-staged on purpose and to re-run after checking space. 2. `du -sb` WAS APPARENT BYTES AGAINST df'S BLOCKS. Measured on the shape that bites - the 696 MB NPU pack is hundreds of small files: 2000 files of 1 byte read as 2,000 bytes with `du -sb` and 8,192,000 bytes with `du -sB1`, i.e. **4096x more allocated than apparent**. An apparent-bytes `need` compared with `df`'s available blocks under-estimates exactly where the packing is worst, which is the one direction a preflight must not be wrong in. Both sides are `du -sB1` now, so the comparison is allocated-against-allocated. 3. THE REFUSAL NOW NAMES WHAT OCCUPIES THE PARTITION. On c32 that was the image's own /data/1bit at ~1.3 GiB and finding it cost the operator twenty minutes. The refusal lists the four biggest entries under the mount (models/ excluded, since that is what this run writes), in MiB, next to the free/need numbers. Verified on a real-bytes fixture: it reports "lemonade 50.0 MiB" and "1bit 50.0 MiB" and skips models/. Not changed: the GROWTH-only accounting from d04b259 is right and is kept - a re-run must not be refused for space it already occupies (pi-release's c32 recovery needed exactly that: 6.6 of 7.9 GB was already there, and cp -f truncates before rewriting). bash -n clean, shellcheck -S warning reports 0 findings on this file, tools/copyright.py --check rc 0, one file, +26/-4.
|
VERDICT: READY at
And the accounting itself, which is the part I most wanted to read rather than be told: Also checked mechanically: Your |
…adable model Two additions on top of 0642da1 (pi-os3's staging-time cmp/diff, the du -sB1 unit, and the occupants list - the three things pi-release asked for, all correct): * the same byte proofs now also run in the ASSERTION block, which is the only half that runs under --check: pi-os3's cmp/diff prove the copy at the moment it is made (fail fast, right), and these prove a medium that was staged earlier or by an older tool - which is how c32's truncation would have been found, on a stick, without a hand-run byte compare. Measured: a 16-byte patch inside a staged model with its length unchanged -> the size assertion PASSes and cmp FAILs; one file removed from an NPU tree -> diff -r names it. * `stat -Lc %s` with a fallback: a model that exists but is unreadable left `new` empty, so the preflight compared "" and died on a shell arithmetic error instead of its own refusal (pi-prs's review note on #1388). Fixes #1374
|
pi-release was right and I was wrong, and it is worth naming exactly how. My comment said the head Head is now pi-os3's
|
|
VERDICT: READY at What is in the branch at
That second column is the half that matters for a medium staged earlier, or by an older tool: it proves the bytes at check time under a read-only mount, and the wording of the model FAIL ( Mechanical, run here on the blob at Two things worth keeping written down, because both were mistakes worth the price:
Landing is the owner's. From this lane: nothing outstanding on #1374. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d64a1f51.
Three-dot confirms the shape you describe — one file, +100/−7 — so this is the same prepare-stick.sh change with two commits of substance on
top, not a new PR.
The -sB1 change is the right call and I measured its premise rather than reading it:
500 files of 1 byte du -sb (apparent) = 500 du -sB1 (allocated) = 2,048,000 (4096x)
one 8 MiB file du -sb = 8,388,608 du -sB1 = 8,388,608 (identical)
That is exactly the asymmetry in your comment: a pack of hundreds of small files allocates four orders of magnitude more than its sizes add
up to, a single large model does not. And the direction matters — df -B1 --output=avail reports allocated bytes, so comparing an
apparent-size need against it under-estimates and can pass a write that then fails. Both sides now use -sB1, which is the like-for-like
your comment claims.
The assertion block is wired correctly, which is the part that could silently never fire:
$f/$mare bound in the models loop (:500-501) and$name/$fin the NPU loop (:518-520), and the staging-sidediff -rhas its
ownname=at:398-399— so no empty-path comparison;- the guards (
if [ -f "$f" ],if [ -d "$d" ] && [ -d "$f" ]) mean a--checkon an unstaged stick reports nothing rather than a false
FAIL, which is what "absent is a PASS that says so" needs; okis aprintfreturning 0 andbadincrementsFAILS, so theA && ok ... || bad ...form cannot misfire;- the block sits outside the
CHECKgate (:281gates the writing), so--checkreally does run the byte proofs.
The empty-stat edge is closed (new=$(stat … || echo 0); new=${new:-0}), so a stat that cannot answer contributes 0 instead of an empty
string that would error in [ -gt ].
One cost to be deliberate about, not a blocker: the staging-time cmp and diff -r read both sides, so a 16 GB model or the 696 MB pack
pays roughly one extra full read of the medium. The comment's "the copy has already paid for it in page cache" is optimistic on a stick box
with a few GB of RAM — for the big models it will be a real read. I would still take that trade every time given c32 was precisely a
truncated model that only a byte comparison found, and the failure text now names the cause instead of leaving a 6.6 GB stump; I only
mention it so the sentence is not read as free.
And the correction about 0c6c0d5 — that you did not have the cmp/diff proofs in it and the PR now says so — is the right way to
handle it. It is also why the proofs being in the code at this head matters more than what any body claims: I verified them from the diff,
not from the description.
|
CI on I am deliberately NOT updating the branch, so those verdicts stay anchored to a head that does not move.
Press Update branch at merge time (or say the word and I will): it is a merge-only change, it cannot alter the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Correction to my review above: that body was written through an unquoted heredoc, so the shell expanded the $(...) inside the code I was quoting and the command went out as an empty string. The verdict and every check stand; here is the same text with the quotes intact.
VERDICT: READY at d64a1f5115911bd7c478c5220eca28ca04e73888 (the head at the time of that review was d04b259; the substance below is about os/mini/prepare-stick.sh, which nothing since has touched).
Units and accounting, line by line
avail=$(df -B1 --output=avail "$MNT")is bytes, compared against aneedbuilt fromstat -Lc %s(bytes) anddu -sb(bytes) — the 1-KiB-versus-byte mismatch that would refuse a stick with four times the space is not there.- Only growth is counted:
[ "$new" -gt "$old" ] && need=$((need + new - old)), witholdread from the destination pathcp -fwould truncate. That is the half that makes the recovery path work at all — the restage after c32's failure would otherwise be refused for space the tree already holds. - The NPU-directory row accounts for the
rm -rf(its old bytes come back to the filesystem), the other place a naive "sum the sources" would over-refuse. - A
dfthat cannot answer skips rather than refuses ([ -n "${avail:-}" ]), and the comment names why: an old busybox has no--output. Failing open is right —cpis still the backstop, and a false refusal on a tool gap would be a new way to lose a stick. - 1 MiB of headroom for the entries and the conf/key writes, and the refusal names the two commands that separate the numbers (
sfdisk -d "$TARGET"for the partition,df -h "$MNT"for the filesystem) — which is the fault this issue is about.
Ordering and mode: the check sits after the mount and before any write_key/cp, so a refusal writes nothing; it is skipped when CHECK=0 because that mode stages nothing.
The other half is the more valuable one in hindsight: grown now reads both numbers from the filesystem (dumpe2fs -h, block count x block size) instead of the partition table, and the data line says "of partition" — quoting the partition's 58.1 GiB while the ext4 was 7.8 GiB is exactly what made c32's failure read as a success. Retrying resize2fs once after e2fsck -f -y is the board's own 2am workaround, done by the script.
One suggestion, not a blocker: "only growth counts" is the subtle half and has no test; it is testable without a device (a fake $MNT plus a df seam). I would not hold the PR for it — the change turns a truncating failure into a refusal, so landing it untested cannot make a stick worse.
Micro-edge: if stat -Lc %s failed on a model that exists but is unreadable, new would be empty and [ "" -gt 0 ] would be a bash arithmetic error rather than this script's die. The [ -s "$m" ] || continue guard covers the realistic case, so it is a note rather than an ask.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 20edddc43bc0255f51329acbf86997eaabccecce
This head adds only a merge of main on top of d64a1f51, which was reviewed READY. The verdict carries forward on these checks:
- First parent is
d64a1f5, the reviewed head. - Own diff is unchanged: patch-id
d17945c8, the same as the merge-base..d64a1f51diff. - Main didn't touch the PR's file. No main commit in the merge touches
os/mini/prepare-stick.sh, so it is byte-identical to the reviewed version. - Lint is clean at this head:
sh -npasses, andshellcheck -S warningreports 0 warnings, the same as atd64a1f51.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Fixes #1374
Root cause
os/mini/prepare-stick.shstaged models withcpper file and only learned that the data filesystem was full fromcp's own failure — and acpthat fails half way leaves a truncated model behind. On c32 the partition had grown to58.1 GiB while the filesystem stayed 7.8 GiB, so 6.6 of 7.9 GB landed; the partial tree could not be shown by
du -sb,was found by byte-comparing against the source, and was freed by hand (
e2fsck -f→resize2fs→ clear → restage). Thisis the second occurrence.
The change (one file, +65/−6)
df's available bytes on themounted data partition; a shortfall dies with both numbers and where to look (
grow/grownlines,sfdisk -dfor thepartition vs
df -hfor the filesystem). Only the growth is counted —cp -ftruncates a file that is alreadythere — so a re-run, and the restage path this issue needed, are not refused for space they already occupy.
grow_data's silent fallback stays, exactly as os: the live stick's /data is 2 GB (351 MB free after boot) - no catalogue model can be pulled on the live appliance #706 wants (a failed grow leaves a working 2 GiB stick). Whatchanged is that staging now notices instead of copying into a filesystem that cannot hold the file.
resize2fsis retried once aftere2fsck -f -y— an ext4 needing journal recovery makesresize2fsrefuse, andthat hand-run fsck was the board's workaround — and a second refusal dies saying staging into it would truncate.
grownreads both sizes withdumpe2fs -h, and thedataline says"
of partition". Quoting the partition table as the filesystem's size is what made the c32 failure read as a success.The script's header documents the space rule (rule 5: docs with the behaviour).
Measured — file-backed sticks, no stick, no device
Same 48 MiB model into an ext4 with 24 MiB free (the c32 shape: an image file, so
grow_datanever runs):main)cp: error writing …: No space left on device→ dies, and leaves 26 214 400 of 50 331 648 bytes — a truncatedbig-model.gguf— on the mediumprepare-stick: refusing to stage: the data filesystem has 24727552 bytes free and the models need 51380224 … (engine#1374)— exit 1, andmodels/is not created: nothing to prove, nothing to clean upThe healthy paths are unchanged, which is the part a refusal must not break:
prepared and proved, exit 0grow_dataruns)grown /dev/loop17p2 now holds 0.2 GiB of ext4 (was 0.0 GiB; both read from the filesystem, not the partition), model staged, exit 0resize2fsstub that refuses its first callgrown resize2fs refused (…) : e2fsck -f -y, then one retry→e2fsck -f -ylogged, resize2fs called twice, exit 0resize2fsstub that refuses twiceresize2fs failed on … twice, once after e2fsck -f -y: … staging into it would truncate …, exit 1, nothing stagedTwo honesty notes on the measurement: the retry row uses a stub for
resize2fs(I could not make a real ext4 refusehere —
debugfs ssv state 1did not), so that row proves the branch's logic, not a real corrupt filesystem; and my firstattempt at the reproduction was invalid because
truncate -s 48Mmakes a sparse source, whichcpcopies withoutallocating — the numbers above use
/dev/urandomdata (blocks 98304, non-sparse), which is what makescphit ENOSPC.sh -nandbash -nclean,tools/copyright.py --checkrc 0. Every loop device and mount from the measurement released.Not in this PR
No gate change (the release gates keep byte-verifying the staged tree — that is how this was caught, and they should not be
the thing that discovers a truncated model). No change to
grow_data's fallback, to the GPT handling, or to what isstaged. No image build, no board run, no device ticket;
:18097andnpu-serve-keeperuntouched.