From ef72b16dcfd1eeab409e30f39be6c20535edb270 Mon Sep 17 00:00:00 2001 From: "Brian L. Troutwine" Date: Tue, 12 Nov 2024 11:02:08 -0800 Subject: [PATCH 1/4] Validate operations on FileHandles This commit allows State to track whether a file handle is valid for read etc operations. Our GC mechanism keeps track of the open-handles but assumes the correctness of callers, meaning if multiple calls to `release` are made at the filesystem level a node may be GC'ed before its time. The State now keeps a record of which FileHandles exist and whether they remain valid, meaning the GC mechanism cannot be misled. REF SMPTNG-532 Signed-off-by: Brian L. Troutwine --- lading/src/generator/file_gen/logrotate_fs.rs | 9 ++- .../generator/file_gen/logrotate_fs/model.rs | 60 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/lading/src/generator/file_gen/logrotate_fs.rs b/lading/src/generator/file_gen/logrotate_fs.rs index 3b0368720..50045775b 100644 --- a/lading/src/generator/file_gen/logrotate_fs.rs +++ b/lading/src/generator/file_gen/logrotate_fs.rs @@ -366,19 +366,18 @@ impl Filesystem for LogrotateFS { ) { let tick = self.get_current_tick(); let mut state = self.state.lock().expect("lock poisoned"); - state.advance_time(tick); - - counter!("fs_release").increment(1); - // Remove the FileHandle from the mapping + // Remove `fh->FileHandle` from the set of open_files. let file_handle = { let mut open_files = self.open_files.lock().expect("lock poisoned"); open_files.remove(&fh) }; if let Some(file_handle) = file_handle { - // Close the file in the state + // Close the file in the model, advance time. state.close_file(tick, file_handle); + state.advance_time(tick); + reply.ok(); } else { reply.error(ENOENT); diff --git a/lading/src/generator/file_gen/logrotate_fs/model.rs b/lading/src/generator/file_gen/logrotate_fs/model.rs index 9e8c1fe69..8a6bf79ca 100644 --- a/lading/src/generator/file_gen/logrotate_fs/model.rs +++ b/lading/src/generator/file_gen/logrotate_fs/model.rs @@ -298,6 +298,7 @@ pub(crate) struct State { next_file_handle: u64, inode_scratch: Vec, load_profile: LoadProfile, + valid_file_handles: FxHashMap, // Track valid FileHandle IDs -> Inode } impl std::fmt::Debug for State { @@ -387,6 +388,7 @@ impl State { next_file_handle: 0, inode_scratch: Vec::with_capacity(concurrent_logs as usize), load_profile, + valid_file_handles: FxHashMap::default(), }; if concurrent_logs == 0 { @@ -501,6 +503,7 @@ impl State { file.open(now); let id = self.next_file_handle; self.next_file_handle = self.next_file_handle.wrapping_add(1); + self.valid_file_handles.insert(id, inode); Some(FileHandle { id, inode }) } else { None @@ -519,6 +522,7 @@ impl State { if let Some(Node::File { file, .. }) = self.nodes.get_mut(&handle.inode) { file.close(now); + self.valid_file_handles.remove(&handle.id); } else { panic!("Invalid file handle"); } @@ -816,6 +820,17 @@ impl State { ) -> Option { self.advance_time(now); + // Check if the FileHandle is still valid and maps to the correct inode + if let Some(&inode) = self.valid_file_handles.get(&file_handle.id) { + // Doesn't match the node. + if inode != file_handle.inode { + return None; + } + } else { + // No longer valid. + return None; + } + let inode = file_handle.inode; match self.nodes.get_mut(&inode) { Some(Node::File { ref mut file }) => { @@ -1240,6 +1255,51 @@ mod test { ); } } + + // Property 9: Unlinked files remain readable so long as there is a + // valid file handle. + // + // Files that are unlinked and read-only should remain in the state's + // nodes as long as they have open handles. They should be removed only + // when open_handles == 0. Reads should still be possible via valid open + // file handles. + for (&inode, node) in &state.nodes { + if let Node::File { file } = node { + if file.unlinked && file.read_only { + if file.open_handles > 0 { + // Should remain in state.nodes + assert!( + state.nodes.contains_key(&inode), + "Unlinked, read-only file with open handles should remain in state.nodes" + ); + + // There should be valid file handles pointing to this inode + let valid_handles: Vec<_> = state + .valid_file_handles + .iter() + .filter_map(|(&handle_id, &fh_inode)| { + if fh_inode == inode { + Some(handle_id) + } else { + None + } + }) + .collect(); + + assert!( + !valid_handles.is_empty(), + "Unlinked, read-only file with open handles should have valid file handles" + ); + } else { + // Should be removed from state.nodes after GC + assert!( + !state.nodes.contains_key(&inode), + "Unlinked, read-only file with zero open handles should be removed from state.nodes" + ); + } + } + } + } } fn compute_expected_bytes_written( From 07ff0f35f62109062772a0d3ae775573e51ab4c8 Mon Sep 17 00:00:00 2001 From: "Brian L. Troutwine" Date: Tue, 12 Nov 2024 11:46:10 -0800 Subject: [PATCH 2/4] accidentally dropped a counter Signed-off-by: Brian L. Troutwine --- lading/src/generator/file_gen/logrotate_fs.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lading/src/generator/file_gen/logrotate_fs.rs b/lading/src/generator/file_gen/logrotate_fs.rs index 50045775b..1e8f89fa6 100644 --- a/lading/src/generator/file_gen/logrotate_fs.rs +++ b/lading/src/generator/file_gen/logrotate_fs.rs @@ -367,6 +367,8 @@ impl Filesystem for LogrotateFS { let tick = self.get_current_tick(); let mut state = self.state.lock().expect("lock poisoned"); + counter!("fs_release").increment(1); + // Remove `fh->FileHandle` from the set of open_files. let file_handle = { let mut open_files = self.open_files.lock().expect("lock poisoned"); From ce9149dc067912fba4928350c2a9848712c8dd62 Mon Sep 17 00:00:00 2001 From: "Brian L. Troutwine" Date: Tue, 12 Nov 2024 11:53:13 -0800 Subject: [PATCH 3/4] scoot advance time back Signed-off-by: Brian L. Troutwine --- lading/src/generator/file_gen/logrotate_fs.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lading/src/generator/file_gen/logrotate_fs.rs b/lading/src/generator/file_gen/logrotate_fs.rs index 1e8f89fa6..1c4068414 100644 --- a/lading/src/generator/file_gen/logrotate_fs.rs +++ b/lading/src/generator/file_gen/logrotate_fs.rs @@ -366,6 +366,7 @@ impl Filesystem for LogrotateFS { ) { let tick = self.get_current_tick(); let mut state = self.state.lock().expect("lock poisoned"); + state.advance_time(tick); counter!("fs_release").increment(1); @@ -376,10 +377,8 @@ impl Filesystem for LogrotateFS { }; if let Some(file_handle) = file_handle { - // Close the file in the model, advance time. + // Close the file in the model state.close_file(tick, file_handle); - state.advance_time(tick); - reply.ok(); } else { reply.error(ENOENT); From 1247f506aa6fa978144ec71704c2c1fe5c8dd57c Mon Sep 17 00:00:00 2001 From: "Brian L. Troutwine" Date: Tue, 12 Nov 2024 16:08:16 -0800 Subject: [PATCH 4/4] indentation Signed-off-by: Brian L. Troutwine --- .../generator/file_gen/logrotate_fs/model.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/lading/src/generator/file_gen/logrotate_fs/model.rs b/lading/src/generator/file_gen/logrotate_fs/model.rs index 8a6bf79ca..b962c720b 100644 --- a/lading/src/generator/file_gen/logrotate_fs/model.rs +++ b/lading/src/generator/file_gen/logrotate_fs/model.rs @@ -1268,10 +1268,8 @@ mod test { if file.unlinked && file.read_only { if file.open_handles > 0 { // Should remain in state.nodes - assert!( - state.nodes.contains_key(&inode), - "Unlinked, read-only file with open handles should remain in state.nodes" - ); + assert!(state.nodes.contains_key(&inode), + "Unlinked, read-only file with open handles should remain in state.nodes"); // There should be valid file handles pointing to this inode let valid_handles: Vec<_> = state @@ -1286,16 +1284,12 @@ mod test { }) .collect(); - assert!( - !valid_handles.is_empty(), - "Unlinked, read-only file with open handles should have valid file handles" - ); + assert!(!valid_handles.is_empty(), + "Unlinked, read-only file with open handles should have valid file handles"); } else { // Should be removed from state.nodes after GC - assert!( - !state.nodes.contains_key(&inode), - "Unlinked, read-only file with zero open handles should be removed from state.nodes" - ); + assert!(!state.nodes.contains_key(&inode), + "Unlinked, read-only file with zero open handles should be removed from state.nodes"); } } }