Fix/posix block size - #467
Open
ftarasenko wants to merge 5 commits into
Open
ftarasenko wants to merge 5 commits into
ftarasenko wants to merge 5 commits into
Conversation
fsBlockSize probes the parent directory to decide what block size to hand
dd, and asked for stat's %s. That is the size of the directory's own data,
not a block size. On ext4 a directory occupies whole blocks, so %s returns
4096 and matches what %o would have said, which is why this looked correct
everywhere it was tried. XFS stores a small directory inline in the inode:
/var/lib/k0s/images holding one 15-character filename reports 29 bytes, and
the upload then ran as
dd of=/var/lib/k0s/images/bundle.tar bs=29 seek=0 conv=notrunc
Streaming a multi-hundred-megabyte airgap bundle 29 bytes at a time takes
hours instead of seconds. Ask for %o, the optimal I/O block size, which is
what the BSD %k in the same fallback already means.
Reject an implausible answer as well, so a future misprobe degrades to the
default rather than to a block size that cannot work. That also removes a
crash: when stat exited 0 but printed something unparseable, blockSize was
left at 0 and ddParams panicked with an integer divide by zero.
Signed-off-by: Fedor Tarasenko <ftarasenko@itkey.com>
CopyFrom pipes a whole file into dd's stdin, so dd's bs is only the size of the chunks it reads and writes; it has nothing to do with the remote file system's block size. Taking it from fsBlockSize meant a 4096 byte block even in the good case, which is 128k syscall pairs per gigabyte for no reason. Use a fixed 1 MiB instead. The same line also passed f.pos to seek, which dd counts in output blocks rather than in bytes, so a resumed copy started writing at f.pos * bs. It went unnoticed because the offset is 0 for an ordinary upload. Divide the offset by the block size, and shrink the block size when it does not divide the offset evenly. Signed-off-by: Fedor Tarasenko <ftarasenko@itkey.com>
There was a problem hiding this comment.
🟡 Changes recommended
The resumed-copy behavior and power-of-two validation need direct regression coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes POSIX transfer performance and resume offsets by correcting block-size detection and dd usage.
Changes:
- Uses
stat’s optimal I/O block size with validation and fallback. - Streams uploads in 1 MiB chunks and converts byte offsets to
ddblock offsets. - Adds block-size and transfer command tests.
File summaries
| File | Description |
|---|---|
remotefs/posixfs.go |
Defines block-size bounds and streaming chunk size. |
remotefs/posixfile.go |
Corrects probing and resumed dd writes. |
remotefs/posixfile_test.go |
Updates mocks and adds validation coverage. |
Review details
Suppressed comments (1)
remotefs/posixfile_test.go:94
- This
29case is belowminBlockSize, so it passes even if the new power-of-two check is accidentally removed. Add an in-range non-power-of-two value to cover that distinct validation branch.
{"implausible", "29"}, // an XFS directory holding a single short-named file
{"unparseable", "?"},
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Odd resume offsets can reduce the streaming block size to one byte and recreate the severe performance problem.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
remotefs/posixfile_test.go:101
- This explanation contradicts the assertion below: 16384 is not divisible by the reported value 29, and rejected values fall back to 4096 rather than making
ddParamsuse 1. Please describe that 16384 is divisible by both the accepted 8192-byte value and the 4096-byte default, allowing the command to reveal whether the report was accepted.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| // dd counts seek in output blocks rather than in bytes, so the block size has | ||
| // to divide the offset being resumed from. | ||
| bs := alignBlockSize(streamBlockSize, f.pos) |
The existing CopyFrom test only ever ran with f.pos == 0, where the byte offset and the block offset are both zero, so neither dd's seek unit nor the block size reduction that keeps the two consistent was exercised. Add a table of resume points and assert that the bs and seek dd is handed multiply back to the byte offset: whole blocks, an offset smaller than the streaming block size, an offset that is not a power-of-two multiple, and an odd one that forces bs down to 1. Against the previous byte-valued seek, a 3 MiB resume asks dd to write at byte 3298534883328. Signed-off-by: Fedor Tarasenko <ftarasenko@itkey.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKNtKouZgoxKp5udM2DdVu
ddParams picked the block size from the alignment of the length alone and then divided the offset by it, so an offset that was not a whole number of blocks rounded down: seeking to 2048 and reading 8192 bytes on a 4096 byte block size was handed skip=0 and returned the first 8 KiB of the file. It is the same bytes-versus-blocks confusion as dd's seek, one argument over. Reduce the block size until it divides the offset and the length both, and share that reduction with CopyFrom, which grew its own copy of the loop. Lengths that do not fit the block size no longer collapse straight to bs=1 either: a 100 byte read at offset 4096 now runs at bs=4. count can drop its rounding up, since the block size divides the length by construction, and blocksize*count stays exactly numBytes as Write requires. Signed-off-by: Fedor Tarasenko <ftarasenko@itkey.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKNtKouZgoxKp5udM2DdVu
CopyFrom handed dd the resume offset as seek, which dd counts in output blocks rather than in bytes, so the block size had to divide that offset. An offset that is not a whole number of blocks pushed the block size down and an odd one pushed it to 1, which streams the file a byte per read and write pair: the same hours-instead-of-seconds upload this branch set out to remove, one resume later. The file is already cut back to the resume point by the truncate above it, so an append lands exactly there and needs no offset at all. Write it as a shell append redirect into cat, as PosixFS.WriteFile already does for the whole-file case, and the size of the chunks moved is cat's own business rather than a function of where the copy resumes from. streamBlockSize has no users left. Signed-off-by: Fedor Tarasenko <ftarasenko@itkey.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QfdKBk5gG18ruLNru1nogo
ftarasenko
force-pushed
the
fix/posix-block-size
branch
from
September 17, 2026 16:53
622a0e9 to
5ceb6bc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #466
PosixFile.fsBlockSizeasked stat for%swhere%owas meant.%sis thesize of the probed directory's own data;
%ois the optimal I/O block size,which is what the BSD
%kin the same fallback already asks for.On ext4 a directory occupies whole blocks, so
%sanswers 4096 and happens toagree with
%o— the reason this survived. XFS keeps a small directory inlinein the inode, so
/var/lib/k0s/imagesholding one 15-character filenameanswers 29, and a k0sctl airgap bundle upload ran as
dd ... bs=29. That is~18 million syscall pairs for a 500 MB bundle, and turns a seconds-long upload
into an hours-long one. The issue has the byte-by-byte breakdown of the 29.
Three commits' worth of behaviour in two:
Block size probe. Ask for
%o, and reject an answer that is not aplausible power-of-two block size so a future misprobe degrades to the default
instead of to something unusable. That also removes a crash: when stat exited 0
but printed something unparseable,
blockSizestayed 0 andddParamspanickedon
numBytes % 0.CopyFrom. dd's
bsthere is only the size of the chunks it streams stdininto the file with, unrelated to the remote filesystem, so it now uses a fixed
1 MiB rather than the probed value — 4096 was costing 128k syscall pairs per
gigabyte even in the good case. The same line passed
f.postoseek=, whichdd counts in output blocks and not bytes, so a resumed copy wrote at
f.pos * bs; the offset is 0 for an ordinary upload, which is why it wentunnoticed. The block size now shrinks when it does not divide the offset, which
is a real if unlikely regression in throughput for an unaligned resume — an
appending write would avoid it, but
oflag=appendis not portable to the BSDand macOS dd this library targets, so I left it.
WinFShas no block-size probe, so there is nothing to mirror there. Noexported API changes.
Verified: both commits build, vet and test green independently; the two new
test cases fail against the unfixed code (one by assertion, one by the panic
above). I could not run
make lint— the golangci-lint I have is built againstGo 1.25 and refuses this repo's 1.27 target — so the lint job is unverified.
Written by an AI coding agent working for @ftarasenko, who hit this on their
own XFS hosts, confirmed the
stat -c "%s %o"output above and reviewed thechange before it was opened.
That agent's session has no API access to this repository, so it will not see
review comments or CI results here. Follow-up is @ftarasenko's — they will
relay review feedback and push changes.
Generated by Claude Code