fix(pcie): spdk requires nvme path id

When destroying a pcie nvme bdev we must not specify the nvme path id. Also it was noticed that when destroy fails we end up with no alias which could cause some panics. For this particular case we now add the alias back if the bdev is still present though we need a more robust fix.. Signed-off-by: Tiago Castro <[email protected]>
openebs · Aug 25, 2023 · 43f2d5a · 43f2d5a
1 parent bfa99c5
commit 43f2d5a
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 7 deletions.
diff --git a/io-engine/src/bdev/nvme.rs b/io-engine/src/bdev/nvme.rs
@@ -11,7 +11,12 @@ use url::Url;
 
 use spdk_rs::{
     ffihelper::copy_str_with_null,
-    libspdk::{bdev_nvme_create, bdev_nvme_delete, spdk_nvme_transport_id},
+    libspdk::{
+        bdev_nvme_create,
+        bdev_nvme_delete,
+        nvme_path_id,
+        spdk_nvme_transport_id,
+    },
 };
 
 use crate::{
@@ -130,12 +135,21 @@ impl CreateDestroy for NVMe {
     async fn destroy(self: Box<Self>) -> Result<(), Self::Error> {
         if let Some(mut bdev) = UntypedBdev::lookup_by_name(&self.get_name()) {
             bdev.remove_alias(self.url.as_ref());
+
+            let mut path_id = nvme_path_id::default();
+            copy_str_with_null(&self.name, &mut path_id.trid.traddr);
+            path_id.trid.trtype = spdk_rs::libspdk::SPDK_NVME_TRANSPORT_PCIE;
+
             let errno = unsafe {
                 bdev_nvme_delete(
                     self.name.clone().into_cstring().as_ptr(),
-                    std::ptr::null(),
+                    &path_id,
                 )
             };
+            if errno != 0 {
+                UntypedBdev::lookup_by_name(&self.get_name())
+                    .map(|mut b| b.add_alias(self.url.as_ref()));
+            }
             errno_result_from_i32((), errno).context(
                 bdev_api::DestroyBdevFailed {
                     name: self.name.clone(),

diff --git a/io-engine/src/lvs/lvs_store.rs b/io-engine/src/lvs/lvs_store.rs
@@ -290,7 +290,7 @@ impl Lvs {
                 lvs.name()
             );
             let pool_name = lvs.name().to_string();
-            lvs.export().await.unwrap();
+            lvs.export().await?;
             Err(Error::Import {
                 source: Errno::EINVAL,
                 name: pool_name,
@@ -553,7 +553,7 @@ impl Lvs {
 
         info!("{}: lvs exported successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap_or_default())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,
@@ -647,7 +647,7 @@ impl Lvs {
 
         info!("{}: lvs destroyed successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap_or_default())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,

diff --git a/test/grpc/test_nexus.js b/test/grpc/test_nexus.js
@@ -779,7 +779,7 @@ describe('nexus', function () {
       };
       client.createNexusV2(args, (err) => {
         if (!err) return done(new Error('Expected error'));
-        assert.equal(err.code, grpc.status.INTERNAL);
+        assert.equal(err.code, grpc.status.INVALID_ARGUMENT);
         done();
       });
     });
@@ -799,7 +799,7 @@ describe('nexus', function () {
       };
       client.createNexusV2(args, (err) => {
         if (!err) return done(new Error('Expected error'));
-        assert.equal(err.code, grpc.status.INTERNAL);
+        assert.equal(err.code, grpc.status.INVALID_ARGUMENT);
         done();
       });
     });