Majority of the DIsk IO for small block size is more than one cluster mapping. THis PR acutlaly does almost 90% of the sync read. What benefit can we get from this partially implemented Async IO?

Thanks for working on this! Code wise, mostly looking good. My io_uring knowledge is very limited. I've consulted Claude Code with Sonnet 4.6 model for review. So please take the following remarks with a grain of salt - I can't assess if these are valid or not.

Thanks for the review and for being transparent about using Claude Code! I use AI tools too, they're genuinely helpful for spotting patterns and sanity checking assumptions, especially in unfamiliar territory like io_uring. I'm learning io_uring as I go myself, same as you, so I'm happy to double check on any questions or do further investigation together to make sure we get this right.

Show Claude Code (Sonnet 4.6) Remarks

io_uring-related issues

Confirmed bugs

* **`next_completed_request` missing `cq.sync()`** — without it the kernel-side CQ head is
  not refreshed before reading, so completions can be silently missed on rings not using
  `IORING_SETUP_SQPOLL`

Addressed inline, false positive. completion() would create a brand new object, no stale state to refresh.

* **Fast-path short reads** — io_uring returns the raw `readv` result (which may be less than
  `total_len`), while the slow path always reports `total_len` as the result; upper layers see
  inconsistent completion values

A QCOW2 file is always a regular file. The fast path operates on clusters fully allocated and contiguous on disk. There is same logic in raw_async.rs. Working on a file descriptor, not a socket or alike.

Partial submit on error

* If `submit_batch_requests` errors mid-batch (e.g. a metadata failure on an `Out` request),
  SQEs already pushed to the SQ but not yet `sync()`'d are abandoned — their `user_data`
  completions never arrive and the virtio queue stalls waiting for them

Addressed inline. It's valid and it's a pre existing patttern.

Asymmetric eventfd signaling

* The slow path explicitly calls `eventfd.write(1)` to signal completions; the fast path relies
  on the kernel writing to the eventfd via `register_eventfd` — this is correct but
  undocumented, making the two paths harder to reason about together

It's actually the same eventfd. It's registered with io_uring for kernel driven signaling, and manually written to for synthetic completions. That's what unifies both paths under a single notification mechanism. The epoll loop doesn't need to know where the completion came from. See QcowAsync::new().

split() lifetime coupling

* `io_uring.split()` is called in both `read_vectored` and `submit_batch_requests`; holding two
  split guards simultaneously would cause a borrow conflict — currently safe because they are in
  separate call frames, but fragile if the call structure changes

It's unclear why Claude tells it's fragile. If &mut self would cause a data race, it would fail to compile. So it's not fragile. But should it change, it'll need to be restructured.

submit_batch_requests misattributes submit errors

* The `submitter.submit()` error is wrapped in `AsyncIoError::ReadVectored` even when the batch
  contained only write requests — there is no appropriate variant for a generic submission
  failure, so errors are misattributed

Valid. I just fixed this, using the more appropriate AsyncIoError::SubmitBatchRequests now.

Overall - a nice demonstration of AI assisted review catching real patterns (the partial submit issue, the misattributed error variant) alongside false positives (the cq.sync() one). Good to scrutinize both together. Thanks for taking the time to go through these and happy to dig deeper into any of the points if needed.

Thanks!

Majority of the DIsk IO for small block size is more than one cluster mapping. THis PR acutlaly does almost 90% of the sync read. What benefit can we get from this partially implemented Async IO?

Thanks for looking at this.

Could you share how you arrived at the 90% sync figure? I would like to understand the reasoning, because the math I get points in the opposite direction.

The fast path condition is - single allocated cluster mapping covering the full request. The default QCOW2 cluster size is 64K. With 4K block I/O, a read crosses a cluster boundary only when its starting offset falls in the last 4095 bytes of a cluster, that is 4095/65536 = ~6.25% of random offsets. So about 94% of random 4K reads land entirely within one cluster and, if allocated, take the async io_uring path. With smaller block sizes it gets even better - 512 byte I/O only crosses a boundary at 511/65536 = ~0.78% of offsets, so over 99% of those reads hit the fast path. Smaller block sizes make the async path more likely, not less. For aligned sequential reads, 4K divides evenly into 64K clusters, so 100% likely take the async path.

The sync fallback is not about block size. It fires for unallocated clusters, compressed clusters, backing file reads, and writes. Those are image structure properties. On a warmed up uncompressed QCOW2 without backing, virtually all clusters are allocated, so virtually all reads go through io_uring.

