fix(nexus): allow re-adding child removed by AEN

Add a destroying flag to NexusChild and only set it in destroy() so
a nexus child destroyed via the normal close() path actually removes
the bdev.

This allows a remove via e.g. an NVMe async event notification to only
mark the child as faulted, which does remove it from the I/O path, and
allows a subsequent normal destroy of a child to remove the bdev.
Previously it would have set NexusChild::bdev to None without
destroying the bdev, which leaks it.

This requires that the shutdown process destroys nexuses first so that
it then destroys each child and its bdev, in that order, as the remove
event from SPDK as part of shutdown would not destroy the bdev, which
would result in the process hanging.

mayastor/src/bdev/nexus/mod.rs

@@ -59,3 +59,17 @@ pub fn nexus_instance_new(name: String, size: u64, children: Vec<String>) {
     let list = instances();
     list.push(Nexus::new(&name, size, None, Some(&children)));
 }
+
+/// called during shutdown so that the bdevs for each child is destroyed first
+pub async fn nexus_destroy_all() {
+    info!("destroying all nexuses...");
+    for nexus in instances() {
+        if let Err(e) = nexus.destroy().await {
+            error!(
+                "failed to destroy nexus {} during shutdown: {}",
+                nexus.name, e
+            );
+        }
+    }
+    info!("destroyed all nexuses");
+}

mayastor/src/bdev/nexus/nexus_child.rs

@@ -132,6 +132,9 @@ pub struct NexusChild {
     /// current state of the child
     #[serde(skip_serializing)]
     pub state: AtomicCell<ChildState>,
+    /// destruction started, differentiates remove events
+    #[serde(skip_serializing)]
+    destroying: AtomicCell<bool>,
     /// record of most-recent IO errors
     #[serde(skip_serializing)]
     pub(crate) err_store: Option<NexusErrStore>,
@@ -229,6 +232,7 @@ impl NexusChild {
             },
         )?);
 
+        self.destroying.store(false);
         self.desc = Some(desc);
 
         let cfg = Config::get();
@@ -318,6 +322,11 @@ impl NexusChild {
         self.state.load()
     }
 
