Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate operations on FileHandles #1113

Merged
merged 4 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lading/src/generator/file_gen/logrotate_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,14 +370,14 @@ impl Filesystem for LogrotateFS {

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
state.close_file(tick, file_handle);
reply.ok();
} else {
Expand Down
60 changes: 60 additions & 0 deletions lading/src/generator/file_gen/logrotate_fs/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ pub(crate) struct State {
next_file_handle: u64,
inode_scratch: Vec<Inode>,
load_profile: LoadProfile,
valid_file_handles: FxHashMap<u64, Inode>, // Track valid FileHandle IDs -> Inode
}

impl std::fmt::Debug for State {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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");
}
Expand Down Expand Up @@ -816,6 +820,17 @@ impl State {
) -> Option<Bytes> {
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 }) => {
Expand Down Expand Up @@ -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"
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: indentation is off here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it is and cargo-fmt didn't catch it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Macros :( There was a google summer of code project that I think will improve this in the next few releases.

} 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(
Expand Down
Loading