kernel: TRIM/multiqueue, io_uring, TCP teardown, wakeup_one, ELF errors by gburd · Pull Request #1399 · cloudius-systems/osv

gburd · 2026-06-24T11:22:33Z

A batch of independent kernel- and block-layer changes found while running the ZFS and Crucible workloads. This branch bases directly on current master (3df7df7). Five commits, grouped below.

Block layer

block: TRIM/DISCARD support and multiqueue I/O (f9903dc).
- BIO_DISCARD: a new block-layer request type plumbed into virtio-blk's request descriptor (VIRTIO_BLK_T_DISCARD). Maps to ZFS's ZIO_TYPE_TRIM and to Crucible's protocol-level discard. Drivers without discard support return ENOTSUP; the caller falls back to overwrite-with-zero.
- Multiqueue: per-CPU queue dispatch in virtio-blk so a multi-vCPU guest submits I/O on multiple queues concurrently. Adds a scripts/run.py helper wiring QEMU's num-queues, a per-queue completion-notifier locking-race fix (the lock was released too early under preemption), and a device_delete_child return-type fix surfaced while threading the multi-queue teardown path.

Async I/O

fs: implement io_uring async I/O interface (df0962e).
io_uring(7) implementation backed by OSv's existing async-I/O plumbing. Supported opcodes: READ, WRITE, FSYNC, NOP, OPENAT, CLOSE, READV, WRITEV, POLL_ADD, POLL_REMOVE, TIMEOUT, ACCEPT, CONNECT, RECV, SEND. Submission/completion queues mmap'd into user memory; SQE/CQE layout matches the Linux 5.15 ABI. io_uring_setup/enter/register syscalls; SQ poll thread (IORING_SETUP_SQPOLL); linked SQEs (IOSQE_IO_LINK) and drain barriers (IOSQE_IO_DRAIN). tst-io_uring.cc covers each opcode, link semantics, drain ordering, and SQ-poll. Note: OSv's io_uring is not used by OpenJDK 21 virtual threads (JDK 21 still routes through epoll); native code can use it.

Kernel correctness

net: harden TCP net-channel fast path against connection teardown (f96d88f).
tcp_net_channel_packet() assumed SOCK_LOCK held and the inpcb still attached. Under connection churn a queued packet can be drained after in_pcbdetach() cleared inp_socket, or from the IRQ classifier path without SOCK_LOCK, and a packet can arrive after the tcpcb moved to CLOSED/TIME_WAIT -- tripping tcp_do_segment()'s state KASSERT and panicking. Re-resolve the socket in the callback and drop the packet if detached; acquire SOCK_LOCK if not already held (OSv's recursive mutex makes re-locking a cheap depth bump); skip segments for connections at/below LISTEN or in TIME_WAIT, matching the slow path. This fix is load-bearing for the Crucible driver's sustained network I/O.
synch: fix wakeup_one to honor empty equal_range result (f5a8ac5).
wakeup_one() tested the equal_range lower bound against _evlist.end(). When the channel is absent, equal_range returns an empty range whose first iterator points at the next-larger key, not end(), so the old guard woke an unrelated waiter and erased its node while leaving the intended sleeper stranded. Test first != second.
elf: include path and sizes in "executable too short" error (7a82add).
Report pathname, actual file size, and required length on a short ELF read so a truncated or mis-pathed binary is diagnosable.

Verification

Kernel compiles and links clean on GCC 14.3 / Boost 1.87 (./scripts/build image=empty, fresh loader.elf, RC=0).

Two block-layer features that the ZFS and Crucible drivers depend on: - BIO_DISCARD: a new request type plumbed through the block layer and into virtio-blk's request descriptor (VIRTIO_BLK_T_DISCARD). Maps to ZFS's ZIO_TYPE_TRIM and to Crucible's protocol-level discard. Drivers that don't support discard return ENOTSUP and the caller falls back to overwrite-with-zero. - Multiqueue: per-CPU queue dispatch in virtio-blk so that a 16-vCPU guest can submit I/O on 16 queues concurrently. Adds a scripts/run.py helper that wires up QEMU's num-queues option, a per-queue locking-race fix (the per-queue completion-notifier lock was being released too early under preemption), removed next_queue_idx (became unused after dispatch was hashed by CPU), and a sys/dev device_delete_child return-type fix that surfaced while threading the multi-queue tear-down path. Verified: tst-vblk-multiqueue, tst-zfs-trim, tst-zfs-direct-io.

Add an io_uring(7) implementation backed by OSv's existing async-I/O plumbing. Supported opcodes: READ, WRITE, FSYNC, NOP, OPENAT, CLOSE, READV, WRITEV, POLL_ADD, POLL_REMOVE, TIMEOUT, ACCEPT, CONNECT, RECV, SEND. Submission and completion queues are mmap'd into user memory; the SQE/CQE layout matches Linux 5.15 ABI. Surface: - io_uring_setup(), io_uring_enter(), io_uring_register() syscalls. - SQ poll thread (IORING_SETUP_SQPOLL) for kernel-side submission. - Linked SQEs (IOSQE_IO_LINK) and drain barriers (IOSQE_IO_DRAIN). Tests: tst-io_uring.cc, 14 sub-tests covering each opcode, link semantics, drain ordering, and SQ-poll mode. Registered in modules/tests/Makefile and zfs-tools/usr.manifest. Note: OSv's io_uring is not used by OpenJDK 21 virtual threads (JDK 21 still goes through epoll); see the cover letter for details.

