Details

Reviewers

jhb
corvink
markj

Group Reviewers

bhyve

Summary

By preallocating all I/O requests on all queues, we can take most
allocations out of the hot I/O code paths and simplify the code
significantly. While here, make sure we check all allocations for
success and make sure to handle failures gracefully.

Additionally, check for I/O request validity as early as possible,
and return illegal requests immediately.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 68264
Build 65147: arc lint + arc unit

Event Timeline

rosenfeld_grumpf.hope-2000.org created this revision.Oct 30 2025, 7:38 AM

Herald added a reviewer: bhyve. · View Herald TranscriptOct 30 2025, 7:38 AM

rosenfeld_grumpf.hope-2000.org requested review of this revision.Oct 30 2025, 7:38 AM

Harbormaster completed remote builds in B68264: Diff 165400.Oct 30 2025, 7:38 AM

rosenfeld_grumpf.hope-2000.org added a parent revision: D53468: bhyve/virtio: Rework iovec handling functions for efficiency and clarity.Oct 30 2025, 7:38 AM

rosenfeld_grumpf.hope-2000.org added a child revision: D53470: bhyve/virtio-scsi: Check LUN address validity.

Rebase.

Harbormaster completed remote builds in B68267: Diff 165403.Oct 30 2025, 7:46 AM

pci_vtscsi_proc()

Harbormaster completed remote builds in B68273: Diff 165411.Oct 30 2025, 8:09 AM

Rebase.

Harbormaster completed remote builds in B69047: Diff 167542.Dec 4 2025, 6:21 PM

emaste added a subscriber: emaste.Tue, Feb 3, 5:25 PM

rosenfeld_grumpf.hope-2000.org retitled this revision from bhyve/virtio-scsi: preallocate all I/O requests to bhyve/virtio-scsi: Preallocate all I/O requests.Mon, Feb 9, 4:26 PM

Sync with latest illumos bits after code review.

Use a size_t type where appropriate instead of the abomination that is uint.

Harbormaster completed remote builds in B70550: Diff 171527.Mon, Feb 9, 4:27 PM

This broadly looks ok, I have a few inline comments.

usr.sbin/bhyve/pci_virtio_scsi.c
111	Some comment explaining how these three mutexes are used would be appreciated.
573	Is the `vsr_queue` field used for anything?
670	Is it really necessary to clear the whole request structure like this?
841	There's no cleanup of the threads in this error path. If you allocate all of the worker structures at once, before the loop, then you don't need to think about cleaning up in this error path (aside from the requests allocated in the loop above, which are easy to deal with).

This revision is now accepted and ready to land.Mon, Feb 16, 9:41 PM

Address review from markj.

This revision now requires review to proceed.Thu, Feb 19, 6:36 PM

Harbormaster completed remote builds in B70791: Diff 172267.Thu, Feb 19, 6:36 PM

rosenfeld_grumpf.hope-2000.org added inline comments.Thu, Feb 19, 6:36 PM

usr.sbin/bhyve/pci_virtio_scsi.c
573	Looks like it isn't used anywhere by the code, but I found it useful during debugging, especially with multiple I/O queue support.
670	I don't know whether it's strictly necessary, but I feel much better knowing there's no stale data left in there before we reuse it, especially not in any of the iovecs.

bhyve/virtio-scsi: Preallocate all I/O requests
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 165400

usr.sbin/bhyve/pci_virtio_scsi.c

bhyve/virtio-scsi: Preallocate all I/O requestsNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 165400

usr.sbin/bhyve/pci_virtio_scsi.c

bhyve/virtio-scsi: Preallocate all I/O requests
Needs ReviewPublic
Actions

Revision Contents
Changeset List