Add Async FS #271
Add Async FS #271
Conversation
liqinggd
force-pushed
the
dev-add-async-fs
branch
5 times, most recently
from
5fb57ab to
0877bc3
Compare
There was a problem hiding this comment.
Good job on implementing async file systems. This PR is a major step towards the completion of NGO and improving its file I/O performance.
Given the complexity of this PR, it is unsurprising that one can identify some issues. Actually, I made 50+ comments. I summarize some of the major issues here.
- The page cache must be configurable by the user of this crate. In particular, the page allocator cannot be hardcoded in
AsyncSimpleFs. - The error handling in file system needs to be more robust. There are some failure points where
Erris simply returned without rolling back the side effects, leaving inodes or the file system in an inconsistent state. - There are some race conditions that may not be handled.
| /// The Dentry is used to speed up the pathname lookup | ||
| pub struct Dentry { | ||
| inode: Arc<dyn AsyncInode>, | ||
| open_path: String, |
There was a problem hiding this comment.
It is unclear whether the open_path is absolute or it relative to inode.
There was a problem hiding this comment.
the path is absolute, change the name to abs_path
| dentry: Dentry, | ||
| offset: AsyncMutex<usize>, | ||
| access_mode: AccessMode, | ||
| status_flags: RwLock<StatusFlags>, |
There was a problem hiding this comment.
Using Atomic<StatusFlags> seems to be a better choice, as it is more light-weight.
There was a problem hiding this comment.
the set_status_flags() method needs to handle the race conditions, so I prefer to use rwlock
|
|
||
| /// Sync all data and metadata | ||
| async fn sync_all(&self) -> Result<()> { | ||
| return_errno!(ENOSYS, "not support"); |
There was a problem hiding this comment.
I think nop is a better default implementation.
liqinggd
force-pushed
the
dev-add-async-fs
branch
11 times, most recently
from
bce8db3 to
b8370d1
Compare
liqinggd
force-pushed
the
dev-add-async-fs
branch
from
b8370d1 to
d324ee2
Compare
Need to merge occlum/sefs#43 first
issue: #265