-
Notifications
You must be signed in to change notification settings - Fork 92
bump Propolis, Crucible, do not set NVMe VWC for local volumes #10932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fc41816
9a70bc3
cf46c4c
45a3b95
567701d
d2510bb
87cac93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,7 @@ impl DisksByIdBuilder { | |
| Self { map: BTreeMap::new(), slot_usage: BTreeSet::new() } | ||
| } | ||
|
|
||
| fn add_generic_disk( | ||
| fn add_nvme_disk( | ||
| &mut self, | ||
| disk: &Disk, | ||
| backend: Component, | ||
|
|
@@ -246,12 +246,58 @@ impl DisksByIdBuilder { | |
|
|
||
| let pci_path = slot_to_pci_bdf(slot, PciDeviceKind::Disk)?; | ||
|
|
||
| // Generally we report that storage devices have volatile write cache | ||
| // semantics. This is a conservative default that matches most system | ||
| // behaviors. Crucible fast-acks writes and requires flushes to persist | ||
| // writes to non-volatile storage, and - theoretically - file-backed | ||
| // disks could be *any file* which may include normal POSIX "you must | ||
| // fdatasync() for writes to not be lost" semantics. | ||
| // | ||
| // This being anything other than "true" must be carefully considered; | ||
| // incorrectly claiming there is no write cache while the backing | ||
| // storage has volatile write cache semantics risks guest data loss in | ||
| // the event of power loss or crashes. | ||
| // | ||
| // On the other hand, when we can avoid claiming volatile write cache | ||
| // semantics, guest OSes know to not send spurious flushes. This can | ||
| // have important performance consequences from avoided VM exits, | ||
| // Propolis syscalls, interrupts, etc. | ||
| let volatile_write_cache = match &backend { | ||
| // We match on all fields so that if FileStorageBackend changes, | ||
| // those changes must consider if volatile-write-cache semantics are | ||
| // correctly captured here. | ||
| Component::FileStorageBackend(FileStorageBackend { | ||
| path, | ||
| readonly: _, | ||
| block_size: _, | ||
| workers: _, | ||
| }) => { | ||
| // In the product, for the forseeable future, local storage raw | ||
| // zvols are on enterprise U.2s which do not report volatile | ||
| // write caches. "/rdsk/" here refers to the character device | ||
| // for that raw volume with unbuffered semantics (versus | ||
| // `/dsk/`, the block device, which can buffer writes when not | ||
| // opened O_DIRECT - see spec_write(), vpm_data_copy(), and | ||
| // vpm_sync_pages()). | ||
| // | ||
| // XXX: In development and non-product environments where | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was kinda on the fence, my actual impulse here was to have a type that means "path under secretly, I know that once Nexus has a real idea of instance platforms I want to at least move it into a distinct crate so I can weld it into propolis-cli too. I don't want to do that prematurely because Nexus is definitely the primary consumer, but.. in that sense I think a newtype or const or something would make sense to be declared by the instance platform crate and consumed by Nexus? maybe?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm not gonna make a fuss about this part, it was just a thought for the future. leaving this as-is is fine here. |
||
|
|
||
| vwc_semantics | ||
| } | ||
| _ => true, | ||
| }; | ||
|
|
||
| let device = Component::NvmeDisk(NvmeDisk { | ||
| backend_id: SpecKey::Uuid(disk.id()), | ||
| pci_path, | ||
| serial_number: zero_padded_nvme_serial_from_str( | ||
| disk.name().as_str(), | ||
| ), | ||
| has_write_cache: volatile_write_cache, | ||
| }); | ||
|
|
||
| let device_name = component_names::device_name_from_id(&disk.id()); | ||
|
|
@@ -279,10 +325,10 @@ impl DisksByIdBuilder { | |
| request_json: volume.data().to_owned(), | ||
| }); | ||
|
|
||
| self.add_generic_disk(disk, backend) | ||
| self.add_nvme_disk(disk, backend) | ||
| } | ||
|
|
||
| fn add_file_backed_disk( | ||
| fn add_local_disk( | ||
| &mut self, | ||
| disk: &Disk, | ||
| path: String, | ||
|
|
@@ -296,7 +342,7 @@ impl DisksByIdBuilder { | |
| workers: Some(LOCAL_STORAGE_WORKERS), | ||
| }); | ||
|
|
||
| self.add_generic_disk(disk, backend) | ||
| self.add_nvme_disk(disk, backend) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -526,7 +572,7 @@ impl super::Nexus { | |
| } | ||
|
|
||
| db::datastore::Disk::LocalStorage(local_storage_disk) => { | ||
| builder.add_file_backed_disk( | ||
| builder.add_local_disk( | ||
| disk, | ||
| // Use the delegated zvol as the target for the file | ||
| // backed disk | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| cc07512e0ea13cbd3fec470e8704399a7ebae19a:openapi/sled-agent/sled-agent-43.0.0-53ef3d.json |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| sled-agent-43.0.0-53ef3d.json | ||
| sled-agent-44.0.0-9fbab2.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing crucible along for the ride!