Add io_uring SSD-style benchmark with configurable workload#203
Open
LED-0102 wants to merge 2 commits into
Open
Add io_uring SSD-style benchmark with configurable workload#203LED-0102 wants to merge 2 commits into
LED-0102 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a custom io_uring benchmark binary (harness = false) to measure SSD-style mixed read/write performance and latency percentiles, and exposes the uring backing engine so the benchmark can call into it.
Changes:
- Introduces
benches/uring_bench.rsimplementing a configurable randrw-style workload with HDR histogram latency reporting. - Makes
pegaflow_core::backing::uringand keyuringtypes/functions public so the benchmark can access them. - Adds new dependencies (
clap,hdrhistogram) and registers the new[[bench]]target.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pegaflow-core/src/lib.rs | Exposes backing publicly for bench access. |
| pegaflow-core/src/backing/mod.rs | Exposes uring module publicly. |
| pegaflow-core/src/backing/uring.rs | Makes UringConfig, UringIoEngine, and async read/write APIs public. |
| pegaflow-core/Cargo.toml | Adds deps and registers uring_bench bench target. |
| pegaflow-core/benches/uring_bench.rs | New custom benchmark implementation and fio parameter mapping docs. |
| Cargo.lock | Locks new transitive dependencies. |
Comments suppressed due to low confidence (3)
pegaflow-core/src/backing/uring.rs:260
readv_at_async/writev_at_asyncare nowpuband take raw pointers, but they’re still safe functions with a documented safety precondition (“caller must ensure pointers remain valid”). With a public API, this should be enforced by types (e.g., accept pinned slices/IoSlice/IoSliceMut) or the functions should be markedunsafe fnto avoid exposing an unsound safe API that can trigger UB via invalid pointers.
/// Vectorized read (readv) - reads into multiple buffers in a single syscall.
///
/// # Arguments
/// * `iovecs` - Array of (ptr, len) pairs to read into sequentially
/// * `offset` - File offset to start reading
///
/// # Safety
/// Caller must ensure all buffer pointers remain valid until the returned receiver completes.
pub fn readv_at_async(
&self,
iovecs: Vec<(*mut u8, usize)>,
offset: u64,
) -> io::Result<oneshot::Receiver<io::Result<usize>>> {
pegaflow-core/src/backing/uring.rs:307
writev_at_asyncis nowpuband accepts raw pointers under a safety precondition, but it’s still a safe function. If this remains publicly accessible, it should beunsafe fnor reworked to accept safe buffer types so callers can’t violate the invariants from safe Rust.
/// Vectorized write (writev) - writes multiple buffers in a single syscall.
///
/// # Arguments
/// * `iovecs` - Array of (ptr, len) pairs to write sequentially
/// * `offset` - File offset to start writing
///
/// # Safety
/// Caller must ensure all buffer pointers remain valid until the returned receiver completes.
pub fn writev_at_async(
&self,
iovecs: Vec<(*const u8, usize)>,
offset: u64,
) -> io::Result<oneshot::Receiver<io::Result<usize>>> {
pegaflow-core/src/backing/uring.rs:224
UringIoEngine::newvalidatesthreads > 0but doesn’t validateio_depth > 0. With this now being a public constructor, it would be better to rejectio_depth == 0explicitly with a clearInvalidInputerror rather than relying onio_uringbuilder failures / zero-capacity channels.
impl UringIoEngine {
pub fn new(fd: RawFd, cfg: UringConfig) -> io::Result<Self> {
if cfg.threads == 0 {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"threads must be > 0",
));
}
let mut txs = Vec::with_capacity(cfg.threads);
let mut handles = Vec::with_capacity(cfg.threads);
for _ in 0..cfg.threads {
let (tx, rx) = mpsc::sync_channel(cfg.io_depth * 2);
let mut builder = IoUring::builder();
if cfg.sqpoll {
builder.setup_sqpoll(cfg.sqpoll_idle);
}
let uring = builder.build(cfg.io_depth as u32)?;
let shard = UringShard {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
Hi @xiaguan! |
Collaborator
|
Can you provide a running result and compare it with the fio test? |
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.
Closes #100
This PR adds a custom
io_uringbenchmark using[[bench]]withharness = false, implementing a reproducible mixed read/write workload with configurable parameters.The benchmark reports throughput (MB/s), IOPS, and latency percentiles (p50/p95/p99) using
hdrhistogram. It also includes basicfioparameter mapping to allow easier comparison.Please let me know if any additional scenarios, parameters, or improvements would be useful, or if there’s anything I may have overlooked.