Abstracted WASI FS #2412

syrusakbary · 2021-06-09T06:41:51Z

Description

This PR is in progress (created so I can comment and give feedback)

Review

Add a short description of the change to the CHANGELOG.md file

The next step is to split it (and WasiFile, etc.) out into an independent crate and implement WasiFS in terms of that.

lib/virtual-fs/src/lib.rs

syrusakbary · 2021-06-09T06:53:33Z

lib/virtual-fs/src/lib.rs

+    #[inline]
+    pub fn downcast_ref<T: 'static>(&'_ self) -> Option<&'_ T> {
+        self.upcast_any_ref().downcast_ref::<T>()
+    }
+    #[inline]
+    pub fn downcast_mut<T: 'static>(&'_ mut self) -> Option<&'_ mut T> {
+        self.upcast_any_mut().downcast_mut::<T>()
+    }


I believe this can simplified a bit if we have a as_any method? (and we move the downcasting back to the code that uses and needs it)

Well, we're dealing with Box<dyn VirtualFile> (previously Box<dyn WasiFile>), due to the way this trait works, everything just works and users don't have to do anything but they can use these methods if they want, so I think it's pretty good. We can discuss possible improvements though

lib/engine/src/artifact.rs

syrusakbary · 2021-06-09T06:57:54Z

lib/virtual-fs/src/lib.rs

