Merge #1489

1489: fix(snapshot): use customize function to judge snapshot inplace of spdk native function r=hrudaya21 a=hrudaya21 To Judge if a Lvol is a snapshot, the custom xattr is checked now. Before it was using native SPDK function. Co-authored-by: Hrudaya <[email protected]>
openebs · Aug 14, 2023 · 956a027 · 956a027
2 parents ac45bb4 + be7e44e
commit 956a027
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 5 deletions.
diff --git a/io-engine/src/lvs/lvol_snapshot.rs b/io-engine/src/lvs/lvol_snapshot.rs
@@ -429,7 +429,6 @@ impl SnapshotOps for Lvol {
                     if let Some(parent_lvol) = parent {
                         // Skip snapshots if it's parent is not matched.
                         if curr_attr_val != parent_lvol.uuid() {
-                            warn!("presisted parent ?curr_attr_val not matched to input parent ?parent_lvol.uuid()");
                             return None;
                         }
                     }
@@ -589,6 +588,7 @@ impl SnapshotOps for Lvol {
                 if !snapshot_lvol.is_snapshot() {
                     continue;
                 }
+
                 match snapshot_lvol.snapshot_descriptor(Some(self)) {
                     Some(snapshot_descriptor) => {
                         snapshot_list.push(snapshot_descriptor)

diff --git a/io-engine/src/lvs/lvs_lvol.rs b/io-engine/src/lvs/lvs_lvol.rs
@@ -21,7 +21,6 @@ use spdk_rs::libspdk::{
     spdk_blob_get_num_clusters_ancestors,
     spdk_blob_get_xattr_value,
     spdk_blob_is_read_only,
-    spdk_blob_is_snapshot,
     spdk_blob_is_thin_provisioned,
     spdk_blob_set_xattr,
     spdk_blob_sync_md,
@@ -48,6 +47,7 @@ use crate::{
         ShareProps,
         SnapshotOps,
         SnapshotParams,
+        SnapshotXattrs,
         ToErrno,
         UntypedBdev,
         UpdateProps,
@@ -716,8 +716,15 @@ impl LvsLvol for Lvol {
     }
 
     /// Returns a boolean indicating if the lvol is a snapshot.
+    /// Currently in place of SPDK native API to judge lvol as snapshot, xattr
+    /// is checked here. When there is only single Lvol(snapshot) present in
+    /// the system and there is restart of io-engine and pool import
+    /// happens, SPDK native API consider lvol(snapshot) as normal lvol.
+    /// Looks like a bug in SPDK, but all snapshot attribute are intact in
+    /// SPDK after io-engine restarts.
     fn is_snapshot(&self) -> bool {
-        unsafe { spdk_blob_is_snapshot(self.blob_checked()) }
+        Lvol::get_blob_xattr(self, SnapshotXattrs::SnapshotCreateTime.name())
+            .is_some()
     }
 
     /// Lvol is considered as clone if its sourceuuid attribute is a valid

diff --git a/io-engine/tests/snapshot_lvol.rs b/io-engine/tests/snapshot_lvol.rs
@@ -451,10 +451,26 @@ async fn test_lvol_list_snapshot() {
             .await
             .expect("Failed to create a snapshot");
 
-        let snapshot_list = lvol.list_snapshot_by_source_uuid();
+        let mut snapshot_list = lvol.list_snapshot_by_source_uuid();
         info!("Total number of snapshots: {}", snapshot_list.len());
         assert_eq!(2, snapshot_list.len(), "Snapshot Count not matched!!");
-        clean_snapshots(snapshot_list).await;
+        lvol.destroy()
+            .await
+            .expect("Failed to destroy the original replica");
+        let snap_lvol_1 = snapshot_list.remove(0).snapshot_lvol;
+        let snap_lvol_2 = snapshot_list.remove(0).snapshot_lvol;
+        snap_lvol_1
+            .destroy()
+            .await
+            .expect("Failed to destroy first snapshot");
+        assert!(
+            snap_lvol_2.is_snapshot(),
+            "It is a snapshot, wrongly recognized as normal replica"
+        );
+        snap_lvol_2
+            .destroy()
+            .await
+            .expect("Failed to destroy last snapshot");
     })
     .await;
 }