Changed readdir to handle directory iteration properly.

crates/btlib/src/blocktree.rs

@@ -180,18 +180,24 @@ mod private {
 
     struct InodeTableValue {
         blocks: HashMap<Handle, Box<dyn Block>>,
+        dirs: HashMap<Handle, Directory>,
         next_handle: Handle,
         lookup_count: AtomicU64,
         unclaimed_handles: Vec<Handle>,
     }
 
     impl InodeTableValue {
+        /// If more than this number of unclaimed blocks are open, then blocks are closed until
+        /// only this number remain open.
+        const UNCLAIMED_HANDLE_LIMIT: usize = 3;
+
         fn new(block: Box<dyn Block>) -> InodeTableValue {
             const FIRST_HANDLE: Handle = 1;
             let mut blocks = HashMap::with_capacity(1);
             blocks.insert(FIRST_HANDLE, block);
             Self {
                 blocks,
+                dirs: HashMap::new(),
                 next_handle: FIRST_HANDLE + 1,
                 lookup_count: AtomicU64::new(1),
                 unclaimed_handles: vec![FIRST_HANDLE],
@@ -243,6 +249,10 @@ mod private {
 
         fn give_handle(&mut self, handle: Handle) {
             self.unclaimed_handles.push(handle);
+            while self.unclaimed_handles.len() > Self::UNCLAIMED_HANDLE_LIMIT {
+                let handle = self.unclaimed_handles.pop().unwrap();
+                self.blocks.remove(&handle);
+            }
         }
 
         fn incr_lookup_count(&self) -> u64 {
@@ -754,13 +764,13 @@ mod private {
 
         fn release(
             &self,
-            ctx: &Context,
+            _ctx: &Context,
             inode: Self::Inode,
-            flags: u32,
+            _flags: u32,
             handle: Self::Handle,
             flush: bool,
-            flock_release: bool,
-            lock_owner: Option<u64>,
+            _flock_release: bool,
+            _lock_owner: Option<u64>,
         ) -> io::Result<()> {
             debug!("Blocktree::release called on inode {inode}");
             if flush {
@@ -773,12 +783,20 @@ mod private {
             &self,
             ctx: &Context,
             inode: Self::Inode,
-            flags: u32,
+            _flags: u32,
         ) -> io::Result<(Option<Self::Handle>, OpenOptions)> {
             debug!("Blocktree::opendir called on inode {inode}");
-            let handle = self.take_block_if_ok(inode, |handle, block| {
-                let ctx = AuthzContext::new(ctx, block.meta());
-                self.authorizer.can_exec(&ctx)?;
+            let handle = self.access_value_mut(inode, |value| {
+                let dir = value.try_borrow_block(|block| {
+                    let ctx = AuthzContext::new(ctx, block.meta());
+                    self.authorizer.can_exec(&ctx)?;
+                    block.seek(SeekFrom::Start(0))?;
+                    let dir: Directory = read_from(block)?;
+                    Ok(dir)
+                })?;
+                let handle = value.next_handle;
+                value.next_handle += 1;
+                value.dirs.insert(handle, dir);
                 Ok(handle)
             })?;
             Ok((Some(handle), OpenOptions::empty()))
@@ -786,13 +804,16 @@ mod private {
 
         fn releasedir(
             &self,
-            ctx: &Context,
+            _ctx: &Context,
             inode: Self::Inode,
-            flags: u32,
+            _flags: u32,
             handle: Self::Handle,
         ) -> io::Result<()> {
             debug!("Blocktree::releasedir called for inode {inode}");
-            self.give_handle(inode, handle)
+            self.access_value_mut(inode, |value| {
+                value.dirs.remove(&handle);
+                Ok(())
+            })
         }
 
         fn create(
@@ -814,6 +835,9 @@ mod private {
 
             // Add a directory entry to the parent for the new inode.
             let mut block_path = self.borrow_block(parent, |block| {
+                let ctx = AuthzContext::new(ctx, block.meta());
+                self.authorizer.can_write(&ctx)?;
+
                 block.seek(SeekFrom::Start(0))?;
                 let mut dir: Directory = read_from(block).box_err()?;
                 dir.add_file(name.clone(), inode)?;
@@ -830,9 +854,9 @@ mod private {
                     let block = value.block(handle)?;
                     Ok(block.mut_meta_body().access_secrets(|secrets| {
                         secrets.inode = inode;
-                        secrets.mode = Self::default_file_mode();
-                        secrets.uid = Self::uid();
-                        secrets.gid = Self::gid();
+                        secrets.mode = args.mode;
+                        secrets.uid = ctx.uid;
+                        secrets.gid = ctx.gid;
                         let now = Epoch::now();
                         secrets.atime = now;
                         secrets.ctime = now;
@@ -962,7 +986,7 @@ mod private {
 
         fn readdir(
             &self,
-            ctx: &Context,
+            _ctx: &Context,
             inode: Self::Inode,
             handle: Self::Handle,
             size: u32,
@@ -972,31 +996,29 @@ mod private {
             ) -> io::Result<usize>,
         ) -> io::Result<()> {
             debug!("Blocktree::readdir called on inode {inode}");
-            let dir = self.access_block(inode, handle, |block| {
-                block.seek(SeekFrom::Start(0))?;
-                Ok(read_from::<Directory, _>(block)?)
-            })?;
-            let mut index = 0u64;
-            for (name, entry) in dir.entries() {
-                index += 1;
-                // TODO: Using a simple index will fail when the directory is being modified
-                // concurrently.
-                if index <= offset {
-                    continue;
+            self.access_value(inode, |value| {
+                let dir = value.dirs.get(&handle)
+                    .ok_or(Error::InvalidHandle { handle, inode })?;
+                let mut index: u64 = 0;
+                for (name, entry) in dir.entries() {
+                    index += 1;
+                    if index <= offset {
+                        continue;
+                    }
+                    let inode = match entry.inode() {
+                        Some(inode) => inode,
+                        None => continue,
+                    };
+                    let dir_entry = FuseDirEntry {
+                        ino: inode,
+                        offset: index,
+                        type_: entry.kind() as u32,
+                        name: name.as_bytes(),
+                    };
+                    add_entry(dir_entry)?;
                 }
-                let inode = match entry.inode() {
-                    Some(inode) => inode,
-                    None => continue,
-                };
-                let dir_entry = FuseDirEntry {
-                    ino: inode,
-                    offset: index,
-                    type_: entry.kind() as u32,
-                    name: name.as_bytes(),
-                };
-                add_entry(dir_entry)?;
-            }
-            Ok(())
+                Ok(())
+            })
         }
 
         fn getattr(
@@ -1313,8 +1335,10 @@ mod private {
 #[cfg(test)]
 mod tests {
     use std::ffi::CString;
-
-    use fuse_backend_rs::api::filesystem::{FileSystem, FsOptions};
+    use fuse_backend_rs::{
+        abi::fuse_abi::CreateIn,
+        api::filesystem::{Context, FileSystem, FsOptions}
+    };
     use tempdir::TempDir;
 
     use test_helpers::*;
@@ -1509,6 +1533,16 @@ mod tests {
         fn creds() -> ConcreteCreds {
             test_helpers::NODE_CREDS.clone()
         }
+
+        fn context(&self) -> Context {
+            let (stat, ..) = self.bt.getattr(&Default::default(), SpecInodes::RootDir.into(), None)
+                .expect("getattr failed");
+            Context {
+                uid: stat.st_uid,
+                gid: stat.st_gid,
+                pid: 1,
+            }
+        }
     }
 
     /// Tests that a new file can be created, written to and the written data can be read from it.
@@ -1516,15 +1550,15 @@ mod tests {
     fn create_write_lseek_read() {
         let case = BtTestCase::new_empty();
         let bt = &case.bt;
-
+        let ctx = case.context();
         let name = CString::new("README.md").unwrap();
 
         let (entry, handle, ..) = bt
             .create(
-                &Default::default(),
+                &ctx,
                 SpecInodes::RootDir.into(),
                 name.as_c_str(),
-                Default::default(),
+                CreateIn { mode: libc::S_IFREG | 0o644, umask: 0, flags: 0, fuse_flags: 0 },
             )
             .expect("failed to create file");
         let inode = entry.inode;
@@ -1534,7 +1568,7 @@ mod tests {
         let mut expected = BtCursor::new([1u8; LEN]);
         let written = bt
             .write(
-                &Default::default(),
+                &ctx,
                 inode,
                 handle,
                 &mut expected,
@@ -1548,13 +1582,13 @@ mod tests {
             .expect("write failed");
         assert_eq!(LEN, written);
 
-        bt.lseek(&Default::default(), inode, handle, 0, 0)
+        bt.lseek(&ctx, inode, handle, 0, 0)
             .expect("lseek failed");
 
         let mut actual = BtCursor::new([0u8; LEN]);
         let read = bt
             .read(
-                &Default::default(),
+                &ctx,
                 inode,
                 handle,
                 &mut actual,
@@ -1573,11 +1607,12 @@ mod tests {
     fn lookup() {
         let case = BtTestCase::new_empty();
         let bt = &case.bt;
+        let ctx = case.context();
 
         let name = CString::new("README.md").unwrap();
         let (expected, ..) = bt
             .create(
-                &Default::default(),
+                &ctx,
                 SpecInodes::RootDir.into(),
                 name.as_c_str(),
                 Default::default(),
@@ -1598,17 +1633,17 @@ mod tests {
     fn new_existing() {
         const EXPECTED: &[u8] = b"cool as cucumbers";
         let name = CString::new("RESIGNATION.docx").unwrap();
-
         let case = BtTestCase::new_empty();
         let bt = &case.bt;
+        let ctx = case.context();
 
         {
             let (entry, handle, ..) = bt
                 .create(
-                    &Default::default(),
+                    &ctx,
                     SpecInodes::RootDir.into(),
                     name.as_c_str(),
-                    Default::default(),
+                    CreateIn { mode: libc::S_IFREG | 0o644, umask: 0, flags: 0, fuse_flags: 0 },
                 )
                 .expect("failed to create file");
             let inode = entry.inode;

crates/btlib/src/lib.rs

@@ -933,7 +933,7 @@ impl Directory {
     }
 
     pub fn entries(&self) -> impl Iterator<Item = (&str, &DirEntry)> {
-        self.entries.iter().map(|kv| (kv.0.as_str(), kv.1))
+        self.entries.iter().map(|(name, entry)| (name.as_str(), entry))
     }
 }