do not set NVMe VWC for local volumes - #10932
Draft
iximeow wants to merge 6 commits into
Draft
Conversation
iximeow
added
virtualization
This was referenced Jul 27, 2026
hawkw
reviewed
Jul 30, 2026
| // storage may be commodity M.2s or worse, this can claim "no | ||
| // VWC semantics" when the underlying storage actually does. | ||
| // This could be improved. See Omicron#10933. | ||
| let vwc_semantics = !path.starts_with("/dev/zvol/rdsk/"); |
Member
There was a problem hiding this comment.
n.b. that this path fragment occurs in other code that manages local volumes, and I kinda wonder if there ought to be a const for it? https://github.com/search?q=repo%3Aoxidecomputer%2Fomicron%20%2Fdev%2Fzvol%2Frdsk%2F&type=code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this closes the loop on oxidecomputer/propolis#1170 . I've written up some measurements and comparison at https://github.com/oxidecomputer/meta/blob/master/engineering/propolis-local-io-timing/vwc.adoc which, I think would be kind of neat to post about more generally, but at least here describe the relative difference and why we know this does anything one way or the other.
getting clever with the VWC bit requires care. I've elaborated on why in the change. the new indicated write cache behavior for local volumes will be wrong for most local development. local disks will behave right, but if a kernel panics or power is lost, writes a guest had believed were complete may be lost. we can do better for development environments and will want some additional work in Propolis as it relates to handling degraded hardware.
this draft depends on oxidecomputer/propolis#1178 and all the commits referenced are to change. since this pulls in a new Propolis, I owe this PR description a list of what else (if anything) is changing in the bump.