From eaf4a715857a20c94ae375eb82e6ef774f80e365 Mon Sep 17 00:00:00 2001
From: "Brian L. Troutwine" <brian.troutwine@datadoghq.com>
Date: Wed, 16 Oct 2024 18:10:18 -0700
Subject: [PATCH] Avoid extra name lookup

This commit avoids an extra name lookup but using the name of an inode
as its key in the directory. This will be slightly complicated by
rotating files depending on how I do them but it is a win in terms
of code simplification now.

Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
---
 lading/src/bin/logrotate_fs.rs         | 21 ++++++------
 lading/src/generator/file_gen/model.rs | 45 ++++++++------------------
 2 files changed, 24 insertions(+), 42 deletions(-)

diff --git a/lading/src/bin/logrotate_fs.rs b/lading/src/bin/logrotate_fs.rs
index 73c3a5cee..e4e759e2b 100644
--- a/lading/src/bin/logrotate_fs.rs
+++ b/lading/src/bin/logrotate_fs.rs
@@ -189,6 +189,11 @@ impl Filesystem for LogrotateFS {
     ) {
         let tick = self.get_current_tick();
 
+        // NOTE this call to State::read is almost surely allocating. It'd be
+        // pretty slick if we could get the buffer directly from the OS to pass
+        // down for writing but we can't. I suppose we could send up the raw
+        // blocks and then chain them together as needed but absent a compelling
+        // reason to do that the simplicity of this API is nice.
         if let Some(data) = self
             .state
             .read(ino as usize, offset as usize, size as usize, tick)
@@ -234,16 +239,12 @@ impl Filesystem for LogrotateFS {
 
         // reaming children
         if let Some(child_inodes) = self.state.readdir(ino as usize) {
-            for child_ino in child_inodes {
-                if let Some(name) = self.state.get_name(*child_ino) {
-                    let file_type = self
-                        .state
-                        .get_file_type(*child_ino)
-                        .expect("inode must have file type");
-                    entries.push((*child_ino as u64, file_type, name.to_string()));
-                } else {
-                    error!("child inode has no name");
-                }
+            for (child_name, child_ino) in child_inodes {
+                let file_type = self
+                    .state
+                    .get_file_type(*child_ino)
+                    .expect("inode must have file type");
+                entries.push((*child_ino as u64, file_type, child_name.clone()));
             }
         } else {
             reply.error(ENOENT);
diff --git a/lading/src/generator/file_gen/model.rs b/lading/src/generator/file_gen/model.rs
index 3a1528bff..bbb2b1389 100644
--- a/lading/src/generator/file_gen/model.rs
+++ b/lading/src/generator/file_gen/model.rs
@@ -2,7 +2,7 @@
 
 //use lading_payload::block;
 
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 
 use bytes::Bytes;
 use lading_payload::block;
@@ -89,7 +89,7 @@ impl File {
 /// instances or `File` instances. Root directory will not have a `parent`.
 #[derive(Debug)]
 pub struct Directory {
-    children: HashSet<Inode>,
+    children: HashMap<String, Inode>,
     parent: Option<Inode>,
 }
 
@@ -172,10 +172,10 @@ impl State {
         let mut nodes = HashMap::new();
 
         let mut root_dir = Directory {
-            children: HashSet::new(),
+            children: HashMap::new(),
             parent: None,
         };
-        root_dir.children.insert(logs_inode);
+        root_dir.children.insert("logs".to_string(), logs_inode);
         nodes.insert(
             root_inode,
             Node::Directory {
@@ -185,10 +185,12 @@ impl State {
         );
 
         let mut logs_dir = Directory {
-            children: HashSet::new(),
+            children: HashMap::new(),
             parent: Some(root_inode),
         };
-        logs_dir.children.insert(foo_log_inode);
+        logs_dir
+            .children
+            .insert("foo.log".to_string(), foo_log_inode);
         nodes.insert(
             logs_inode,
             Node::Directory {
@@ -252,20 +254,10 @@ impl State {
         self.advance_time(now);
 
         if let Some(Node::Directory { dir, .. }) = self.nodes.get(&parent_inode) {
-            for child_inode in &dir.children {
-                let child_node = &self
-                    .nodes
-                    .get(child_inode)
-                    .expect("catastrophic programming error");
-                let child_name = match child_node {
-                    Node::Directory { name, .. } | Node::File { name, .. } => name,
-                };
-                if child_name == name {
-                    return Some(*child_inode);
-                }
-            }
+            dir.children.get(name).copied()
+        } else {
+            None
         }
-        None
     }
 
     /// Look up the attributes for an `Inode`.
@@ -342,7 +334,7 @@ impl State {
     ///
     /// Function does not advance time in the model.
     #[tracing::instrument(skip(self))]
-    pub fn readdir(&self, inode: Inode) -> Option<&HashSet<Inode>> {
+    pub fn readdir(&self, inode: Inode) -> Option<&HashMap<String, Inode>> {
         if let Some(Node::Directory { dir, .. }) = self.nodes.get(&inode) {
             Some(&dir.children)
         } else {
@@ -350,17 +342,6 @@ impl State {
         }
     }
 
-    /// Get the name of an inode if it exists
-    #[tracing::instrument(skip(self))]
-    pub fn get_name(&self, inode: Inode) -> Option<&str> {
-        self.nodes
-            .get(&inode)
-            .map(|node| match node {
-                Node::Directory { name, .. } | Node::File { name, .. } => name,
-            })
-            .map(String::as_str)
-    }
-
     /// Get the fuser file type of an inode if it exists
     #[tracing::instrument(skip(self))]
     pub fn get_file_type(&self, inode: Inode) -> Option<fuser::FileType> {
@@ -396,7 +377,7 @@ impl State {
             let subdirectory_count = dir
                 .children
                 .iter()
-                .filter(|child_inode| {
+                .filter(|(_, child_inode)| {
                     matches!(self.nodes.get(child_inode), Some(Node::Directory { .. }))
                 })
                 .count();