nvme: Pass controller instance ID to NvmeDriver for better tracing #1661
+176
−43
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,58 @@ use std::collections::HashMap; | |
| use std::collections::hash_map; | ||
| use thiserror::Error; | ||
| use tracing::Instrument; | ||
|
|
||
| /// Strongly typed wrapper for NVMe device name (formerly debug_id/controller_instance_id) | ||
| #[derive(Debug, Clone, PartialEq, Eq, MeshPayload)] | ||
| pub struct NvmeDeviceName(pub String); | ||
|
|
||
| impl From<String> for NvmeDeviceName { | ||
| fn from(s: String) -> Self { | ||
| NvmeDeviceName(s) | ||
| } | ||
| } | ||
|
|
||
| impl From<&str> for NvmeDeviceName { | ||
| fn from(s: &str) -> Self { | ||
| NvmeDeviceName(s.to_string()) | ||
| } | ||
| } | ||
|
|
||
| impl AsRef<str> for NvmeDeviceName { | ||
| fn as_ref(&self) -> &str { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| /// Strongly typed wrapper for PCI ID | ||
| #[derive(Debug, Clone, PartialEq, Eq, MeshPayload)] | ||
| pub struct PciId(pub String); | ||
|
There was a problem hiding this comment. Promote this to a type in the same module as While you are there, a few other fixups:
There was a problem hiding this comment. Completed all requested changes:
Note: DeviceBacking::id() usage is primarily for diagnostics already - no non-diagnostic usage found. (commit a9818bd) |
||
|
|
||
| impl From<String> for PciId { | ||
| fn from(s: String) -> Self { | ||
| PciId(s) | ||
| } | ||
| } | ||
|
|
||
| impl From<&str> for PciId { | ||
| fn from(s: &str) -> Self { | ||
| PciId(s.to_string()) | ||
| } | ||
| } | ||
|
|
||
| impl AsRef<str> for PciId { | ||
| fn as_ref(&self) -> &str { | ||
| &self.0 | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. These aren't needed, so don't add them. If this is just for debugging sake, then the There was a problem hiding this comment. Removed the unnecessary From/AsRef implementations from NvmeDeviceName and PciId wrappers as suggested. They only use Debug derive now. (commit a9818bd) |
||
|
|
||
| /// Parameters for getting an NVMe namespace | ||
| #[derive(Debug, Clone, MeshPayload)] | ||
| pub struct GetNamespaceParams { | ||
| pub name: NvmeDeviceName, | ||
| pub pci_id: PciId, | ||
| pub nsid: u32, | ||
| } | ||
| use user_driver::vfio::VfioDevice; | ||
| use vm_resource::AsyncResolveResource; | ||
| use vm_resource::ResourceId; | ||
|
|
@@ -175,7 +227,7 @@ impl NvmeManager { | |
| enum Request { | ||
| Inspect(inspect::Deferred), | ||
| ForceLoadDriver(inspect::DeferredUpdate), | ||
| GetNamespace(Rpc<GetNamespaceParams, Result<nvme_driver::Namespace, NamespaceError>>), | ||
| Save(Rpc<(), Result<NvmeManagerSavedState, anyhow::Error>>), | ||
| Shutdown { | ||
| span: tracing::Span, | ||
|
|
@@ -191,13 +243,24 @@ pub struct NvmeManagerClient { | |
| impl NvmeManagerClient { | ||
| pub async fn get_namespace( | ||
| &self, | ||
| name: impl Into<NvmeDeviceName>, | ||
| pci_id: impl Into<PciId>, | ||
| nsid: u32, | ||
| ) -> anyhow::Result<nvme_driver::Namespace> { | ||
| let params = GetNamespaceParams { | ||
| name: name.into(), | ||
| pci_id: pci_id.into(), | ||
| nsid, | ||
| }; | ||
| Ok(self | ||
| .sender | ||
| .call(Request::GetNamespace, params.clone()) | ||
| .instrument(tracing::info_span!( | ||
| "nvme_get_namespace", | ||
| name = params.name.as_ref(), | ||
| pci_id = params.pci_id.as_ref(), | ||
| nsid | ||
| )) | ||
| .await | ||
| .context("nvme manager is shut down")??) | ||
| } | ||
|
|
@@ -235,7 +298,10 @@ impl NvmeManagerWorker { | |
| match req { | ||
| Request::Inspect(deferred) => deferred.inspect(&self), | ||
| Request::ForceLoadDriver(update) => { | ||
| match self | ||
| .get_driver("force-load".to_string(), update.new_value().to_owned()) | ||
| .await | ||
| { | ||
| Ok(_) => { | ||
| let pci_id = update.new_value().to_string(); | ||
| update.succeed(pci_id); | ||
|
|
@@ -246,9 +312,12 @@ impl NvmeManagerWorker { | |
| } | ||
| } | ||
| Request::GetNamespace(rpc) => { | ||
| rpc.handle(async |params| { | ||
| self.get_namespace(params.name.0, params.pci_id.0.clone(), params.nsid) | ||
| .map_err(|source| NamespaceError { | ||
| pci_id: params.pci_id.0, | ||
| source, | ||
| }) | ||
| .await | ||
| }) | ||
| .await | ||
|
|
@@ -283,9 +352,12 @@ impl NvmeManagerWorker { | |
| if !nvme_keepalive || !self.save_restore_supported { | ||
| async { | ||
| join_all(self.devices.drain().map(|(pci_id, driver)| { | ||
| let name = driver.name(); | ||
| driver.shutdown().instrument(tracing::info_span!( | ||
| "shutdown_nvme_driver", | ||
| pci_id, | ||
| name | ||
| )) | ||
| })) | ||
| .await | ||
| } | ||
|
|
@@ -296,6 +368,7 @@ impl NvmeManagerWorker { | |
|
|
||
| async fn get_driver( | ||
| &mut self, | ||
| name: String, | ||
| pci_id: String, | ||
| ) -> Result<&mut nvme_driver::NvmeDriver<VfioDevice>, InnerError> { | ||
| let driver = match self.devices.entry(pci_id.to_owned()) { | ||
|
|
@@ -328,6 +401,7 @@ impl NvmeManagerWorker { | |
| self.vp_count, | ||
| device, | ||
| self.is_isolated, | ||
| name, | ||
| ) | ||
| .instrument(tracing::info_span!( | ||
| "nvme_driver_init", | ||
|
|
@@ -344,10 +418,11 @@ impl NvmeManagerWorker { | |
|
|
||
| async fn get_namespace( | ||
| &mut self, | ||
| name: String, | ||
| pci_id: String, | ||
| nsid: u32, | ||
| ) -> Result<nvme_driver::Namespace, InnerError> { | ||
| let driver = self.get_driver(name, pci_id.to_owned()).await?; | ||
| driver | ||
| .namespace(nsid) | ||
| .await | ||
|
|
@@ -407,6 +482,7 @@ impl NvmeManagerWorker { | |
| vfio_device, | ||
| &disk.driver_state, | ||
| self.is_isolated, | ||
| format!("restored-{}", disk.pci_id), // Use PCI ID as name for restored drivers | ||
| ) | ||
| .instrument(tracing::info_span!("nvme_driver_restore")) | ||
| .await?; | ||
|
|
@@ -440,20 +516,31 @@ impl AsyncResolveResource<DiskHandleKind, NvmeDiskConfig> for NvmeDiskResolver { | |
| ) -> Result<Self::Output, Self::Error> { | ||
| let namespace = self | ||
| .manager | ||
| .get_namespace(rsrc.debug_id, rsrc.pci_id, rsrc.nsid) | ||
| .await | ||
| .context("could not open nvme namespace")?; | ||
|
|
||
| Ok(ResolvedDisk::new(disk_nvme::NvmeDisk::new(namespace)).context("invalid disk")?) | ||
| } | ||
| } | ||
|
|
||
| #[derive(MeshPayload)] | ||
| pub struct NvmeDiskConfig { | ||
| pub name: String, | ||
| pub pci_id: String, | ||
| pub nsid: u32, | ||
| } | ||
|
|
||
| impl Default for NvmeDiskConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| name: "force-load".to_string(), | ||
| pci_id: String::new(), | ||
| nsid: 0, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl ResourceId<DiskHandleKind> for NvmeDiskConfig { | ||
| const ID: &'static str = "nvme"; | ||
| } | ||
|
|
||
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
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
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.
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.
These aren't needed, so don't add them. If this is just for debugging sake, then the Debug derive should do the trick.
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.
Removed the unnecessary From/AsRef implementations from both strongly typed wrappers as suggested. (commit a9818bd)