Skip to content

Fix F_PREALLOCATE delta calculation, use x/sys/unix, keep nullseed limit#1

Open
folbricht wants to merge 2 commits into
nsakaimbo:fix/nullseed-no-reflink-chunk-limitfrom
folbricht:fix/darwin-preallocate-review
Open

Fix F_PREALLOCATE delta calculation, use x/sys/unix, keep nullseed limit#1
folbricht wants to merge 2 commits into
nsakaimbo:fix/nullseed-no-reflink-chunk-limitfrom
folbricht:fix/darwin-preallocate-review

Conversation

@folbricht

Copy link
Copy Markdown

Proposed fixes for folbricht#326, targeting its branch so they can be merged into the PR.

Changes

Only preallocate the size delta

F_PREALLOCATE with F_PEOFPOSMODE allocates relative to the file's current end. AssembleFile also runs against existing, non-empty files (in-place updates), where passing the full target size requests that much space beyond the current EOF — for a 350 GB in-place reassembly that's ~350 GB of extra allocation before the subsequent truncate trims it back, which can spuriously fail with ENOSPC. Now the file is stat'd first and only size - current is requested, skipping the fcntl entirely when no growth is needed (also avoids F_PREALLOCATE with a zero/negative length).

Use golang.org/x/sys/unix

The module already depends on x/sys, which provides unix.FcntlFstore, unix.Fstore_t, and the F_PREALLOCATE/F_ALLOCATEALL/F_PEOFPOSMODE constants. This drops the hand-rolled struct, magic constants, unsafe, and the deprecated syscall.Syscall.

Fall back to truncate on ENOTSUP

Not all filesystems support F_PREALLOCATE (SMB, some FUSE mounts). The sparse-hole issue is APFS-specific, so on ENOTSUP fall back to the previous plain-truncate behavior instead of failing the whole assembly.

Revert the nullseed.go change

The 100-chunk limit was added in folbricht#220 for worker load-balancing, not correctness: when isBlank=false and reflink is unavailable, nullChunkSection.WriteInto really does copy zeros, and without the limit a single worker serially zero-fills an unbounded region (plus the per-chunk read-back validation) while other workers idle. Chunks beyond the limit don't produce incorrect data — they're picked up by the next LongestMatchWith call or fetched from the store. The preallocation alone fixes the APFS corruption.

Make the non-Darwin implementation create the file too

The Darwin version opens with O_CREATE while the fallback used bare os.Truncate, which fails on a nonexistent file. Both now behave identically (caught by the new tests on Linux).

Tests

Added preallocate_test.go covering new, grown (original content preserved, tail reads as zeros), shrunk, same-size, and empty files. These run the real F_PREALLOCATE path on the macOS CI runners.

folbricht added 2 commits June 7, 2026 11:32
- Only preallocate the difference between the current file size and the
  target size: F_PREALLOCATE with F_PEOFPOSMODE allocates relative to the
  existing end of file, so passing the full size over-allocates when
  assembling in-place into an existing file (and can fail with ENOSPC).
- Replace the raw syscall.Syscall/unsafe implementation with
  unix.FcntlFstore from golang.org/x/sys, which the module already
  depends on. This drops the hand-rolled fstore_t struct and constants.
- Fall back to a plain truncate when the filesystem doesn't support
  F_PREALLOCATE (ENOTSUP, e.g. SMB or FUSE mounts); the sparse-hole
  issue is specific to APFS.
- Create the file in the non-darwin preallocateFile too, so both
  implementations behave the same on a nonexistent target.
- Revert the nullseed.go limit removal. The 100-chunk limit was added in
  #220 for worker load-balancing and removing it would serialize large
  zero-region writes on non-reflink filesystems when isBlank=false. The
  preallocation alone fixes the APFS corruption.
- Add tests for preallocateFile covering new, grown, shrunk, same-size,
  and empty files. These run the real F_PREALLOCATE path on macOS CI.
On a nearly-full APFS volume (hdiutil image), re-preallocating an
existing file to its current size must not request additional disk
space. Passing the full size to F_PREALLOCATE with F_PEOFPOSMODE
instead of only the missing difference over-allocates beyond the
current end of file and fails with ENOSPC.
@folbricht

Copy link
Copy Markdown
Author

Verification

To confirm the F_PREALLOCATE delta fix solves a real problem, I ran the new TestPreallocateTightDisk regression test against both versions of preallocate_darwin.go on GitHub's macOS runners (real APFS, via a 32 MB hdiutil image):

Code macOS run Result
Original (PR 326 head, a46c5b4) 27088940303 F_PREALLOCATE: no space left on device
This branch (ddd4cee) 27088949619 ✅ all OS jobs pass

The test fills most of a small APFS volume with a file already at its target size, then calls preallocateFile on it. The original code passes the full size to F_PREALLOCATE with F_PEOFPOSMODE — which allocates relative to the existing end of file — so it requests the file's entire size again and fails with ENOSPC. The delta calculation allocates nothing, as expected.

Also worth noting: the content-level tests alone could not catch this (the original Darwin code passed them — the over-allocation is transient since the trailing truncate releases the extra blocks), which is why the ENOSPC test exists.

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