From 03e19a3d9f628bc22649d9f666c58c80ebe00452 Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 24 Jul 2024 15:33:23 -0400 Subject: [PATCH] Change destroy_filesystems 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 --- src/engine/sim_engine/pool.rs | 63 +++++++++------ src/engine/strat_engine/pool/v1.rs | 31 +------- src/engine/strat_engine/pool/v2.rs | 31 +------- src/engine/strat_engine/thinpool/thinpool.rs | 83 +++++++++++++++----- 4 files changed, 105 insertions(+), 103 deletions(-) diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs index 20a8caabce..976f742295 100644 --- a/src/engine/sim_engine/pool.rs +++ b/src/engine/sim_engine/pool.rs @@ -125,13 +125,6 @@ impl SimPool { } } - fn filesystems_mut(&mut self) -> Vec<(Name, FilesystemUuid, &mut SimFilesystem)> { - self.filesystems - .iter_mut() - .map(|(name, uuid, x)| (name.clone(), *uuid, x)) - .collect() - } - pub fn record(&self, name: &str) -> PoolSave { PoolSave { name: name.to_owned(), @@ -489,30 +482,52 @@ impl Pool for SimPool { _pool_name: &str, fs_uuids: &HashSet, ) -> StratisResult> { - let mut removed = Vec::new(); - for &uuid in fs_uuids { - if self.filesystems.remove_by_uuid(uuid).is_some() { - removed.push(uuid); - } - } - - let updated_origins: Vec = self - .filesystems_mut() - .iter_mut() + let snapshots = self + .filesystems() + .iter() .filter_map(|(_, u, fs)| { fs.origin().and_then(|x| { - if removed.contains(&x) { - if fs.unset_origin() { - Some(*u) - } else { - None - } + if fs_uuids.contains(&x) { + Some((x, (*u, fs.merge_scheduled()))) } else { None } }) }) - .collect(); + .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::>(); + 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::>().join(", ")); + return Err(StratisError::Msg(err_str)); + } + + let (mut removed, mut updated_origins) = (Vec::new(), Vec::new()); + for &uuid in fs_uuids { + if self.filesystems.remove_by_uuid(uuid).is_some() { + 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.filesystems.get_mut_by_uuid(*sn_uuid) { + assert!(sn.unset_origin()); + updated_origins.push(*sn_uuid); + }; + } + } + } Ok(SetDeleteAction::new(removed, updated_origins)) } diff --git a/src/engine/strat_engine/pool/v1.rs b/src/engine/strat_engine/pool/v1.rs index dfea378c55..3a5987ed02 100644 --- 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, ) -> StratisResult> { - 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 = 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 index 38554ff888..2d68869261 100644 --- 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, ) -> StratisResult> { - 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 = 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 index e1e4831e84..654409aa51 100644 --- 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 ThinPool { /// * 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 ThinPool { ) .map_err(|e| e.into()) } + + pub fn destroy_filesystems( + &mut self, + pool_name: &str, + fs_uuids: &HashSet, + ) -> StratisResult> { + 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::>(); + 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::>().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 { @@ -1737,22 +1798,6 @@ where Ok(changed) } - pub fn unset_fs_origin(&mut self, fs_uuid: FilesystemUuid) -> StratisResult { - 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,