Merge pull request #3639 from mulkieran/issue_stratisd_3634

Ensured filesystem Used signal is sent if used value has changed on filesystem check

Author: mulkieran

src/engine/strat_engine/thinpool/filesystem.rs

@@ -258,20 +258,36 @@ impl StratFilesystem {
         }
     }
 
-    /// Check the filesystem usage and determine whether it should extend.
+    /// Check the filesystem usage and determine whether it should extend
+    /// or just update. If extend, return the amount to extend.
     ///
     /// Returns:
-    /// * Some(mount_point) if the filesystem should be extended
-    /// * None if the filesystem does not need to be extended
-    pub fn should_extend(&self) -> Option<PathBuf> {
-        fn should_extend_fail(fs: &StratFilesystem) -> StratisResult<Option<PathBuf>> {
+    /// * Some(_) if the filesystem is mounted and should be visited
+    /// * Some((_, 0)) if the filesystem does not need to be extended
+    /// * None if the filesystem is unmounted and should not be visited
+    pub fn visit_values(
+        &self,
+        no_op_remaining_size: Option<&mut Sectors>,
+    ) -> Option<(PathBuf, Sectors)> {
+        fn visit_values_fail(
+            fs: &StratFilesystem,
+            no_op_remaining_size: Option<&mut Sectors>,
+        ) -> StratisResult<Option<(PathBuf, Sectors)>> {
             match fs.thin_dev.status(get_dm(), DmOptions::default())? {
                 ThinStatus::Working(_) => {
                     if let Some(mount_point) = fs.mount_points()?.first() {
-                        let (fs_total_bytes, fs_total_used_bytes) = fs_usage(mount_point)?;
-                        if 2u64 * fs_total_used_bytes > fs_total_bytes {
-                            return Ok(Some(mount_point.clone()));
-                        }
+                        return fs_usage(mount_point).map(
+                            |(fs_total_bytes, fs_total_used_bytes)| {
+                                Some((
+                                    mount_point.clone(),
+                                    if 2u64 * fs_total_used_bytes > fs_total_bytes {
+                                        fs.extend_size(no_op_remaining_size)
+                                    } else {
+                                        Sectors(0)
+                                    },
+                                ))
+                            },
+                        );
                     }
                     Ok(None)
                 }
@@ -286,54 +302,56 @@ impl StratFilesystem {
             }
         }
 
-        match should_extend_fail(self) {
+        match visit_values_fail(self, no_op_remaining_size) {
             Ok(mt_pt) => mt_pt,
             Err(e) => {
                 warn!(
-                    "Checking whether the filesystem should be extended failed: {}; ignoring",
+                    "Checking whether the filesystem should be visited failed: {}; ignoring",
                     e
                 );
                 None
             }
         }
     }
 
-    /// Handle the extension once the filesystem has been determined to be getting full
-    /// and needs to be extended.
+    /// Handle filesystem changes while checking, including updating cached
+    /// state.
     ///
-    /// Precondition: should_extend has returned Some(_) before invocation.
-    pub fn handle_extension(
+    /// If extend_size is 0, it is not necessary to extend the filesystem,
+    /// but the state may have changed.
+    pub fn handle_fs_changes(
         &mut self,
         mount_point: &Path,
         extend_size: Sectors,
     ) -> StratisResult<StratFilesystemDiff> {
         let original_state = self.cached();
 
-        let old_table = self.thin_dev.table().table.clone();
-        let mut new_table = old_table.clone();
-        new_table.length = original_state.size.sectors() + extend_size;
-        self.thin_dev.set_table(get_dm(), new_table)?;
-        if let Err(causal) = xfs_growfs(mount_point) {
-            if let Err(rollback) = self.thin_dev.set_table(get_dm(), old_table) {
-                return Err(StratisError::RollbackError {
-                    causal_error: Box::new(causal),
-                    rollback_error: Box::new(StratisError::from(rollback)),
-                    level: ActionAvailability::NoPoolChanges,
-                });
-            } else {
-                return Err(causal);
+        if extend_size > Sectors(0) {
+            let old_table = self.thin_dev.table().table.clone();
+            let mut new_table = old_table.clone();
+            new_table.length = original_state.size.sectors() + extend_size;
+            self.thin_dev.set_table(get_dm(), new_table)?;
+            if let Err(causal) = xfs_growfs(mount_point) {
+                if let Err(rollback) = self.thin_dev.set_table(get_dm(), old_table) {
+                    return Err(StratisError::RollbackError {
+                        causal_error: Box::new(causal),
+                        rollback_error: Box::new(StratisError::from(rollback)),
+                        level: ActionAvailability::NoPoolChanges,
+                    });
+                } else {
+                    return Err(causal);
+                }
             }
         }
 
         Ok(original_state.diff(&self.dump(())))
     }
 
     /// Return an extend size for the thindev under the filesystem
-    pub fn extend_size(
-        current_size: Sectors,
-        no_op_remaining_size: Option<&mut Sectors>,
-        fs_limit_remaining_size: Option<Sectors>,
-    ) -> Sectors {
+    /// If no_op_remaining_size is None, then the pool allows overprovisioning.
+    fn extend_size(&self, no_op_remaining_size: Option<&mut Sectors>) -> Sectors {
+        let current_size = self.thindev_size();
+        let fs_limit_remaining_size = self.size_limit().map(|sl| sl - self.thindev_size());
         match (no_op_remaining_size, fs_limit_remaining_size) {
             (Some(no_op_rem_size), Some(fs_lim_rem_size)) => {
                 let extend_size = min(min(*no_op_rem_size, current_size), fs_lim_rem_size);

src/engine/strat_engine/thinpool/thinpool.rs

@@ -23,7 +23,7 @@ use devicemapper::{
 
 use crate::{
     engine::{
-        engine::{DumpState, Filesystem, StateDiff},
+        engine::{DumpState, StateDiff},
         strat_engine::{
             backstore::Backstore,
             cmd::{thin_check, thin_metadata_size, thin_repair},
@@ -682,10 +682,6 @@ impl ThinPool {
             None
         };
 
-        if let Some(Sectors(0)) = remaining_space.as_ref() {
-            return Ok(HashMap::default());
-        };
-
         scope(|s| {
             // This collect is needed to ensure all threads are spawned in
             // parallel, not each thread being spawned and immediately joined
@@ -696,24 +692,12 @@ impl ThinPool {
                 .filesystems
                 .iter_mut()
                 .filter_map(|(name, uuid, fs)| {
-                    if let Some(mt_pt) = fs.should_extend() {
-                        let extend_size = StratFilesystem::extend_size(
-                            fs.thindev_size(),
-                            remaining_space.as_mut(),
-                            fs.size_limit().map(|sl| sl - fs.thindev_size()),
-                        );
-                        if extend_size == Sectors(0) {
-                            None
-                        } else {
-                            Some((name, *uuid, fs, mt_pt, extend_size))
-                        }
-                    } else {
-                        None
-                    }
+                    fs.visit_values(remaining_space.as_mut())
+                        .map(|(mt_pt, extend_size)| (name, *uuid, fs, mt_pt, extend_size))
                 })
                 .map(|(name, uuid, fs, mt_pt, extend_size)| {
                     s.spawn(move || -> StratisResult<_> {
-                        let diff = fs.handle_extension(&mt_pt, extend_size)?;
+                        let diff = fs.handle_fs_changes(&mt_pt, extend_size)?;
                         Ok((name, uuid, fs, diff))
                     })
                 })
@@ -724,15 +708,19 @@ impl ThinPool {
                 .filter_map(|h| {
                     h.join()
                         .map_err(|_| {
-                            warn!("Failed to get status of filesystem extension");
+                            warn!("Failed to get status of filesystem operation");
                         })
                         .ok()
                 })
                 .fold(Vec::new(), |mut acc, res| {
                     match res {
                         Ok((name, uuid, fs, diff)) => {
-                            updated.insert(uuid, diff);
-                            acc.push((name, uuid, fs));
+                            if diff.size.is_changed() {
+                                acc.push((name, uuid, fs));
+                            }
+                            if diff.size.is_changed() || diff.used.is_changed() {
+                                updated.insert(uuid, diff);
+                            }
                         }
                         Err(e) => {
                             warn!("Failed to extend filesystem: {}", e);