Merge branch 'Fix-leaking-storage-on-file-delete' into 'master'

Fix leaking tree node on file delete

See merge request redox-os/redoxfs!85

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::<Node>::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);
+    });
+}

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<T: BlockTrait + DerefMut<Target = [u8]>>(
+        &mut self,
+        ptr: TreePtr<T>,
+    ) -> 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<T: Deref<Target = [u8]>>(&mut self, nodes: &[TreeData<T>]) -> 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);
                     }