Merge branch 'node_leak' into 'master'

Don't leak node block on node remove.

See merge request redox-os/redoxfs!81

src/mount/redox/mod.rs

@@ -14,7 +14,7 @@ pub fn mount<D, P, T, F>(filesystem: FileSystem<D>, mountpoint: P, mut callback:
 where
     D: Disk,
     P: AsRef<Path>,
-    F: FnMut(&Path) -> T,
+    F: FnOnce(&Path) -> T,
 {
     let mountpoint = mountpoint.as_ref();
     let socket = Socket::<V2>::create(&format!("{}", mountpoint.display()))?;

src/tests.rs

@@ -1,49 +1,61 @@
-use std::ops::DerefMut;
 use std::path::Path;
 use std::process::Command;
-use std::{fs, sync, thread, time};
+use std::{fs, thread, time};
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering::Relaxed;
+use crate::{unmount_path, DiskSparse, FileSystem, Node, TreePtr};
 
-use crate::{unmount_path, DiskSparse, FileSystem};
+static IMAGE_SEQ: AtomicUsize = AtomicUsize::new(0);
 
 fn with_redoxfs<T, F>(callback: F) -> T
 where
     T: Send + Sync + 'static,
-    F: FnMut(&Path) -> T + Send + Sync + 'static,
+    F: FnOnce(FileSystem<DiskSparse>) -> T + Send + Sync + 'static,
 {
-    let disk_path = "image.bin";
+    let disk_path = format!("image{}.bin", IMAGE_SEQ.fetch_add(1, Relaxed));
-    let mount_path = "image";
 
     let res = {
-        let disk = DiskSparse::create(dbg!(disk_path), 1024 * 1024 * 1024).unwrap();
+        let disk = DiskSparse::create(dbg!(&disk_path), 1024 * 1024 * 1024).unwrap();
-
-        if cfg!(not(target_os = "redox")) {
-            if !Path::new(mount_path).exists() {
-                dbg!(fs::create_dir(dbg!(mount_path))).unwrap();
-            }
-        }
 
         let ctime = dbg!(time::SystemTime::now().duration_since(time::UNIX_EPOCH)).unwrap();
         let fs = FileSystem::create(disk, None, ctime.as_secs(), ctime.subsec_nanos()).unwrap();
 
-        let callback_mutex = sync::Arc::new(sync::Mutex::new(callback));
+        callback(fs)
+    };
+
+    dbg!(fs::remove_file(dbg!(disk_path))).unwrap();
+
+    res
+}
+
+fn with_mounted<T, F>(callback: F) -> T
+where
+    T: Send + Sync + 'static,
+    F: FnOnce(&Path) -> T + Send + Sync + 'static,
+{
+    let mount_path_o = format!("image{}", IMAGE_SEQ.fetch_add(1, Relaxed));
+    let mount_path = mount_path_o.clone();
+
+    let res = with_redoxfs(move |fs| {
+        if cfg!(not(target_os = "redox")) {
+            if !Path::new(&mount_path).exists() {
+                dbg!(fs::create_dir(dbg!(&mount_path))).unwrap();
+            }
+        }
         let join_handle = crate::mount(fs, dbg!(mount_path), move |real_path| {
-            let callback_mutex = callback_mutex.clone();
             let real_path = real_path.to_owned();
             thread::spawn(move || {
-                let res = {
+                let res = callback(&real_path);
-                    let mut callback_guard = callback_mutex.lock().unwrap();
+                let real_path = real_path.to_str().unwrap();
-                    let callback = callback_guard.deref_mut();
-                    callback(&real_path)
-                };
 
                 if cfg!(target_os = "redox") {
-                    dbg!(fs::remove_file(dbg!(format!(":{}", mount_path)))).unwrap();
+                    dbg!(fs::remove_file(dbg!(format!(":{}", real_path)))).unwrap();
                 } else {
                     if !dbg!(Command::new("sync").status()).unwrap().success() {
                         panic!("sync failed");
                     }
 
-                    if !unmount_path(mount_path).is_ok() {
+                    if !unmount_path(real_path).is_ok() {
                         panic!("umount failed");
                     }
                 }
@@ -54,12 +66,10 @@ where
         .unwrap();
 
         join_handle.join().unwrap()
-    };
+    });
-
-    dbg!(fs::remove_file(dbg!(disk_path))).unwrap();
 
     if cfg!(not(target_os = "redox")) {
-        dbg!(fs::remove_dir(dbg!(mount_path))).unwrap();
+        dbg!(fs::remove_dir(dbg!(mount_path_o))).unwrap();
     }
 
     res
@@ -67,7 +77,7 @@ where
 
 #[test]
 fn simple() {
-    with_redoxfs(|path| {
+    with_mounted(|path| {
         dbg!(fs::create_dir(&path.join("test"))).unwrap();
     })
 }
@@ -78,7 +88,7 @@ fn mmap() {
     use syscall;
 
     //TODO
-    with_redoxfs(|path| {
+    with_mounted(|path| {
         use std::slice;
 
         let path = dbg!(path.join("test"));
@@ -129,3 +139,25 @@ fn mmap() {
         mmap_inner(false);
     })
 }
+
+#[test]
+fn 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";
+        let _ = fs.tx(|tx| {
+            tx.create_node(
+                tree_ptr,
+                name,
+                Node::MODE_FILE | 0644,
+                1,
+                0,
+            )?;
+            tx.remove_node(tree_ptr, name, Node::MODE_FILE)
+        }).unwrap();
+
+        assert_eq!(fs.allocator().free(), initially_free);
+    });
+}

src/transaction.rs

@@ -372,10 +372,10 @@ impl<'a, D: Disk> Transaction<'a, D> {
         Ok(block.create_ptr())
     }
 
-    pub fn read_tree<T: BlockTrait + DerefMut<Target = [u8]>>(
+    fn read_tree_and_addr<T: BlockTrait + DerefMut<Target = [u8]>>(
         &mut self,
         ptr: TreePtr<T>,
-    ) -> Result<TreeData<T>> {
+    ) -> Result<(TreeData<T>, BlockAddr)> {
         if ptr.is_null() {
             // ID is invalid (should this return None?)
             #[cfg(feature = "log")]
@@ -401,7 +401,14 @@ impl<'a, D: Disk> Transaction<'a, D> {
         };
         data.copy_from_slice(raw.data());
 
-        Ok(TreeData::new(ptr.id(), data))
+        Ok((TreeData::new(ptr.id(), data), raw.addr()))
+    }
+
+    pub fn read_tree<T: BlockTrait + DerefMut<Target = [u8]>>(
+        &mut self,
+        ptr: TreePtr<T>,
+    ) -> Result<TreeData<T>> {
+        Ok(self.read_tree_and_addr(ptr)?.0)
     }
 
     //TODO: improve performance, reduce writes
@@ -666,7 +673,7 @@ impl<'a, D: Disk> Transaction<'a, D> {
                 if let Some(entry_name) = entry.name() {
                     if entry_name == name {
                         // Read node and test type against requested type
-                        let node = self.read_tree(node_ptr)?;
+                        let (node, addr) = self.read_tree_and_addr(node_ptr)?;
                         if node.data().mode() & Node::MODE_TYPE == mode {
                             if node.data().is_dir()
                                 && node.data().size() > 0
@@ -677,7 +684,7 @@ impl<'a, D: Disk> Transaction<'a, D> {
                             }
 
                             // Save node and clear entry
-                            node_opt = Some(node);
+                            node_opt = Some((node, addr));
                             *entry = DirEntry::default();
                             break;
                         } else if node.data().is_dir() {
@@ -691,14 +698,16 @@ impl<'a, D: Disk> Transaction<'a, D> {
                 }
             }
 
-            if let Some(mut node) = node_opt {
+            if let Some((mut node, addr)) = node_opt {
                 let links = node.data().links();
-                if links > 1 {
+                let remove_node = if links > 1 {
                     node.data_mut().set_links(links - 1);
+                    false
                 } else {
                     node.data_mut().set_links(0);
                     self.truncate_node_inner(&mut node, 0)?;
-                }
+                    true
+                };
 
                 if record_offset == records - 1 && dir.data().is_empty() {
                     let mut remove_record = record_offset;
@@ -728,8 +737,15 @@ impl<'a, D: Disk> Transaction<'a, D> {
                     self.sync_node_record_ptr(&mut parent, record_offset, dir_record_ptr)?;
                 }
 
-                // Sync both parent and node at the same time
+                if remove_node {
-                self.sync_trees(&[parent, node])?;
+                    self.sync_tree(parent)?;
+                    unsafe {
+                        self.deallocate(addr);
+                    }
+                } else {
+                    // Sync both parent and node at the same time
+                    self.sync_trees(&[parent, node])?;
+                }
 
                 return Ok(());
             }