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

VFS feature #76

Merged
merged 11 commits into from
Mar 12, 2023
Merged

VFS feature #76

merged 11 commits into from
Mar 12, 2023

Conversation

trungnt2910
Copy link
Collaborator

This PR contains the "VFS feature" as discussed in the Discord server.

As this feature is expensive in terms of syscalls and memory, it is disabled by default.
Before this PR I've made a reasonable attempt to fix as many memory leaks as possible by running the tests with MODE=asan.

I've also left some more details in the commit messages.

@trungnt2910
Copy link
Collaborator Author

This pull request makes me wonder if it's possible to do the same for PID/TID emulation.

I wonder if it's possible to wrap all syscall/library calls that take a PID value and send it to a subsystem of blink, just like what this PR is doing with file descriptors. When this "PID emulation" subsystem is disabled, the wrappers become simple macros.

This feature would be really useful for platforms without fork like Emscripten.

jart added a commit that referenced this pull request Mar 6, 2023
@trungnt2910
Copy link
Collaborator Author

Did this PR cause any build warning/failures? I've tested both on Cygwin and Linux and ensured that there are no build failures and/or warnings.

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

Please apply the changes from 6d72c93 you may want to consider adding your username to build/config.mk so that -Werror is enabled in your dev environment. I don't enable it by default for obvious reasons, but it should be enabled for development.

blink/vfs.h Outdated
#define VfsMsync msync
#endif

#define VFS_SYSTEM_ROOT_MOUNT "/SystemRoot"
Copy link
Owner

Choose a reason for hiding this comment

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

Can we not hard code this? Shouldn't this be behind the -C flag?

Copy link
Collaborator Author

@trungnt2910 trungnt2910 Mar 6, 2023

Choose a reason for hiding this comment

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

I don't enable it by default for obvious reasons, but it should be enabled for development.

I see that enabling this breaks the build, even after cherry-picking 6d72c93:

blink/disinst.c: In function ‘DisName’:
blink/disinst.c:121:37: error: array subscript has type ‘char’ [-Werror=char-subscripts]
  121 |     for (np = name; *np && (islower(*np) || isdigit(*np)); ++np) {
      |                                     ^~~
cc -fno-align-functions -fno-common -pthread -fpie -g -O2 -fno-omit-frame-pointer -fno-optimize-sibling-calls -gdwarf-2 -U_FORTIFY_SOURCE -Wall -Werror -Wno-unused-function -isystem third_party/libz -D_FILE_OFFSET_BITS=64 -D_DARWIN_C_SOURCE -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_GNU_SOURCE -iquote.  -c -o o//blink/divmul.o blink/divmul.c
blink/disinst.c:121:53: error: array subscript has type ‘char’ [-Werror=char-subscripts]
  121 |     for (np = name; *np && (islower(*np) || isdigit(*np)); ++np) {
      |                                                     ^~~

Should I fix this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The -C flag is the equivalent of the $BLINK_PREFIX env var in this PR.
I'll edit it to respect -C as well.

/SystemRoot is a default hostfs mount, similar to the default devfs mount at /dev and procfs mount at /proc. This should suffice until we implement fstab support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've fixed my code to respect the -C flag when mounting / as hostfs.
The system root / is still mounted to /SystemRoot, unless you think that I should keep that under a flag too.

trungnt2910 pushed a commit to trungnt2910/blink that referenced this pull request Mar 6, 2023
@trungnt2910 trungnt2910 closed this Mar 6, 2023
@trungnt2910 trungnt2910 reopened this Mar 6, 2023
trungnt2910 pushed a commit to trungnt2910/blink that referenced this pull request Mar 8, 2023
trungnt2910 pushed a commit to trungnt2910/blink that referenced this pull request Mar 8, 2023
trungnt2910 and others added 7 commits March 9, 2023 23:26
Build support for the VFS feature. This feature is disabled by default
and can be enabled by passing --enable-vfs to configure.

VFS and overlays are two mutually exclusive features.

At this stage, --enable-vfs does not work yet and the build will fail
with undefined symbol errors. The default build is unaffected.
This commit comes with the full VFS feature. A built-in `hostfs` is also
included. `procfs` and `devfs` are currently not implemented but exist
as an alias to `hostfs` that by default maps to the `/proc` and `/dev`
folders of the host.

Now, all system calls that requires a file descriptor should be
redirected to the corresponding function with a `Vfs` prefix. When this
feature is disabled (the default behavior), these functions will
become macros that redirect to the original ones (or functions
with `Overlays` prefix if they are available).

Two blink syscalls are also fixed:
- `sendfile` now does not cause an infinite loop.
- `select` is now implemented using `poll` and respects the custom
`poll` implementations of each file descriptor.
VFS feature uses the same -C flag as the overlays system.
hostfs operations now do not abuse dup, openat, and close syscalls.
This doubles the performance of hostfs operations.

Also fixes some errors of the `./configure` script on Cygwin.
- Correct error for unsupported msync operation.
- VfsAccess now properly checks for symlinks.
- Fix bug in HostfsGetOptimalDirFdName when "dir" is the root directory.
- Fix mmap for Cygwin systems when the file offset is past the end.
- Purge all potential build errors, including switch statements
with system macros, struct member names that are potential macros,
and void* arithmetic.
- Fix a performance issue in VFS where every traverse would take
O(n^2) instead of O(n).
- Take advantage of the PATH_MAX and NAME_MAX constants to use static
buffers and improve string performance.
Do not traverse VFS paths when dealing with a abstract UNIX socket.
Blinkenlights should now correctly uses `Vfs`-prefixed functions.
Tested with tests from the LTP.
@jart jart merged commit 8609bda into jart:master Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants