-
Notifications
You must be signed in to change notification settings - Fork 163
virtiofs: Fix issue with getdents #2661
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a critical bug in virtiofs directory enumeration where files could be skipped when deleted between getdents calls. It implements a caching mechanism with stable offsets and also fixes a reference counting issue with hard links.
Changes:
- Implements a sliding window cache (32 entries) for directory enumeration with stable offsets that survive file deletions
- Adds
inc_lookup()method toVirtioFsInodefor incrementing lookup count without path updates - Fixes hard link reference counting by incrementing lookup count when returning existing inode
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/virtio/virtiofs/src/file.rs | Implements DirEntryCursor with stable offset caching, refactors read_dir to use caching mechanism, adds comprehensive unit tests |
| vm/devices/virtio/virtiofs/src/inode.rs | Adds inc_lookup() method for manual lookup count increment |
| vm/devices/virtio/virtiofs/src/lib.rs | Fixes hard link handling to increment lookup count for existing inodes |
| vm/devices/virtio/virtiofs/Cargo.toml | Adds arrayvec dependency for fixed-size cache |
| Cargo.lock | Updates lock file for arrayvec dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
In WSL we have the following testcase which has been failing when running against our virtiofs implementation: https://github.com/microsoft/WSL/blob/116f8d6bf5257b9c5ddef141d4ae6946ef38a4e3/test/linux/unit_tests/lxtfs.c#L807.
The test case does the following:
The test case fails in the following manner:
Initial state: 500 files (positions 0-499)
First getdents64: Reads entries 0-99, returns offset 100
Guest deletes files at positions 0-99
Second getdents64 with offset 100: Windows seeks to position 100, but this is now what was originally position 200 (since 100 files were deleted)
Result: Files 100-199 are skipped entirely.
The solution introduced in this PR is to cache 32 dirent entries (which is roughly 4kB worth of data matching what Linux requests). Each entry is assigned a stable offset that doesn't change as files are deleted.