Updates for PR comments

Author: leftwo

common/src/lib.rs

@@ -159,8 +159,8 @@ pub enum CrucibleError {
     #[error("missing block context for non-empty block")]
     MissingBlockContext,
 
-    #[error("Incompatable RegionDefinition {0}")]
-    RegionIncompatable(String),
+    #[error("Incompatible RegionDefinition {0}")]
+    RegionIncompatible(String),
 }
 
 impl From<std::io::Error> for CrucibleError {
@@ -366,7 +366,7 @@ impl From<CrucibleError> for dropshot::HttpError {
             | CrucibleError::ModifyingReadOnlyRegion
             | CrucibleError::OffsetInvalid
             | CrucibleError::OffsetUnaligned
-            | CrucibleError::RegionIncompatable(_)
+            | CrucibleError::RegionIncompatible(_)
             | CrucibleError::ReplaceRequestInvalid(_)
             | CrucibleError::SnapshotExistsAlready(_)
             | CrucibleError::Unsupported(_) => {

common/src/region.rs

@@ -189,39 +189,39 @@ impl RegionDefinition {
     ) -> Result<(), CrucibleError> {
         // These fields should be the same.
         if self.block_size != other.block_size {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "block_size".to_string(),
             ));
         }
         if self.extent_size != other.extent_size {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "extent_size".to_string(),
             ));
         }
         if self.extent_count != other.extent_count {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "extent_count".to_string(),
             ));
         }
         if self.encrypted != other.encrypted {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "encrypted".to_string(),
             ));
         }
         if self.database_read_version != other.database_read_version {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "database_read_version".to_string(),
             ));
         }
         if self.database_write_version != other.database_write_version {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "database_write_version".to_string(),
             ));
         }
 
         // If the UUIDs are the same, this is invalid.
         if self.uuid == other.uuid {
-            return Err(CrucibleError::RegionIncompatable(
+            return Err(CrucibleError::RegionIncompatible(
                 "UUIDs are the same".to_string(),
             ));
         }

downstairs/src/extent.rs

@@ -144,7 +144,7 @@ impl ExtentMeta {
     }
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Copy, Clone)]
 #[allow(clippy::enum_variant_names)]
 pub enum ExtentType {
     Data,
@@ -169,7 +169,7 @@ impl fmt::Display for ExtentType {
  * FileType from the repair client.
  */
 impl ExtentType {
-    pub fn to_file_type(&self) -> FileType {
+    pub fn to_file_type(self) -> FileType {
         match self {
             ExtentType::Data => FileType::Data,
             ExtentType::Db => FileType::Db,
@@ -654,15 +654,27 @@ pub(crate) fn move_replacement_extent<P: AsRef<Path>>(
     sync_path(&original_file, log)?;
 
     // We distinguish between SQLite-backend and raw-file extents based on the
-    // presence of the `.db` file.  Under normal conditions, we should never
-    // do extent repair across different extent formats; in fact, we should
-    // never extent repair SQLite-backed extents at all, but must still
-    // handle the case of unfinished migrations.
-    // If we are replacing a downstairs, and have created the region empty
-    // with the plan to replace it, then we can have a mismatch between
-    // what files we have and what files we want to replace them with.  When
-    // repairing a downstairs, we don't know in advance if the source of our
-    // repair will be SQLite-backend or not.
+    // presence of the `.db` file.  We should never do extent repair across
+    // different extent formats; it must be SQLite-to-SQLite or raw-to-raw.
+    //
+    // It is uncommon to perform extent repair on SQLite-backed extents at all,
+    // because they are automatically migrated into raw file extents or
+    // read-only.  However, it must be supported for two cases:
+    //
+    // - If there was an unfinished replacement, we must finish that replacement
+    //   before migrating from SQLite -> raw file backend, which happens
+    //   automatically later in startup.
+    // - We use this same code path to perform clones of read-only regions,
+    //   which may be SQLite-backed (and will never migrate to raw files,
+    //   because they are read only).  This is only the case when the `clone`
+    //   argument is `true`.
+    //
+    // In the first case, we know that we are repairing an SQLite-based extent
+    // because the target (original) extent includes a `.db` file.
+    //
+    // In the second case, the target (original) extent is not present, so we
+    // check whether the new files include a `.db` file.
+
     new_file.set_extension("db");
     original_file.set_extension("db");
 
@@ -676,12 +688,6 @@ pub(crate) fn move_replacement_extent<P: AsRef<Path>>(
                 e
             );
         }
-        info!(
-            log,
-            "Moved file {:?} to {:?}",
-            new_file.clone(),
-            original_file.clone()
-        );
         sync_path(&original_file, log)?;
 
         // The .db-shm and .db-wal files may or may not exist.  If they don't
@@ -701,14 +707,11 @@ pub(crate) fn move_replacement_extent<P: AsRef<Path>>(
                     e
                 );
             }
-            info!(
-                log,
-                "Moved db-shm file {:?} to {:?}",
-                new_file.clone(),
-                original_file.clone()
-            );
             sync_path(&original_file, log)?;
         } else if original_file.exists() {
+            // If we are cloning, then our new region will have been
+            // created with Backend::RawFile, and we should have no SQLite
+            // files.
             assert!(!clone);
             info!(
                 log,
@@ -732,14 +735,11 @@ pub(crate) fn move_replacement_extent<P: AsRef<Path>>(
                     e
                 );
             }
-            info!(
-                log,
-                "Moved db-wal file {:?} to {:?}",
-                new_file.clone(),
-                original_file.clone()
-            );
             sync_path(&original_file, log)?;
         } else if original_file.exists() {
+            // If we are cloning, then our new region will have been
+            // created with Backend::RawFile, and we should have no SQLite
+            // files.
             assert!(!clone);
             info!(
                 log,

downstairs/src/lib.rs

@@ -3356,12 +3356,12 @@ pub async fn start_downstairs(
 /// Use the reconcile/repair extent methods to copy another downstairs.
 /// The source downstairs must have the same RegionDefinition as we do,
 /// and both downstairs must be running in read only mode.
-pub async fn start_clone(
+pub async fn clone_region(
     d: Arc<Mutex<Downstairs>>,
     source: SocketAddr,
 ) -> Result<()> {
     let info = crucible_common::BuildInfo::default();
-    let log = d.lock().await.log.new(o!("task" => "main".to_string()));
+    let log = d.lock().await.log.new(o!("task" => "clone".to_string()));
     info!(log, "Crucible Version: {}", info);
     info!(
         log,

downstairs/src/main.rs

@@ -298,7 +298,7 @@ async fn main() -> Result<()> {
             )
             .await?;
 
-            start_clone(d, source).await
+            clone_region(d, source).await
         }
         Args::Create {
             block_size,