Even with only async reads landed so far, the performance results in the PR description already show measurable gains for uncompressed QCOW2 workloads. Those gains will only grow as the remaining sync paths move to io_uring in follow up cycles.

Regarding the "partially implemented" aspect - this is Cycle 1 of the refactoring plan tracked in #7560. The follow up cycles cover async writes, async backing file reads, and async compressed cluster reads. Each of those adds substantial complexity and is scoped separately intentionally, not because it was overlooked.

Thanks

Majority of the DIsk IO for small block size is more than one cluster mapping. THis PR acutlaly does almost 90% of the sync read. What benefit can we get from this partially implemented Async IO?

Thanks for looking at this.

Could you share how you arrived at the 90% sync figure? I would like to understand the reasoning, because the math I get points in the opposite direction.

The fast path condition is - single allocated cluster mapping covering the full request. The default QCOW2 cluster size is 64K. With 4K block I/O, a read crosses a cluster boundary only when its starting offset falls in the last 4095 bytes of a cluster, that is 4095/65536 = ~6.25% of random offsets. So about 94% of random 4K reads land entirely within one cluster and, if allocated, take the async io_uring path. With smaller block sizes it gets even better - 512 byte I/O only crosses a boundary at 511/65536 = ~0.78% of offsets, so over 99% of those reads hit the fast path. Smaller block sizes make the async path more likely, not less. For aligned sequential reads, 4K divides evenly into 64K clusters, so 100% likely take the async path.

The sync fallback is not about block size. It fires for unallocated clusters, compressed clusters, backing file reads, and writes. Those are image structure properties. On a warmed up uncompressed QCOW2 without backing, virtually all clusters are allocated, so virtually all reads go through io_uring.

Even with only async reads landed so far, the performance results in the PR description already show measurable gains for uncompressed QCOW2 workloads. Those gains will only grow as the remaining sync paths move to io_uring in follow up cycles.

Regarding the "partially implemented" aspect - this is Cycle 1 of the refactoring plan tracked in #7560. The follow up cycles cover async writes, async backing file reads, and async compressed cluster reads. Each of those adds substantial complexity and is scoped separately intentionally, not because it was overlooked.

Thanks

I misunderstood your code, thanks for the clarification. How about the block size is 1MB?

Majority of the DIsk IO for small block size is more than one cluster mapping. THis PR acutlaly does almost 90% of the sync read. What benefit can we get from this partially implemented Async IO?

Thanks for looking at this.
Could you share how you arrived at the 90% sync figure? I would like to understand the reasoning, because the math I get points in the opposite direction.
The fast path condition is - single allocated cluster mapping covering the full request. The default QCOW2 cluster size is 64K. With 4K block I/O, a read crosses a cluster boundary only when its starting offset falls in the last 4095 bytes of a cluster, that is 4095/65536 = ~6.25% of random offsets. So about 94% of random 4K reads land entirely within one cluster and, if allocated, take the async io_uring path. With smaller block sizes it gets even better - 512 byte I/O only crosses a boundary at 511/65536 = ~0.78% of offsets, so over 99% of those reads hit the fast path. Smaller block sizes make the async path more likely, not less. For aligned sequential reads, 4K divides evenly into 64K clusters, so 100% likely take the async path.
The sync fallback is not about block size. It fires for unallocated clusters, compressed clusters, backing file reads, and writes. Those are image structure properties. On a warmed up uncompressed QCOW2 without backing, virtually all clusters are allocated, so virtually all reads go through io_uring.
Even with only async reads landed so far, the performance results in the PR description already show measurable gains for uncompressed QCOW2 workloads. Those gains will only grow as the remaining sync paths move to io_uring in follow up cycles.
Regarding the "partially implemented" aspect - this is Cycle 1 of the refactoring plan tracked in #7560. The follow up cycles cover async writes, async backing file reads, and async compressed cluster reads. Each of those adds substantial complexity and is scoped separately intentionally, not because it was overlooked.
Thanks

I misunderstood your code, thanks for the clarification. How about the block size is 1MB?

With 1MB I/O and 64K clusters, a single request spans 16 clusters, so it produces multiple mappings and takes the sync fallback. That is correct.

A couple of things to consider:

