Make IOop use absolute block indices (#1693)

mkeeter · web-flow · commit e59e3fe62e36 · 2025-04-03T12:04:20.000-04:00
Following up from #1685, the upstairs only cares about the block → extent mapping when checking live-repair stuff. However, all of our `IOop` variants store `(ExtentId, BlockOffset)` tuples. This PR makes the `IOop` more compact by using an absolute `BlockIndex` instead, then unpacks it into an `(ExtentId, BlockOffset)` only when needed for live-repair checks.
diff --git a/upstairs/src/downstairs.rs b/upstairs/src/downstairs.rs
@@ -26,7 +26,7 @@ use crate::{
     RawWrite, ReconcileIO, ReconciliationId, RegionDefinition, ReplaceResult,
     SnapshotDetails, WorkSummary,
 };
-use crucible_common::{BlockIndex, BlockOffset, ExtentId, NegotiationError};
+use crucible_common::{BlockIndex, ExtentId, NegotiationError};
 use crucible_protocol::WriteHeader;
 
 use ringbuffer::RingBuffer;
@@ -1510,8 +1510,7 @@ impl Downstairs {
 
         let aread = IOop::Read {
             dependencies,
-            start_eid: ExtentId(0),
-            start_offset: BlockOffset(7),
+            start_block: BlockIndex(7),
             count: 1,
             block_size: 512,
         };
@@ -1565,25 +1564,17 @@ impl Downstairs {
 
         // TODO: can anyone actually give us an empty write?
         let start = blocks.start().unwrap_or(BlockIndex(0));
-
-        // XXX change IOop to take `BlockIndex` instead?
-        let extent_size = self.ddef.unwrap().extent_size().value;
-        let start_eid = ExtentId((start.0 / extent_size) as u32);
-        let start_offset = BlockOffset(start.0 % extent_size);
-
         let awrite = if is_write_unwritten {
             IOop::WriteUnwritten {
                 dependencies,
-                start_eid,
-                start_offset,
+                start_block: start,
                 data: write.data.freeze(),
                 blocks: write.blocks,
             }
         } else {
             IOop::Write {
                 dependencies,
-                start_eid,
-                start_offset,
+                start_block: start,
                 data: write.data.freeze(),
                 blocks: write.blocks,
             }
@@ -2164,16 +2155,9 @@ impl Downstairs {
         // TODO: can anyone actually give us an empty write?
         let start = blocks.start().unwrap_or(BlockIndex(0));
         let ddef = self.ddef.unwrap();
-
-        // XXX change IOop to take `BlockIndex` instead?
-        let extent_size = ddef.extent_size().value;
-        let start_eid = ExtentId((start.0 / extent_size) as u32);
-        let start_offset = BlockOffset(start.0 % extent_size);
-
         let aread = IOop::Read {
             dependencies,
-            start_eid,
-            start_offset,
+            start_block: start,
             count: blocks.blocks().len() as u64,
             block_size: ddef.block_size(),
         };
@@ -2267,7 +2251,10 @@ impl Downstairs {
             let r = match client.should_send() {
                 Ok(r) => r,
                 Err(ShouldSendError::InLiveRepair) => {
-                    if io.send_io_live_repair(last_repair_extent) {
+                    if io.send_io_live_repair(
+                        last_repair_extent,
+                        self.ddef.as_ref().unwrap(),
+                    ) {
                         EnqueueResult::Send
                     } else {
                         EnqueueResult::Skip
@@ -2320,9 +2307,6 @@ impl Downstairs {
 
     /// Sends the given job to the given client
     fn send(&mut self, ds_id: JobId, io: IOop, client_id: ClientId) {
-        let def = self.ddef.unwrap();
-        let blocks_per_extent = def.extent_size().value;
-
         let job = self.clients[client_id].prune_deps(
             ds_id,
             io,
@@ -2331,8 +2315,7 @@ impl Downstairs {
         let message = match job {
             IOop::Write {
                 dependencies,
-                start_eid,
-                start_offset,
+                start_block,
                 blocks,
                 data,
             } => {
@@ -2343,18 +2326,14 @@ impl Downstairs {
                         session_id: self.cfg.session_id,
                         job_id: ds_id,
                         dependencies,
-                        start: BlockIndex(
-                            start_eid.0 as u64 * blocks_per_extent
-                                + start_offset.0,
-                        ),
+                        start: start_block,
                         contexts: blocks,
                     },
                     data,
                 }
             }
             IOop::WriteUnwritten {
-                start_eid,
-                start_offset,
+                start_block,
                 blocks,
                 dependencies,
                 data,
@@ -2369,10 +2348,7 @@ impl Downstairs {
                         session_id: self.cfg.session_id,
                         job_id: ds_id,
                         dependencies,
-                        start: BlockIndex(
-                            start_eid.0 as u64 * blocks_per_extent
-                                + start_offset.0,
-                        ),
+                        start: start_block,
                         contexts: blocks,
                     },
                     data,
@@ -2408,8 +2384,7 @@ impl Downstairs {
             }
             IOop::Read {
                 dependencies,
-                start_eid,
-                start_offset,
+                start_block,
                 count,
                 ..
             } => {
@@ -2419,9 +2394,7 @@ impl Downstairs {
                     session_id: self.cfg.session_id,
                     job_id: ds_id,
                     dependencies,
-                    start: BlockIndex(
-                        start_eid.0 as u64 * blocks_per_extent + start_offset.0,
-                    ),
+                    start: start_block,
                     count,
                 }
             }
@@ -3544,7 +3517,7 @@ impl Downstairs {
     fn submit_test_write_block(
         &mut self,
         eid: ExtentId,
-        block: BlockOffset,
+        block: crucible_common::BlockOffset,
         is_write_unwritten: bool,
     ) -> JobId {
         let extent_size = self.ddef.unwrap().extent_size().value;
@@ -3597,7 +3570,7 @@ impl Downstairs {
     fn submit_read_block(
         &mut self,
         eid: ExtentId,
-        block: BlockOffset,
+        block: crucible_common::BlockOffset,
     ) -> JobId {
         let extent_size = self.ddef.unwrap().extent_size().value;
         let block = BlockIndex(u64::from(eid.0) * extent_size + block.0);
diff --git a/upstairs/src/lib.rs b/upstairs/src/lib.rs
@@ -1018,10 +1018,8 @@ enum IOop {
     Write {
         /// Jobs that mush finish before this
         dependencies: Vec<JobId>,
-        /// Extent in which the write starts
-        start_eid: ExtentId,
-        /// Relative offset (within that extent) to start writing
-        start_offset: BlockOffset,
+        /// Absolute block at which the write starts
+        start_block: BlockIndex,
         /// Per-block context
         blocks: Vec<BlockContext>,
         /// Raw data, tightly packed
@@ -1030,10 +1028,8 @@ enum IOop {
     WriteUnwritten {
         /// Jobs that mush finish before this
         dependencies: Vec<JobId>,
-        /// Extent in which the write starts
-        start_eid: ExtentId,
-        /// Relative offset (within that extent) to start writing
-        start_offset: BlockOffset,
+        /// Absolute block at which the write starts
+        start_block: BlockIndex,
         /// Per-block context
         blocks: Vec<BlockContext>,
         /// Raw data, tightly packed
@@ -1042,10 +1038,8 @@ enum IOop {
     Read {
         /// Jobs that must finish before this
         dependencies: Vec<JobId>,
-        /// Extent in which the read starts
-        start_eid: ExtentId,
-        /// Relative offset (within that extent) to start reading
-        start_offset: BlockOffset,
+        /// Absolute block at which the read starts
+        start_block: BlockIndex,
         /// Number of blocks to read
         count: u64,
         /// Block size for this region (stored here for convenience)
@@ -1197,7 +1191,11 @@ impl IOop {
     // repair), and determine if this IO should be sent to the downstairs or not
     // (skipped).
     // Return true if we should send it.
-    fn send_io_live_repair(&self, extent_limit: Option<ExtentId>) -> bool {
+    fn send_io_live_repair(
+        &self,
+        extent_limit: Option<ExtentId>,
+        ddef: &RegionDefinition,
+    ) -> bool {
         // Always send live-repair IOs
         if matches!(
             self,
@@ -1216,9 +1214,12 @@ impl IOop {
             // repaired is handled with dependencies, and IOs should arrive
             // here with those dependencies already set.
             match &self {
-                IOop::Write { start_eid, .. }
-                | IOop::WriteUnwritten { start_eid, .. }
-                | IOop::Read { start_eid, .. } => *start_eid <= extent_limit,
+                IOop::Write { start_block, .. }
+                | IOop::WriteUnwritten { start_block, .. }
+                | IOop::Read { start_block, .. } => {
+                    let start_eid = start_block.0 / ddef.extent_size().value;
+                    start_eid as u32 <= extent_limit.0
+                }
                 IOop::Flush { .. } => {
                     // If we have set extent limit, then we go ahead and
                     // send the flush with the extent_limit in it, and allow
diff --git a/upstairs/src/test.rs b/upstairs/src/test.rs
