virtiofs/section: respect lookup_count in forget()#2781
virtiofs/section: respect lookup_count in forget()#2781benhillis wants to merge 2 commits intomicrosoft:mainfrom
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
This PR updates the SectionFs virtio-fs backend to avoid dropping inode state on FORGET until the kernel’s lookup references have been released, aligning behavior with FUSE’s lookup_count semantics.
Changes:
- Add per-inode
lookup_counttracking toSectionFsinodes. - Update
SectionFs::forget()to decrement bylookup_countand only remove the inode when the count reaches zero.
| let inode = Inode { | ||
| size, | ||
| state: InodeState::Open(section), | ||
| lookup_count: 1, | ||
| }; |
There was a problem hiding this comment.
lookup_count is always initialized to 1 when creating an Inode, but there’s no code path that ever increments it on subsequent lookups of the same inode/node_id. As a result, this won’t actually prevent premature removal in scenarios where the kernel holds multiple lookup references to the same node_id. To truly “track the count per inode”, you’ll need to reuse an existing node_id for repeated lookups and increment the stored count (similar to how VirtioFsInode::lookup() works), or otherwise redesign the inode table to key by object identity/path and bump the count when returning an existing entry.
| fn forget(&self, node_id: u64, lookup_count: u64) { | ||
| let mut inodes = self.inodes.lock(); | ||
| if let Some(inode) = inodes.get_mut(node_id) { | ||
| inode.lookup_count = inode.lookup_count.saturating_sub(lookup_count); |
There was a problem hiding this comment.
saturating_sub() will silently clamp to 0 if lookup_count is greater than the tracked count, which can hide lookup/forget mismatches and cause unexpected inode removal. Consider matching the behavior used elsewhere in this crate (e.g., warn when too many forgets are observed) so unexpected kernel/client behavior is diagnosable.
| inode.lookup_count = inode.lookup_count.saturating_sub(lookup_count); | |
| if lookup_count > inode.lookup_count { | |
| tracing::warn!( | |
| node_id, | |
| lookup_count, | |
| current_lookup_count = inode.lookup_count, | |
| "received forget with lookup_count greater than tracked count" | |
| ); | |
| inode.lookup_count = 0; | |
| } else { | |
| inode.lookup_count -= lookup_count; | |
| } |
SectionFs::forget() unconditionally removed the inode regardless of the lookup_count parameter. This could cause premature inode removal when multiple kernel references existed. Track the count per inode and only remove when it reaches zero.
9eb7bd3 to
2f918b8
Compare
SectionFs::forget() unconditionally removed the inode regardless of the lookup_count parameter. This could cause premature inode removal when multiple kernel references existed. Track the count per inode and only remove when it reaches zero.