Merged
Conversation
Collaborator
ming1
commented
Jul 12, 2025
ming1
commented
- abstract generic offload handling, so far, just for sync io handling
Move submit_poll_sqe() into OffloadHandler, and make it private function of OffloadHandler. Meantime add new() method for calling it when initialize one OffloadHandler. Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit introduces a generic offload handling mechanism to simplify the implementation of ublk targets that need to offload I/O operations to worker threads. The key changes are: - **`src/offload/handler.rs`**: A new `OffloadHandler` struct is introduced. This struct encapsulates the logic for managing a worker thread, an eventfd for signaling, and the communication channels (Sender/Receiver) for jobs and completions. It provides a generic `setup_worker_thread` function that can be used by any target. - **`src/offload/mod.rs`**: The `OffloadTargetLogic` trait is defined to provide a common interface for targets that use the offload mechanism. It requires the implementation of `setup_read_worker`, `setup_flush_worker`, and `handle_io`. - **`src/compress.rs`**: The `compress` target is refactored to use the new offload handler. The `QueueHandler` is simplified, and the logic for setting up worker threads and handling offloaded I/O is now delegated to the `OffloadHandler`. This makes the `compress` target's code cleaner and more focused on its specific logic (i.e., RocksDB operations). In Rust, this is implemented by: - Using generic types (`<Job, C, F>`) in `setup_worker_thread` to make it adaptable to different types of jobs and completion data. - Leveraging traits (`OffloadTargetLogic`) to define a contract for offloadable targets, promoting polymorphism and code reuse. - Using `mpsc::channel` for communication between the main thread and the worker threads, which is a standard and safe way to handle concurrent operations in Rust. - The `nix` crate is used for creating and managing the `eventfd`, which is a low-level Linux feature for event notification. This refactoring improves the overall design by separating the generic offload mechanism from the specific implementation details of each target, leading to more maintainable and reusable code. Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit refactors the offload handling mechanism to use a single,
generic `OffloadJob` struct instead of separate structs for each type
of I/O operation (e.g., `ReadJob`, `FlushJob`). This simplifies the
offload handler and makes it more extensible for future I/O types.
The key changes are:
- **`src/offload/handler.rs`**:
- The `ReadJob` and `FlushJob` structs have been removed and replaced
with a single `OffloadJob` struct. This new struct is designed to be
a superset of the previous structs, containing all necessary fields
for different I/O operations.
- The `OffloadHandler` is now generic over a single `OffloadJob` type,
simplifying its definition.
- The `send_read_job` and `send_flush_job` functions have been moved
into `OffloadHandler` as methods, making the API more cohesive.
- **`src/offload/mod.rs`**:
- The `OffloadTargetLogic` trait is updated to use `OffloadJob` in the
signatures of `setup_read_worker` and `setup_flush_worker`.
- **`src/compress.rs`**:
- The `compress` target is updated to use the new `OffloadJob` struct
when setting up its worker threads.
How this is implemented in Rust:
- The `OffloadJob` struct is defined with `#[derive(Debug, Default)]`.
The `Default` trait is used in the `send_flush_job` method to easily
create an `OffloadJob` for a flush operation, which doesn't require
all the fields of the struct. This is a convenient way to handle
structs with many optional fields.
- The `OffloadTargetLogic` trait is updated to use the unified
`OffloadJob` type. This demonstrates how traits in Rust can be used to
define a generic interface that can be adapted to different underlying
data structures.
- The `OffloadHandler` is no longer generic over the job type, which
simplifies its implementation and usage. This is a good example of
how refactoring can lead to simpler, more maintainable code.
This change improves the design by reducing code duplication and making
the offload handler more generic and easier to extend with new I/O
operation types in the future.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit introduces a more abstract and simplified way to set up
offload handlers, further improving the generic offload mechanism.
Instead of requiring the target logic to implement separate setup
functions for each type of worker (e.g., read, flush), this change
unifies the worker setup and allows the target to register handlers
dynamically.
The key changes are:
- **`src/offload/handler.rs`**:
- `QueueHandler::new` now initializes an empty `Vec<Option<OffloadHandler>>`
with a pre-defined capacity (`NR_OFFLOAD_HANDLERS`).
- It then calls a new `setup_offload_handlers` method on the target
logic, passing a mutable reference to itself. This allows the target
to register its specific handlers in the `offload_handlers` vector.
- **`src/offload/mod.rs`**:
- The `OffloadTargetLogic` trait is simplified. The specific
`setup_read_worker` and `setup_flush_worker` methods are removed.
- They are replaced by a single `setup_offload_handlers` method that
takes `&mut QueueHandler`.
- **`src/compress.rs`**:
- A new private helper function, `setup_one_handler`, is introduced
within the `CompressTarget` implementation. This function encapsulates
the logic for creating a single offload handler, including setting
up the worker thread and its communication channel.
- The `setup_offload_handlers` implementation now simply calls
`setup_one_handler` for each required handler (read and flush),
making the setup process cleaner and more declarative.
- The `handle_io` function is updated to use the registered handlers
from the `offload_handlers` vector.
How this is implemented in Rust:
- The most significant change is the inversion of control in the setup
process. Instead of the `QueueHandler` pulling sender/receiver pairs
from the target logic, it now passes a mutable reference to itself
(`&mut QueueHandler`) to the target logic's `setup_offload_handlers`
method. This is a powerful Rust pattern that leverages the borrow
checker to allow the target to safely modify the `QueueHandler`'s
state (i.e., register its handlers) during initialization.
- The `offload_handlers` vector is now a `Vec<Option<OffloadHandler>>`.
Using `Option` here is idiomatic Rust for representing a collection
where some slots may be empty. It provides a safe way to handle
optional values, preventing null pointer-like errors.
- The `setup_one_handler` function in `compress.rs` demonstrates how to
encapsulate repeated logic within a private helper method in an `impl`
block, improving code organization and readability.
This refactoring results in a more flexible and decoupled design. The
`QueueHandler` is now more generic, and the target implementation has
full control over how its offload handlers are created and registered.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit improves the offload handling mechanism by addressing two
key shortcomings in the previous implementation: the use of "magic
numbers" for handler indexing and the potential for panics due to
out-of-bounds access.
The key changes are:
- **`src/offload/mod.rs`**:
- An `OffloadType` enum is introduced to represent the different
types of offload handlers (e.g., `Read`, `Flush`). This replaces
the integer constants, providing type safety and making the code
more self-documenting.
- A `From<OffloadType> for usize` implementation is provided to easily
convert the enum variants into vector indices.
- **`src/compress.rs`**:
- The `setup_one_handler` function is updated to accept an
`OffloadType` enum variant instead of a `u32` index. This makes
the code more readable and less error-prone.
- The `handle_io` function is refactored to use `get_mut()` and `get()`
for accessing the `offload_handlers` vector. This is a key change
for improving safety. Instead of panicking on an out-of-bounds
index, `get()` and `get_mut()` return an `Option`, allowing the code
to handle invalid indices gracefully. The `if let Some(Some(h))`
pattern is used to safely unwrap both the result of the `get()`
call and the `Option` inside the vector.
How this is implemented in Rust:
- The use of an **`enum`** for `OffloadType` is a classic example of
leveraging Rust's strong type system to create more robust and
expressive APIs. It prevents a whole class of bugs related to invalid
handler indices.
- The `From` trait is used to provide a canonical conversion from
`OffloadType` to `usize`. This is an idiomatic way to handle type
conversions in Rust.
- The use of **`get()`** and **`get_mut()`** instead of direct indexing
(`[]`) is a fundamental aspect of safe Rust programming. It forces the
developer to handle the case where the index might be invalid,
preventing panics and making the code more resilient. The nested
`if let Some(Some(...))` pattern is a concise way to handle the two
levels of `Option` (one from `get()` and one from the `Option` in the
vector).
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit refactors the offload handling mechanism to improve safety
and clarity by removing the need for raw buffer pointers (`bufs_ptr`)
in the handler structs. The I/O buffer is now passed explicitly through
the call chain, making the code easier to follow and less prone to
`unsafe` errors.
The key changes are:
- **`src/offload/handler.rs`**:
- The `bufs_ptr` field has been removed from both `OffloadHandler`
and `QueueHandler`. This eliminates a source of `unsafe` code and
makes these structs simpler.
- A `buf_addr: u64` field was added to the `Completion` struct. This
allows the worker thread to pass the address of the completed I/O
buffer back to the main thread.
- `OffloadHandler::handle_completion` now uses the `buf_addr` from the
`Completion` struct to complete the I/O, rather than relying on a
stored pointer.
- `QueueHandler::handle_event` is updated to accept an
`Option<&mut [u8]>`, making it explicit that a buffer may not always
be present (e.g., for poll events).
- **`src/offload/mod.rs`**:
- The `OffloadTargetLogic::handle_io` trait method is updated to
accept `Option<&mut [u8]>`, propagating the safer buffer handling
to all target implementations.
- **`src/compress.rs`**:
- The `q_sync_fn` is updated to pass the buffer to `handle_event` as
an `Option`. It provides `Some(buf)` for regular I/O and `None` for
poll events, which is determined by checking the `tag`.
- The `handle_io` implementation now receives the buffer as an
`Option` and unwraps it, removing the previous `unsafe` block that
calculated the buffer address from a raw pointer.
- The `QueueHandler::new` call is updated to remove the `dev`
parameter, which is no longer needed.
How this is implemented in Rust:
- The core of this refactoring is the use of **`Option<&mut [u8]>`** to
represent a buffer that may or may not be present. This is a highly
idiomatic Rust pattern that leverages the type system to enforce safe
handling of optional values at compile time, preventing null-pointer
style errors.
- By passing the buffer as a parameter, the data flow becomes much more
explicit. This is a key principle of safe and maintainable Rust code:
making data ownership and borrowing clear through function signatures.
- The removal of the `bufs_ptr` fields from the handler structs reduces
the amount of `unsafe` code and simplifies the logic, as the handlers
no longer need to manage this state.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
…r compress read and flush IO Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit improves the project's encapsulation by changing the visibility of the offload handling components from `pub` to `pub(crate)`. Since `rublk` is a binary executable and not a library, its internal modules are not intended to be used by external crates. Using `pub(crate)` makes this intent explicit and leverages Rust's type system to enforce these boundaries. The key changes are: - **`src/offload/mod.rs`**: The `OffloadTargetLogic` trait is now `pub(crate)`, as it defines a contract for modules *within* this crate only. - **`src/offload/handler.rs`**: All public structs (`OffloadJob`, `Completion`, `OffloadHandler`, `QueueHandler`) and the `setup_worker_thread` function are now `pub(crate)`. This correctly identifies them as internal implementation details of the offload framework. How this is implemented in Rust: - This change relies on Rust's visibility modifiers. `pub` makes an item part of the public API, while `pub(crate)` restricts its visibility to the current crate. - For an application like `rublk`, `pub(crate)` is the idiomatic choice for sharing code between modules. It provides the necessary visibility for internal use while preventing the components from being exposed as a public library API. Signed-off-by: Ming Lei <tom.leiming@gmail.com>
This commit refactors the offload handling mechanism to improve
encapsulation and simplify the target implementation. The responsibility
for creating the `eventfd` and setting up the worker thread is now moved
entirely inside `OffloadHandler::new`.
The key changes are:
- **`src/offload/handler.rs`**:
- `OffloadHandler::new` is now a higher-order function that accepts the
worker logic as a closure (`worker_fn`).
- Inside `new`, it now creates the `eventfd` and calls the private
`setup_worker_thread` function, passing it the `eventfd` and the
closure. This makes the `OffloadHandler` a self-contained component
that manages its own resources.
- `setup_worker_thread` is now a private function within the
`offload::handler` module, as it's an implementation detail that
should not be called directly from outside.
- **`src/compress.rs`**:
- The `setup_offload_handlers` function in the `CompressTarget`
implementation is now much simpler. It no longer needs to create the
`eventfd` or call `setup_worker_thread`.
- It now just defines the worker logic in a closure and passes this
closure directly to `OffloadHandler::new`.
How this is implemented in Rust:
- The use of a **higher-order function** for `OffloadHandler::new` is a
key aspect of this change. By accepting a closure (`F: Fn(...)`),
`new` can be generic over the behavior of the worker thread, while
still encapsulating the boilerplate of thread and `eventfd` creation.
This is a powerful pattern in Rust for abstracting behavior.
- The use of a **`move` closure** in `compress.rs` is important. It
allows the closure to take ownership of the captured variables (like
`db` and `lbs`), which is necessary for the closure to be sent to a
new thread (i.e., to have a `'static` lifetime).
This refactoring improves the design by making the `OffloadHandler`
responsible for its own setup, which simplifies the target-specific
code and strengthens the encapsulation of the offload module.
Signed-off-by: Ming Lei <tom.leiming@gmail.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.