tcp_net_channel_packet() assumed it ran with SOCK_LOCK held and the inpcb still attached to its socket. Under connection churn a queued packet can be drained after in_pcbdetach() cleared inp_socket, or from the IRQ classifier path that does not hold SOCK_LOCK, and a packet can arrive after the tcpcb has moved to CLOSED/TIME_WAIT -- which tripped tcp_do_segment()'s state KASSERT and panicked the kernel. Re-resolve the socket inside the callback and drop the packet if the inpcb has detached; acquire SOCK_LOCK if we do not already own it (OSv's recursive mutex makes re-locking a cheap depth bump); and skip segments for connections at or below LISTEN or in TIME_WAIT, matching what the slow path (tcp_input) does before reaching tcp_do_segment.

wakeup_one() tested the equal_range lower bound against _evlist.end() to decide whether a waiter existed for the channel. When the channel is absent, equal_range returns an empty range whose first iterator points at the next-larger key rather than end(), so the old guard woke an unrelated waiter and erased its node while leaving the intended sleeper stranded. Test first != second so an absent channel is a no-op.

When an ELF read runs past the end of the file, report the pathname, actual file size, and the required length instead of a bare message, so a truncated or mis-pathed binary is diagnosable from the failure.

wkozaczuk · 2026-06-27T18:27:35Z

Hi,

Would you mind breaking this PR into 5 separate ones corresponding to the commits? It makes it easier to review and work on them independently.

Thanks,
Waldek

wkozaczuk

Most of it looks great to me except the mq/discard commit where I left most comments. Also I am asking to to split it into 5 separate PRs.

wkozaczuk · 2026-06-27T18:31:58Z

I know that the discard and multi-queue changes are somewhat intertwined, or at least touch the same areas of the code. But logically, I think, this commit should be split into 2, which could still be part of the same PR.

Done. Discard and multiqueue are now in #1400, kept in a single PR since they touch the same regions of virtio-blk.cc, but the docs (block-discard.md, block-multiqueue.md) and the change are organized so each concern is independently reviewable.

wkozaczuk · 2026-06-27T18:57:35Z

 {
    printf("Implement me, line %s:%d\n",__FILE__,  __LINE__);
-    return NULL;
+    return 0;


Is this related to this part of the comment above, and if so, which part?

a per-queue locking-race fix (the per-queue completion-notifier lock was being released too early under preemption), removed next_queue_idx (became unused after dispatch was hashed by CPU), and a sys/dev device_delete_child return-type fix that surfaced while threading the multi-queue tear-down path.

Not related — it was an orphan change. device_delete_child() returns int but had return NULL;; virtio-blk never calls it (its destructor is a no-op TODO), and the only in-tree caller is xenbusb.cc. I removed the hunk entirely; it is not present in #1400.

wkozaczuk · 2026-06-27T19:05:10Z

+#include <vector>
+#include <errno.h>
+
+int blk_mq_init_tag_set(struct blk_mq_tag_set* set)


Are blk_mq_init_tag_set() and other functions in this module part of the C API intended to be used by the applications that want to take advantage of the multi-queue support? Does it mean that apps using standard libc functions like read()/write() will not benefit from your changes? Or will they, because of the changes to virtio-blk.cc, but to a lesser extent?
Or will some of these functions be used by the drivers you will add in future requests?

These were dead code. blk-mq.cc/blk_mq.h were not referenced by any driver, test, or app, and read()/write() already benefit from multiqueue purely via virtio-blk.cc (queue selected by submitting CPU). Rather than document/lock a speculative, unused API, I deleted the module; it is not present in #1400. If a future driver needs a generic MQ layer we can add it then, against a real consumer.

wkozaczuk · 2026-06-27T19:09:52Z

+        return -EINVAL;
+    }
+
+    set->queue_map.resize(set->nr_hw_queues);


Should any of this be protected by a lock? I saw a lock mutex field in one of the structures.

Same as above — this was unused scaffolding and has been removed in #1400, so there is no API to lock. The real multiqueue locking lives in virtio-blk.cc, where each queue has its own mutex in _queue_locks.

wkozaczuk · 2026-06-27T19:13:52Z

+    return set->queue_map[queue_idx];
+}
+
+int blk_mq_submit_bio(struct blk_mq_tag_set* set, struct bio* bio)


So maybe blk_mq_submit_bio() is an app API, and other functions in these modules are used by the driver?
Can we add some comments explaining the intention behind these functions, and maybe something in the beginning of this file?

There was no app or driver consumer of any blk_mq_* function — it was speculative. The module is deleted in #1400; the multiqueue support that actually matters is the per-CPU queue selection in virtio-blk.cc.