• Cloud Hypervisor supports all cluster sizes from 9 to 21 bit as defined by the QCOW2 spec, with default 16 bit. With larger clusters, more I/O lands in the fast path. For example with 1MB clusters, a 1MB aligned read is a single mapping and goes through io_uring.
• The tradeoff of larger clusters is the increase of the internal fragmentation and COW amplification on partial writes, so the default 64K is a reasonable middle ground for most workloads.
• The current batching batches across virtio requests, not across cluster mappings within a single request. When one read spans 16 clusters, those 16 preadv ops are still synchronous. Submitting them to io_uring as a batch can be done as a follow up.
• Adjacent allocated mappings could also be coalesced into a single preadv spanning multiple clusters, avoiding the per cluster overhead entirely. The metadata layer already returns mappings in guest address order, so detecting contiguous host offsets should be straightforward.

This is the initial async implementation. Not all possible optimizations are in place yet and I am sure this will be a returning point for multiple rounds of improvements. The performance data in the description was measured with the default 64K cluster size and standard fio workloads. The gains reflect the common case where the majority of reads hit the fast path.

As a side note - QCOW2 performance tuning guide would also be a good topic to address in the future. I'll make a note to do some writing, when time permits.

Thanks

Would it make sense to use something more scalable than RwLock?

Would it make sense to use something more scalable than RwLock?

Good question. RwLock is fundamental to the current QCOW metadata design. std::sync::RwLock is the conservative choice here, providing poisoning tracking for free across several complex multistep mutation paths in metadata.

parking_lot::RwLock would have lower overhead but drops poisoning and adds an external dependency. Could be revisited if contention shows up at higher queue counts.

If you have a specific alternative in mind, I'd be interested to hear it.

Thanks

If you have a specific alternative in mind, I'd be interested to hear it.

I was thinking of a read-optimized design, like RCU, hazard pointers, or sharded locking. These avoid needing atomic operations in the read path (no metadata change) at the expense of slowing down the write path. I believe that RwLock bottlenecks at less than 2 million IOPS due to cache line ping-pong.

If you have a specific alternative in mind, I'd be interested to hear it.

I was thinking of a read-optimized design, like RCU, hazard pointers, or sharded locking. These avoid needing atomic operations in the read path (no metadata change) at the expense of slowing down the write path. I believe that RwLock bottlenecks at less than 2 million IOPS due to cache line ping-pong.

Please note that we at Cyberus Technology have a comprehensive benchmarking infrastructure (fio throughput, IOPS) including visualizations in Grafana - we might could help investigating such optimizations

If you have a specific alternative in mind, I'd be interested to hear it.

I was thinking of a read-optimized design, like RCU, hazard pointers, or sharded locking. These avoid needing atomic operations in the read path (no metadata change) at the expense of slowing down the write path. I believe that RwLock bottlenecks at less than 2 million IOPS due to cache line ping-pong.

The suggestions are ambitious but for sure interesting.

RCU is a natural fit for the read path conceptually, the L2 lookup is a pure shared read. The difficulty is on the write side. Today the metadata mutations are already multistep (allocate cluster, update L2, adjust refcounts, evict cache, grow L1), and some are recursive. Beyond that, QCOW2 features like snapshots, dirty bitmaps, and extended L2 entries would add more structures that must stay consistent with each other under writes.

Adapting those to RCU grace periods or hazard pointer retire semantics means the "copy" side needs to capture a consistent view across multiple interdependent structures, not just swap one pointer. That is substantial additional complexity. That's also why I chose clearly simplicity of one coarse lock vs. fine grained multiple locks/mutexes. As in, which object should be protected, when a single operation touches 5 structures? :)

On the 2M IOPS ceiling, the lock protects only the metadata lookup (two array indexes + a cache map get), not the data I/O itself. The actual read goes through io_uring after the lock is released. Per op hold time is very short. Whether the lock becomes the bottleneck at higher queue counts is worth profiling before committing to a more complex scheme.

@phip1611 that benchmarking infrastructure sounds useful for validating whether metadata locking is the limiter at scale. Happy to collaborate on that.

Worth investigating as a separate effort once the async refactoring is complete and there is a stable baseline to measure against.

Thanks

I was thinking of a read-optimized design, like RCU,

I think this out of scope here. Is there a follow-up ticket?

I was thinking of a read-optimized design, like RCU,

I think this out of scope here. Is there a follow-up ticket?

Yep, it's out of scope here as it's another broad refactoring on top of this one, once complete. On my side also there's no plan targeting a change in the locking model, but rather to focus on the features made possible thanks to the separated metadata I/O and further optimizations upon the initial implementation.

