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 :-)
| @@ -0,0 +1,91 @@ | |||
| # Block Device DISCARD/TRIM Support | |||
There was a problem hiding this comment.
We already have a "documentation/" subdirectory (with very sporadic content...). I agree "docs" is shorter and nicer, but I think we should pick just one directory and stick to it.
If you prefer docs/, can you please create a followup PR to move the stuff now in documentation/ to docs/? If you do, don't forget to change the references in LICENSE and README.md to the new directory.
Thanks.
There was a problem hiding this comment.
Resolved by standardizing on the existing documentation/ directory: both files now live there (documentation/block-discard.md, documentation/block-multiqueue.md) and there is no new docs/ directory in this PR. So no split to reconcile and no LICENSE/README references to update.
There was a problem hiding this comment.
One correction to flag on this file: documentation/block-discard.md:15 shows the define as #define BIO_DISCARD 0x20, but the actual value in include/osv/bio.h is 0x80 (it replaced the old BIO_CMD2 slot). 0x20 is already taken by BIO_SCSI. The doc line should read 0x80 to match the header. Code is correct; only the doc value is stale.
|
|
||
| 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.
| } | ||
|
|
||
| /* Select a queue by CPU id so parallel CPUs use independent rings. */ | ||
| int qid = sched::cpu::current()->id % _num_queues; |
There was a problem hiding this comment.
If each of these threads is responsible for specific CPU or CPUs, I wonder if we should pin its completion thread to that CPU/CPUs. But I guess we can consider this optimization later.
There was a problem hiding this comment.
Agreed to defer. With the single-IRQ / single-completion-thread design in da5595c there's one drain thread rather than a per-CPU thread, so per-CPU pinning doesn't apply as written. If we later move to per-queue MSI-X vectors with a thread per queue, pinning each to its queue's CPU is the natural follow-up. Left out of this PR.
| VIRTIO_BLK_F_TOPOLOGY = 10, /* Topology information is available */ | ||
| VIRTIO_BLK_F_CONFIG_WCE = 11, /* Writeback mode available in config */ | ||
| VIRTIO_BLK_F_DISCARD = 13, /* DISCARD is supported */ | ||
| VIRTIO_BLK_F_MQ = 12, /* Multi-queue support */ |
There was a problem hiding this comment.
The feature-bit enum (virtio-blk.hh:20-34) is sorted by bit value: 0,1,2,4,5,6,7,9,10,11,12,13, with VIRTIO_BLK_F_MQ=12 and VIRTIO_BLK_F_DISCARD=13 appended in order. Sorted in da5595c.
| #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). |
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. - make_request() selects a queue by CPU id, removing the single global submission lock as a cross-CPU contention point. - Completions use a simple single-interrupt model: queue 0's thread is woken by the IRQ and drains every queue under its per-queue lock, while higher-numbered queues poll their own ring as drain helpers. drain_queue() factors out the shared ring-draining logic. - scripts/run.py gains --virtio-blk-queues to set QEMU's num-queues. docs/block-multiqueue.md documents the feature and the completion model.
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. |
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.