+    /// returns whether destruction has started
+    pub fn destroying(&self) -> bool {
+        self.destroying.load()
+    }
+
     pub(crate) fn rebuilding(&self) -> bool {
         match RebuildJob::lookup(&self.name) {
             Ok(_) => self.state() == ChildState::Faulted(Reason::OutOfSync),
@@ -369,8 +378,14 @@ impl NexusChild {
     pub(crate) fn remove(&mut self) {
         info!("Removing child {}", self.name);
 
-        // The bdev is being removed, so ensure we don't use it again.
-        self.bdev = None;
+        // Only remove the bdev if the child is being destroyed instead of a
+        // hot remove event e.g. NVMe AEN (asynchronous event notification)
+        let destroying = self.destroying();
+        info!("Destroying child: {}", destroying);
+        if destroying {
+            // The bdev is being removed, so ensure we don't use it again.
+            self.bdev = None;
+        }
 
         let state = self.state();
 
@@ -385,7 +400,7 @@ impl NexusChild {
         }
 
         // Remove the child from the I/O path. If we had an IO error the bdev,
-        // the channels where already reconfigured so we dont have to do
+        // the channels were already reconfigured so we don't have to do
         // that twice.
         if state != ChildState::Faulted(Reason::IoError) {
             let nexus_name = self.parent.clone();
@@ -397,10 +412,12 @@ impl NexusChild {
             });
         }
 
-        // Dropping the last descriptor results in the bdev being removed.
-        // This must be performed in this function.
-        let desc = self.desc.take();
-        drop(desc);
+        if destroying {
+            // Dropping the last descriptor results in the bdev being removed.
+            // This must be performed in this function.
+            let desc = self.desc.take();
+            drop(desc);
+        }
 
         self.remove_complete();
         info!("Child {} removed", self.name);
@@ -428,6 +445,7 @@ impl NexusChild {
             parent,
             desc: None,
             state: AtomicCell::new(ChildState::Init),
+            destroying: AtomicCell::new(false),
             err_store: None,
             remove_channel: mpsc::channel(0),
         }
@@ -436,7 +454,8 @@ impl NexusChild {
     /// destroy the child bdev
     pub(crate) async fn destroy(&self) -> Result<(), NexusBdevError> {
         trace!("destroying child {:?}", self);
-        if let Some(_bdev) = &self.bdev {
+        if self.bdev.is_some() {
+            self.destroying.store(true);
             bdev_destroy(&self.name).await
         } else {
             warn!("Destroy child without bdev");

mayastor/src/core/bdev.rs

@@ -161,7 +161,7 @@ impl Bdev {
         bdev: *mut spdk_bdev,
         _ctx: *mut c_void,
     ) {
-        let bdev = Bdev(NonNull::new(bdev).unwrap());
+        let bdev = Bdev::from_ptr(bdev).unwrap();
         // Take the appropriate action for the given event type
         match event {
             spdk_sys::SPDK_BDEV_EVENT_REMOVE => {
@@ -171,9 +171,9 @@ impl Bdev {
                 }
             }
             spdk_sys::SPDK_BDEV_EVENT_RESIZE => {
-                info!("Received resize event for bdev {}", bdev.name())
+                warn!("Received resize event for bdev {}", bdev.name())
             }
-            spdk_sys::SPDK_BDEV_EVENT_MEDIA_MANAGEMENT => info!(
+            spdk_sys::SPDK_BDEV_EVENT_MEDIA_MANAGEMENT => warn!(
                 "Received media management event for bdev {}",
                 bdev.name()
             ),

mayastor/src/core/env.rs

@@ -40,7 +40,7 @@ use spdk_sys::{
 };
 
 use crate::{
-    bdev::nexus::nexus_child_status_config::ChildStatusConfig,
+    bdev::{nexus, nexus::nexus_child_status_config::ChildStatusConfig},
     core::{
         reactor::{Reactor, ReactorState, Reactors},
         Cores,
@@ -279,6 +279,7 @@ async fn do_shutdown(arg: *mut c_void) {
     }
 
     iscsi::fini();
+    nexus::nexus_destroy_all().await;
     unsafe {
         spdk_rpc_finish();
         spdk_subsystem_fini(Some(reactors_stop), arg);

test/grpc/test_nexus.js

@@ -827,7 +827,17 @@ describe('nexus', function () {
       }
     });
 
-    // must be last nvmf test as it removes ns
+    it('should create nexus with nvmf target as child', async () => {
+      const args = {
+        uuid: UUID,
+        size: diskSize,
+        children: [
+        `nvmf://127.0.0.1:8420/nqn.2019-05.io.openebs:${TGTUUID}`
+        ]
+      };
+      await createNexus(args);
+    });
+
     it('should remove namespace from nvmf subsystem', (done) => {
       const args = {
         nqn: `nqn.2019-05.io.openebs:${TGTUUID}`,
@@ -836,6 +846,39 @@ describe('nexus', function () {
       common.jsonrpcCommand('/tmp/target.sock', 'nvmf_subsystem_remove_ns', args, done);
     });
 
+    it('should still have bdev of removed child after remove event', (done) => {
+      common.jsonrpcCommand(null, 'bdev_get_bdevs', (err, out) => {
+        if (err) return done(err);
+        const bdevs = JSON.parse(out);
+        const match = `127.0.0.1:8420/nqn.2019-05.io.openebs:${TGTUUID}n1`;
+        var i;
+        for (i in bdevs) {
+          if (bdevs[i].name === match) {
+            return done();
+          }
+        }
+        done(new Error('bdev not found'));
+      });
+    });
+
+    it('should have nexus in faulted state and its child in degraded state', (done) => {
+      client.listNexus({}, (err, res) => {
+        if (err) return done(err);
+        assert.lengthOf(res.nexus_list, 1);
+        const nexus = res.nexus_list[0];
+
+        assert.equal(nexus.uuid, UUID);
+        assert.equal(nexus.state, 'NEXUS_FAULTED');
+        assert.lengthOf(nexus.children, 1);
+        assert.equal(nexus.children[0].state, 'CHILD_DEGRADED');
+        done();
+      });
+    });
+
+    it('should destroy nexus', async () => {
+      await destroyNexus({ uuid: UUID });
+    });
+
     it('should fail to create nexus with child that has no namespaces', (done) => {
       const args = {
         uuid: UUID,
@@ -852,6 +895,28 @@ describe('nexus', function () {
       });
     });
 
+    it('should add namespace back to nvmf subsystem', (done) => {
+      const args = {
+        nqn: `nqn.2019-05.io.openebs:${TGTUUID}`,
+        namespace: {
+          bdev_name: 'Malloc0'
+        }
+      };
+      common.jsonrpcCommand('/tmp/target.sock', 'nvmf_subsystem_add_ns', args, done);
+    });
+
+    it('should create then destroy nexus with previously asynchronously removed nvmf target as child', async () => {
+      const args = {
+        uuid: UUID,
+        size: diskSize,
+        children: [
+        `nvmf://127.0.0.1:8420/nqn.2019-05.io.openebs:${TGTUUID}`
+        ]
+      };
+      await createNexus(args);
+      await destroyNexus({ uuid: UUID });
+    });
+
     it('should have zero nexus devices left', (done) => {
       client.listNexus({}, (err, res) => {
         if (err) return done(err);