feat(slayerfs): std-like SDK + fs/file-io layer alignment#403
feat(slayerfs): std-like SDK + fs/file-io layer alignment#403mon3stera merged 4 commits intork8s-dev:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements a std/tokio-like SlayerFS SDK and aligns the VFS/meta/chunk I/O stack by introducing a new path-based fs::FileSystem layer, inode-based write/xattr/ACL helpers, and refactoring file_io as a shared module.
Changes:
- Add
fs::FileSystem(path-based ops + permissions + access logging) and a newsdk_fsmodule (OpenOptions,File, and convenience functions). - Refactor VFS and
file_iointegration (inode-based write path, statfs, xattr/ACL hooks, chunk-id helpers moved). - Add xattr storage in the metadata layer (SeaORM entity + DatabaseMetaStore support), and update docs/examples.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| project/slayerfs/src/vfs/sdk.rs | SDK client switched to FileSystem backend; adds *_io methods and local constructors. |
| project/slayerfs/src/vfs/mod.rs | Re-exports chunk-id helpers from file_io; module visibility adjustments. |
| project/slayerfs/src/vfs/io/mod.rs | Redirects VFS I/O exports to file_io types. |
| project/slayerfs/src/vfs/fs.rs | Adds mkdir/create-file structured variants, inode-based write, xattr/ACL helpers, statfs. |
| project/slayerfs/src/vfs/error.rs | Adds VfsError::from_meta mapping helper. |
| project/slayerfs/src/sdk_fs.rs | New std-like async SDK (SdkClient, OpenOptions, File, helpers + tests). |
| project/slayerfs/src/meta/stores/database_store.rs | Adds xattr table + CRUD wiring; deletes xattr on inode removal. |
| project/slayerfs/src/meta/store.rs | Extends MetaStore trait with xattr getters/listing defaults. |
| project/slayerfs/src/meta/layer.rs | Extends MetaLayer with xattr/ACL methods. |
| project/slayerfs/src/meta/entities/xattr_meta.rs | Adds SeaORM entity model for xattr table. |
| project/slayerfs/src/meta/entities/mod.rs | Exposes xattr entity and adjusts entity module visibility/exports. |
| project/slayerfs/src/meta/config.rs | Introduces MetaClientConfig and TTL normalization helper. |
| project/slayerfs/src/meta/client.rs | Implements xattr/ACL forwarding in MetaClient’s MetaLayer impl. |
| project/slayerfs/src/main.rs | Adds file_io/fs modules to binary; global dead-code allowance added. |
| project/slayerfs/src/lib.rs | Exports new file_io, fs, and sdk_fs modules. |
| project/slayerfs/src/fuse/mod.rs | Uses inode-based write, implements statfs, and adds xattr FUSE ops. |
| project/slayerfs/src/fs/mod.rs | New path-based FileSystem implementation + tests. |
| project/slayerfs/src/file_io/mod.rs | New shared file_io module (chunk-id helpers, registry, IO factory). |
| project/slayerfs/src/file_io/inode.rs | Makes Inode public and cleans up docs. |
| project/slayerfs/src/file_io/reader.rs | Refactors reader internals and invalidation logic; updates tests. |
| project/slayerfs/src/file_io/writer.rs | Refactors writer internals to new file_io layout; updates tests. |
| project/slayerfs/examples/sdk_fs_demo.rs | New example showcasing std-like SDK usage. |
| project/slayerfs/examples/sdk_demo.rs | Updates example to match new SDK client usage. |
| project/slayerfs/examples/s3_demo.rs | Updates S3 demo to use Client::new (FileSystem-backed). |
| project/slayerfs/README_CN.md | Updates architecture description (fs vs vfs separation). |
| project/slayerfs/README.md | Updates architecture description (fs vs vfs separation). |
Comments suppressed due to low confidence (5)
project/slayerfs/src/file_io/reader.rs:293
read_atnow prepares slices and copies available ranges, but it no longer pre-fillsbuf[..actual_len]with zeros. If there are holes/missing slices, the corresponding bytes in the caller’s buffer will retain their previous contents rather than reading as zero-filled. Ifread_atis intended to provide the same semantics asread/DataFetcher(zero-fill gaps), reintroduce an explicit zero-fill step.
project/slayerfs/src/file_io/reader.rs:18- There are multiple unused imports here (
chunk_id_for,split_chunk_spans, and likelyChunkLayout/TryFromdepending on the final implementation). This will trigger warnings/clippy in new code; please remove unused imports or switchread_atback to using the sharedsplit_chunk_spanshelper.
project/slayerfs/src/file_io/writer.rs:1060 WriteConfig::newcurrently takes a singleChunkLayoutargument (seevfs/config.rs), but this test calls it with(layout, 4 * 1024), which will not compile. Either revert toWriteConfig::new(layout).page_size(4 * 1024)or changeWriteConfig::new’s signature and update all call sites consistently.
project/slayerfs/src/file_io/writer.rs:1296- Same issue as above:
WriteConfig::new(layout, 4 * 1024)does not match the currentWriteConfig::new(layout)API and will fail to compile. Use the existing builder (.page_size(...)) or update the constructor signature everywhere.
project/slayerfs/src/file_io/reader.rs:609 - This test uses
WriteConfig::new(layout, 4 * 1024), butWriteConfig::newcurrently only acceptsChunkLayout. As written this won’t compile; useWriteConfig::new(layout).page_size(4 * 1024)(or update the constructor signature consistently across the codebase).
project/slayerfs/src/vfs/sdk.rs
Outdated
| pub type LocalClient = Client<ObjectBlockStore<LocalFsBackend>, Arc<dyn MetaStore>>; | ||
|
|
||
| #[allow(dead_code)] | ||
| impl LocalClient { | ||
| #[allow(dead_code)] | ||
| pub async fn new_local<P: AsRef<Path>>(root: P, layout: ChunkLayout) -> Result<Self, VfsError> { | ||
| let client = ObjectClient::new(LocalFsBackend::new(root)); | ||
| let meta_handle = create_meta_store_from_url("sqlite::memory:").await?; | ||
| let metadata = meta_handle.layer(); | ||
| let metadata: Arc<dyn MetaStore> = meta_handle.store(); | ||
| let store = ObjectBlockStore::new(client); | ||
| let fs = FileSystem::new(layout, store, metadata) |
There was a problem hiding this comment.
LocalClient uses Arc<dyn MetaStore> as the M type parameter, but Arc<dyn MetaStore> does not implement MetaStore (trait bounds require the concrete type itself to implement the trait, not just deref to it). This will fail to compile unless there is a blanket impl<T: MetaStore + ?Sized> MetaStore for Arc<T> (couldn’t find one). Consider either adding a forwarding impl for Arc<T> or changing Client/FileSystem to store Arc<dyn MetaStore> directly rather than as a generic type parameter.
| async fn create_test_fs() -> FileSystem<ObjectBlockStore<LocalFsBackend>, Arc<dyn MetaStore>> { | ||
| let tmp = tempdir().unwrap(); | ||
| let layout = ChunkLayout::default(); | ||
| let client = ObjectClient::new(LocalFsBackend::new(tmp.path())); | ||
| let meta_handle = create_meta_store_from_url("sqlite::memory:").await.unwrap(); | ||
| let metadata: Arc<dyn MetaStore> = meta_handle.store(); | ||
| let store = ObjectBlockStore::new(client); | ||
| let config = FileSystemConfig::default().with_caller(CallerIdentity::root()); | ||
| FileSystem::with_config(layout, store, metadata, config) |
There was a problem hiding this comment.
This test helper returns FileSystem<..., Arc<dyn MetaStore>>, but Arc<dyn MetaStore> does not implement the MetaStore trait (only dyn MetaStore does). Without a blanket forwarding impl (e.g., impl<T: MetaStore + ?Sized> MetaStore for Arc<T>), this generic instantiation won’t compile.
project/slayerfs/src/main.rs
Outdated
| #![allow(dead_code)] | ||
|
|
There was a problem hiding this comment.
#![allow(dead_code)] at the binary crate root suppresses dead-code warnings across the entire slayerfs CLI, which can hide real regressions and make CI less effective. Prefer scoping the allowance to specific modules/items (or removing it if no longer needed).
| #![allow(dead_code)] |
project/slayerfs/src/fs/mod.rs
Outdated
| let target_size = offset + data.len() as u64; | ||
| if target_size > inode.file_size() { | ||
| inode.update_size(target_size); | ||
| meta_layer | ||
| .set_file_size(ino, target_size) | ||
| .await | ||
| .map_err(|e| meta_error_to_io(path, e))?; |
There was a problem hiding this comment.
write_inode updates target_size using data.len() rather than the actual written byte count returned by write_at. If a partial write occurs, this can incorrectly extend the file size beyond what was persisted. Use offset + written as u64 for size updates.
project/slayerfs/src/fs/mod.rs
Outdated
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | ||
| let parent_attr = self | ||
| .meta_layer | ||
| .stat(parent_ino) | ||
| .await | ||
| .map_err(|e| meta_error_to_io(&dir, e))? | ||
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | ||
| if parent_attr.kind != FileType::Dir { | ||
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | ||
| } | ||
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | ||
|
|
||
| let parent_attr = self | ||
| .meta_layer | ||
| .stat(parent_ino) | ||
| .await | ||
| .map_err(|e| meta_error_to_io(&dir, e))? | ||
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | ||
| if parent_attr.kind != FileType::Dir { | ||
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | ||
| } | ||
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | ||
|
|
There was a problem hiding this comment.
mkdir contains duplicated stat(parent_ino) calls and repeated check_access(...) calls. This is likely accidental duplication and adds extra MetaStore round-trips on a hot path. Consider keeping a single parent_attr fetch and a single permission check before proceeding.
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | |
| let parent_attr = self | |
| .meta_layer | |
| .stat(parent_ino) | |
| .await | |
| .map_err(|e| meta_error_to_io(&dir, e))? | |
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | |
| if parent_attr.kind != FileType::Dir { | |
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | |
| } | |
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | |
| let parent_attr = self | |
| .meta_layer | |
| .stat(parent_ino) | |
| .await | |
| .map_err(|e| meta_error_to_io(&dir, e))? | |
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | |
| if parent_attr.kind != FileType::Dir { | |
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | |
| } | |
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; |
project/slayerfs/src/fuse/mod.rs
Outdated
| let n = self | ||
| .write_ino(ino as i64, offset, data) | ||
| .await | ||
| .map_err(|_| libc::EIO)? as u32; |
There was a problem hiding this comment.
FUSE write currently maps every write_ino failure to EIO, which discards useful errno mapping (e.g., EISDIR, ENOSPC, EACCES, ESTALE, etc.) and makes debugging harder. Prefer mapping the underlying VfsError into Errno (similar to other ops in this file).
| .map_err(|_| libc::EIO)? as u32; | |
| .map_err(Into::<Errno>::into)? as u32; |
| fn make_log_context() -> LogContext { | ||
| let uid = unsafe { geteuid() as u32 }; | ||
| let gid = unsafe { getegid() as u32 }; | ||
| let pid = std::process::id(); | ||
| LogContext::new(uid, gid, pid) | ||
| } | ||
|
|
||
| fn current_ids() -> (u32, u32) { | ||
| let uid = unsafe { geteuid() as u32 }; | ||
| let gid = unsafe { getegid() as u32 }; | ||
| (uid, gid) | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct CallerIdentity { | ||
| pub uid: u32, | ||
| pub gid: u32, | ||
| pub groups: Vec<u32>, | ||
| } | ||
|
|
||
| impl CallerIdentity { | ||
| pub fn new(uid: u32, gid: u32, groups: Vec<u32>) -> Self { | ||
| Self { uid, gid, groups } | ||
| } | ||
|
|
||
| pub fn current() -> Self { | ||
| let (uid, gid) = current_ids(); | ||
| let groups = current_groups(gid); | ||
| Self { uid, gid, groups } | ||
| } | ||
|
|
||
| pub fn root() -> Self { | ||
| Self { | ||
| uid: 0, | ||
| gid: 0, | ||
| groups: vec![0], | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn current_groups(primary_gid: u32) -> Vec<u32> { | ||
| let mut groups = Vec::new(); | ||
| unsafe { | ||
| let count = getgroups(0, std::ptr::null_mut()); | ||
| if count > 0 { | ||
| let mut buf = vec![0 as libc::gid_t; count as usize]; | ||
| let res = getgroups(count, buf.as_mut_ptr()); | ||
| if res >= 0 { | ||
| groups.extend(buf.into_iter().take(res as usize)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This module introduces several unsafe libc calls (geteuid, getegid, getgroups) without a // SAFETY: justification. Per repo guidelines, please document why these calls are safe here (expected invariants, pointer validity, thread-safety) and add/point to tests where appropriate.
project/slayerfs/src/sdk_fs.rs
Outdated
| /// Returns the last status change time as seconds since UNIX epoch. | ||
| pub fn ctime(&self) -> i64 { | ||
| self.0.ctime | ||
| } | ||
|
|
||
| /// Returns the last access time as SystemTime. | ||
| pub fn accessed(&self) -> io::Result<SystemTime> { | ||
| if self.0.atime >= 0 { | ||
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.atime as u64)) | ||
| } else { | ||
| Err(io::Error::other("negative timestamp not supported")) | ||
| } | ||
| } | ||
|
|
||
| /// Returns the last modification time as SystemTime. | ||
| pub fn modified(&self) -> io::Result<SystemTime> { | ||
| if self.0.mtime >= 0 { | ||
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.mtime as u64)) | ||
| } else { | ||
| Err(io::Error::other("negative timestamp not supported")) | ||
| } | ||
| } | ||
|
|
||
| /// Returns the creation/status change time as SystemTime. | ||
| pub fn created(&self) -> io::Result<SystemTime> { | ||
| if self.0.ctime >= 0 { | ||
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.ctime as u64)) |
There was a problem hiding this comment.
Metadata::{atime,mtime,ctime} and the accessed/modified/created helpers document/compute timestamps as seconds, but MetaStore timestamps are nanoseconds (e.g., Utc::now().timestamp_nanos_opt() in meta/stores/redis_store.rs). This will produce incorrect SystemTime values. Update the docs and use Duration::from_nanos(...) (and clarify the unit consistently across the SDK).
| /// Returns the last status change time as seconds since UNIX epoch. | |
| pub fn ctime(&self) -> i64 { | |
| self.0.ctime | |
| } | |
| /// Returns the last access time as SystemTime. | |
| pub fn accessed(&self) -> io::Result<SystemTime> { | |
| if self.0.atime >= 0 { | |
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.atime as u64)) | |
| } else { | |
| Err(io::Error::other("negative timestamp not supported")) | |
| } | |
| } | |
| /// Returns the last modification time as SystemTime. | |
| pub fn modified(&self) -> io::Result<SystemTime> { | |
| if self.0.mtime >= 0 { | |
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.mtime as u64)) | |
| } else { | |
| Err(io::Error::other("negative timestamp not supported")) | |
| } | |
| } | |
| /// Returns the creation/status change time as SystemTime. | |
| pub fn created(&self) -> io::Result<SystemTime> { | |
| if self.0.ctime >= 0 { | |
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.ctime as u64)) | |
| /// Returns the last status change time as nanoseconds since UNIX epoch. | |
| pub fn ctime(&self) -> i64 { | |
| self.0.ctime | |
| } | |
| /// Returns the last access time as SystemTime, interpreting the stored value as | |
| /// nanoseconds since the UNIX epoch. | |
| pub fn accessed(&self) -> io::Result<SystemTime> { | |
| if self.0.atime >= 0 { | |
| Ok(UNIX_EPOCH + Duration::from_nanos(self.0.atime as u64)) | |
| } else { | |
| Err(io::Error::other("negative timestamp not supported")) | |
| } | |
| } | |
| /// Returns the last modification time as SystemTime, interpreting the stored value as | |
| /// nanoseconds since the UNIX epoch. | |
| pub fn modified(&self) -> io::Result<SystemTime> { | |
| if self.0.mtime >= 0 { | |
| Ok(UNIX_EPOCH + Duration::from_nanos(self.0.mtime as u64)) | |
| } else { | |
| Err(io::Error::other("negative timestamp not supported")) | |
| } | |
| } | |
| /// Returns the creation/status change time as SystemTime, interpreting the stored value as | |
| /// nanoseconds since the UNIX epoch. | |
| pub fn created(&self) -> io::Result<SystemTime> { | |
| if self.0.ctime >= 0 { | |
| Ok(UNIX_EPOCH + Duration::from_nanos(self.0.ctime as u64)) |
project/slayerfs/src/sdk_fs.rs
Outdated
| use crate::vfs::sdk::LocalClient; | ||
| use tempfile::tempdir; | ||
|
|
||
| async fn local_client() -> (tempfile::TempDir, DynClient) { | ||
| let tmp = tempdir().expect("tempdir"); | ||
| let layout = ChunkLayout::default(); | ||
| let cli = LocalClient::new_local(tmp.path(), layout) |
There was a problem hiding this comment.
SDK tests create paths under / (e.g., /a.txt, /d/e) but LocalClient::new_local uses default FileSystemConfig which enforces permissions with the current caller. This is likely to fail on CI (non-root cannot write to /). Consider constructing the client with FileSystemConfig::default().with_caller(CallerIdentity::root()) (via new_local_with_config) or disabling permission enforcement for these tests.
| use crate::vfs::sdk::LocalClient; | |
| use tempfile::tempdir; | |
| async fn local_client() -> (tempfile::TempDir, DynClient) { | |
| let tmp = tempdir().expect("tempdir"); | |
| let layout = ChunkLayout::default(); | |
| let cli = LocalClient::new_local(tmp.path(), layout) | |
| use crate::vfs::sdk::{CallerIdentity, FileSystemConfig, LocalClient}; | |
| use tempfile::tempdir; | |
| async fn local_client() -> (tempfile::TempDir, DynClient) { | |
| let tmp = tempdir().expect("tempdir"); | |
| let layout = ChunkLayout::default(); | |
| let cfg = FileSystemConfig::default().with_caller(CallerIdentity::root()); | |
| let cli = LocalClient::new_local_with_config(tmp.path(), layout, cfg) |
project/slayerfs/src/vfs/sdk.rs
Outdated
| pub async fn new(layout: ChunkLayout, store: S, meta: M) -> Result<Self, String> { | ||
| let fs = FileSystem::new(layout, store, meta).await?; |
There was a problem hiding this comment.
Client::new returns Result<_, String>, which drops structured error information (and forces callers to parse strings). Since this SDK is adding std::io-style APIs elsewhere, consider returning a structured error (io::Error/VfsError) here as well to keep error semantics consistent.
| pub async fn new(layout: ChunkLayout, store: S, meta: M) -> Result<Self, String> { | |
| let fs = FileSystem::new(layout, store, meta).await?; | |
| pub async fn new(layout: ChunkLayout, store: S, meta: M) -> Result<Self, io::Error> { | |
| let fs = FileSystem::new(layout, store, meta) | |
| .await | |
| .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?; |
| /// Slayerfs ensures `close-to-open` semantics; each `open` must see the newest file state. | ||
| #[derive(Clone)] | ||
| pub(crate) struct Inode { | ||
| pub struct Inode { |
There was a problem hiding this comment.
Why make Inode public? It is for internal use only.
| // background_fetch to pull data from object storage. | ||
| // - read_from_slice waits until the slice is Ready (or Invalid) and copies data into | ||
| // the caller buffer. Overlapping slices obey "latest slice wins" ordering. | ||
| // - Writer commit will call DataReader::invalidate(...) to mark cached slices stale. |
There was a problem hiding this comment.
This file is older than the current version. Please merge the latest changes from main.
| /// - Path-based operations (Open, Stat, Mkdir, etc.) | ||
| /// - `File` handle with lazy reader/writer | ||
| /// - Optional access logging | ||
| pub struct FileSystem<S, M> |
There was a problem hiding this comment.
Why not do this with wrapping a VFS<S, M>? Is there any obstacle?
| pub struct FileSystem<S, M> | ||
| where | ||
| S: BlockStore + Send + Sync + 'static, | ||
| M: MetaStore + 'static, |
There was a problem hiding this comment.
Consider use the MetaLayer for unified abstraction.
project/slayerfs/src/vfs/mod.rs
Outdated
| pub(crate) mod backend; | ||
| pub(crate) mod cache; | ||
| pub mod config; | ||
| pub mod cache; |
project/slayerfs/src/vfs/sdk.rs
Outdated
| impl<S: BlockStore + Send + Sync + 'static, M: MetaLayer + Send + Sync + 'static> Client<S, M> { | ||
| pub async fn new(layout: ChunkLayout, store: S, meta_layer: Arc<M>) -> Result<Self, VfsError> { | ||
| let fs = VFS::with_meta_layer(layout, store, meta_layer)?; | ||
| impl<S: BlockStore + Send + Sync + 'static, M: MetaStore + 'static> Client<S, M> { |
There was a problem hiding this comment.
Consider use MetaLayer.
project/slayerfs/src/vfs/sdk.rs
Outdated
| } | ||
| let _ = self.fs.closedir(fh); | ||
| Ok(entries) | ||
| pub async fn write_at(&self, path: &str, offset: u64, data: &[u8]) -> Result<usize, String> { |
There was a problem hiding this comment.
It is not a good idea to handle error by String.
jerry609
force-pushed
the
feat/slayerfs-stdlike-sdk-pr
branch
from
0794300 to
d205f41
Compare
|
@codex review |
|
Claude finished @genedna's task in 2m 2s —— View job Code Review Complete
OverviewThis PR introduces a significant refactoring to align SlayerFS with a std-like SDK pattern, adds a new Critical Issues1. Code Duplication in
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 155eba404f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
project/slayerfs/src/file_io/mod.rs
Outdated
| while remaining > 0 { | ||
| let chunk_index = layout.chunk_index_of(cursor); | ||
| let chunk_offset = layout.within_chunk_offset(cursor) as u32; | ||
| let avail = layout.chunk_size.saturating_sub(chunk_offset as u64); | ||
| let take = std::cmp::min(remaining, avail) as u32; |
There was a problem hiding this comment.
The new split_chunk_spans computes chunk_offset with as u32, which silently truncates when layout.chunk_size (or the intra-chunk offset) exceeds u32::MAX. In that configuration, reads/writes will target the wrong offset within the chunk, corrupting data for large chunk sizes. The previous implementation used u32::try_from to reject oversized offsets; this should either validate and error or constrain ChunkLayout so offsets always fit in u32.
Useful? React with 👍 / 👎.
project/slayerfs/src/fs/mod.rs
Outdated
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | ||
| let parent_attr = self | ||
| .meta_layer | ||
| .stat(parent_ino) | ||
| .await | ||
| .map_err(|e| meta_error_to_io(&dir, e))? | ||
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | ||
| if parent_attr.kind != FileType::Dir { | ||
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | ||
| } | ||
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | ||
|
|
||
| let parent_attr = self | ||
| .meta_layer | ||
| .stat(parent_ino) | ||
| .await | ||
| .map_err(|e| meta_error_to_io(&dir, e))? | ||
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | ||
| if parent_attr.kind != FileType::Dir { | ||
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | ||
| } | ||
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | ||
|
|
There was a problem hiding this comment.
mkdir() repeats the same stat(parent_ino) + kind check + check_access(...) sequence multiple times, which is redundant and adds extra metadata round-trips. Please keep a single parent_attr fetch and a single access check before proceeding.
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | |
| let parent_attr = self | |
| .meta_layer | |
| .stat(parent_ino) | |
| .await | |
| .map_err(|e| meta_error_to_io(&dir, e))? | |
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | |
| if parent_attr.kind != FileType::Dir { | |
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | |
| } | |
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; | |
| let parent_attr = self | |
| .meta_layer | |
| .stat(parent_ino) | |
| .await | |
| .map_err(|e| meta_error_to_io(&dir, e))? | |
| .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, dir.clone()))?; | |
| if parent_attr.kind != FileType::Dir { | |
| return Err(io::Error::new(io::ErrorKind::NotADirectory, dir)); | |
| } | |
| self.check_access(&parent_attr, AccessMask::WRITE | AccessMask::EXEC, &dir)?; |
project/slayerfs/src/fs/mod.rs
Outdated
| let fi = self.resolve(&path, true).await?; | ||
| if fi.is_dir() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "cannot open directory as file", | ||
| )); | ||
| } |
There was a problem hiding this comment.
open() returns io::ErrorKind::InvalidInput for "cannot open directory as file". For std/tokio-like semantics this should be ErrorKind::IsADirectory (and helps downstream error mapping stay consistent).
project/slayerfs/src/sdk_fs.rs
Outdated
| /// Returns the last access time as seconds since UNIX epoch. | ||
| pub fn atime(&self) -> i64 { | ||
| self.0.atime | ||
| } | ||
|
|
||
| /// Returns the last modification time as seconds since UNIX epoch. | ||
| pub fn mtime(&self) -> i64 { | ||
| self.0.mtime | ||
| } | ||
|
|
||
| /// Returns the last status change time as seconds since UNIX epoch. | ||
| pub fn ctime(&self) -> i64 { | ||
| self.0.ctime | ||
| } | ||
|
|
||
| /// Returns the last access time as SystemTime. | ||
| pub fn accessed(&self) -> io::Result<SystemTime> { | ||
| if self.0.atime >= 0 { | ||
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.atime as u64)) | ||
| } else { | ||
| Err(io::Error::other("negative timestamp not supported")) | ||
| } | ||
| } | ||
|
|
||
| /// Returns the last modification time as SystemTime. | ||
| pub fn modified(&self) -> io::Result<SystemTime> { | ||
| if self.0.mtime >= 0 { | ||
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.mtime as u64)) | ||
| } else { | ||
| Err(io::Error::other("negative timestamp not supported")) | ||
| } | ||
| } | ||
|
|
||
| /// Returns the creation/status change time as SystemTime. | ||
| pub fn created(&self) -> io::Result<SystemTime> { | ||
| if self.0.ctime >= 0 { | ||
| Ok(UNIX_EPOCH + Duration::from_secs(self.0.ctime as u64)) | ||
| } else { | ||
| Err(io::Error::other("negative timestamp not supported")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Metadata::{accessed,modified,created} (and the atime/mtime/ctime docstrings) treat FileAttr timestamps as seconds, but other parts of this PR (e.g. VFS time updates) use SystemTime::now().duration_since(UNIX_EPOCH).as_nanos() as i64, implying these fields are nanoseconds. This will produce incorrect SystemTime values for SDK consumers; please align the units (and docs) with the actual timestamp representation.
jerry609
force-pushed
the
feat/slayerfs-stdlike-sdk-pr
branch
2 times, most recently
from
fb9dc00 to
02c3a42
Compare
Signed-off-by: jerry609 <[email protected]>
jerry609
force-pushed
the
feat/slayerfs-stdlike-sdk-pr
branch
from
02c3a42 to
0accd10
Compare
Signed-off-by: jerry609 <[email protected]>
Signed-off-by: jerry609 <[email protected]>
Signed-off-by: jerry609 <[email protected]>
Head branch was pushed to by a user without write access
4 participants
Summary
What changed
readahead 行为，chunk_id_for 错误统一传播
逻辑重复
Considerations
close #348