block: TRIM/DISCARD support and virtio-blk multiqueue I/O - #1400
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds two VirtIO block-device capabilities to OSv: DISCARD/TRIM support (via a new BIO_DISCARD command and BLKDISCARD ioctl) and virtio-blk multiqueue I/O (negotiating VIRTIO_BLK_F_MQ, selecting a queue per CPU, and adding per-queue completion threads), along with documentation and QEMU runner wiring.
Changes:
- Add
BIO_DISCARDand aBLKDISCARDioctl path that submits discard bios to block devices. - Extend virtio-blk to negotiate and handle DISCARD requests and multiqueue request submission/completion.
- Add
scripts/run.pysupport for configuring virtio-blknum-queues, plus docs for DISCARD and multiqueue.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run.py | Adds a CLI flag and QEMU device-suffix wiring for virtio-blk num-queues. |
| include/osv/bio.h | Introduces BIO_DISCARD and renumbers bio command constants. |
| include/api/sys/mount.h | Adds BLKDISCARD ioctl definition. |
| drivers/virtio-blk.hh | Adds feature bits/config fields for DISCARD+MQ and request plumbing structs. |
| drivers/virtio-blk.cc | Implements multiqueue submission/completion and DISCARD request translation. |
| drivers/blk-common.cc | Adds BLKDISCARD ioctl handling via a discard bio submission. |
| docs/block-multiqueue.md | Documents virtio-blk multiqueue behavior and configuration. |
| docs/block-discard.md | Documents DISCARD/TRIM design and usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define BIO_DISCARD 0x20 /* Space reclamation (TRIM) */ | ||
| #define BIO_SCSI 0x40 | ||
| #define BIO_CMD1 0x80 /* Available for local hacks */ | ||
| #define BIO_CMD2 0x100 /* Available for local hacks */ |
There was a problem hiding this comment.
Wow, the AI found all the same problems that I found, and then some. I think I can retire :-)
There was a problem hiding this comment.
Resolved in the current head (da5595c). BIO_DISCARD is defined as 0x80 (bio.h:60), which fits in the uint8_t bio_cmd — there is no 0x100/BIO_CMD2 value in this PR anymore. The comment was against an earlier revision that used 0x100; that approach was dropped precisely because of the 8-bit truncation.
| auto* bio = alloc_bio(); | ||
| bio->bio_dev = dev; | ||
| bio->bio_done = destroy_bio; | ||
| bio->bio_cmd = BIO_DISCARD; | ||
| bio->bio_offset = range[0]; | ||
| bio->bio_bcount = range[1]; | ||
|
|
||
| dev->driver->devops->strategy(bio); | ||
| bio_wait(bio); | ||
| } |
There was a problem hiding this comment.
Resolved in da5595c (blk-common.cc:45-61). The BLKDISCARD path no longer sets bio_done, so bio_wait() is woken normally; destroy_bio(bio) is called after bio_wait() returns, and the wait result is propagated as the ioctl return value. This matches your suggested fix exactly.
| // Step 7 - generic init of virtqueues | ||
| probe_virt_queues(); | ||
|
|
||
| //register the single irq callback for the block | ||
| sched::thread* t = sched::thread::make([this] { this->req_done(); }, | ||
| sched::thread::attr().name("virtio-blk")); | ||
| t->start(); | ||
| auto queue = get_virt_queue(0); | ||
|
|
||
| interrupt_factory int_factory; | ||
| // Resize per-queue lock vector now that _num_queues is known. | ||
| _queue_locks = std::vector<mutex>(_num_queues); |
There was a problem hiding this comment.
Resolved in da5595c (virtio-blk.cc:131-141). After probe_virt_queues(), we set _num_queues = virtio_driver::_num_queues so the blk count is synced to the actually-initialized virtqueue count; the per-queue lock vector is then sized from that same value. qid computation and ring indexing now use one consistent count.
| t->start(); | ||
| if (qid == 0) { |
There was a problem hiding this comment.
Resolved in da5595c (virtio-blk.cc:146-147). set_use_indirect(true) is now called in a loop over every initialized queue (for qid 0.._num_queues-1), not just queue 0.
| case BIO_DISCARD: | ||
| if (!get_guest_feature_bit(VIRTIO_BLK_F_DISCARD)) { | ||
| biodone(bio, false); | ||
| return EOPNOTSUPP; | ||
| } | ||
| type = VIRTIO_BLK_T_DISCARD; | ||
| break; |
There was a problem hiding this comment.
Resolved in da5595c (virtio-blk.cc:400-404). Before building the discard descriptor we reject bio_bcount==0 and any bio_offset/bio_bcount not a multiple of sector_size with EINVAL, so no silent round-down of the discard range.
| WITH_LOCK(_queue_locks[qid]) { | ||
| if (!drain_queue(myqueue)) { | ||
| // nothing processed; release lock before yielding | ||
| } else { | ||
| myqueue->wakeup_waiter(); | ||
| } | ||
| } | ||
| sched::thread::yield(); | ||
| } |
There was a problem hiding this comment.
Resolved in da5595c. The per-queue busy-poll thread is gone. There is now a single completion thread (blk::req_done, virtio-blk.cc:336) that sched::thread::wait_until()s on any_queue_not_empty() — an interrupt-driven sleep, not a poll. any_queue_not_empty() re-arms interrupts on every queue and re-checks to close the arrival race before returning false. No CPU is burned when idle.
| `scripts/run.py` exposes this through the `--block-queues`/`num-queues` | ||
| wiring so test images can request multiple queues without hand-editing the |
There was a problem hiding this comment.
Resolved in da5595c. The doc (now documentation/block-multiqueue.md:101) references --virtio-blk-queues, matching scripts/run.py:657. There is no --block-queues flag.
nyh
left a comment
nyh
left a comment
There was a problem hiding this comment.
Looks good, but I had some minor comments.
I have no idea how to actually test this code so I'll trust you about its functionality :-)
|
|
||
| interrupt_factory int_factory; | ||
| // Resize per-queue lock vector now that _num_queues is known. | ||
| _queue_locks = std::vector<mutex>(_num_queues); |
There was a problem hiding this comment.
nitpick: _queue_locks.resize(_num_queues) also does the same thing (and doesn't need the comment because the method name is resize).
There was a problem hiding this comment.
Kept as _queue_locks = std::vector<mutex>(_num_queues) rather than .resize() on purpose: OSv's mutex (include/lockfree/mutex.hh:65) has a user-declared destructor and no move/copy, so it is non-movable. vector::resize() from size 1 to N would have to move the existing element on reallocation, which does not compile for a non-movable element. Full reconstruction sidesteps that. I did drop the now-redundant comment.
| myqueue->wakeup_waiter(); | ||
| } | ||
| } | ||
| sched::thread::yield(); |
There was a problem hiding this comment.
This yield() will help when there are other threads that use CPU, but if there are none, this thread will run in a tight loop. Is this what we really want? Shouldn't it go to sleep for some time, to be woken by the wakeup_waiter() on queue 0's thread?
There was a problem hiding this comment.
Resolved in da5595c — this is exactly what you suggested. The completion thread now sleeps in wait_until(any_queue_not_empty()) and is woken by the shared block IRQ; it does not yield()-spin. On wake it drains every queue under that queue's lock and calls wakeup_waiter(). See virtio-blk.cc:311-350.
| #define BIO_DISCARD 0x20 /* Space reclamation (TRIM) */ | ||
| #define BIO_SCSI 0x40 | ||
| #define BIO_CMD1 0x80 /* Available for local hacks */ | ||
| #define BIO_CMD2 0x100 /* Available for local hacks */ |
There was a problem hiding this comment.
The last one is wrong - bio_cmd is 8 bits, so 0x100 is not available.
|
Done. I've split the single block commit into two, kept within this same PR as you suggested:
Each commit compiles and links standalone (bisect-safe), and the combined tree is byte-identical to the prior single commit. Build-qualified (kernel compile+link, image=empty). |
Who asked you to split it? I wasn't actually bothered that this one commit included both things, they were very easy to tell apart. Please take a look at the specific comments I raised in my review.
|
|
Thanks for the review (and to Copilot). All the concrete points from both of you are addressed in the current head (
On the I believe this is ready for another look — could you re-review when you have a moment? |
|
You caught me. Majority AI assisted on these features and PR comments, hope you're ok with that. These are features that I am using in a project that I am working towards. I hope to share it soon, it's only possible because of the force multiplier that AI is. In this case I needed to push the I/O subsystem forward a lot and opportunistically other things. |
That's fine :-) |
|
On Wed, Jul 1, 2026, at 7:31 AM, nyh wrote:
*nyh* left a comment (cloudius-systems/osv#1400) <#1400 (comment)>
> You caught me. Majority AI assisted on these features and PR comments, hope you're ok with that.
>
That's fine :-)
Thanks, I don't think it's doing a bad job of it. Thanks for spending human cycles on the reviews.
… —
Reply to this email directly, view it on GitHub <#1400?email_source=notifications&email_token=AAABWEAFRZ2FVWBZSKR55WL5CTY7VA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOBVGM4TOMRUGA4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4853972408>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABWEAYJI5RJWWHTVLTO6D5CTY7VAVCNFSNUABDKJSXA33TNF2G64TZHM3TENJYGY4DKO2JONZXKZJ3GQ3TMOJQGQZTQOJQUF3AE>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Thanks for the thorough review (and Copilot's). Every point is addressed in the current head da5595c; I've replied inline on each thread. Summary: Correctness fixes
Busy-poll → interrupt-driven (your main concern)
Nits
No new commits were needed beyond da5595c — these were all already in the pushed head; I'd simply never closed the review loop with replies. Ready for another look. |
|
Pushed b7c213b fixing the doc typo flagged inline: |
|
Ping on this. I believe I've addressed all the review feedback in-thread (replies posted above); the branch is rebased on current master and mergeable. Context for prioritization: this PR is one of three prerequisites (#1398, #1402) beneath the OpenZFS 2.4.3 cutover that replaces the c.2014 BSD-ZFS port. I'd like to submit that as a clean ZFS-only diff, which requires these three to land on master first. If anything here is still open, let me know and I'll turn it around quickly. |
| case BLKDISCARD: | ||
| { | ||
| if (!buf) { | ||
| return EINVAL; | ||
| } | ||
| u64* range = (u64*) buf; | ||
| auto* bio = alloc_bio(); | ||
| bio->bio_dev = dev; | ||
| bio->bio_cmd = BIO_DISCARD; | ||
| bio->bio_offset = range[0]; | ||
| bio->bio_bcount = range[1]; | ||
|
|
||
| dev->driver->devops->strategy(bio); | ||
| int ret = bio_wait(bio); | ||
| destroy_bio(bio); | ||
| return ret; |
There was a problem hiding this comment.
assert() that an operation isn't implemented is fine, we don't need to handle it.
The other cases described in this copilot comment - not calling biodone() or "multiplex_strategy" - I don't really understand. @gburd please take a look.
This may be the same problem that copilot found in the last review a week ago, just restated?
There was a problem hiding this comment.
Fixed in a1fa22e. Two distinct problems, both real:
-
Unsupported-discard hang. The generic BLKDISCARD ioctl (blk-common.cc:45-61) does strategy(bio) then bio_wait(bio). Drivers that don't implement BIO_DISCARD hit the default case of their bio_cmd switch and
return ENOTBLK-- but the strategy wrapper isvoid, so that errno is dropped and biodone() is never called, so bio_wait() hangs forever. Fixed by completing the bio with error in the default case of nvme-queue, ahci, ide and scsi-common (and virtio-blk itself). An unsupported discard now fails cleanly with EIO instead of hanging. (ramdisk already assert(0)s on unknown commands, which per Nadav's note is acceptable.) -
multiplex_strategy nullptr UB. kern_physio.cc splits a request larger than dev->max_io_size by doing
buf += req_sizeon buf = bio->bio_data. For a discard bio_data is nullptr while bio_bcount is non-zero, so a large discard triggered pointer arithmetic on nullptr. Fixed by forwarding discard bios whole (they carry no data payload); the driver enforces its own discard-size limit.
There was a problem hiding this comment.
You're right that the assert() cases are fine as-is. The real issue Copilot flagged (restated below) is separate from asserting, and it is not the same as last week's comment -- last week was about bio_done being set on the waiter; this is about drivers that never call biodone() at all.
The BLKDISCARD ioctl waits synchronously on bio_wait(). A driver that doesn't implement discard returns ENOTBLK from the default case of its bio_cmd switch, but the strategy wrapper is void so that errno is dropped and biodone() is never called -- so bio_wait() hangs. I fixed this in a1fa22e by completing the bio with error in the default case of nvme/ahci/ide/scsi (turning the hang into a clean EIO). ramdisk's assert(0) path is left as-is per your note.
The "multiplex_strategy" half is a genuine nullptr bug: for a large discard, multiplex_strategy would split the bio and do pointer arithmetic on bio_data, which is nullptr for a discard. Fixed by forwarding discard bios whole.
There was a problem hiding this comment.
Fixed in a1fa22e, both halves:
- Generic fast-fail for non-discard drivers. The default: case in every strategy/make_request path (nvme-queue.cc, ahci.cc, ide.cc, scsi-common.cc) now calls
biodone(bio, false)before returningENOTBLK, so an unsupported BIO_DISCARD completes with error and wakesbio_wait()instead of hanging. - multiplex_strategy nullptr split. kern_physio.cc now forwards a BIO_DISCARD whole rather than splitting on
dev->max_io_size, sincebio_datais nullptr and the size limit does not apply to a payload-less request. The driver enforces its own discard-size limit.
There was a problem hiding this comment.
Resolved in a1fa22e. Two parts:
- The default case of the bio_cmd switch in nvme, ahci, ide and scsi now completes the bio with error (biodone(bio, false)) instead of returning ENOTBLK without completing it, so an unsupported discard fails cleanly rather than hanging bio_wait().
- multiplex_strategy no longer splits a discard by pointer arithmetic on bio_data (which is nullptr for a discard): it forwards BIO_DISCARD whole and lets the driver enforce its own discard-size limit (fs/vfs/kern_physio.cc).
| if (get_guest_feature_bit(VIRTIO_BLK_F_SEG_MAX)) { | ||
| if (bio->bio_bcount/mmu::page_size + 1 > _config.seg_max) { | ||
| trace_virtio_blk_make_request_seg_max(bio->bio_bcount, _config.seg_max); | ||
| return EIO; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in a1fa22e. The SEG_MAX check is now applied only to VIRTIO_BLK_T_IN/OUT (the requests that actually add a data SG), after the bio_cmd switch has classified the request. FLUSH and DISCARD carry no data payload and are no longer subjected to a bio_bcount/page_size bound. On failure the bio is now completed with biodone(bio, false) before returning EIO, so a synchronous waiter (notably BLKDISCARD) is not left hung.
There was a problem hiding this comment.
Fixed in a1fa22e. The SEG_MAX check is now after the bio_cmd switch and gated on type == VIRTIO_BLK_T_IN || type == VIRTIO_BLK_T_OUT, so FLUSH and DISCARD (which add no data SG) skip it. On failure it now calls biodone(bio, false) before returning EIO, so a synchronous waiter (BLKDISCARD) no longer deadlocks.
There was a problem hiding this comment.
Resolved in a1fa22e (virtio-blk.cc). The VIRTIO_BLK_F_SEG_MAX segment-count check is now gated on (type == VIRTIO_BLK_T_IN || type == VIRTIO_BLK_T_OUT), i.e. only READ/WRITE requests that actually carry a data payload; FLUSH and DISCARD skip it. On the failure path it now calls biodone(bio, false) before returning EIO so a synchronously-waiting caller (BLKDISCARD) is woken instead of deadlocking.
| if (type == VIRTIO_BLK_T_DISCARD) { | ||
| req->discard_desc.sector = bio->bio_offset / sector_size; | ||
| req->discard_desc.num_sectors = bio->bio_bcount / sector_size; | ||
| req->discard_desc.flags = 0; | ||
| queue->add_out_sg(&req->discard_desc, sizeof(req->discard_desc)); |
There was a problem hiding this comment.
Fixed in a1fa22e. Before building the discard descriptor we now compute nsectors = bio_bcount/sector_size and reject the request with EINVAL if it exceeds the device's advertised max_discard_sectors (or, when the device advertises 0, the u32 field maximum). This bounds num_sectors to the u32 width and to the device limit, so we never silently truncate a large range or submit a request the device would reject.
There was a problem hiding this comment.
Fixed in a1fa22e. In the BIO_DISCARD case, before building the descriptor, num_sectors = bio_bcount / sector_size is validated against both the u32 field width and the advertised max_discard_sectors (falling back to UINT32_MAX when the device advertises 0). An oversized range fails the bio with biodone(bio, false) + EINVAL rather than truncating silently.
There was a problem hiding this comment.
Resolved in a1fa22e (virtio-blk.cc, BIO_DISCARD case). Before building the discard descriptor we compute nsectors = bio_bcount / sector_size and reject the request with EINVAL if it exceeds max_discard_sectors (or UINT32_MAX when the device advertises no limit), so it can neither overflow the u32 num_sectors field nor submit a range the device would reject. The bio is completed with biodone(bio, false) on rejection.
nyh
left a comment
nyh
left a comment
There was a problem hiding this comment.
Looks mostly good to me, but I still have one question and Copilot had a few more. Thanks.
| case BLKDISCARD: | ||
| { | ||
| if (!buf) { | ||
| return EINVAL; | ||
| } | ||
| u64* range = (u64*) buf; | ||
| auto* bio = alloc_bio(); | ||
| bio->bio_dev = dev; | ||
| bio->bio_cmd = BIO_DISCARD; | ||
| bio->bio_offset = range[0]; | ||
| bio->bio_bcount = range[1]; | ||
|
|
||
| dev->driver->devops->strategy(bio); | ||
| int ret = bio_wait(bio); | ||
| destroy_bio(bio); | ||
| return ret; |
There was a problem hiding this comment.
assert() that an operation isn't implemented is fine, we don't need to handle it.
The other cases described in this copilot comment - not calling biodone() or "multiplex_strategy" - I don't really understand. @gburd please take a look.
This may be the same problem that copilot found in the last review a week ago, just restated?
| void blk::req_done() | ||
| { | ||
| while (1) { | ||
| sched::thread::wait_until([this] { return this->any_queue_not_empty(); }); |
There was a problem hiding this comment.
Hmm, is this safe? This thread is not (if I understand correctly) pinned to any particular CPU, or run with interrupts disabled. So it is safe to call ring->disable_interrupts() and so on in this situation?
There was a problem hiding this comment.
Yes, this is safe. any_queue_not_empty() (and thus disable_interrupts()/enable_interrupts()) is called from inside the sched::thread::wait_until() predicate, which is exactly the same context the base-class virtio_driver::wait_for_queue() (drivers/virtio.cc:157-177) uses -- an unpinned thread with interrupts enabled. virtio-net's rx thread drives its queue the same way.
The key point is that vring::disable_interrupts()/enable_interrupts() are not CPU-local IRQ-masking operations. They only write to the vring's avail-ring flag and used_event field in guest memory (drivers/virtio-vring.cc:112-135) -- disable_interrupts() sets _avail->disable_interrupt(); enable_interrupts() sets the flag, updates the used_event index, and issues a seq_cst fence. There is no per-CPU interrupt state involved, so it doesn't matter which CPU the thread runs on and interrupts don't need to be disabled around it. The re-check-after-enable in any_queue_not_empty() closes the same arrival race that wait_for_queue() guards against.
There was a problem hiding this comment.
The vring::disable_interrupts() is also called from the isr routine invoked right after an MSIX interrupt is delivered (see interrupt_manager::easy_register() in msi.cc, so it may race with the any_queue_not_empty() called by the single consumer thread, no?. Now the vring::disable_interrupts()/enable_interrupts() seem to be reading and writing to the atomics, but overall I am not sure if it is all thread safe.
There was a problem hiding this comment.
I wonder if vring::disable_interrupts()/enable_interrupts() are thread-safe, because even before these changes, the mxi isr calling vring::disable_interrupts() could arrive on any cpu (right?, t was not pinned) and the t calling virtio_driver::wait_for_queue() which calls vring::disable_interrupts()/enable_interrupts() could be running on yet another cpu.
The vring_avail:disable_interrupt()/enable_interrupt() called by vring::disable_interrupts()/enable_interrupts() modifies single atomic:
void disable_interrupt() { _flags.store(VRING_AVAIL_F_NO_INTERRUPT, std::memory_order_relaxed); }
void enable_interrupt() { _flags.store(0, std::memory_order_relaxed); }
but vring::enable_interrupts() in addition modifies another atomic:
set_used_event(_used_ring_host_head, std::memory_order_relaxed);
But any_queue_not_empty() iterates over all vrings and reads/modifies the same atomic vring_avail::_flags that the isr routine running on random cpus may be writing to (calls vring::disable_interrupts() );
Is it a race? Maybe not, given isr wakes t after disabling interrupts. I am not 100% sure.
|
Addressed the three new Copilot findings in a1fa22e (already pushed to this branch):
Replies posted inline on each thread. Branch remains rebased on current master and mergeable. |
|
Friendly ping - this one is No rush, but flagging that this PR is the base of the storage stack: the OpenZFS 2.4.3 cutover (#1423) and Crucible driver (#1424) are blocked on it, and I'd like to rebase those to clean, reviewable diffs once it lands. Happy to address anything still outstanding. |
|
Rebased onto current master (the branch had drifted 22 commits behind — it predated the io_uring merge and the loader_options.ld / ENA Makefile reorg, so while GitHub showed it mergeable it would not have built cleanly against today's master; the stale Makefile reverts are now gone) and addressed the remaining Copilot comments:
Rebuilt clean and The earlier round of comments (BIO_CMD2/0x100, bio_done-on-wait, _num_queues sync, indirect-descriptors-per-queue, unaligned discard, busy-poll->interrupt-driven completion thread, sorted enum, doc flag name) all remain addressed as noted above. |
|
I have not reviewed it myself carefully yet, but after applying this PR, when I build a ZFS test image to run Connecting with gdb does not show anything obvious yet: |
|
The same test works just fine with nvme and sata: So clearly something is amiss with virtio-blk. |
|
When I force the number of queues to 1 like so: it does not hang any more. |
|
A have a general question: is is really possible to receive interrupts from single queue - 0 - yet operate on all available? When request placed on queue 2 is consumed by host and guest is notified will be delivered by an interrupt tied to that queue not queue 0? BTW when I also run with cpus = 1 it also works. |
|
You diagnosed it exactly. Answering your question first:
No, and that was the bug. With MSI-X the transport maps queue index i -> MSI-X entry i (1:1, Fix (folded into the multiqueue commit so the PR has no commit with the hang):
Validation: a new bounded regression test The change was also independently reviewed by two reviewers before commit (one focused on interrupt/concurrency/device-spec correctness, one on API/semantics/test quality); both approved, and their robustness note (fail-loud when queues > MSI-X vectors) is included above. Thanks for the sharp catch and the clean repro - it made the root cause obvious. |
wkozaczuk
left a comment
wkozaczuk
left a comment
There was a problem hiding this comment.
Your last change for sure fixes the hang issue. And the unit tests pass. But I have left some questions.
Also I wonder - how easy/difficult is it to tweak this PR to make each vring handled by separate dedicated thread pinned to a cpu?
| Two CPUs mapped to different queues never block each other here; two CPUs | ||
| mapped to the same queue (more vCPUs than queues) share that queue's lock. | ||
|
|
||
| ## Completion path |
There was a problem hiding this comment.
I do not think this description is correct in light of the latest changes, given that we use multiple vrings and multiple MSI vectors. I think we fully use all multiple queues (vrings), but we use a single thread that checks if the queues are not empty and consumes data received from host.
| void blk::req_done() | ||
| { | ||
| while (1) { | ||
| sched::thread::wait_until([this] { return this->any_queue_not_empty(); }); |
There was a problem hiding this comment.
The vring::disable_interrupts() is also called from the isr routine invoked right after an MSIX interrupt is delivered (see interrupt_manager::easy_register() in msi.cc, so it may race with the any_queue_not_empty() called by the single consumer thread, no?. Now the vring::disable_interrupts()/enable_interrupts() seem to be reading and writing to the atomics, but overall I am not sure if it is all thread safe.
Implement the VIRTIO_BLK_F_DISCARD feature so the guest can hand space reclamation (TRIM) requests down to the host. - Define BIO_DISCARD and renumber the trailing bio command flags so the new bit fits without colliding with BIO_SCSI/BIO_CMD1/BIO_CMD2. - Add the BLKDISCARD ioctl and route it through blk_ioctl() as a BIO_DISCARD strategy request. - Negotiate VIRTIO_BLK_F_DISCARD, read the discard config fields, and emit a VIRTIO_BLK_T_DISCARD request carrying a discard descriptor when the host advertises the feature. docs/block-discard.md documents the feature and how to exercise it.
Negotiate VIRTIO_BLK_F_MQ and spread block I/O across the per-device virtqueues the host advertises. - Read num_queues from the device config and size a per-queue mutex vector so each queue is an independent submission channel. The probed virtqueue count (virtio_driver::_num_queues) is used as the authoritative value so make_request()/req_done() never index a queue that was not set up. - make_request() selects a queue by CPU id, removing the single global submission lock as a cross-CPU contention point. - Completions: with MSI-X the transport maps queue index i -> MSI-X entry i (1:1, see virtio_pci_device::setup_queue), so each virtqueue raises its own interrupt vector. We register an ISR for every queue's vector; each ISR disables its own queue's interrupts and wakes a single completion thread, which then drains all queues under their per-queue locks. Draining and re-arming interrupts on every queue (not just queue 0) before sleeping closes the completion-wakeup race for all queues. (An earlier revision serviced only queue 0's interrupt, which hung any I/O steered to queues 1..N-1; a bounded regression test, tst-mq-smoke, exercises concurrent writes+reads across all queues and CPUs to guard against a recurrence.) On the non-MSI-X / MMIO path a single shared IRQ line fans into the same all-queue drain. - interrupt_manager::easy_register() gains a std::vector overload (both it and the initializer_list form forward to a common pointer+count helper, no copy) so a driver can register a runtime-sized, per-queue set of vectors. virtio-blk checks the return value and fails loudly at probe if the device exposes fewer MSI-X vectors than queues, rather than silently registering none and hanging. - scripts/run.py gains --virtio-blk-queues to set QEMU's num-queues. documentation/block-multiqueue.md documents the feature and the completion model.
The generic BLKDISCARD ioctl submits a BIO_DISCARD and waits on bio_wait(). Drivers that do not implement discard returned ENOTBLK from their bio_cmd switch without completing the bio, so the wait hung. Complete the bio with error in the default case of nvme, ahci, ide and scsi so an unsupported discard fails cleanly instead of hanging. multiplex_strategy split a request larger than max_io_size by doing pointer arithmetic on bio_data, which is nullptr for a discard (no data payload). Forward discard bios whole and let the driver enforce its own size limit. In virtio-blk, restrict the SEG_MAX segment-count check to READ/WRITE (the only requests with a data payload) and complete the bio on failure so a waiting caller is not left hung. Reject a discard whose sector count would overflow the u32 num_sectors field or exceed the advertised max_discard_sectors, rather than truncating and discarding the wrong range.
|
Thanks for the careful review, and glad the fix resolves the hang. On the thread-safety question (
|
|
Thanks for your thorough explanation. I will merge this PR as is. And yes, it would be nice to follow up with a PR to switch to a solution that uses multiple dedicated completion threads instead of a shared one. If you do not mind, I would like to take a stab at this. |
Split out of #1399 per review (one PR per subsystem).
This PR adds two related virtio-blk capabilities. They are kept together
because they touch the same regions of
drivers/virtio-blk.ccand share theper-request plumbing, but each concern is independently documented and
reviewable.
TRIM/DISCARD
BIO_DISCARD(0x20) bio command describing a byte range to reclaim.BLKDISCARDioctl (_IO(0x12, 119)) so in-guest code can issue a discardagainst an open block device;
blk_ioctl()builds the bio and submits it.VIRTIO_BLK_F_DISCARD, reads the discard limits fromdevice config, and translates a
BIO_DISCARDbio into aVIRTIO_BLK_T_DISCARD(11) command with a singleblk_discard_write_zeroesdescriptor. If the feature was not negotiated thebio is failed (not silently dropped) so callers can fall back.
docs/block-discard.md.Multiqueue
VIRTIO_BLK_F_MQ, readsnum_queues, and creates thatmany virtqueues. A request is steered to a queue by the submitting CPU id
(
qid = sched::cpu::current()->id % _num_queues); each queue has its ownmutex in
_queue_locks.completion thread drains every queue under each queue's lock, while queues
1..N also poll their own ring under the same per-queue lock. This is
documented honestly as a known limitation in
docs/block-multiqueue.md,along with the per-queue-interrupt path (as
nvme.ccdoes viadriver::register_io_interrupt()) as the future enhancement that lets eachqueue's completions land on its owning CPU and drops the cross-queue drain
loop.
scripts/run.pyexposes--block-queues/num-queues. With one vCPU or onequeue the driver behaves exactly as the original single-queue path.
The speculative
blk-mq.cc/blk_mq.hscaffolding from the original combinedPR has been dropped — it had no driver, test, or app consumer, and
read()/write()already benefit from multiqueue via virtio-blk's per-CPU queueselection. The unrelated
bsd/porting/bus.hdevice_delete_childhunk hasalso been removed.
Build-qualified (kernel compile+link,
image=empty) on a binutils 2.44 /g++ 14.3.0 host.