Page MenuHomeFreeBSD

Increase bhyve BLOCKIF_IOV_MAX and VTBLK_RINGSZ to 128 to support modern Windows guests
ClosedPublic

Authored by richard_meric.id.au on Jan 13 2019, 6:49 AM.

Details

Summary

Windows 10 and Server 2016+ vms both fail if configured with virtio-blk disk devices due to IOV_MAX limitations in bhyve.
This patch increases this limit as seen in https://reviews.freebsd.org/D10581 but also increases the VTBLK_RINGSZ to avoid breaking Linux guests.

It has been discussed here before:
http://freebsd.1045724.x6.nabble.com/pci-virtio-block-c-Assertion-failed-line-216-td6155786.html
https://reviews.freebsd.org/D10581
https://reviews.freebsd.org/D9033

Note: This patch is derived from a similar fix from the smartos.org project (https://smartos.org/bugview/OS-7175)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fix cut-and-paste error in patch

Thanks for the patch, I will take a look on within the next two weeks.

This is effectively what we did in SmartOS/illumos to address the issue:
https://github.com/joyent/illumos-joyent/commit/192e1e6405f98e4b0a12f9488793c5dd000f3f7e

I would suggest including a comment where vbc_seg_max is set to note why that value is clamped as it is.

Added comment where vbc_seg_max is set to note why that value is clamped as it is (as recommended by pmooney_pfmooney.com).

This is effectively what we did in SmartOS/illumos to address the issue:
https://github.com/joyent/illumos-joyent/commit/192e1e6405f98e4b0a12f9488793c5dd000f3f7e
I would suggest including a comment where vbc_seg_max is set to note why that value is clamped as it is.

Thanks Patrick, I've included the comment from your patch as suggested.

The SmartOS patch was the source of my patch - I only raised it here so that it might find its way into the upstream code-base, as both other attempts at this seem to have stalled.
Definitely happy for the original author to claim it.

Hi Richard,

Do you mind share the tests you made??

Best,

Hi Richard,
Do you mind share the tests you made??
Best,

I've currently got these changes running in a FreeNAS-11.2-RELEASE-U1 instance (FreeBSD 11.2 kernel) running a couple of Windows 2016 VMs plus one Centos 7 - all using virtio-blk disk devices without issue. I also have pfSense (FreeBSD 11.2) and OpenSuSE (Leap 42.3) VMs which I haven't tested yet - but can do if required.

Hi Richard,
Do you mind share the tests you made??
Best,

I've currently got these changes running in a FreeNAS-11.2-RELEASE-U1 instance (FreeBSD 11.2 kernel) running a couple of Windows 2016 VMs plus one Centos 7 - all using virtio-blk disk devices without issue. I also have pfSense (FreeBSD 11.2) and OpenSuSE (Leap 42.3) VMs which I haven't tested yet - but can do if required.

OK! Thanks for sharing!
I have a test-bed here where I will test this patch and make some disk stress testing during couple days.

araujo requested changes to this revision.Jan 15 2019, 12:17 AM
This comment was removed by araujo.
This revision now requires changes to proceed.Jan 15 2019, 12:17 AM
NOTE: I blocked this patch, till I finish my analysis, nothing wrong with that...

Not sure about the seg_max value (even in the current code).
Looking at the qemu implementation, I suspect it should be queue length -2, which I guess is what the patched code happens to achieve (based on the definition of VTBLK_RINGSZ), but it could be more explicit.

Only remaining thing is whether we should do something different for the ahci device, as it's currently getting a 128 element IOV, but really only needs 32 (33?). I'm not sure if it is a problem having it larger?

araujo removed a reviewer: araujo.Feb 22 2019, 1:23 AM
jhb added a subscriber: jhb.Apr 25 2019, 4:10 PM
jhb added inline comments.
usr.sbin/bhyve/block_if.c
65 ↗(On Diff #52820)

Should this be using BLOCKIF_IOV_MAX going forward, or maybe something like MAX(64, BLOCKIF_IOV_MAX)?

jhb updated this revision to Diff 56652.Apr 25 2019, 4:58 PM
  • Added a new constant and a static assertion and some more comments to reduce magic numbers a bit.
rgrimes accepted this revision as: rgrimes.Apr 25 2019, 5:03 PM

This makes it much clearer to me, thanks jhb

This revision was not accepted when it landed; it landed in state Needs Review.May 2 2019, 10:46 PM
This revision was automatically updated to reflect the committed changes.