Skip to content

refactor(blk): offload virtqueue drain to worker thread - #95

Open
agicy wants to merge 8 commits into
syswonder:mainfrom
agicy:refactor-blk-worker
Open

refactor(blk): offload virtqueue drain to worker thread#95
agicy wants to merge 8 commits into
syswonder:mainfrom
agicy:refactor-blk-worker

Conversation

@agicy

@agicy agicy commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR refactors the virtio-blk worker threading model to eliminate the intermediate producer-consumer queue between the main thread and the I/O worker thread.

Before

Guest kicks virtqueue
       ↓
  main thread (epoll)
       ↓
  parse descriptors → push to procq (TAILQ, mutex-protected)
       ↓
  worker thread → pop from procq → perform I/O → update used ring → inject IRQ
  • The main thread parsed every descriptor chain and pushed blkp_req structs onto a cross-thread queue (procq).
  • The worker thread popped from the queue, performed disk I/O, and injected IRQs.
  • Both threads contended on procq's mutex for every single I/O request.

After

Guest kicks virtqueue
       ↓
  main thread → signal condvar (no virtqueue access)
       ↓
  worker thread → drain virtqueue directly → perform I/O → update used ring → inject IRQ
  • The main thread only signals the worker via pthread_cond_signal — it never touches the virtqueue.
  • The worker thread owns the virtqueue exclusively: it parses descriptors, performs I/O, adjusts the used ring, and injects IRQs, all from a single thread.
  • Synchronization is reduced to one signal/wakeup per guest notification (batch), rather than per-request lock/unlock on the procq.

Performance

Test setup: ramdisk backend, fio + io_uring, 120s runtime, RK3588.

Metric Old New Change
IOPS (4k rand read, qd=64) 89.0k 93.7k +5.3%
Throughput (1M seq read, qd=16) 3831 MiB/s 4026 MiB/s +5.1%
P99 tail latency (4k rand, qd=64) 906 us 775 us -14.5%
P99.99 tail latency (4k rand, qd=64) 113.8 ms 76.0 ms -33.2%

Bug Fixes

While refactoring, several pre-existing issues were fixed:

  • Crash on early close: Guard pthread_join so virtio_blk_close doesn't join an uninitialized thread handle if the worker was never started.
  • Incorrect written_len on read error: When preadv fails, written_len no longer carries -1 into the used-ring update (was reporting 0 bytes despite the status byte being written).
  • Missing written_len for GET_ID: Now reports correct data length via the used ring so the guest sees the full device ID string.
  • close(-1) on open failure: Removed spurious close(-1) in the error path when open() fails.
  • Block device size detection: When fstat returns st_size = 0 (real block devices), fall back to ioctl(BLKGETSIZE64).

@agicy
agicy force-pushed the refactor-blk-worker branch 2 times, most recently from 10eebf0 to f90ae43 Compare June 9, 2026 12:54
@agicy
agicy requested a review from li041 June 9, 2026 12:56
@agicy
agicy marked this pull request as ready for review June 9, 2026 12:56
@agicy
agicy requested a review from dallasxy July 1, 2026 06:48
agicy added 8 commits July 31, 2026 09:07
Add NULL checks for vdev and dev to prevent crashes on
misconfigured or partially-initialized devices.  Guard
close(img_fd) so we only close valid file descriptors.
When open(2) fails, img_fd is -1. Calling close(-1) is harmless
(returns EBADF) but semantically wrong. Just remove it.
Extract the queue-empty check into a proper inline function in
virtio.h.  Uses __atomic_load_n with __ATOMIC_ACQUIRE on
avail_ring->idx so that a consumer seeing a new idx also observes
all descriptor writes the guest made before writing idx.

virtqueue_is_empty() in virtio.c now delegates to vq_is_empty().
Advertise VIRTIO_BLK_F_FLUSH and handle VIRTIO_BLK_T_FLUSH requests
via fdatasync(2) on the backing image.  This allows the guest to
persist previously completed writes to stable storage.
Add pre-allocated iovec arrays to BlkDev so the hot path can avoid
malloc/free for descriptor parsing.  Add thread_started flag for
safe worker lifecycle management.  Change close from int to bool.
Add start_blk_worker() declaration.
Add blk_do_read, blk_do_write, blk_do_get_id, and blk_complete as
static functions encapsulating the per-operation I/O logic and
used-ring update.  Not wired in yet — the existing blkproc and
complete_block_operation are unchanged.  These will replace them
in the next commit.
Replace the two-phase architecture (parse in main thread, enqueue
to procq, dequeue in worker, dispatch) with a single-phase design
where the worker thread owns the virtqueue exclusively:

- Rewrite virtq_blk_handle_one_request() to parse, validate,
  dispatch, and complete in one pass using process_descriptor_chain_buf
  with pre-allocated buffers — zero heap allocations per request.
- Rewrite blkproc_thread() to drain the avail_ring directly via
  the standard virtio disable-notify / process / enable-notify loop.
- Simplify notify_handler() to just signal the worker via condvar.
- Advance last_avail_idx on descriptor chain parse failure to avoid
  spinning forever on corrupt descriptor chains.
- Validate header and status byte presence before dereferencing.

Remove the intermediate TAILQ/procq, struct blkp_req, and the old
parsing/dispatch functions no longer needed.
…worker

Use calloc with NULL check in init_blk_dev.  Check pthread_mutex_init
and pthread_cond_init return values with self-cleanup on failure.

Split start_blk_worker out of init_blk_dev so the worker thread is
only started after the backing image is successfully opened by
virtio_blk_init.  Add idempotency guard via thread_started flag.

Add BLKGETSIZE64 ioctl fallback in virtio_blk_init for real block
devices where st_size is zero.  Use SECTOR_BSIZE instead of magic
512 constant.

Update virtio_blk_close with thread_started guard so we only join
a thread that was actually created.

Update create_virtio_device in virtio.c to check init_blk_dev
return value and call start_blk_worker after virtio_blk_init.
@agicy
agicy force-pushed the refactor-blk-worker branch from f90ae43 to 9836392 Compare July 31, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant