From 1d55c5aa6a42510efd5e615ba3aa1dfe7ae93baf Mon Sep 17 00:00:00 2001 From: Andrey Turkin Date: Tue, 29 Oct 2024 10:06:07 +0300 Subject: [PATCH 1/2] Don't leak node block on node remove. Refs #50 --- src/tests.rs | 70 +++++++++++++++++++++++++++++++--------------- src/transaction.rs | 36 +++++++++++++++++------- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/src/tests.rs b/src/tests.rs index bf90242ed9..0f0db6a52a 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,40 +1,46 @@ -use std::ops::DerefMut; use std::path::Path; use std::process::Command; -use std::{fs, sync, thread, time}; - -use crate::{unmount_path, DiskSparse, FileSystem}; +use std::{fs, thread, time}; +use crate::{unmount_path, DiskSparse, FileSystem, Node, TreePtr}; fn with_redoxfs(callback: F) -> T where T: Send + Sync + 'static, - F: FnMut(&Path) -> T + Send + Sync + 'static, + F: FnOnce(FileSystem) -> T + Send + Sync + 'static, { let disk_path = "image.bin"; - let mount_path = "image"; let res = { let disk = DiskSparse::create(dbg!(disk_path), 1024 * 1024 * 1024).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(); + + callback(fs) + }; + + dbg!(fs::remove_file(dbg!(disk_path))).unwrap(); + + res +} + +fn with_mounted(callback: F) -> T +where + T: Send + Sync + 'static, + F: FnOnce(&Path) -> T + Send + Sync + 'static, +{ + let mount_path = "image"; + + 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 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)); 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 mut callback_guard = callback_mutex.lock().unwrap(); - let callback = callback_guard.deref_mut(); - callback(&real_path) - }; + let res = callback(&real_path); if cfg!(target_os = "redox") { dbg!(fs::remove_file(dbg!(format!(":{}", mount_path)))).unwrap(); @@ -54,9 +60,7 @@ 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(); @@ -67,7 +71,7 @@ where #[test] fn simple() { - with_redoxfs(|path| { + with_mounted(|path| { dbg!(fs::create_dir(&path.join("test"))).unwrap(); }) } @@ -78,7 +82,7 @@ fn mmap() { use syscall; //TODO - with_redoxfs(|path| { + with_mounted(|path| { use std::slice; let path = dbg!(path.join("test")); @@ -129,3 +133,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::::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); + }); +} diff --git a/src/transaction.rs b/src/transaction.rs index 708436c1b1..82e5e40b54 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -372,10 +372,10 @@ impl<'a, D: Disk> Transaction<'a, D> { Ok(block.create_ptr()) } - pub fn read_tree>( + fn read_tree_and_addr>( &mut self, ptr: TreePtr, - ) -> Result> { + ) -> Result<(TreeData, 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>( + &mut self, + ptr: TreePtr, + ) -> Result> { + 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 - self.sync_trees(&[parent, node])?; + if remove_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(()); } From c22fb790b87256413da0486d2e4a86a4b1428597 Mon Sep 17 00:00:00 2001 From: Andrey Turkin Date: Tue, 29 Oct 2024 12:22:15 +0300 Subject: [PATCH 2/2] Fix parallel test runs --- src/mount/redox/mod.rs | 2 +- src/tests.rs | 22 ++++++++++++++-------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/mount/redox/mod.rs b/src/mount/redox/mod.rs index 76ba021c56..53ab9ba947 100644 --- a/src/mount/redox/mod.rs +++ b/src/mount/redox/mod.rs @@ -14,7 +14,7 @@ pub fn mount(filesystem: FileSystem, mountpoint: P, mut callback: where D: Disk, P: AsRef, - F: FnMut(&Path) -> T, + F: FnOnce(&Path) -> T, { let mountpoint = mountpoint.as_ref(); let socket = Socket::::create(&format!("{}", mountpoint.display()))?; diff --git a/src/tests.rs b/src/tests.rs index 0f0db6a52a..989d042a20 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,17 +1,21 @@ use std::path::Path; use std::process::Command; use std::{fs, thread, time}; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering::Relaxed; use crate::{unmount_path, DiskSparse, FileSystem, Node, TreePtr}; +static IMAGE_SEQ: AtomicUsize = AtomicUsize::new(0); + fn with_redoxfs(callback: F) -> T where T: Send + Sync + 'static, F: FnOnce(FileSystem) -> T + Send + Sync + 'static, { - let disk_path = "image.bin"; + let disk_path = format!("image{}.bin", IMAGE_SEQ.fetch_add(1, Relaxed)); let res = { - let disk = DiskSparse::create(dbg!(disk_path), 1024 * 1024 * 1024).unwrap(); + let disk = DiskSparse::create(dbg!(&disk_path), 1024 * 1024 * 1024).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(); @@ -29,27 +33,29 @@ where T: Send + Sync + 'static, F: FnOnce(&Path) -> T + Send + Sync + 'static, { - let mount_path = "image"; + 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(); + 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 real_path = real_path.to_owned(); thread::spawn(move || { let res = callback(&real_path); + let real_path = real_path.to_str().unwrap(); 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"); } } @@ -63,7 +69,7 @@ where }); if cfg!(not(target_os = "redox")) { - dbg!(fs::remove_dir(dbg!(mount_path))).unwrap(); + dbg!(fs::remove_dir(dbg!(mount_path_o))).unwrap(); } res