DLPX-81000 Generate Disk GUID for ZettaCache Devices (openzfs#434)

Side-Change: Also record sector_size on-disk

cmd/zfs_object_agent/server/src/main.rs

@@ -11,6 +11,7 @@ use util::writeln_stderr;
 use util::TrackingAllocator;
 use util::ALLOCATOR_PRINT_MIN_ALLOCS;
 use util::ALLOCATOR_PRINT_MIN_BYTES;
+use zettacache::base_types::CacheGuid;
 use zettacache::CacheOpenMode;
 use zettaobject::object_access::BlobCredentials;
 use zettaobject::object_access::ObjectAccessProtocol;
@@ -90,7 +91,7 @@ struct Cli {
         value_name = "GUID",
         conflicts_with = "cache-device"
     )]
-    guid: Option<u64>,
+    guid: Option<CacheGuid>,
 
     /// Clear the cache when it has incompatible features
     #[clap(long)]

cmd/zfs_object_agent/zcdb/src/main.rs

@@ -3,6 +3,7 @@ use std::path::PathBuf;
 use clap::Parser;
 use clap::Subcommand;
 use git_version::git_version;
+use zettacache::base_types::CacheGuid;
 use zettacache::CacheOpenMode;
 use zettacache::DumpSlabsOptions;
 use zettacache::DumpStructuresOptions;
@@ -33,7 +34,7 @@ struct Cli {
 
     /// Specific cache GUID to look for in cache device directory
     #[clap(short = 'g', long, value_name = "GUID")]
-    guid: Option<u64>,
+    guid: Option<CacheGuid>,
 
     /// Sets the verbosity level for logging and debugging
     #[clap(short = 'v', long, parse(from_occurrences), global = true)]

cmd/zfs_object_agent/zettacache/src/base_types.rs

@@ -2,8 +2,10 @@ use std::borrow::Borrow;
 use std::cmp::min;
 use std::fmt::*;
 use std::num::NonZeroU64;
+use std::num::ParseIntError;
 use std::ops::Add;
 use std::ops::Sub;
+use std::str::FromStr;
 
 use more_asserts::*;
 use serde::Deserialize;
@@ -18,6 +20,38 @@ impl Display for PoolGuid {
     }
 }
 
+#[derive(Serialize, Deserialize, Debug, Clone, Copy, Eq, PartialEq, Hash)]
+pub struct CacheGuid(pub u64);
+impl CacheGuid {
+    pub fn new() -> Self {
+        CacheGuid(rand::random())
+    }
+}
+impl Default for CacheGuid {
+    fn default() -> Self {
+        CacheGuid::new()
+    }
+}
+impl FromStr for CacheGuid {
+    type Err = ParseIntError;
+    fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
+        s.parse().map(CacheGuid)
+    }
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)]
+pub struct DiskGuid(pub u64);
+impl DiskGuid {
+    pub fn new() -> Self {
+        DiskGuid(rand::random())
+    }
+}
+impl Default for DiskGuid {
+    fn default() -> Self {
+        DiskGuid::new()
+    }
+}
+
 #[derive(Serialize, Deserialize, Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd, Hash)]
 pub struct BlockId(pub u64);
 impl Display for BlockId {

cmd/zfs_object_agent/zettacache/src/features.rs

@@ -28,6 +28,7 @@ lazy_static! {
         SLAB_ALLOCATOR.clone(),
         SLAB_SIZE_32MB.clone(),
         TRIMMED_INDEX.clone(),
+        DISK_GUIDS.clone(),
     ]
     .map(|feature: Feature| (feature.name, feature.info))
     .into_iter()
@@ -48,6 +49,10 @@ lazy_static! {
         name: FeatureName("com.delphix:trimmed_index".to_string()),
         info: FeatureType::Upgradeable
     };
+    pub static ref DISK_GUIDS: Feature = Feature {
+        name: FeatureName("com.delphix:disk_guids".to_string()),
+        info: FeatureType::Upgradeable
+    };
 }
 
 #[derive(Debug)]

cmd/zfs_object_agent/zettacache/src/open.rs

@@ -16,15 +16,17 @@ use tokio::fs::File;
 use tokio::io::AsyncReadExt;
 use util::message::SUPERBLOCK_SIZE;
 
+use crate::base_types::CacheGuid;
+use crate::base_types::DiskGuid;
 use crate::base_types::DiskId;
 use crate::block_access::BlockAccess;
 use crate::superblock::SuperblockPhys;
 
 #[derive(Debug, Clone)]
 pub enum CacheOpenMode {
     DeviceList(Vec<PathBuf>),
-    DiscoveryDirectory(PathBuf, Option<u64>), // u64 is the cache guid
-    None,                                     // no zettacache
+    DiscoveryDirectory(PathBuf, Option<CacheGuid>),
+    None, // no zettacache
 }
 
 impl CacheOpenMode {
@@ -73,8 +75,8 @@ impl DiscoveredDevice {
     }
 }
 
-async fn discover_devices(dir_path: &Path, target_guid: Option<u64>) -> Result<Vec<PathBuf>> {
-    let mut caches = HashMap::<u64, BTreeMap<DiskId, DiscoveredDevice>>::new();
+async fn discover_devices(dir_path: &Path, target_guid: Option<CacheGuid>) -> Result<Vec<PathBuf>> {
+    let mut caches = HashMap::<_, BTreeMap<_, _>>::new();
 
     let mut discovery = FuturesUnordered::new();
     let mut canonical_entries = HashSet::new();
@@ -121,28 +123,26 @@ async fn discover_devices(dir_path: &Path, target_guid: Option<u64>) -> Result<V
         match result {
             Ok(device) => {
                 debug!("discovery: found device: {device:?}");
-                let cache_guid = device.superblock.guid;
+                let cache_guid = device.superblock.cache_guid;
                 let cache = caches.entry(cache_guid).or_default();
-                if let Some(old_device) = cache.insert(device.superblock.disk, device) {
-                    // If we crash in the middle of a zcache-add for one device
-                    // and then do zcache-add for another device, we may get
-                    // into a situation where discovery runs into two devices
-                    // with the same DiskID and cache GUID. Until we make cache
-                    // device import more deterministic (see DLPX-81000) error
-                    // out.
+                let disk_guid = device.superblock.disk_guid;
+                if let Some(old_device) = cache.insert((device.superblock.disk, disk_guid), device)
+                {
                     return Err(anyhow!(
-                        "found two disks with {:?} for cache {cache_guid}",
-                        old_device.superblock.disk
+                        "found two disks with {:?} for {cache_guid:?} with {disk_guid:?}",
+                        old_device.superblock.disk,
                     ));
                 }
             }
             Err(why) => debug!("discovery: error: {why:?}"),
         };
     }
-    filter_invalid_caches(&mut caches);
+
+    caches.retain(|&guid, disks| is_valid_cache(guid, disks));
     if let Some(guid) = target_guid {
         caches.retain(|cache_guid, _| *cache_guid == guid);
     }
+
     match caches.values().next() {
         Some(cache) => {
             if caches.len() > 1 {
@@ -161,25 +161,51 @@ async fn discover_devices(dir_path: &Path, target_guid: Option<u64>) -> Result<V
     }
 }
 
-fn filter_invalid_caches(caches: &mut HashMap<u64, BTreeMap<DiskId, DiscoveredDevice>>) {
-    // Only retain caches that have a primary block
-    caches.retain(|_, disks| disks.values().any(|disk| disk.superblock.primary.is_some()));
-
-    // Only retain caches whose devices we've discovered
-    caches.retain(|cache_guid, disks| {
-        let mut disk_ids_from_discovery = HashSet::new();
-        let mut disk_ids_from_primary = HashSet::new();
-        for (&disk_id, disk) in disks {
-            disk_ids_from_discovery.insert(disk_id);
-            if let Some(primary) = &disk.superblock.primary {
-                disk_ids_from_primary = primary.disks.keys().copied().collect();
-            }
+fn is_valid_cache(
+    cache_guid: CacheGuid,
+    disks: &mut BTreeMap<(DiskId, Option<DiskGuid>), DiscoveredDevice>,
+) -> bool {
+    let primary = disks
+        .values()
+        .find(|&disk| disk.superblock.primary.is_some());
+
+    match primary {
+        Some(disk) => {
+            let disks_from_primary = disk
+                .superblock
+                .primary
+                .as_ref()
+                .unwrap()
+                .disks
+                .iter()
+                .map(|(k, v)| (*k, v.guid))
+                .collect::<HashMap<_, _>>();
+
+            // Only retain disks with IDs that are part of the primary block
+            disks.retain(
+                |(id, disk_guid), _| match (disks_from_primary.get(id), *disk_guid) {
+                    (Some(&guid_from_primary), Some(guid_from_superblock)) => {
+                        guid_from_superblock == guid_from_primary
+                    }
+                    (Some(_guid_from_primary), None) => true,
+                    (None, _) => false,
+                },
+            );
+
+            // If we are missing any device mentioned in the primary block,
+            // then this cache is invalid
+            let disks_ids_from_discovery = disks
+                .keys()
+                .map(|(id, _)| id)
+                .copied()
+                .collect::<HashSet<_>>();
+            let disk_ids_from_primary = disks_from_primary.keys().copied().collect::<HashSet<_>>();
+            disk_ids_from_primary
+                .difference(&disks_ids_from_discovery)
+                .inspect(|id| info!("cache {cache_guid:?} can't find {id:?}"))
+                .count()
+                == 0
         }
-        assert!(!disk_ids_from_primary.is_empty());
-        disk_ids_from_primary
-            .difference(&disk_ids_from_discovery)
-            .inspect(|id| info!("cache {cache_guid} can't find {id:?}"))
-            .count()
-            == 0
-    });
+        None => false,
+    }
 }