Fix/posix block size - #467
Open
ftarasenko wants to merge 2 commits into
Open
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>
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
1 participant
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