-
Notifications
You must be signed in to change notification settings - Fork 233
Create storage slab read simulator script #1696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # packages/shared/pkg/storage/storage_cache_seekable.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d5bb30a5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for chunk := range fileSize / chunkSize { | ||
| offset := chunk * chunkSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read final partial chunk in parallel loop
The parallel read loop iterates for chunk := range fileSize / chunkSize, which truncates the chunk count. When the object size is not an exact multiple of chunkSize, the last partial chunk is never scheduled, so those tail bytes are skipped entirely and the timing stats for the parallel scenario underreport total work. This only shows up for non‑aligned object sizes; using a ceiling division (e.g., (fileSize+chunkSize-1)/chunkSize) or an explicit remainder check would ensure the final chunk is read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional
| const totalAttempts = 10 | ||
| totalChunks := info.size / f.chunkSize | ||
| for range totalAttempts { | ||
| offset := f.rand.Int63n(totalChunks-1) * f.chunkSize // the last one might not be full, just skip it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Panic when file has only one chunk
Low Severity
The nextRead function computes totalChunks := info.size / f.chunkSize and then calls f.rand.Int63n(totalChunks-1). If a file has only one chunk (size between chunkSize and 2*chunkSize), totalChunks-1 becomes 0, and rand.Int63n(0) panics since it requires n > 0. While default parameters (100MB min file size, 4MB chunks) make this unlikely, changing flags or edge-case file sizes could trigger the panic.
| } | ||
| } | ||
|
|
||
| return errors.Join(errs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System settings not restored if setup partially fails
Low Severity
The scenario.setup method continues processing all experiments even when one fails, collecting errors instead of stopping. If an early experiment like setReadAhead or setSysFs succeeds (modifying kernel parameters), but a later experiment fails, the function returns an error and the caller returns early without calling teardown. This leaves system settings (like /sys/class/bdi/.../read_ahead_kb or sysctl values) permanently modified until manual cleanup.
Mostly posting this for visibility right now.
4 MB read ahead, ReadAt = 12ms-45ms
4 MB read ahead, Read = 12-45 ms
128 KB (default) read ahead, Read = 21-100+ms
128 KB (default) read ahead, ReadAt = 21-90ms
Note
Adds tooling to measure storage read performance and export results for analysis.
cmd/hammer-file: sequential/parallel GCS range reads with timing output and Mermaid Gantt filescmd/simulate-gcs-traffic: configurable GCS read simulator (experiments for concurrency, gRPC opts, chunk size, etc.), CSV export with histograms, GCE metadata, and testscmd/simulate-nfs-traffic: NFS read simulator (concurrency, read method, read-ahead/sysctl tuning), optional nfsstat capture/parse, CSV export with Filestore metadata, and testsstorage_cache_seekable.go, read cached chunk viaReadinstead ofReadAt.go.modupdates (promotegoogle.golang.org/apito direct dep).Written by Cursor Bugbot for commit 1b5fd6e. This will update automatically on new commits. Configure here.