-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Use the NODEFS filesystem in statfs when available #23912
Closed
bgrgicak
wants to merge
5
commits into
emscripten-core:main
from
bgrgicak:fix/statfsNode-should-use-NODEFS-if-available
Closed
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7d0c9a6
Use the NODEFS filesystem in statfs when available
bgrgicak cd33f4e
Test number of block in statfs
bgrgicak b1b9b01
Test NODERAWFS
bgrgicak b53842c
Test with real blocks count
bgrgicak 5e8d396
Limit real disk space test to only nodefs
bgrgicak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
statfs
should figure out which filesystem the path is in and then return the filesystem info for that filesystem.i.e. if
path
is in the MEMFS you should get the memfs info. Ifpath
is a NODEFS you should get the node FS info.The presense of NODEFS alone should not be a factor, right? The question should be "is path part of a node FS mount" I think,
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.
I've been thinking about this for a week and I'm not sure how to handle it, so let me start with our usecase and why the above approach doesn't work for us.
WordPress Playground mounts all files into MEMFS because some files like the WordPress code are being downloaded from remote URL while a plugin might be imported from a local folder. So, the resulting codebase that we are running inside Emscription is composed out of multiple data sources.
Curently when the code asks for available disk space using
statfs("/")
it get's the default values because/
is a MEMFS directory.I was able to work around this by overriding
statfs
duringonRuntimeInitialized
.Checking what FS the path belongs to, makes sense overall, but in some cases like the above it would be great if we could override it and force a FS to be used.
Specifically for
statfs
this makes sense to force NODEFS, because it only works in NODEFS (and NODERAWFS).If the requested path is available in that NODEFS it would be reasonable to override MEMFS and return the result fromNODEFS.node_ops.statfs.statfs
.What do you think? How would you approach our usecase?
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.
Which code is doing this? Asking for the available disk space by using
statfs("/")
seem odd since normally the root FS has very little space in it right? Where is that code that is doing this? Why is it using/
?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.
But won't the
statfs
from node give very different results if you stat/
vs/tmp
vs/home
, for example. What is the code in question interested in exactly?