+pub trait Upcastable {
+    fn upcast_any_ref(&'_ self) -> &'_ dyn Any;
+    fn upcast_any_mut(&'_ mut self) -> &'_ mut dyn Any;
+    fn upcast_any_box(self: Box<Self>) -> Box<dyn Any>;


This method is not used anywhere

It's a public method, we can review its utility but it's useful when working with dyn Thing and you might, as a user, care about getting your concrete thing out of the dynamic thing. This is a useful part of the WasiFile API which allows users to define whatever logic they want for WasiFiles

syrusakbary · 2021-06-09T06:58:06Z

lib/virtual-fs/src/lib.rs

+/// Trait needed to get downcasting from `VirtualFile` to work.
+pub trait Upcastable {
+    fn upcast_any_ref(&'_ self) -> &'_ dyn Any;
+    fn upcast_any_mut(&'_ mut self) -> &'_ mut dyn Any;


I believe a better name is as_any_mut

syrusakbary · 2021-06-09T06:58:15Z

lib/virtual-fs/src/lib.rs

+// Implementation of `Upcastable` taken from https://users.rust-lang.org/t/why-does-downcasting-not-work-for-subtraits/33286/7 .
+/// Trait needed to get downcasting from `VirtualFile` to work.
+pub trait Upcastable {
+    fn upcast_any_ref(&'_ self) -> &'_ dyn Any;


syrusakbary · 2021-06-09T07:00:49Z

lib/virtual-fs/src/lib.rs

+    fn get_raw_fd(&self) -> Option<i32> {
+        None
+    }


This method is not used publicly, so I believe we can remove it.

At the same time by removing it we also assure that we shield from any specific host requirement in the Virtual FS.

Small correction. Is indeed used in state.rs for polling.
I think we can abstract the polling as well into the Virtual FS. This means removing get_raw_fd and adding a new method for polling.

That way we don't need to expose any host requirements in the VirtualFS itself.

syrusakbary · 2021-06-10T20:17:35Z

lib/virtual-fs/src/mem_fs.rs

+pub struct MemFileSystemInner {
+    // done for recursion purposes
+    fs: MemKind,
+    inodes: HashMap<u64, Box<dyn VirtualFile>>,


I think inodes should not be needed for a MemoryFilesystem? Or is this designed with symlinks in mind?

Why do we need the MemFileHandle?

It seems we have MemFileHandle and MemFile as two ways of having a file in the memfs. I'm still trying to wrap my head around on why do we need two?

It's a bit of a band-aid to cover up a mismatch: the API is written in terms of Box<dyn VirtualFile> but because it's also the backing storage we need to share it. So the internal file implementation here (which is also implemented in terms of VirtualFile but doesn't have to be) is shared with Arc and we return a Box<dyn VirtualFile> for the API itself. This can be fixed after more iteration, I just didn't want to make too many big changes all at once

syrusakbary · 2021-06-10T20:20:46Z

lib/virtual-fs/src/mem_fs.rs

+    }
+
+    fn set_created_time(&self, created_time: u64) {
+        let inner = self.fs.inner.lock().unwrap();


If set_created_time uses &mut self we can move from interior mutability to exterior very easily (so no locks are needed)

In this case we need still need it because it's shared. Interior mutability wouldn't work unless we use atomics. Which is possible here but probably not worth it

What I mean is that if we do the following instead perhaps we can get rid of the lock on this layer:

fn set_created_time(&mut self, created_time: u64) { let inner = self.inner;

Yeah if we make OpenOptions have a lifetime then maybe that can work if we change the whole api to use &mut and require locking from API users (like our existing wasmer code)

syrusakbary · 2021-06-10T20:22:48Z

lib/virtual-fs/src/vfs_fs.rs

+pub struct VfsFileSystem {
+    inner: Arc<dyn vfs::FileSystem>,
+}
+
+impl FileSystem for VfsFileSystem {


Why do we need a VfsFileSystem?

We don't, strictly, but the code is pretty much written. We'll probably delete it before shipping but I'll keep it around until the PR starts being less of draft in case we decide it is something that we want.

Yeah, I was trying to find a use case for it and was not able to find any. Let's keep the file up until merging just in case it results useful

syrusakbary · 2021-06-10T20:39:26Z

lib/virtual-fs/src/lib.rs

+
+pub trait FileSystem: Send + Sync + 'static {
+    fn read_dir(&self, path: &Path) -> Result<std::fs::ReadDir, FsError>;
+    fn create_dir(&self, path: &Path) -> Result<(), FsError>;


Can we have this as &mut self?

Yeah, it could be, I'm not sure it's any better though, then users of dyn FileSystem will need to do the sharing themselves, which is a fine pattern, this way is kind of simpler for users of the API though

In case their code is single-threaded (for example, when compiled to wasm and used in js) it will make things easier and not lock dependent (since threads are not available in Wasm WASI), so I think it will good to do that to unlock it

Hmm, interesting, so the use case for Wasmer absolutely needs locking due to how WasmerEnv works and how we'd need to make things work in WASI and emscripten... that's a good point though, if it's for use in Wasm, maybe it's worth it to move the locking to the API user as needed

lib/virtual-fs/src/lib.rs

syrusakbary · 2021-06-10T20:41:26Z

lib/virtual-fs/src/lib.rs

+    fn set_len(&mut self, new_size: u64) -> Result<(), FsError>;
+
+    /// Request deletion of the file
+    fn unlink(&mut self) -> Result<(), FsError>;


Can we have this as self (so it's consumed)?

Thinking more, I think perhaps unlink and rename can be in the FileSystem api rather than in the VirtualFile itself. It makes things a bit easier to reason about I think

Yeah, it's due to WasiFile that it is this way, I agree that it's probably possible to move some of that logic away, it's really just a clean up for the file itself. I think self is problematic in trait objects, Drop::drop takes self by &mut so I think this is fine

Ok, I added a side comment on other place... perhaps we can delete this method and use only the filesystem one (remove_file)

Yeah, I'm in no rush to do this yet (as I haven't done it) because I think depending on how we want to compose different types of VirtualFiles and file systems it may still make sense to have this as-is.

Working out the exact lines between the abstractions is still a work in progress

syrusakbary · 2021-06-10T20:42:05Z

lib/virtual-fs/src/lib.rs

+    /// Store file contents and metadata to disk
+    /// Default implementation returns `Ok(())`.  You should implement this method if you care
+    /// about flushing your cache to permanent storage
+    fn sync_to_disk(&self) -> Result<(), FsError> {
+        Ok(())
+    }


I think this only make sense for some kind of filesystems. Can we move it to other layer?

Well it's fine for filesystems to not implement it, if this is in WasiFile then it's fine. But WASI needs a method like this to call for things to be emulated accurately

Why not move it to the WASI filesystem layer?

How would we? This is the layer through which WASI is interacting with the filesystem

I think it's important to have not only these types of methods, but a lot more if we want this to be generally useful as a drop-in replacement abstraction for real file system usage...

lib/virtual-fs/src/lib.rs

syrusakbary · 2021-10-28T16:00:00Z

This is already addressed by #2491. Closing the PR

Mark McCaskey added 3 commits June 4, 2021 14:22

Build core abstraction for misc FS operations

e9dc6fe

The next step is to split it (and WasiFile, etc.) out into an independent crate and implement WasiFS in terms of that.

Split out vfs abstractions into their own crate

ecf4c04

Move host fs impl into virtual fs crate

d7e1b57

syrusakbary requested review from Hywan and MarkMcCaskey as code owners June 9, 2021 06:41

syrusakbary commented Jun 9, 2021

View reviewed changes

lib/virtual-fs/src/lib.rs Outdated Show resolved Hide resolved

syrusakbary commented Jun 9, 2021

View reviewed changes