diff --git a/cmd/zfs_object_agent/util/src/zettacache_stats.rs b/cmd/zfs_object_agent/util/src/zettacache_stats.rs index 63a6e47cd913..e1e47c54b77a 100644 --- a/cmd/zfs_object_agent/util/src/zettacache_stats.rs +++ b/cmd/zfs_object_agent/util/src/zettacache_stats.rs @@ -104,7 +104,9 @@ impl Sub for &StatCount { type Output = StatCount; fn sub(self, other: Self) -> Self::Output { - StatCount(AtomicU64::new(self.0.load(Relaxed) - other.0.load(Relaxed))) + StatCount(AtomicU64::new( + self.0.load(Relaxed).saturating_sub(other.0.load(Relaxed)), + )) } } @@ -144,7 +146,9 @@ impl Sub for &StatBytes { type Output = StatBytes; fn sub(self, other: Self) -> Self::Output { - StatBytes(AtomicU64::new(self.0.load(Relaxed) - other.0.load(Relaxed))) + StatBytes(AtomicU64::new( + self.0.load(Relaxed).saturating_sub(other.0.load(Relaxed)), + )) } } @@ -265,6 +269,7 @@ impl Sub for &IoStatValues { #[derive(Clone, Debug, Serialize, Deserialize)] pub struct DiskIoStats { pub name: String, + pub instance: u64, // unique instance for this disk pub stats: EnumMap, } @@ -272,6 +277,7 @@ impl DiskIoStats { pub fn new(name: String) -> DiskIoStats { DiskIoStats { name, + instance: rand::random(), stats: Default::default(), } } @@ -320,38 +326,41 @@ pub struct IoStatsRef<'a> { pub disk_stats: Vec<&'a DiskIoStats>, } -impl Sub for &IoStats { - type Output = IoStats; +impl IoStats { + pub fn max_name_len(&self) -> usize { + self.disk_stats + .iter() + .map(|stats| stats.name.len()) + .max() + .unwrap_or_default() + } - /// Subtract two IoStats. Used to create the net values between two IoStats samples. - fn sub(self, other: Self) -> IoStats { + /// Subtract two IoStats to create the net values between two samples. + /// Returns None if the two stats cannot be compared (agent restarted or a disk changed). + pub fn checked_sub(&self, other: &IoStats) -> Option { if other.disk_stats.is_empty() { - return self.clone(); + return Some(self.clone()); + } + // Avoid comparing stats when the agent was restarted or disks were changed + if self.cache_runtime_id != other.cache_runtime_id + || self.disk_stats.len() != other.disk_stats.len() + { + return None; } - assert_eq!(self.cache_runtime_id, other.cache_runtime_id); - let mut difference = IoStats { timestamp: self.timestamp - other.timestamp, ..Default::default() }; - - assert_eq!(self.disk_stats.len(), other.disk_stats.len()); for (self_stat, other_stat) in self.disk_stats.iter().zip(other.disk_stats.iter()) { - assert_eq!(self_stat.name, other_stat.name); + // Avoid comparing stats when disks have changed + if self_stat.instance != other_stat.instance { + return None; + } + // We're now confident that stats are comparable so the underlying stat subtraction + // (i.e. StatCount or StatBytes) is a saturating_sub() difference.disk_stats.push(self_stat - other_stat); } - - difference - } -} - -impl IoStats { - pub fn max_name_len(&self) -> usize { - self.disk_stats - .iter() - .map(|stats| stats.name.len()) - .max() - .unwrap_or_default() + Some(difference) } } diff --git a/cmd/zfs_object_agent/zcache/src/iostat.rs b/cmd/zfs_object_agent/zcache/src/iostat.rs index f928d73b3761..be3e5f66272c 100644 --- a/cmd/zfs_object_agent/zcache/src/iostat.rs +++ b/cmd/zfs_object_agent/zcache/src/iostat.rs @@ -1,6 +1,9 @@ //! The iostat subcommand for zcache. use std::cmp::max; +use std::collections::hash_map::DefaultHasher; +use std::hash::Hash; +use std::hash::Hasher; use std::sync::atomic::Ordering::Relaxed; use std::thread::sleep; use std::time::Duration; @@ -335,11 +338,7 @@ impl IoStatDisplay { debug!("{latest:?}"); - if previous.disk_stats.is_empty() - || latest.cache_runtime_id == previous.cache_runtime_id - { - let delta = &latest - &previous; - + if let Some(delta) = latest.checked_sub(&previous) { match &self.histogram_name { None => self.display_iostat_default(iteration, &delta), Some(name) => self.display_iostat_histogram(name, &delta), @@ -347,7 +346,7 @@ impl IoStatDisplay { // Flush stdout in case output is redirected to a file flush_stdout()?; } else { - info!("object agent restarted"); + info!("object agent restarted or disks changed"); } iteration += 1; @@ -371,11 +370,19 @@ impl IoStatDisplay { } fn insert_summary_disk(disk_stats: &mut IoStats) { - let mut summary = DiskIoStats::new("summary".to_string()); + let mut summary = DiskIoStats { + name: "summary".to_string(), + instance: 0, + stats: Default::default(), + }; + let mut hasher = DefaultHasher::new(); for disk in &disk_stats.disk_stats { summary += disk; + disk.instance.hash(&mut hasher); } + // Since the summay disk is compared first, make its instance depend on every disk + summary.instance = hasher.finish(); disk_stats.disk_stats.insert(0, summary); } }