Change destroy_fileysystems to work with merging

Move all the functionality into thinpool implementation. Do more checks to avoid deletings filesystems that shouldn't be deleted. Check for situations where multiple snapshots are referring to the same deleted filesystem. Remove a now unused function. Signed-off-by: mulhern <[email protected]>
stratis-storage · Aug 15, 2024 · 7d9664d · 7d9664d
1 parent 2b37186
commit 7d9664d
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 79 deletions.
diff --git a/src/engine/strat_engine/pool/v1.rs b/src/engine/strat_engine/pool/v1.rs
@@ -1023,36 +1023,7 @@ impl Pool for StratPool {
         pool_name: &str,
         fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
-        let mut removed = Vec::new();
-        for &uuid in fs_uuids {
-            if let Some(uuid) = self.thin_pool.destroy_filesystem(pool_name, uuid)? {
-                removed.push(uuid);
-            }
-        }
-
-        let snapshots: Vec<FilesystemUuid> = self
-            .thin_pool
-            .filesystems()
-            .iter()
-            .filter_map(|(_, u, fs)| {
-                fs.origin()
-                    .and_then(|x| if removed.contains(&x) { Some(*u) } else { None })
-            })
-            .collect();
-
-        let mut updated_origins = vec![];
-        for sn_uuid in snapshots {
-            if let Err(err) = self.thin_pool.unset_fs_origin(sn_uuid) {
-                warn!(
-                    "Failed to write null origin to metadata for filesystem with UUID {}: {}",
-                    sn_uuid, err
-                );
-            } else {
-                updated_origins.push(sn_uuid);
-            }
-        }
-
-        Ok(SetDeleteAction::new(removed, updated_origins))
+        self.thin_pool.destroy_filesystems(pool_name, fs_uuids)
     }
 
     #[pool_mutating_action("NoRequests")]

diff --git a/src/engine/strat_engine/pool/v2.rs b/src/engine/strat_engine/pool/v2.rs
@@ -928,36 +928,7 @@ impl Pool for StratPool {
         pool_name: &str,
         fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
-        let mut removed = Vec::new();
-        for &uuid in fs_uuids {
-            if let Some(uuid) = self.thin_pool.destroy_filesystem(pool_name, uuid)? {
-                removed.push(uuid);
-            }
-        }
-
-        let snapshots: Vec<FilesystemUuid> = self
-            .thin_pool
-            .filesystems()
-            .iter()
-            .filter_map(|(_, u, fs)| {
-                fs.origin()
-                    .and_then(|x| if removed.contains(&x) { Some(*u) } else { None })
-            })
-            .collect();
-
-        let mut updated_origins = vec![];
-        for sn_uuid in snapshots {
-            if let Err(err) = self.thin_pool.unset_fs_origin(sn_uuid) {
-                warn!(
-                    "Failed to write null origin to metadata for filesystem with UUID {}: {}",
-                    sn_uuid, err
-                );
-            } else {
-                updated_origins.push(sn_uuid);
-            }
-        }
-
-        Ok(SetDeleteAction::new(removed, updated_origins))
+        self.thin_pool.destroy_filesystems(pool_name, fs_uuids)
     }
 
     #[pool_mutating_action("NoRequests")]

diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs
@@ -24,7 +24,7 @@ use devicemapper::{
 
 use crate::{
     engine::{
-        engine::{DumpState, StateDiff},
+        engine::{DumpState, Filesystem, StateDiff},
         strat_engine::{
             backstore::backstore::{v1, v2, InternalBackstore},
             cmd::{thin_check, thin_metadata_size, thin_repair},
@@ -38,7 +38,10 @@ use crate::{
             writing::wipe_sectors,
         },
         structures::Table,
-        types::{Compare, Diff, FilesystemUuid, Name, PoolUuid, StratFilesystemDiff, ThinPoolDiff},
+        types::{
+            Compare, Diff, FilesystemUuid, Name, PoolUuid, SetDeleteAction, StratFilesystemDiff,
+            ThinPoolDiff,
+        },
     },
     stratis::{StratisError, StratisResult},
 };
@@ -603,7 +606,7 @@ impl<B> ThinPool<B> {
     /// * Ok(Some(uuid)) provides the uuid of the destroyed filesystem
     /// * Ok(None) is returned if the filesystem did not exist
     /// * Err(_) is returned if the filesystem could not be destroyed
-    pub fn destroy_filesystem(
+    fn destroy_filesystem(
         &mut self,
         pool_name: &str,
         uuid: FilesystemUuid,
@@ -765,6 +768,64 @@ impl<B> ThinPool<B> {
         )
         .map_err(|e| e.into())
     }
+
+    pub fn destroy_filesystems(
+        &mut self,
+        pool_name: &str,
+        fs_uuids: &HashSet<FilesystemUuid>,
+    ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
+        let snapshots = self
+            .filesystems()
+            .iter()
+            .filter_map(|(_, u, fs)| {
+                fs.origin().and_then(|x| {
+                    if fs_uuids.contains(&x) {
+                        Some((x, (*u, fs.merge_scheduled())))
+                    } else {
+                        None
+                    }
+                })
+            })
+            .fold(HashMap::new(), |mut acc, (u, v)| {
+                acc.entry(u)
+                    .and_modify(|e: &mut Vec<_>| e.push(v))
+                    .or_insert(vec![v]);
+                acc
+            });
+
+        let scheduled_for_merge = snapshots
+            .iter()
+            .filter(|(_, snaps)| snaps.iter().any(|(_, scheduled)| *scheduled))
+            .collect::<Vec<_>>();
+        if !scheduled_for_merge.is_empty() {
+            let err_str = format!("The filesystem destroy operation can not be begun until the revert operations for the following filesystem snapshots have been cancelled: {}", scheduled_for_merge.iter().map(|(u, _)| u.to_string()).collect::<Vec<_>>().join(", "));
+            return Err(StratisError::Msg(err_str));
+        }
+
+        let (mut removed, mut updated_origins) = (Vec::new(), Vec::new());
+        for &uuid in fs_uuids {
+            if let Some(uuid) = self.destroy_filesystem(pool_name, uuid)? {
+                removed.push(uuid);
+
+                let snaps = snapshots
+                    .get(&uuid)
+                    .expect("snapshots map construction ensures key is present");
+                // The filesystems in snaps may have been removed; any one of
+                // them may also be a filesystem that was scheduled for removal.
+                for (sn_uuid, _) in snaps {
+                    if let Some((_, sn)) = self.get_mut_filesystem_by_uuid(*sn_uuid) {
+                        assert!(sn.unset_origin());
+                        updated_origins.push(*sn_uuid);
+
+                        let (name, sn) = self.get_filesystem_by_uuid(*sn_uuid).expect("just got");
+                        self.mdv.save_fs(&name, *sn_uuid, sn)?;
+                    };
+                }
+            }
+        }
+
+        Ok(SetDeleteAction::new(removed, updated_origins))
+    }
 }
 
 impl ThinPool<v1::Backstore> {
@@ -1737,22 +1798,6 @@ where
         Ok(changed)
     }
 
-    pub fn unset_fs_origin(&mut self, fs_uuid: FilesystemUuid) -> StratisResult<bool> {
-        let changed = {
-            let (_, fs) = self.get_mut_filesystem_by_uuid(fs_uuid).ok_or_else(|| {
-                StratisError::Msg(format!("No filesystem with UUID {fs_uuid} found"))
-            })?;
-            fs.unset_origin()
-        };
-        if changed {
-            let (name, fs) = self
-                .get_filesystem_by_uuid(fs_uuid)
-                .expect("Looked up above.");
-            self.mdv.save_fs(&name, fs_uuid, fs)?;
-        }
-        Ok(changed)
-    }
-
     /// Set the filesystem merge scheduled value for filesystem with given UUID.
     pub fn set_fs_merge_scheduled(
         &mut self,