Fixed a bug were block files were not being deleted.

Cargo.lock

@@ -198,6 +198,7 @@ version = "0.1.0"
 dependencies = [
  "btfproto",
  "btlib",
+ "btlib-tests",
  "btserde",
  "lazy_static",
  "libc",
@@ -286,6 +287,7 @@ version = "0.1.0"
 dependencies = [
  "btlib",
  "swtpm-harness",
+ "tempdir",
 ]
 
 [[package]]

crates/btfproto-tests/Cargo.toml

@@ -13,3 +13,6 @@ tempdir = { version = "0.3.7" }
 lazy_static = { version = "1.4.0" }
 libc = { version = "0.2.137" }
 tokio = { version = "1.24.2" }
+
+[dev-dependencies]
+btlib-tests = { path = "../btlib-tests" }

crates/btfproto-tests/src/local_fs_tests.rs

@@ -124,7 +124,9 @@ mod tests {
 
     use btfproto::{local_fs::Error, msg::*};
     use btlib::{Inode, Result, SECTOR_SZ_DEFAULT};
+    use btlib_tests::fs_queries::num_files;
     use std::{
+        fs::read_dir,
         io::{self, Cursor, Write as IoWrite},
         ops::Deref,
         sync::Arc,
@@ -918,4 +920,131 @@ mod tests {
             .map(|e| *e)
             .eq(std::iter::repeat(1u8).take(sect_sz)));
     }
+
+    #[tokio::test]
+    async fn unlink_after_forget_file_is_deleted() {
+        const FILENAME: &str = "file";
+        let case = LocalFsTest::new_empty().await;
+        let block_dir = case.dir.path();
+        let bt = &case.fs;
+        let from = case.from();
+        let expected = num_files(block_dir).unwrap();
+
+        let msg = Create {
+            parent: SpecInodes::RootDir.into(),
+            name: FILENAME,
+            flags: FlagValue::ReadWrite.into(),
+            mode: 0o644,
+            umask: 0,
+        };
+        let CreateReply { inode, handle, .. } = bt.create(from, msg).await.unwrap();
+        const DATA: [u8; 8] = [1u8; 8];
+        let msg = Write {
+            inode,
+            handle,
+            offset: 0,
+            data: DATA.as_slice(),
+        };
+        bt.write(from, msg).await.unwrap();
+        let msg = Close { inode, handle };
+        bt.close(from, msg).await.unwrap();
+        let more = num_files(block_dir).unwrap();
+        assert!(more > expected);
+        let msg = Forget { inode, count: 1 };
+        bt.forget(from, msg).await.unwrap();
+        let msg = Unlink {
+            parent: SpecInodes::RootDir.into(),
+            name: FILENAME,
+        };
+        bt.unlink(from, msg).await.unwrap();
+
+        let actual = num_files(block_dir).unwrap();
+        assert_eq!(expected, actual);
+    }
+
+    #[tokio::test]
+    async fn forget_after_unlink_file_is_deleted() {
+        const FILENAME: &str = "file";
+        let case = LocalFsTest::new_empty().await;
+        let block_dir = case.dir.path();
+        let bt = &case.fs;
+        let from = case.from();
+        let expected = num_files(block_dir).unwrap();
+
+        let msg = Create {
+            parent: SpecInodes::RootDir.into(),
+            name: FILENAME,
+            flags: FlagValue::ReadWrite.into(),
+            mode: 0o644,
+            umask: 0,
+        };
+        let CreateReply { inode, handle, .. } = bt.create(from, msg).await.unwrap();
+        const DATA: [u8; 8] = [1u8; 8];
+        let msg = Write {
+            inode,
+            handle,
+            offset: 0,
+            data: DATA.as_slice(),
+        };
+        bt.write(from, msg).await.unwrap();
+        let msg = Close { inode, handle };
+        bt.close(from, msg).await.unwrap();
+        let more = num_files(block_dir).unwrap();
+        let msg = Unlink {
+            parent: SpecInodes::RootDir.into(),
+            name: FILENAME,
+        };
+        bt.unlink(from, msg).await.unwrap();
+        assert!(more > expected);
+        let msg = Forget { inode, count: 1 };
+        bt.forget(from, msg).await.unwrap();
+
+        let actual = num_files(block_dir).unwrap();
+        assert_eq!(expected, actual);
+    }
+
+    #[tokio::test]
+    async fn after_unlink_no_empty_directories() {
+        const FILENAME: &str = "file";
+        let case = LocalFsTest::new_empty().await;
+        let bt = &case.fs;
+        let from = case.from();
+
+        let msg = Create {
+            parent: SpecInodes::RootDir.into(),
+            name: FILENAME,
+            flags: FlagValue::ReadWrite.into(),
+            mode: 0o644,
+            umask: 0,
+        };
+        let CreateReply { inode, handle, .. } = bt.create(from, msg).await.unwrap();
+        const DATA: [u8; 8] = [1u8; 8];
+        let msg = Write {
+            inode,
+            handle,
+            offset: 0,
+            data: DATA.as_slice(),
+        };
+        bt.write(from, msg).await.unwrap();
+        let msg = Close { inode, handle };
+        bt.close(from, msg).await.unwrap();
+        let msg = Forget { inode, count: 1 };
+        bt.forget(from, msg).await.unwrap();
+        let msg = Unlink {
+            parent: SpecInodes::RootDir.into(),
+            name: FILENAME,
+        };
+        bt.unlink(from, msg).await.unwrap();
+
+        let mut path = case.dir.path().to_owned();
+        let entries = read_dir(&path).unwrap();
+        path.push("x");
+        for entry in entries {
+            let entry = entry.unwrap();
+            path.pop();
+            path.push(entry.file_name());
+            let empty = read_dir(&path).unwrap().next().is_none();
+            assert!(!empty);
+        }
+    }
 }

crates/btfproto/src/local_fs.rs

@@ -898,6 +898,17 @@ mod private {
             Ok(TableGuard::new_owned(self.inodes.clone()).await)
         }
 
+        fn delete_block_file(&self, inode: Inode) -> Result<()> {
+            let mut path = Self::block_path(&self.path, inode, &self.root_principal)?;
+            std::fs::remove_file(&path)?;
+            path.pop();
+            let mut contents = std::fs::read_dir(&path)?;
+            if contents.next().is_none() {
+                std::fs::remove_dir(&path)?;
+            }
+            Ok(())
+        }
+
         async fn inode_forget<'a>(
             &self,
             from: Arc<BlockPath>,
@@ -922,8 +933,7 @@ mod private {
                     .map_err(|_| bterr!("LOGIC ERROR: entry for inode {inode} was still in use while it was being forgotten"))?;
                 let delete = entry.into_inner().delete;
                 if delete {
-                    let path = Self::block_path(&self.path, inode, &self.root_principal)?;
-                    std::fs::remove_file(path)?;
+                    self.delete_block_file(inode)?;
                 }
             }
             Ok(())
@@ -1568,6 +1578,10 @@ mod private {
 
         type UnlinkFut<'c> = impl 'c + Send + Future<Output = Result<()>>;
         fn unlink<'c>(&'c self, from: &'c Arc<BlockPath>, msg: Unlink<'c>) -> Self::UnlinkFut<'c> {
+            fn decr_nlink(secrets: &mut BlockMetaSecrets) -> Result<u32> {
+                secrets.nlink -= 1;
+                Ok(secrets.nlink)
+            }
             async move {
                 let Unlink { parent, name } = msg;
                 debug!("unlink: parent {parent}, name {name}");
@@ -1598,18 +1612,44 @@ mod private {
                     (block_path, inode, parent_key)
                 };
 
-                let table_guard = self
-                    .ensure_open(from, inode, block_path, Some(parent_key))
-                    .await?;
-                let mut value = table_guard.write(inode).await?;
-                // We mark the block for deletion if `nlink` drops to zero.
-                let block = value.block_mut();
-                let nlink = block.mut_meta_body().access_secrets(|secrets| {
-                    secrets.nlink -= 1;
-                    Ok(secrets.nlink)
-                })?;
-                block.flush_meta()?;
-                value.delete = 0 == nlink;
+                let table_guard = self.inodes.read().await;
+                let delete = if let Some(entry) = table_guard.get(&inode) {
+                    let mut value = entry.write().await;
+                    let nlink = value
+                        .block_mut()
+                        .mut_meta_body()
+                        .access_secrets(decr_nlink)?;
+                    value.delete = 0 == nlink;
+                    // If the block is about to be deleted then there's no point in flushing its
+                    // metadata.
+                    if !value.delete {
+                        value.block_mut().flush_meta()?;
+                    }
+                    // Since this block was already open, a client is keeping it alive. When they
+                    // choose to forget this inode it will be deleted. Thus we return false here.
+                    false
+                } else {
+                    // It may be tempting to drop the table_guard here, but if this were done then
+                    // another this block file could be opened concurrently.
+                    let mut block = Self::open_block(
+                        &self.path,
+                        inode,
+                        self.creds.clone(),
+                        block_path,
+                        Some(parent_key),
+                        &self.root_principal,
+                    )?;
+                    let nlink = block.mut_meta_body().access_secrets(decr_nlink)?;
+                    if nlink > 0 {
+                        block.flush_meta()?;
+                        false
+                    } else {
+                        true
+                    }
+                };
+                if delete {
+                    self.delete_block_file(inode)?;
+                }
                 Ok(())
             }
         }

crates/btfuse/src/fuse_fs.rs

@@ -191,6 +191,16 @@ mod private {
             })
         }
 
+        fn forget(&self, ctx: &Context, inode: Self::Inode, count: u64) {
+            block_on(async move {
+                let path = self.path_from_luid(ctx.uid);
+                let msg = Forget { inode, count };
+                if let Err(err) = self.provider.forget(path, msg).await {
+                    error!("error when sending the Forget message: {err}");
+                }
+            })
+        }
+
         fn create(
             &self,
             ctx: &Context,
@@ -256,6 +266,19 @@ mod private {
             })
         }
 
+        fn release(
+            &self,
+            ctx: &Context,
+            inode: Self::Inode,
+            flags: u32,
+            handle: Self::Handle,
+            _flush: bool,
+            _flock_release: bool,
+            _lock_owner: Option<u64>,
+        ) -> io::Result<()> {
+            self.releasedir(ctx, inode, flags, handle)
+        }
+
         fn opendir(
             &self,
             ctx: &Context,
@@ -266,6 +289,21 @@ mod private {
             self.open(ctx, inode, flags, 0)
         }
 
+        fn releasedir(
+            &self,
+            ctx: &Context,
+            inode: Self::Inode,
+            _flags: u32,
+            handle: Self::Handle,
+        ) -> io::Result<()> {
+            block_on(async move {
+                let path = self.path_from_luid(ctx.uid);
+                let msg = Close { inode, handle };
+                self.provider.close(path, msg).await?;
+                Ok(())
+            })
+        }
+
         fn read(
             &self,
             ctx: &Context,

crates/btfuse/src/main.rs

@@ -256,7 +256,7 @@ mod test {
             (swtpm, cred_store)
         }
 
-        fn mnt_dir(&self) -> &PathBuf {
+        fn mnt_dir(&self) -> &Path {
             &self.config.mnt_dir
         }
 
@@ -532,7 +532,7 @@ mod test {
 
     //#[test]
     #[allow(dead_code)]
-    /// KMC: This test is currently not working, and I've not been able to figure out why, not
+    /// KMC: This test is currently not working, and I've not been able to figure out why, nor
     /// reproduce it at a lower layer of the stack.
     fn read_more_than_whats_buffered() {
         const FILE_NAME: &str = "big.dat";

crates/btlib-tests/Cargo.toml

@@ -8,3 +8,6 @@ edition = "2021"
 [dependencies]
 btlib = { path = "../btlib" }
 swtpm-harness = { path = "../swtpm-harness" }
+
+[dev-dependencies]
+tempdir = "0.3.7"

crates/btlib-tests/src/fs_queries.rs

@@ -0,0 +1,185 @@
+use btlib::Result;
+use std::fs::{metadata, read_dir, Metadata};
+use std::path::Path;
+
+/// Calls the given closure on every entry recursively contained in the given path.
+pub fn visit<P: AsRef<Path>, R, F: FnMut(Metadata) -> R>(path: P, visitor: &mut F) -> Result<()> {
+    let meta = metadata(&path)?;
+    if !meta.is_dir() {
+        visitor(meta);
+        return Ok(());
+    }
+    let contents = read_dir(&path)?;
+    let mut entry_path = path.as_ref().to_owned();
+    entry_path.push("x");
+    for entry in contents {
+        let entry = entry?;
+        entry_path.pop();
+        entry_path.push(entry.file_name());
+        visit(&entry_path, visitor)?;
+    }
+    Ok(())
+}
+
+/// Recursively sums up the length of each file stored under the given path and returns the total.
+/// Note that if the given path contains hard links, the disk usage for the hard linked file will
+/// be counted multiple times, once for each link.
+pub fn disk_usage<P: AsRef<Path>>(path: P) -> Result<u64> {
+    let mut total = 0;
+    visit(path, &mut |meta| {
+        if meta.is_file() {
+            total += meta.len();
+        }
+    })?;
+    Ok(total)
+}
+
+pub fn num_files<P: AsRef<Path>>(path: P) -> Result<u64> {
+    let mut total = 0;
+    visit(path, &mut |meta| {
+        if meta.is_file() {
+            total += 1;
+        }
+    })?;
+    Ok(total)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use std::fs::{create_dir, create_dir_all, write};
+    use tempdir::TempDir;
+
+    #[test]
+    fn disk_usage_direct_descendants() {
+        const BUF: [u8; 8] = [1u8; 8];
+        const EXPECTED: u64 = 3 * BUF.len() as u64;
+        let dir = TempDir::new("disk_usage").unwrap();
+        let dir_path = dir.path();
+
+        for k in 0..3 {
+            write(dir_path.join(k.to_string()), &BUF).unwrap();
+        }
+        let actual = disk_usage(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn disk_usage_first_gen_dirs() {
+        const BUF: [u8; 8] = [1u8; 8];
+        const EXPECTED: u64 = 3 * BUF.len() as u64;
+        let dir = TempDir::new("disk_usage").unwrap();
+        let dir_path = dir.path();
+
+        write(dir_path.join("1"), &BUF).unwrap();
+        let sub1 = dir_path.join("sub1");
+        create_dir(&sub1).unwrap();
+        write(sub1.join("2"), &BUF).unwrap();
+        let sub2 = dir_path.join("sub2");
+        create_dir(&sub2).unwrap();
+        write(sub2.join("3"), &BUF).unwrap();
+        let actual = disk_usage(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn disk_usage_second_gen_dir() {
+        const BUF: [u8; 8] = [1u8; 8];
+        const EXPECTED: u64 = 3 * BUF.len() as u64;
+        let dir = TempDir::new("disk_usage").unwrap();
+        let dir_path = dir.path();
+
+        for k in 0..2 {
+            write(dir_path.join(k.to_string()), &BUF).unwrap();
+        }
+        let mut sub = dir_path.to_owned();
+        sub.push("sub");
+        sub.push("sub");
+        create_dir_all(&sub).unwrap();
+        sub.push("2");
+        write(&sub, &BUF).unwrap();
+        let actual = disk_usage(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn disk_usage_empty_dir() {
+        const EXPECTED: u64 = 0;
+        let dir = TempDir::new("disk_usage").unwrap();
+        let dir_path = dir.path();
+
+        let actual = disk_usage(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn num_files_direct_descendants() {
+        const BUF: [u8; 8] = [1u8; 8];
+        const EXPECTED: u64 = 3;
+        let dir = TempDir::new("num_files").unwrap();
+        let dir_path = dir.path();
+
+        for k in 0..3 {
+            write(dir_path.join(k.to_string()), &BUF).unwrap();
+        }
+        let actual = num_files(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn num_files_first_gen_dirs() {
+        const BUF: [u8; 8] = [1u8; 8];
+        const EXPECTED: u64 = 3;
+        let dir = TempDir::new("num_files").unwrap();
+        let dir_path = dir.path();
+
+        write(dir_path.join("1"), &BUF).unwrap();
+        let sub1 = dir_path.join("sub1");
+        create_dir(&sub1).unwrap();
+        write(sub1.join("2"), &BUF).unwrap();
+        let sub2 = dir_path.join("sub2");
+        create_dir(&sub2).unwrap();
+        write(sub2.join("3"), &BUF).unwrap();
+        let actual = num_files(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn num_files_second_gen_dir() {
+        const BUF: [u8; 8] = [1u8; 8];
+        const EXPECTED: u64 = 3;
+        let dir = TempDir::new("num_files").unwrap();
+        let dir_path = dir.path();
+
+        for k in 0..2 {
+            write(dir_path.join(k.to_string()), &BUF).unwrap();
+        }
+        let mut sub = dir_path.to_owned();
+        sub.push("sub");
+        sub.push("sub");
+        create_dir_all(&sub).unwrap();
+        sub.push("2");
+        write(&sub, &BUF).unwrap();
+        let actual = num_files(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+
+    #[test]
+    fn num_files_empty_dir() {
+        const EXPECTED: u64 = 0;
+        let dir = TempDir::new("num_files").unwrap();
+        let dir_path = dir.path();
+
+        let actual = num_files(dir_path).unwrap();
+
+        assert_eq!(EXPECTED, actual);
+    }
+}

crates/btlib-tests/src/lib.rs

@@ -1,3 +1,5 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
 mod tpm_cred_store_harness;
 pub use tpm_cred_store_harness::TpmCredStoreHarness;
+
+pub mod fs_queries;