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.

Sign up for GitHub

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

Jump to bottom

Sentry Virtual Filesystem v2 (VFS2) #1035

Open
fvoznika opened this issue Oct 18, 2019 · 4 comments
Open

Sentry Virtual Filesystem v2 (VFS2) #1035

fvoznika opened this issue Oct 18, 2019 · 4 comments
Assignees
@nixprime
Labels
area: filesystem Issue related to filesystem area: performance Issue related to performance & benchmarks revived The Issue has been revived by the issue reviver. type: enhancement New feature or request
Milestone
VFS2

Comments

@fvoznika
Copy link
Member

Current VFS implementation in the Sentry has performance shortcomings with regarding to the number of operations that need to be performed for path walks. This is especially noticeable when using gofer mounted filesystems, where the round trip cost for each operation is aggravated by the RPC and scheduling costs.

VFS2 addresses this by delegating path resolution to the filesystem, making it possible to send a single RPC for each syscall operation, instead of one RPC per path component in the operation. For example, stat(/foo/bar/goo) generates at least 3 RPC round trips to the gofer (foo, bar, goo), while VFS2 makes only 1.

VFS2 also allows for the implementation of recursive bind mounts, which would require a major refactoring with the current implementation.
@fvoznika fvoznika added area: performance Issue related to performance & benchmarks area: filesystem Issue related to filesystem labels Oct 18, 2019
@fvoznika fvoznika mentioned this issue Oct 18, 2019
Closed
@fvoznika fvoznika added the type: enhancement New feature or request label Oct 18, 2019
gvisor-bot pushed a commit that referenced this issue Oct 31, 2019
@nixprime @gvisor-bot
Implement basic umounting for vfs2.
c45b003 
This is required to test filesystems with a non-trivial implementation of
FilesystemImpl.Release(). Propagation isn't handled yet, and umount isn't yet
plumbed out to VirtualFilesystem.UmountAt(), but otherwise the implementation
of umount is believed to be correct.

- Move entering mountTable.seq writer critical sections to callers of
  mountTable.{insert,remove}Seqed. This is required since umount(2) must ensure
  that no new references are taken on the candidate mount after checking that
  it isn't busy, which is only possible by entering a vfs.mountTable.seq writer
  critical section before the check and remaining in it until after
  VFS.umountRecursiveLocked() is complete. (Linux does the same thing:
  fs/namespace.c:do_umount() => lock_mount_hash(),
  fs/pnode.c:propagate_mount_busy(), umount_tree(), unlock_mount_hash().)

- It's not possible for dentry deletion to umount while only holding
  VFS.mountMu for reading, but it's also very unappealing to hold VFS.mountMu
  exclusively around e.g. gofer unlink RPCs. Introduce dentry.mu to avoid these
  problems. This means that VFS.mountMu is never acquired for reading, so
  change it to a sync.Mutex.

Updates #1035

PiperOrigin-RevId: 277607680
@amscanne amscanne added this to the VFSv2 milestone Nov 18, 2019
@gvisor-bot gvisor-bot mentioned this issue Dec 24, 2019
Merged
gvisor-bot pushed a commit that referenced this issue Dec 24, 2019
@fvoznika @gvisor-bot
Fix deadlock in kernfs.Filesystem.revalidateChildLocked
574e988 
It was calling Dentry.InsertChild with the dentry's mutex
already locked.

Updates #1035

PiperOrigin-RevId: 286962742
@gvisor-bot gvisor-bot mentioned this issue Feb 4, 2020
Merged
gvisor-bot pushed a commit that referenced this issue Feb 4, 2020
@fvoznika @gvisor-bot
Remove argument from vfs.MountNamespace.DecRef()
93e6854 
Updates #1035

PiperOrigin-RevId: 293037526
gvisor-bot pushed a commit that referenced this issue Feb 4, 2020
@fvoznika @gvisor-bot
Remove argument from vfs.MountNamespace.DecRef()
dcffddf 
Updates #1035

PiperOrigin-RevId: 293194631
copybara-service bot pushed a commit that referenced this issue Mar 10, 2020
@fvoznika @gvisor-bot
Enforce file size rlimits in VFS2
c3cc54a 
Updates #1035

PiperOrigin-RevId: 298371120
@copybara-service copybara-service bot mentioned this issue Mar 10, 2020
Merged
copybara-service bot pushed a commit that referenced this issue Mar 16, 2020
@fvoznika @gvisor-bot
Enforce file size rlimits in VFS2
2a6c436 
Updates #1035

PiperOrigin-RevId: 301255357
copybara-service bot pushed a commit that referenced this issue Nov 13, 2020
@fvoznika @gvisor-bot
Remove TESTONLY tag from vfs2 flag
74be0dd 
Updates #1035