wkozaczuk · 2026-06-27T19:56:37Z

 /zfs.so: zfs.so
 /libzfs.so: libzfs.so
 /libuutil.so: libuutil.so
+/libtpool.so: libtpool.so


Again seems like an unrelated change.

Correct, unrelated. Reverted — the change is not present in the io_uring PR (#1401).

wkozaczuk · 2026-06-27T19:59:53Z

+ * socket (in_pcbdetach sets inp_socket to NULL) between when the
+ * net_channel was registered and when this callback runs.
+ *
+ * Defensive shape:


Could #936 be related?

As I understand, this affects the slow-path only, right?

I am re-reading this Wiki page - https://github.com/cloudius-systems/osv/wiki/Networking-Stack#net-channels-slow-path-vs-fast-path - I wrote some time ago, and I believe the part starting with this is the most relevant to understand your changes:

"The post_packet() pushes an mbuf onto the net channel only if one exists. But when does a net channel get created?"

Plausibly, yes — I'll reference #936 in the PR. Note this is not a slow-path change; it's the opposite. It hardens the net-channel fast path (tcp_net_channel_packet) so it mirrors what the slow path (tcp_input) already does: the slow path drops segments for CLOSED/LISTEN/TIME_WAIT before tcp_do_segment(), but the fast-path callback did not, so under connection churn a packet could be queued on the channel after the tcpcb went CLOSED and trip the KASSERT in tcp_do_segment(). Two fixes: (1) re-resolve so in the callback and bail if the inpcb was detached (inp_socket == NULL), since the callback can run from the IRQ-side classifier path without SOCK_LOCK; (2) reject state <= TCPS_LISTEN || state == TCPS_TIME_WAIT, matching the slow path. This guards exactly the lifetime window where the channel outlives the socket detach. Now in #1402.

wkozaczuk · 2026-06-27T20:18:27Z

 	tcp_do_segment(m, th, so, tp, drop_hdrlen, tlen, iptos, TI_UNLOCKED, want_close);
+	if (taken) {
+		SOCK_UNLOCK(so);
+	}


I think it looks all correct.

Thanks — no change. Carried as-is into #1402.

wkozaczuk · 2026-06-27T20:25:44Z

    auto ppp = _evlist.equal_range(chan);
-    auto it = ppp.first;
-    if (it != _evlist.end()) {
+    if (ppp.first != ppp.second) {


So this is to make sure we do not try to process "empty" range represented by ppp?

Exactly. equal_range returns [first, second); an empty result has first == second, but first is not necessarily end() (it points at the first element greater than chan), so the old if (it != _evlist.end()) could dereference a non-matching element. The fix tests ppp.first != ppp.second, matching how the range is correctly consumed elsewhere in the file. Now in #1403.

wkozaczuk · 2026-06-27T20:26:47Z

    // file is not long enough. So we need to check first.
-    if (::size(_f) < offset + size) {
-        throw osv::invalid_elf_error("executable too short");
+    auto fsize = ::size(_f);


Looks good!

Thanks — carried as-is into #1404.

gburd · 2026-06-29T14:35:56Z

Thanks for the thorough review. Per your structural request, I've split this into one PR per subsystem and addressed every inline comment (replies posted in-thread above). Closing this in favor of:

block: TRIM/DISCARD support and virtio-blk multiqueue I/O #1400 — block: TRIM/DISCARD + virtio-blk multiqueue (discard and MQ kept together since they touch the same code; speculative blk-mq.cc scaffolding deleted; orphan bus.h hunk removed)
fs: implement io_uring async I/O interface #1401 — fs: io_uring (Linux 5.15 ABI on OSv primitives; usr.manifest change reverted; lzloader split out)
net: harden TCP net-channel fast path against connection teardown #1402 — net: harden TCP net-channel fast path against connection teardown (mirrors the slow-path CLOSED/LISTEN/TIME_WAIT drops; likely related to page_fault on lockfree::mutex::lock() #936)
synch: fix wakeup_one to honor empty equal_range result #1403 — synch: fix wakeup_one empty equal_range handling
elf: include path and sizes in "executable too short" error #1404 — elf: richer "executable too short" error
lzloader: give .rodata its own output section #1405 — lzloader: give .rodata its own output section (the unrelated linker-script change)

Each split branch was build-qualified (kernel compile+link, image=empty) on a binutils 2.44 / g++ 14.3.0 host before pushing.

gburd added 5 commits June 24, 2026 06:29

elf: include path and sizes in "executable too short" error

7a82add

When an ELF read runs past the end of the file, report the pathname, actual file size, and the required length instead of a bare message, so a truncated or mis-pathed binary is diagnosable from the failure.

wkozaczuk reviewed Jun 27, 2026

View reviewed changes

gburd closed this Jun 29, 2026

Uh oh!

Uh oh!

Conversation

gburd commented Jun 24, 2026

Block layer

Async I/O

Kernel correctness

Verification

Uh oh!

wkozaczuk commented Jun 27, 2026

Uh oh!

wkozaczuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gburd commented Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants