From afd601337e5a48ccc470296100ccd01cfc34d104 Mon Sep 17 00:00:00 2001
From: James MacMahon <james@oxide.computer>
Date: Tue, 29 Sep 2026 17:08:10 +0000
Subject: [PATCH] Properly reset block_dirty for raw extents

`flush_inner` is not called for raw extents in crucible-downstairs
binaries built with the `omicron_build` feature, those use the `syncfs`
path. This lead to block_dirty never being cleared, and a proliferation
of "extent N was in dirty_extents but not actually dirty" messages and
unnecessary syncs.
---
 downstairs/src/extent.rs              | 14 ++++++++++
 downstairs/src/extent_inner_raw.rs    | 16 ++++++++++-
 downstairs/src/extent_inner_sqlite.rs |  6 +++++
 downstairs/src/region.rs              | 38 ++++++++++++++++++++++++++-
 4 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/downstairs/src/extent.rs b/downstairs/src/extent.rs
index 1fab7b2cf..abb4ea812 100644
--- a/downstairs/src/extent.rs
+++ b/downstairs/src/extent.rs
@@ -115,6 +115,15 @@ pub(crate) trait ExtentInner: Send + Sync + Debug {
         &mut self,
         block_context: &DownstairsBlockContext,
     ) -> Result<(), CrucibleError>;
+
+    /// Return whether a specific block is dirty or not (note this is distinct
+    /// from the dirty bit that is set for the entire extent!).
+    ///
+    /// This should only be called from test functions, where we want to
+    /// assert if the Downstairs considers a block dirty after various
+    /// operations.
+    #[cfg(test)]
+    fn block_dirty(&self, block: u64) -> bool;
 }
 
 /// BlockContext, with the addition of block index and on_disk_hash
@@ -716,6 +725,11 @@ impl Extent {
     ) -> Result<Vec<Option<DownstairsBlockContext>>, CrucibleError> {
         self.inner.get_block_contexts(block, count)
     }
+
+    #[cfg(test)]
+    pub fn block_dirty(&self, block: u64) -> bool {
+        self.inner.block_dirty(block)
+    }
 }
 
 /**
diff --git a/downstairs/src/extent_inner_raw.rs b/downstairs/src/extent_inner_raw.rs
index 1a9a052cd..eb132ffd5 100644
--- a/downstairs/src/extent_inner_raw.rs
+++ b/downstairs/src/extent_inner_raw.rs
@@ -185,6 +185,13 @@ impl BlockBitArray {
     fn reset(&mut self) {
         self.data.fill(0);
     }
+
+    #[cfg(test)]
+    fn get(&self, block: u64) -> bool {
+        let (index, mask) = self.decode(block);
+        let target = &self.data[index];
+        *target & mask > 0
+    }
 }
 
 impl std::ops::Index<u64> for BlockBitArray {
@@ -562,7 +569,6 @@ impl ExtentInner for RawInner {
                 self.extent_number,
             )));
         }
-        self.block_dirty.reset();
         cdt::extent__flush__file__done!(|| {
             (job_id.get(), self.extent_number.0)
         });
@@ -575,6 +581,9 @@ impl ExtentInner for RawInner {
         _new_gen: u64,
         job_id: JobOrReconciliationId,
     ) -> Result<(), CrucibleError> {
+        // Clear block_dirty, we did a flush!
+        self.block_dirty.reset();
+
         // Check for fragmentation in the context slots leading to worse
         // performance, and defragment if that's the case.
         let extra_syscalls_per_rw = self
@@ -687,6 +696,11 @@ impl ExtentInner for RawInner {
     ) -> Result<Vec<Option<DownstairsBlockContext>>, CrucibleError> {
         RawInner::get_block_contexts(self, block, count)
     }
+
+    #[cfg(test)]
+    fn block_dirty(&self, block: u64) -> bool {
+        self.block_dirty.get(block)
+    }
 }
 
 impl RawInner {
diff --git a/downstairs/src/extent_inner_sqlite.rs b/downstairs/src/extent_inner_sqlite.rs
index 61b4964bd..f3c397ba6 100644
--- a/downstairs/src/extent_inner_sqlite.rs
+++ b/downstairs/src/extent_inner_sqlite.rs
@@ -118,6 +118,12 @@ impl ExtentInner for SqliteInner {
             .unwrap()
             .set_dirty_and_block_context(block_context)
     }
+
+    #[cfg(test)]
+    fn block_dirty(&self, block: u64) -> bool {
+        let block: usize = block as usize;
+        self.0.lock().unwrap().dirty_blocks.contains_key(&block)
+    }
 }
 
 impl SqliteInner {
diff --git a/downstairs/src/region.rs b/downstairs/src/region.rs
index 01dce10d1..409ec6e9d 100644
--- a/downstairs/src/region.rs
+++ b/downstairs/src/region.rs
@@ -3732,6 +3732,41 @@ pub(crate) mod test {
         validate_whole_region(&mut region, &data);
     }
 
+    fn test_flush_resets_block_dirty(backend: Backend) {
+        let dir = tempdir().unwrap();
+        let mut region =
+            Region::create(&dir, new_region_options(), csl()).unwrap();
+        region.extend(1, backend).unwrap();
+
+        let mut data: Vec<u8> = vec![0; region.def().total_size() as usize];
+        let writes = RegionWrite(prepare_writes(0..10, &mut data));
+
+        {
+            let ext = region.get_opened_extent_mut(ExtentId(0));
+            for i in 0..10 {
+                assert!(!ext.block_dirty(i));
+            }
+        }
+
+        region.region_write(&writes, JobId(0), false).unwrap();
+
+        {
+            let ext = region.get_opened_extent_mut(ExtentId(0));
+            for i in 0..10 {
+                assert!(ext.block_dirty(i));
+            }
+        }
+
+        region.region_flush(1, 1, &None, JobId(1), None).unwrap();
+
+        {
+            let ext = region.get_opened_extent_mut(ExtentId(0));
+            for i in 0..10 {
+                assert!(!ext.block_dirty(i));
+            }
+        }
+    }
+
     /// Macro defining the full region test suite
     ///
     /// Functions in the test suite should take a `b: Backend` parameter and
@@ -3783,7 +3818,8 @@ pub(crate) mod test {
                 test_write_single_large_contiguous_span_extents,
                 test_write_unwritten_single_large_contiguous,
                 test_write_unwritten_single_large_contiguous_span_extents,
-                test_read_single_large_contiguous_span_extents
+                test_read_single_large_contiguous_span_extents,
+                test_flush_resets_block_dirty
             );
         };