Thanks

Code-wise, looking good. thanks for your work on that! It is hard for me to verify the io_uring/async specific parts. Could you add a unit test or an integration test? this would give me more confidence for an approval

thanks for your great work!

Thanks for the thorough reviews!

I've added async implementation specific unit tests, covering the most important situations like read/write roundtrip, cross cluster boundary, mixed batch requests, etc. Probably more unit tests can be written as it goes and certainly more to come in the form of micro benchmarks.

For the integration tests - the existing integration tests already cover QcowAsync, since io_uring is available. That's the pattern used for RAW as well - if io_uring is available, it's preferred. It was also the reason I haven't done more integration tests. Otheriwese, there are some ideas, I'm going to add more during the further optimizations (async writes, decompression rework, and much more).

Thanks!

This PR adds partial Async support, shouldn't the PR description/ subject mention that?

This PR adds partial Async support, shouldn't the PR description/ subject mention that?

I've added an "Async scope" section to the description that breaks down which paths are async and which remain synchronous, with the rationale for each.

That said, "partial" is nuanced here. The backend implements the full AsyncIo trait and every operation goes through the async interface. What varies is the internal path - not all I/O can go through io_uring, so it will never be 100% io_uring. What can move to io_uring in follow up cycles is planned through the RFC.

Thanks

I have followed this PR through multiple review rounds. At this point, though, given the substantial qcow2 and io_uring involvement, I do not feel confident enough to give it a safe approval. That said, the code has improved a lot since the initial version.

Thanks for the thorough reviews across all the rounds, the feedback has genuinely improved the code. Completely understand the hesitation given the qcow2 and io_uring depth.

The async backend already shows measurable gains on the read side, particularly for random reads, multi queue workloads, and backing file reads. The existing RAW async backend using the same io_uring patterns has been running in production for a while now. The QCOW2 side follows those established patterns closely, and the unit tests cover the key paths. The integration tests also exercise this backend transparently on any io_uring capable host.

That said, if there are specific areas that feel uncertain, happy to add more targeted tests or documentation to build confidence. Let me know what would be most useful.

Thanks

@weltling I don’t think this PR makes the existing situation worse. However, it does make fixing it more difficult. In particular, it’s much better to have a single, safe abstraction around io_uring (and possibly AIO), rather than trying to reimplement them.

If any of the APIs in #7971 aren’t suitable for your needs, please let me know. My goal is that all disk I/O in Cloud Hypervisor should go through them.

@weltling I don’t think this PR makes the existing situation worse. However, it does make fixing it more difficult. In particular, it’s much better to have a single, safe abstraction around io_uring (and possibly AIO), rather than trying to reimplement them.

If any of the APIs in #7971 aren’t suitable for your needs, please let me know. My goal is that all disk I/O in Cloud Hypervisor should go through them.

@DemiMarie thanks for the detailed review and for linking #7955 and #7971.

Both points are valid. The AsyncIo trait taking &[libc::iovec] in a safe function is a design issue in the trait itself, not introduced by this PR, but that doesn't make it less of a concern. And the guest memory lifetime concern is real - if io_uring holds references to guest memory that gets unmapped, that's unsound regardless of how unlikely the trigger is today.

I've looked at #7971 and the approach of tracking in-flight operations by user_data and holding a guest memory snapshot until the CQE is reaped makes sense. Those APIs will also be helpful when implementing async writes
for QCOW2. I'm going for some more detailed checks on those PRs.

This PR currently follows the same pattern as the existing RawFileAsync backend, which #7971 also fixes. The sequencing between the two PRs needs to be coordinated - whichever lands last will need to adapt and rebase on top of the other. Happy to rebase this one on top of #7971 once it lands. If this one happens to go first, the adaptation would be on the #7971 side.

Thanks

@DemiMarie to note here also - wrapping RawFileAsync isn't directly applicable here because QCOW2 reads involve address translation before the io_uring read. QcowAsync does its own sq.push() at the resolved host offset after the metadata lookup; it's not a pass-through to a raw file. That said, if #7971 exposes safe primitives for io_uring submission with guest memory lifetime tracking, so QcowAsync can adopt those instead of calling io_uring::IoUring directly.

Thanks

@weltling What if BatchRequestInner allowed setting its offset and decreasing its size? That would allow RawFileAsync to be used.