Conversation
WriteChunk checksums the chunk with io.Copy before sending it. For a reader that implements neither WriterTo nor a memory backed Read - a local file opened by a multi-thread copy - io.Copy falls back to its own 32 KiB buffer, so the source is read in 32 KiB pieces and the disk sees thousands of requests per second where it should see hundreds. Hand io.CopyBuffer a 1 MiB buffer instead. It is allocated for the checksum pass rather than taken from the pool, where it could wait for ever on a --max-buffer-memory below the page size. Readers that do implement WriterTo, such as pool.RW, keep their fast path and don't get a buffer.
3 tasks done
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.
What does this change do?
Use a 1 MiB scratch buffer for the checksum pass in
s3ChunkWriter.WriteChunkwhen the source does not implementio.WriterTo.For an unbuffered local range reader,
io.Copy(md5, reader)falls back to 32 KiB reads.io.CopyBuffermakes the checksum reads larger; readers withWriterTo, includingpool.RW, retain their fast path without a scratch allocation.The buffer is allocated directly, avoiding a new global-pool wait on
--max-buffer-memory. The additional buffer is 1 MiB per concurrent fallback checksum call, not a full part.Checksums, multipart sizing, concurrency and retries are unchanged. Only checksum reads change, not HTTP body reads or source rereading.
An earlier implementation with the same 1 MiB checksum reads, but a pool-backed scratch buffer, was measured on Windows Server with a local NVMe source and an S3-compatible target:
These measurements are from the earlier pool-backed version, not a benchmark of the final
make-based allocation in this PR.Linked issue
None. This is a small, self-contained performance fix in the existing S3 checksum path; it has no dependency on the proposed crypt changes.
For new or changed backends
go run ./fstest/test_allpassed for S3 on this branch against MinIO RELEASE.2025-09-07 on Linux (Ubuntu 24.04, Go 1.27.1), using the upstreamTestS3Miniotest and ignore lists. This covered backend, operations, sync, bisync, git-annex and VFS tests.S3 unit tests and
FsOpenChunkWriter/FsPutChunkedagainst an S3-compatible service also passed.Fork CI: https://github.com/rbutor/rclone/actions/runs/36925298046 — successful on the first attempt, commit
cc390b351.Integration-test retry detail
FsPutFiles/FromRoot/ListRneeded one retry after a disappearing bucket during root listing. Other test packages create and remove buckets concurrently. Master and the combined branch passed first time. Final result:PASS: All tests finished OK.Checklist
test_allpasses for this backend and if submitting a new backend can provide a test account for the integration tester - see CONTRIBUTING.md.