bhyve/virtio-scsi: various small improvements

[rosenfeld_grumpf.hope-2000.org]

Summary

• indicate support for indirect descriptors
• simplify iov functions and let them work in-place, preventing unnecessary allocations and memcpy() calls in hot code paths
• preallocate all I/O requests on all queues, taking most allocations out of hot code paths
• check for I/O request validity as early as possible, and return illegal requests immediately
• check lun format
• check all allocations for success and handle failures gracefully
• add a few more DPRINTFs
• style cleanups

This is currently also under review in illumos: https://code.illumos.org/c/illumos-gate/+/4422

[rosenfeld_grumpf.hope-2000.org]

I've tested this extensively with VMs running FreeBSD, NetBSD, and OmniOS, each using the same ZFS pool on the same CTL LUNs. This causes a significant speedup of 'zpool scrub' on OmniOS (as in: now twice as fast), while that effect isn't quite as noticable on a FreeBSD VM, it's at least still measurable.

[corvink]

• indicate support for indirect descriptors
• simplify iov functions and let them work in-place, preventing unnecessary allocations and memcpy() calls in hot code paths
• preallocate all I/O requests on all queues, taking most allocations out of hot code paths
• check for I/O request validity as early as possible, and return illegal requests immediately
• check lun format
• check all allocations for success and handle failures gracefully
• add a few more DPRINTFs
• style cleanups

It would make it easier to review and merge when each point gets it own commit.

usr.sbin/bhyve/pci_virtio_scsi.c

32

unrelated

121–124

This style fix seems to make it even more worse (at least in phabricator). Not sure if your editor or phabricator uses odd settings for indentation.

[rosenfeld_grumpf.hope-2000.org]

In D53220#1215995, @corvink wrote:

It would make it easier to review and merge when each point gets it own commit.

Actually, all of this was done together with D53223, D53222, and D53221. I've moved this part out and ahead of the other changes as I figured they'd needlessly complicate the review of the larger functional changes.

It may seem as a bunch of individual changes lumped together, but I've really just listed what this change does. Except perhaps for the style cleanups (which I'm not sure are well received in FreeBSD anyway) and the 1-line change to enable indirect descriptors, I'm not sure much is gained if I try to split it up further as all of this was really developed and tested together.

Anyway, if you insist that I split it up, I will of course try to do so.

usr.sbin/bhyve/pci_virtio_scsi.c

32

I'm a bit unsure about what you mean with "unrelated" here. :)

Do you want me to remove this new empty line?

121–124

I'm using only tabs for indentation. Looks like an issue of Phabricator.

I have no problem undoing this part of the change if you wish so, but given that all of this is moving into a separate header file in D53223 anyway, does it really matter?

[corvink]

In D53220#1216044, @rosenfeld_grumpf.hope-2000.org wrote:

In D53220#1215995, @corvink wrote:

It would make it easier to review and merge when each point gets it own commit.

Actually, all of this was done together with D53223, D53222, and D53221. I've moved this part out and ahead of the other changes as I figured they'd needlessly complicate the review of the larger functional changes.

It may seem as a bunch of individual changes lumped together, but I've really just listed what this change does. Except perhaps for the style cleanups (which I'm not sure are well received in FreeBSD anyway) and the 1-line change to enable indirect descriptors, I'm not sure much is gained if I try to split it up further as all of this was really developed and tested together.

Anyway, if you insist that I split it up, I will of course try to do so.

IMO, there are a bunch of benefits by creating small commits.

In this case, I'm not familiar with SCSI, so it will take me some time to review the change. By splitting this into smaller commits, I can merge simple changes early and can focus on more complex changes more easily.

Nevertheless, if you think it's not worth splitting, keep it as is.

usr.sbin/bhyve/pci_virtio_scsi.c

32

Yes, it just bloats the diff.

121–124

I personally dislike random style changes a lot. It just bloats your diff making it a lot harder to read. Additionally, your history is filled with random style changes making it hard to search e.g. when using git blame to search for the commit adding a feature to read more about it in the commit message.

[rosenfeld_grumpf.hope-2000.org]

usr.sbin/bhyve/pci_virtio_scsi.c

121–124

I wouldn't go as far as to call them random, but anyway. I've reverted all the cleanups from this change and split it into smaller parts. I'll close this review and open a bunch of new ones for the smaller changes.