Merge #1500

1500: fix(snapshot): reset usage cache for the parent lvol and successor lvol when snapshot is destroyed (Cherrypick PR) r=hrudaya21 a=hrudaya21



Co-authored-by: Hrudaya <[email protected]>

Author: mayastor-bors

io-engine/src/core/snapshot.rs

@@ -349,6 +349,14 @@ pub trait SnapshotOps {
         &self,
         total_ancestor_snap_size: u64,
     ) -> Option<u64>;
+
+    /// When snapshot is destroyed, reset the parent lvol usage cache and its
+    /// successor snapshot and clone usage cache.
+    fn reset_snapshot_parent_successor_usage_cache(&self);
+
+    /// When snapshot is destroyed, reset cache of successor snapshots and
+    /// clones based on snapshot parent uuid.
+    fn reset_successor_lvol_usage_cache(&self, snapshot_parent_uuid: String);
 }
 
 /// Traits gives the Snapshots Related Parameters.

io-engine/src/lvs/lvol_snapshot.rs

@@ -19,6 +19,7 @@ use futures::channel::oneshot;
 use nix::errno::Errno;
 use spdk_rs::libspdk::{
     spdk_blob,
+    spdk_blob_reset_used_clusters_cache,
     spdk_lvol,
     spdk_xattr_descriptor,
     vbdev_lvol_create_clone_ext,
@@ -562,6 +563,7 @@ impl SnapshotOps for Lvol {
     /// Destroy snapshot.
     async fn destroy_snapshot(mut self) -> Result<(), Self::Error> {
         if self.list_clones_by_snapshot_uuid().is_empty() {
+            self.reset_snapshot_parent_successor_usage_cache();
             self.destroy().await?;
         } else {
             self.set_blob_attr(
@@ -731,7 +733,7 @@ impl SnapshotOps for Lvol {
         let Some(bdev) = UntypedBdev::bdev_first() else {
             return; /* No devices available */
         };
-        let futures = bdev
+        let snap_list = bdev
             .into_iter()
             .filter(|b| b.driver() == "lvol")
             .map(|b| Lvol::try_from(b).unwrap())
@@ -740,8 +742,11 @@ impl SnapshotOps for Lvol {
                     && b.is_discarded_snapshot()
                     && b.list_clones_by_snapshot_uuid().is_empty()
             })
-            .map(|s| s.destroy())
-            .collect::<Vec<_>>();
+            .collect::<Vec<Lvol>>();
+        snap_list
+            .iter()
+            .for_each(|s| s.reset_snapshot_parent_successor_usage_cache());
+        let futures = snap_list.into_iter().map(|s| s.destroy());
         let result = join_all(futures).await;
         for r in result {
             match r {
@@ -792,4 +797,68 @@ impl SnapshotOps for Lvol {
             None
         }
     }
+    /// When snapshot is destroyed, reset the parent lvol usage cache and its
+    /// successor snapshot and clone usage cache.
+    fn reset_snapshot_parent_successor_usage_cache(&self) {
+        if let Some(snapshot_parent_uuid) =
+            Lvol::get_blob_xattr(self, SnapshotXattrs::ParentId.name())
+        {
+            if let Some(bdev) =
+                UntypedBdev::lookup_by_uuid_str(snapshot_parent_uuid.as_str())
+            {
+                if let Ok(parent_lvol) = Lvol::try_from(bdev) {
+                    unsafe {
+                        spdk_blob_reset_used_clusters_cache(
+                            parent_lvol.blob_checked(),
+                        );
+                    }
+                }
+            }
+            self.reset_successor_lvol_usage_cache(snapshot_parent_uuid);
+        }
+    }
+
+    /// When snapshot is destroyed, reset cache of successor snapshots and
+    /// clones based on snapshot parent uuid.
+    fn reset_successor_lvol_usage_cache(&self, snapshot_parent_uuid: String) {
+        let mut successor_snapshots = Lvol::list_all_snapshots()
+            .iter()
+            .map(|v| v.snapshot_lvol())
+            .filter_map(|l| {
+                let uuid =
+                    Lvol::get_blob_xattr(self, SnapshotXattrs::ParentId.name());
+                match uuid {
+                    Some(uuid) if uuid == snapshot_parent_uuid => {
+                        Some(l.clone())
+                    }
+                    _ => None,
+                }
+            })
+            .collect::<Vec<Lvol>>();
+        let mut successor_clones: Vec<Lvol> = vec![];
+
+        while !successor_snapshots.is_empty() || !successor_clones.is_empty() {
+            if let Some(snapshot) = successor_snapshots.pop() {
+                unsafe {
+                    spdk_blob_reset_used_clusters_cache(
+                        snapshot.blob_checked(),
+                    );
+                }
+                let new_clone_list = snapshot.list_clones_by_snapshot_uuid();
+                successor_clones.extend(new_clone_list);
+            }
+
+            if let Some(clone) = successor_clones.pop() {
+                unsafe {
+                    spdk_blob_reset_used_clusters_cache(clone.blob_checked());
+                }
+                let new_snap_list = clone
+                    .list_snapshot_by_source_uuid()
+                    .iter()
+                    .map(|v| v.snapshot_lvol().clone())
+                    .collect::<Vec<Lvol>>();
+                successor_snapshots.extend(new_snap_list);
+            }
+        }
+    }
 }

io-engine/src/lvs/lvs_lvol.rs

@@ -1016,6 +1016,7 @@ impl LvsLvol for Lvol {
             if snapshot_lvol.list_clones_by_snapshot_uuid().is_empty()
                 && snapshot_lvol.is_discarded_snapshot()
             {
+                snapshot_lvol.reset_snapshot_parent_successor_usage_cache();
                 snapshot_lvol.destroy().await?;
             }
         }

io-engine/tests/snapshot_lvol.rs

@@ -1055,7 +1055,7 @@ async fn test_snapshot_clone() {
         let clones = snapshot_lvol.list_clones_by_snapshot_uuid();
 
         assert_eq!(clones.len(), 2, "Number of Clones Doesn't match");
-        for clone in clones {
+        for clone in &clones {
             assert!(
                 clone.is_snapshot_clone().is_some(),
                 "Wrongly judge as not a clone"
@@ -1066,6 +1066,10 @@ async fn test_snapshot_clone() {
             snapshot_lvol.is_snapshot_clone().is_none(),
             "Wrongly judge as clone"
         );
+        for clone in clones {
+            clone.destroy().await.expect("destroy clone failed");
+        }
+        clean_snapshots(Lvol::list_all_snapshots()).await;
     })
     .await;
 }
@@ -1554,3 +1558,228 @@ async fn test_snapshot_verify_restore_data() {
     // Check the original replica and restore clone is identical.
     validate_replicas(&vec![repl_1.clone(), restore_1.clone()]).await;
 }
+
+#[tokio::test]
+async fn test_snapshot_parent_usage_post_snapshot_destroy() {
+    let ms = get_ms();
+    const LVOL_NAME: &str = "lvol16";
+
+    ms.spawn(async move {
+        // Create a pool and lvol.
+        let pool = create_test_pool(
+            "pool16",
+            "malloc:///disk16?size_mb=128".to_string(),
+        )
+        .await;
+        let lvol = pool
+            .create_lvol(
+                LVOL_NAME,
+                32 * 1024 * 1024,
+                Some(&Uuid::new_v4().to_string()),
+                true,
+            )
+            .await
+            .expect("Failed to create test lvol");
+        let cluster_size = pool.blob_cluster_size();
+        // Create a test snapshot.
+        let entity_id = String::from("lvol16_e1");
+        let parent_id = lvol.uuid();
+        let txn_id = Uuid::new_v4().to_string();
+        let snap_name = String::from("lvol16_snap1");
+        let snapshot_uuid = Uuid::new_v4().to_string();
+
+        let snapshot_params = SnapshotParams::new(
+            Some(entity_id),
+            Some(parent_id),
+            Some(txn_id),
+            Some(snap_name),
+            Some(snapshot_uuid),
+            Some(Utc::now().to_string()),
+            false,
+        );
+
+        bdev_io::write_some(LVOL_NAME, 2 * cluster_size, 16, 0xaau8)
+            .await
+            .expect("Failed to write data to volume");
+        lvol.create_snapshot(snapshot_params.clone())
+            .await
+            .expect("Failed to create the first snapshot for test volume");
+        let snapshot_list = Lvol::list_all_snapshots();
+        assert_eq!(1, snapshot_list.len(), "Snapshot Count not matched!!");
+
+        let snapshot_lvol = UntypedBdev::lookup_by_uuid_str(
+            snapshot_list
+                .get(0)
+                .unwrap()
+                .snapshot_params()
+                .snapshot_uuid()
+                .unwrap_or_default()
+                .as_str(),
+        )
+        .map(|b| Lvol::try_from(b).expect("Can't create Lvol from device"))
+        .unwrap();
+        assert_eq!(
+            lvol.usage().allocated_bytes,
+            0,
+            "Source Lvol size should be 0, after snapshot created from it"
+        );
+
+        bdev_io::write_some(LVOL_NAME, 3 * cluster_size, 16, 0xbbu8)
+            .await
+            .expect("Failed to write data to volume");
+        bdev_io::write_some(LVOL_NAME, 4 * cluster_size, 16, 0xccu8)
+            .await
+            .expect("Failed to write data to volume");
+        snapshot_lvol
+            .destroy()
+            .await
+            .expect("Destroy snapshot failed");
+        assert_eq!(
+            lvol.usage().allocated_bytes,
+            5 * cluster_size,
+            "Source Lvol size should be restored after snapshot destroy"
+        );
+    })
+    .await;
+}
+
+#[tokio::test]
+async fn test_clone_snapshot_usage_post_clone_destroy() {
+    let ms = get_ms();
+    const LVOL_NAME: &str = "lvol17";
+
+    ms.spawn(async move {
+        // Create a pool and lvol.
+        let pool = create_test_pool(
+            "pool17",
+            "malloc:///disk17?size_mb=128".to_string(),
+        )
+        .await;
+        let lvol = pool
+            .create_lvol(
+                LVOL_NAME,
+                32 * 1024 * 1024,
+                Some(&Uuid::new_v4().to_string()),
+                true,
+            )
+            .await
+            .expect("Failed to create test lvol");
+        let cluster_size = pool.blob_cluster_size();
+        // Create a test snapshot.
+        let entity_id = String::from("lvol17_e1");
+        let parent_id = lvol.uuid();
+        let txn_id = Uuid::new_v4().to_string();
+        let snap_name = String::from("lvol17_snap1");
+        let snapshot_uuid = Uuid::new_v4().to_string();
+
+        let mut snapshot_params = SnapshotParams::new(
+            Some(entity_id),
+            Some(parent_id),
+            Some(txn_id),
+            Some(snap_name),
+            Some(snapshot_uuid),
+            Some(Utc::now().to_string()),
+            false,
+        );
+
+        bdev_io::write_some(LVOL_NAME, 2 * cluster_size, 16, 0xaau8)
+            .await
+            .expect("Failed to write data to volume");
+        lvol.create_snapshot(snapshot_params.clone())
+            .await
+            .expect("Failed to create the first snapshot for test volume");
+        let snapshot_list = Lvol::list_all_snapshots();
+        assert_eq!(1, snapshot_list.len(), "Snapshot Count not matched!!");
+
+        let snapshot_lvol = UntypedBdev::lookup_by_uuid_str(
+            snapshot_list
+                .get(0)
+                .unwrap()
+                .snapshot_params()
+                .snapshot_uuid()
+                .unwrap_or_default()
+                .as_str(),
+        )
+        .map(|b| Lvol::try_from(b).expect("Can't create Lvol from device"))
+        .unwrap();
+        assert_eq!(
+            lvol.usage().allocated_bytes,
+            0,
+            "Source Lvol size should be 0, after snapshot created from it"
+        );
+
+        let clone_name = String::from("lvol17_snap1_clone_1");
+        let clone_uuid = Uuid::new_v4().to_string();
+        let source_uuid = snapshot_lvol.uuid();
+
+        let clone_param = CloneParams::new(
+            Some(clone_name),
+            Some(clone_uuid),
+            Some(source_uuid),
+            Some(Utc::now().to_string()),
+        );
+        let clone1 = snapshot_lvol
+            .create_clone(clone_param.clone())
+            .await
+            .expect("Failed to create a clone");
+        bdev_io::write_some("lvol17_snap1_clone_1", 0, 16, 0xbbu8)
+            .await
+            .expect("Failed to write data to volume");
+        snapshot_params.set_parent_id(clone1.uuid());
+        snapshot_params.set_entity_id(String::from("lvol17_clone1_e1"));
+        snapshot_params.set_name(String::from("lvol17_clone_1_snap1"));
+        snapshot_params.set_snapshot_uuid(Uuid::new_v4().to_string());
+        snapshot_params.set_txn_id(Uuid::new_v4().to_string());
+
+        clone1
+            .create_snapshot(snapshot_params.clone())
+            .await
+            .expect("Failed to create the first snapshot for test volume");
+        snapshot_params.set_parent_id(clone1.uuid());
+        snapshot_params.set_entity_id(String::from("lvol17_clone1_e2"));
+        snapshot_params.set_name(String::from("lvol17_clone_1_snap2"));
+        snapshot_params.set_snapshot_uuid(Uuid::new_v4().to_string());
+        snapshot_params.set_txn_id(Uuid::new_v4().to_string());
+        bdev_io::write_some(
+            "lvol17_snap1_clone_1",
+            3 * cluster_size,
+            16,
+            0xbbu8,
+        )
+        .await
+        .expect("Failed to write data to volume");
+        clone1
+            .create_snapshot(snapshot_params.clone())
+            .await
+            .expect("Failed to create the first snapshot for test volume");
+        let mut clone_snapshot: Vec<Lvol> = clone1
+            .list_snapshot_by_source_uuid()
+            .iter()
+            .map(|v| v.snapshot_lvol().clone())
+            .collect();
+        lvol.destroy()
+            .await
+            .expect("Original replica destroy failed");
+        clone1
+            .destroy_replica()
+            .await
+            .expect("Destroy Clone failed");
+        assert_eq!(
+            clone_snapshot.len(),
+            2,
+            "Number of Clone Snapshot not matched"
+        );
+        assert_eq!(
+            clone_snapshot.remove(0).usage().allocated_bytes,
+            cluster_size,
+            "Clone1 snap1 cache is not cleared"
+        );
+        assert_eq!(
+            clone_snapshot.remove(0).usage().allocated_bytes,
+            cluster_size,
+            "Clone1 snap2 cache is not cleared"
+        );
+        clean_snapshots(Lvol::list_all_snapshots()).await;
+    })
+    .await;
+}

nix/pkgs/libspdk/default.nix

@@ -56,13 +56,13 @@ let
   # 7. Copy SHA256 from 'got' of the error message to 'sha256' field.
   # 8. 'nix-shell' build must now succeed.
   drvAttrs = rec {
-    version = "23.01-716b1ed";
+    version = "23.01-6c81b80";
 
     src = fetchFromGitHub {
       owner = "openebs";
       repo = "spdk";
-      rev = "716b1ed489f1bc17698f7c4e5be347b0b7dc56ca";
-      sha256 = "sha256-Q9fwW8x4kPNslYjnlG08aZQ7aX8En92BAP6cXG81JaI=";
+      rev = "6c81b80f1f202160de57924549655813b28f7120";
+      sha256 = "sha256-Nwxpw25jffQ1srQh+rBNbX6z24YVAdvWg+r7nh2Ku4c=";
       fetchSubmodules = true;
     };