Skip to content

diskbench narrowed its offset to off_t, so 8 GB of file was 2 GiB of reads - #43

Closed
mfethe1 wants to merge 1 commit into
sqliteai:mainfrom
mfethe1:fix/diskbench-64bit-offset
Closed

diskbench narrowed its offset to off_t, so 8 GB of file was 2 GiB of reads#43
mfethe1 wants to merge 1 commit into
sqliteai:mainfrom
mfethe1:fix/diskbench-64bit-offset

Conversation

@mfethe1

@mfethe1 mfethe1 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

waste_pread takes an int64_t offset, and on Windows splits it into ReadFile's Offset/OffsetHigh pair (src/platform.h:189-198) precisely so a read past 2 GiB works. tools/diskbench.c:250 handed it an off_t instead:

off_t off = (off_t)(seed % nrec) * g_rec;

Under LLP64 — MSYS2 UCRT64, the documented Windows build path (#36) — off_t is a 32-bit long, while g_file and g_rec are size_t. So the multiply truncates. The default file size is 8 GB (diskbench.c:268), so this is the default path, not a corner.

Evidence

Same path, same arguments, the two runs back to back, temp file deleted between them.

Before After
7 short read errors and an inverted random curve no errors, monotonic random curve
diskbench PATH 8 12 4    before               after
short reads              7 x "-1: No error"   none
rand  1 thr              0.36 GB/s            1.48 GB/s
rand  2 thr              0.11 GB/s            2.23 GB/s
rand  4 thr              0.06 GB/s            2.25 GB/s
seq write / seq read     2.25 / 2.17          2.21 / 2.13

The sequential rows barely move, which is what makes the pair comparable — only the random path changed.

Two failure modes, and the quiet one is worse

Loud: offsets that wrap negative are refused by waste_pread's off < 0 guard, which returns -1 without touching errno — hence short read -1: No error. The loop then breaks, so that thread stops early while the bytes it never read still divide into its elapsed time. That is why the before column inverts with thread count: more threads means some thread truncates sooner.

Quiet: offsets that wrap positive do not fail at all. They read the wrong place, successfully. A run keeps its entire working set inside the first 2 GiB while reporting the size it was asked for — and a 2 GiB hot region sits inside a consumer SSD's SLC cache and DRAM, which is exactly the flattery this tool exists to avoid. That is the reading I would worry about: not the runs that errored, but any run that didn't.

The garbage is also not reproducible. An earlier run of the same unmodified binary gave 0.34 / 0.80 / 1.02 rather than 0.36 / 0.11 / 0.06. At 2 GB, where nothing truncates, the same binary already reports a clean monotonic 1.58 / 2.00 / 2.33.

This matters beyond the tool: docs/GATES.md Gate H sized the whole storage premise on this program, and README.md quotes 12.78 GB/s internal vs 0.94 GB/s USB from it. Those were measured on macOS, where off_t is 64-bit, so they are unaffected — but any x86/Windows contributor reproducing Gate H would have been measuring a 2 GiB working set.

The change

One line plus a comment explaining why it must not drift back:

int64_t off = (int64_t)(seed % nrec) * (int64_t)g_rec;

Builds clean under -Wall -Wextra. No other file touched.

Noted, not fixed

diskbench.c:277 has g_rec = (size_t)(rec_mb * (1u << 20)) & ~4095UL;. Under LLP64 ~4095UL is a 32-bit mask that zero-extends, so it clears the high 32 bits of a size_t. Harmless today — it only bites a record size ≥ 4 GiB — but it is the same class of bug, and ~(size_t)4095 would close it. Left out to keep this diff to the defect I actually measured; say the word and I will fold it in.

Caveat on the absolute numbers

Throughput here is depressed by an unrelated model conversion running on the same drive. That load is common to both columns and is visible in the sequential rows. These are a before/after pair, not a device rating for the SN7100 — I will send a clean number for that separately.

Context

Found while standing up a native Windows x86 measurement box (Zen 2, 128 GB, PCIe 4.0 NVMe) to work on Gate 7. Companion to #42, which fixes the suite reporting missing tools as engine failures on the same platform.

🤖 Generated with Claude Code

https://claude.ai/code/session_01B5eVMiauR4Lkt8Dhc67MNb

…reads

waste_pread takes an int64_t offset, and on Windows splits it into
ReadFile's Offset/OffsetHigh pair, precisely so a read past 2 GiB works.
The random-read loop handed it an off_t instead.

Under LLP64 — MSYS2 UCRT64, the documented Windows build — off_t is a
32-bit long while g_file and g_rec are size_t, so `(off_t)(seed % nrec) *
g_rec` truncates. The default file size is 8 GB, so this was the default
path, not a corner.

Two failure modes, and the quiet one is worse. Offsets that wrapped
negative were refused by waste_pread's `off < 0` guard, which returns -1
without touching errno; the loop prints "short read -1: No error" and
breaks, so that thread stops early and the bytes it never read still
divide into its elapsed time. Offsets that wrapped positive did not fail
at all — they read the wrong place, successfully. A run kept its whole
working set inside the first 2 GiB while reporting the size it was asked
for, and a 2 GiB hot region sits inside a consumer SSD's SLC cache, which
is exactly the flattery this tool exists to avoid. docs/GATES.md Gate H
sized the storage premise on this program.

Measured on Windows 11 / Ryzen 7 3700X / MSYS2 UCRT64 / gcc 16.2.0,
WD_BLACK SN7100 4 TB. Same path, same parameters, the two runs back to
back:

  diskbench PATH 8 12 4    before               after
  short reads              7 x "-1: No error"   none
  rand  1 thr              0.36 GB/s            1.48 GB/s
  rand  2 thr              0.11 GB/s            2.23 GB/s
  rand  4 thr              0.06 GB/s            2.25 GB/s
  seq write / seq read     2.25 / 2.17          2.21 / 2.13

The sequential columns barely move, which is what makes the pair
comparable — only the random path changed. The before column is not a slow
disk and not even a stable wrong answer: it *inverts* with thread count,
because each thread breaks out of its loop at whatever offset first
truncates, and more threads means that happens sooner. An earlier run of
the same unmodified binary gave 0.34 / 0.80 / 1.02; the garbage is not
reproducible, which is its own argument. At 2 GB, where nothing truncates,
it reports a clean monotonic 1.58 / 2.00 / 2.33.

Absolute throughput here is depressed by an unrelated model conversion
running on the same drive. That load is common to both columns and the
sequential rows show it; the numbers are a before/after pair, not a
device rating.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B5eVMiauR4Lkt8Dhc67MNb
@marcobambini

Copy link
Copy Markdown
Member

Closing this because the fix has landed, in acba190 via #47 — but the diagnosis
and the evidence here are yours, and the commit message says so rather than
absorbing it quietly.

Two people found this independently, from opposite ends: @GTSUltear from the
suite side in #36 gap 4, you from the tool side here, with the same one-line
change and the same reading of why it matters. What made me take it as settled
rather than plausible was the part neither of you had to include — your 1.5 GB
control row, where every offset is under 2^31 and the two builds agree. That is
what says the difference is the offset and not the patch.

The framing I kept from this PR is the one about the quiet half. An offset
that wraps negative gets refused and complains; an offset that wraps positive
reads the wrong place successfully and keeps the whole working set inside the
first 2 GiB — inside a consumer SSD's SLC cache. That is the flattery this tool
exists to avoid, and it is the reading I would not have arrived at from the
error messages alone. It is in the source comment now, in those terms.

I also folded in ~4095UL, which you flagged and deliberately left out as a
defect you had not measured. You were right to leave it out and I think it was
right to take it: nothing observable changes, since it only bites a record size

= 4 GiB, but this file already carried one truncation that looked like a
measurement and ~(size_t)4095 closes the other by construction.

On docs/GATES.md Gate H and the README's 12.78 vs 0.94 GB/s: your reasoning
was correct and I confirmed it here rather than taking it on faith — those were
measured on macOS, where off_t is 64-bit, and the patched tool still reports
9.6-12.5 GB/s on the internal SSD. So nothing in the docs moves.

Your other three are open and CI is green on all of them now — that was my
oversight, not a judgement: they were sitting at action_required and had never
been built, which is a bad way to receive four PRs. #42 in particular is a
report I should have acted on faster, since a first-time Windows contributor's
first run of this suite currently accuses the expert cache of returning
different logits.

Sorry to close rather than merge; the three Windows defects wanted to land
together and one of them needed a comment correcting something I had written.
Thank you for this one.

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.

2 participants