PiperOrigin-RevId: 342168926
@fvoznika
Copy link
Member Author

fvoznika commented Nov 13, 2020 •
edited

VFS2 is feature complete. Vast majority of tests were converted to run with VFS2 enabled and they are all passing. We're starting to enable VFS2 internally at Google and will flip the default to VFS2 once we have soaked it in production for some time.

We recommend everyone to start running with the --vfs2 flag to catch any possible issue early on, before we switch over to VFS2 by default.

@iangudger
Copy link
Contributor

@fvoznika Is VFS2 ready to be the default? The last update was almost a year ago.

@fvoznika
Copy link
Member Author

fvoznika commented Oct 6, 2021

Yes, it's ready to be the default. We're just waiting for a few internal users to enable it before we flip the flag. But everyone is encouraged to run with --vfs2 at this point.

copybara-service bot pushed a commit that referenced this issue Oct 14, 2021
@fvoznika @gvisor-bot
Mount namespace can be nil after task exits
1bf4992 
Updates #1035

PiperOrigin-RevId: 403219644
@copybara-service copybara-service bot mentioned this issue Oct 14, 2021
Merged
copybara-service bot pushed a commit that referenced this issue Oct 15, 2021
@fvoznika @gvisor-bot
Change test to use VFS2
cd2594b 
Updates #1035

PiperOrigin-RevId: 403237718
@copybara-service copybara-service bot mentioned this issue Oct 15, 2021
Merged
copybara-service bot pushed a commit that referenced this issue Oct 18, 2021
@fvoznika @gvisor-bot
Mount namespace can be nil after task exits
eafa3f1 
Updates #1035

PiperOrigin-RevId: 404017795
@copybara-service copybara-service bot mentioned this issue Oct 18, 2021
Merged
copybara-service bot pushed a commit that referenced this issue Oct 18, 2021
@fvoznika @gvisor-bot
Change test to use VFS2
832c309 
Updates #1035

PiperOrigin-RevId: 404043283
copybara-service bot pushed a commit that referenced this issue Oct 18, 2021
@fvoznika @gvisor-bot
Report ramdiskfs usage correctly
fa56fbf 
Updates #1035

PiperOrigin-RevId: 404072231
copybara-service bot pushed a commit that referenced this issue Oct 20, 2021
@fvoznika @gvisor-bot
Report correct error when restore fails
3bc75d6 
When file corruption is detected, report vfs.ErrCorruption to
distinguish corruption error from other restore errors.

Updates #1035

PiperOrigin-RevId: 404402516
@copybara-service copybara-service bot mentioned this issue Oct 20, 2021
Merged
copybara-service bot pushed a commit that referenced this issue Oct 20, 2021
@fvoznika @gvisor-bot
Enable VFS2 by default
98c065d 
This change enables VFS2 by default. VFS2 is much faster than the previous
implementation and it's also more compatible. VFS1 is no longer supported and
will be deleted from the code.

Use `--vfs2=false` if you need to disable it. Make sure to report a bug if you
have the need to disable VFS2 or something is not working for you.

Closes #1035

PiperOrigin-RevId: 404404173
@copybara-service copybara-service bot mentioned this issue Oct 20, 2021
Closed
copybara-service bot pushed a commit that referenced this issue Oct 20, 2021
@fvoznika @gvisor-bot
Report correct error when restore fails
c23d67f 
When file corruption is detected, report vfs.ErrCorruption to
distinguish corruption error from other restore errors.

Updates #1035

PiperOrigin-RevId: 404588445
@github-actions github-actions bot reopened this Oct 22, 2021
@github-actions github-actions bot added the revived The Issue has been revived by the issue reviver. label Oct 22, 2021
@github-actions
Copy link

There are TODOs still referencing this issue:

  1. pkg/sentry/loader/elf.go:97: Once VFS2 ships, rewrite this to wrap
  2. pkg/sentry/vfs/file_description.go:278: FileDescriptionImpl.SetOAsync()?
  3. pkg/sentry/vfs/mount.go:255: Linux requires that either both the mount
  4. pkg/sentry/vfs/mount.go:320: Linux special-cases umount of the caller's
  5. pkg/sentry/vfs/mount_test.go:58: concurrent lookup/insertion/removal.

Search TODO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nixprime nixprime

Labels
area: filesystem Issue related to filesystem area: performance Issue related to performance & benchmarks revived The Issue has been revived by the issue reviver. type: enhancement New feature or request
Projects
None yet
Milestone
VFS2
Development

Successfully merging a pull request may close this issue.

Enable VFS2 by default google/gvisor
5 participants
@ianlewis @amscanne @nixprime @fvoznika @iangudger