diff --git a/src/tests.rs b/src/tests.rs index b9d809648b..2adf51de37 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -157,3 +157,35 @@ fn create_remove_should_not_increase_size() { assert_eq!(fs.allocator().free(), initially_free); }); } + +#[test] +fn many_create_remove_should_not_increase_size() { + with_redoxfs(|mut fs| { + let initially_free = fs.allocator().free(); + let tree_ptr = TreePtr::::root(); + let name = "test"; + + // Iterate over 255 times to prove deleted files don't retain space within the node tree + for i in 0..600 { + let _ = fs + .tx(|tx| { + tx.create_node( + tree_ptr, + &format!("{}{}", name, i), + Node::MODE_FILE | 0644, + 1, + 0, + )?; + tx.remove_node(tree_ptr, &format!("{}{}", name, i), Node::MODE_FILE) + }) + .unwrap(); + } + + // Sync with squash to exclude the allocation log growth from the assertion + let _ = fs.tx(|tx| tx.sync(true)); + + // Any value greater than 0 indicates a storage leak + let diff = initially_free - fs.allocator().free(); + assert_eq!(diff, 0); + }); +} diff --git a/src/transaction.rs b/src/transaction.rs index b547b473b4..d2473ab410 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -500,6 +500,36 @@ impl<'a, D: Disk> Transaction<'a, D> { Err(Error::new(ENOSPC)) } + /// Clear the previously claimed slot in the tree for the given `ptr`. Note that this + /// should only be called after the corresponding node block has already been deallocated. + fn remove_tree>( + &mut self, + ptr: TreePtr, + ) -> Result<()> { + if ptr.is_null() { + // ID is invalid (should this return None?) + #[cfg(feature = "log")] + log::error!("READ_TREE: ID IS NULL"); + return Err(Error::new(ENOENT)); + } + + let (i3, i2, i1, i0) = ptr.indexes(); + let mut l3 = self.read_block(self.header.tree)?; + let mut l2 = self.read_block(l3.data().ptrs[i3])?; + let mut l1 = self.read_block(l2.data().ptrs[i2])?; + let mut l0 = self.read_block(l1.data().ptrs[i1])?; + + // Clear the value in the tree, but do not deallocate the block, as that should already + // have been done at the node level. + l0.data_mut().ptrs[i0] = BlockPtr::default(); + l1.data_mut().ptrs[i1] = self.sync_block(l0)?; + l2.data_mut().ptrs[i2] = self.sync_block(l1)?; + l3.data_mut().ptrs[i3] = self.sync_block(l2)?; + self.header.tree = self.sync_block(l3)?; + self.header_changed = true; + Ok(()) + } + pub fn sync_trees>(&mut self, nodes: &[TreeData]) -> Result<()> { for node in nodes.iter().rev() { let ptr = node.ptr(); @@ -737,7 +767,7 @@ impl<'a, D: Disk> Transaction<'a, D> { } // Save node and clear entry - node_opt = Some((node, addr)); + node_opt = Some((entry.node_ptr(), node, addr)); *entry = DirEntry::default(); break; } else if node.data().is_dir() { @@ -751,7 +781,7 @@ impl<'a, D: Disk> Transaction<'a, D> { } } - if let Some((mut node, addr)) = node_opt { + if let Some((node_tree_ptr, mut node, addr)) = node_opt { let links = node.data().links(); let remove_node = if links > 1 { node.data_mut().set_links(links - 1); @@ -792,6 +822,7 @@ impl<'a, D: Disk> Transaction<'a, D> { if remove_node { self.sync_tree(parent)?; + self.remove_tree(node_tree_ptr)?; unsafe { self.deallocate(addr); }