Skip to content

progress: show hashing workers as idle when the queue starves - #143

Merged
martinus merged 2 commits into
masterfrom
progress/park-idle-slot
Jul 25, 2026
Merged

progress: show hashing workers as idle when the queue starves#143
martinus merged 2 commits into
masterfrom
progress/park-idle-slot

Conversation

@martinus

@martinus martinus commented Jul 25, 2026

Copy link
Copy Markdown
Owner

The bug

On a big NAS tree replayed from a huge hashfile, the worker lines froze on commit for the whole run:

  1  commit     /data/databases/immich-postgres/pg_stat_tmp/db_16385.stat  (size: 79.3 KiB)
  ...
⠏ scanning   ⠏ hashing   · dedupe   · done
hashing   8,685 files · 4 need hashing · 2.6 KiB/s

Those 4 files were hashed in milliseconds; the walk then ran for minutes with nothing left to hash.

A csum worker is persistent and keeps its progress line across files (deliberately — re-claiming per file made the display flash "idle" between small files), so between files the line keeps whatever the last file left behind: thread_committing, set for the final DB write. Invisible when hashing is the bottleneck, plainly wrong — and it reads like a hang in the commit path — when the walk is.

The fix

Split it where the knowledge actually is. Only the queue can tell "blocked with no work" from "blocked doing work", so the worker publishes when it started waiting — pscan_slot_waiting(slot, true/false), one atomic store bracketing the pop. Only the renderer should own display timing, so the renderer draws any line whose wait has outlasted a couple of redraws as idle (slot_is_idle()), against a now-named REDRAW_MS instead of two bare literals.

Nothing is polled and no timer is involved: the cost is one atomic store per file (plus one vDSO clock read), and the line flips within one redraw of the threshold. The slot's status is untouched while it waits, so it still looks claimed and no sibling worker takes over its line.

Also: pscan_register_thread() published a fresh slot into pscan.threads while it was still thread_idle — i.e. stealable by a concurrent claim — so it now sets the status before publishing, and is static (nothing outside progress.c used it).

The second commit is a /simplify pass over the first, which had done this from inside scan_workq_pop() with a bounded wait, a park call into the progress module and an owned flag. That worked but put display policy in a queue primitive; the rewrite deletes the timed wait, the park constant, owned, a near-duplicate pscan_slot_wait() and the extra scan_workq_pop() parameter (its signature and unit test are back to what they were). Happy to squash the two.

Verification

Reproduced with a 300k-file tree, cold cache, 3 files touched (--io-threads=1, captured through a pty):

  • before — 1 commit .../hashme2 (size: 97.7 KiB) in every one of the 23 frames;
  • after — commit for 2 frames (~200 ms), then 1 idle for the remaining 2 s.

No-flicker check on a hashing-bound run (94k-file cold scan, 46 frames): an idle worker line appears in exactly one frame, the drain at the end.

test_starved_worker_line_reads_idle pins both directions (a fresh wait still shows the file's status; a backdated one reads idle) plus the no-steal rule — with no sleep, so the C unit suite is back to sub-millisecond. scripts/verify.sh passes (clean build, 120 integration tests, valgrind smoke), as does the full suite under ThreadSanitizer.

🤖 Generated with Claude Code

martinus and others added 2 commits July 25, 2026 21:39
A csum worker keeps its progress slot for its whole life and rolls it
from file to file, so between files the line keeps whatever the last file
left behind - "commit", the status set for the final DB write. That is
invisible on a hashing-bound run (the next file overwrites it within
microseconds) but wrong the moment the walk becomes the bottleneck: on a
big NAS tree where only a handful of files need hashing, all four workers
sat on "commit <last file>" for the entire rest of the run, reading like
a hang in the commit path.

Park the slot as idle once a worker has waited SCAN_IDLE_PARK_MS (250ms,
> one 100ms redraw) for work: long enough that the sub-millisecond gaps
between small files still never flicker, short enough that a genuinely
starved pool stops lying within a blink. The first wait of a starvation
is bounded, the rest is the plain blocking wait as before.

A parked slot stays *owned*, so pscan_claim_slot() cannot hand it to a
sibling worker while its owner is only waiting - pscan_slot_idle() now
means "released for good" (drain, or the churning dedupe pool finishing
a work item) and clears that ownership. pscan_register_thread() sets
status/owned before publishing the slot for the same reason, and is now
static (nothing outside progress.c used it).

Co-Authored-By: Claude <noreply@anthropic.com>
Follow-up cleanup of the previous commit's mechanism (a /simplify pass).

It parked the line from inside scan_workq_pop(): a bounded first wait,
then a call into the progress module to overwrite the slot's status. That
put display timing in a data-structure primitive, needed a display slot
threaded through a queue pop, and needed an `owned` flag so that a
parked-but-still-busy slot wasn't handed to a sibling worker - two fields
encoding one allocation state.

Publish the fact instead of acting on it: the worker records when it
started waiting (pscan_slot_waiting, one atomic store bracketing the
pop), and the renderer draws any line whose wait outlasts a couple of
redraws as idle. Same behaviour, and it deletes the timed wait, the park
constant, the `owned` field, pscan_slot_wait() - a near-duplicate of
pscan_slot_idle() - and the pscan_thread parameter on scan_workq_pop(),
whose signature and unit test go back to what they were. The display
threshold now lives next to the redraw interval it is derived from, both
named rather than bare literals.

The unit test loses its helper thread's 500 ms sleep (the whole C suite
was 0.50 s wall for 0.6 ms of CPU): with the rule a pure function of a
timestamp, backdating waiting_since tests it exactly, in microseconds.

Co-Authored-By: Claude <noreply@anthropic.com>
@martinus
martinus merged commit 947d933 into master Jul 25, 2026
8 checks passed
@martinus
martinus deleted the progress/park-idle-slot branch July 25, 2026 19:58
martinus added a commit that referenced this pull request Jul 26, 2026
…gure (#151)

The README was last substantively updated in #118, two releases ago, so it
was missing everything shipped since and carried one number that no longer
matched the benchmark doc.

Fix a wrong figure: the path-hash row claimed "41 vs 73 MiB on the benchmark
tree". Neither number appears in docs/benchmarks.md, which measures 39.7 vs
70.9 MiB -- and that figure comes from the larger-than-RAM tree, not the
2.07M-file tree the surrounding table describes. Correct both the numbers and
the attribution, and reword the table intro, which named only one of the two
benchmarks the rows actually draw from.

Add the missing user-facing work:
  - paths beyond PATH_MAX are hashed and deduped (#117/#124/#128)
  - the streaming dedupe pipeline (#116), which had no bullet at all
  - the O(extents^2) fragmented-file scan fix (#134)
  - the two dedupe-phase races (#123, #129)
  - clang ASAN/UBSAN/TSAN CI legs, warnings-as-errors, make check-all
  - --cpu-threads, absent from both CLI lists despite being in --help
  - progress polish: scan-phase throughput (#120), idle workers (#143)

Tighten for readability: drop the standalone larger-than-RAM NOTE, which
stated the same ~13x claim a third time; its unique content (RSS, hashfile
size) moves into the speedups table where the reader is already comparing
figures. Mark upstream issue references as "upstream #NNN" throughout -- bare
markfasheh#331/markfasheh#374/markfasheh#376/markfasheh#387 now read as oans issues, since oans has its own numbers
in that range.

Docs-only; no code touched.

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant