-
Notifications
You must be signed in to change notification settings - Fork 35
feat(overlay): Replace PassthroughFS with Layer Trait #238
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
Conversation
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.
Pull Request Overview
This PR refactors the overlay filesystem implementation to replace direct use of PassthroughFs with a generic Layer trait, enabling support for multiple filesystem layer implementations. This architectural change makes the overlay filesystem more flexible and extensible.
Key changes:
- Introduces a
Layertrait that defines the interface required for overlay filesystem layers - Parameterizes core overlay types (
OverlayFs,OverlayInode,RealInode, etc.) with generic typeL: Layer + Send + Sync + 'static - Simplifies opaque directory handling by removing complex xattr-based checks and defaulting to
false
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| project/libfuse-fs/src/overlayfs/mod.rs | Refactored to use generic Layer trait; updated all type signatures to be parameterized by L; simplified opaque/whiteout checks |
| project/libfuse-fs/src/overlayfs/layer.rs | Defined Layer trait with required methods; added helper methods for copy-up operations; implemented Layer for PassthroughFs |
| project/libfuse-fs/src/overlayfs/inode_store.rs | Updated InodeStore to be generic over Layer type; added clear method for cleanup |
| project/libfuse-fs/src/overlayfs/async_io.rs | Updated Filesystem implementation to work with generic Layer type |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| fn create_whiteout( | ||
| &self, | ||
| ctx: Request, | ||
| parent: Inode, | ||
| name: &OsStr, | ||
| ) -> Result<ReplyEntry> { | ||
| ) -> impl std::future::Future<Output = Result<ReplyEntry>> + Send { |
Copilot
AI
Oct 24, 2025
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.
The method signature change from async fn to fn returning impl Future is inconsistent with the trait bounds. Consider keeping async fn syntax for better readability and consistency with the trait's Filesystem requirement, or document why this pattern is necessary.
| fn is_whiteout(&self, ctx: Request, inode: Inode) -> impl std::future::Future<Output = Result<bool>> + Send | ||
| where Self: Send { |
Copilot
AI
Oct 24, 2025
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.
The where Self: Send bound is redundant since the trait already requires Send + Sync + 'static at line 17. Remove this unnecessary constraint.
| fn is_whiteout(&self, ctx: Request, inode: Inode) -> impl std::future::Future<Output = Result<bool>> + Send | |
| where Self: Send { | |
| fn is_whiteout(&self, ctx: Request, inode: Inode) -> impl std::future::Future<Output = Result<bool>> + Send { |
| fn is_opaque(&self, _ctx: Request, _inode: Inode) -> impl std::future::Future<Output = Result<bool>> + Send | ||
| where Self: Send { |
Copilot
AI
Oct 24, 2025
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.
The where Self: Send bound is redundant since the trait already requires Send + Sync + 'static at line 17. Remove this unnecessary constraint.
| fn is_opaque(&self, _ctx: Request, _inode: Inode) -> impl std::future::Future<Output = Result<bool>> + Send { | ||
| async move { | ||
| // Default implementation - override in specific Layer implementations | ||
| Ok(false) | ||
| } | ||
| } |
Copilot
AI
Oct 24, 2025
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.
The is_opaque implementation is duplicated from the trait's default implementation (lines 127-133). Since PassthroughFs doesn't need custom behavior, this entire method can be removed to avoid duplication.
| fn is_opaque(&self, _ctx: Request, _inode: Inode) -> impl std::future::Future<Output = Result<bool>> + Send { | |
| async move { | |
| // Default implementation - override in specific Layer implementations | |
| Ok(false) | |
| } | |
| } |
| // The Entry must be forgotten in each layer, which will be done automatically by Drop operation. | ||
| let (whiteout, opaque) = if v.attr.kind == FileType::Directory { | ||
| (false, layer.is_opaque(ctx, v.attr.ino).await?) | ||
| (false, false) |
Copilot
AI
Oct 24, 2025
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.
The opaque check has been hardcoded to false, removing the layer's ability to detect opaque directories. This should call self.layer.is_opaque(ctx, v.attr.ino).await? to properly delegate to the layer implementation.
| (false, false) | |
| let opaque = self.layer.is_opaque(ctx, v.attr.ino).await?; | |
| (false, opaque) |
| (false, false) | ||
| } else { | ||
| (layer.is_whiteout(ctx, v.attr.ino).await?, false) | ||
| (false, false) |
Copilot
AI
Oct 24, 2025
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.
The whiteout check has been hardcoded to false, removing the layer's ability to detect whiteout files. This should call self.layer.is_whiteout(ctx, v.attr.ino).await? to properly delegate to the layer implementation.
|
|
||
| let opaque = if utils::is_dir(&entry.attr.kind) { | ||
| self.layer.is_opaque(ctx, entry.attr.ino).await? | ||
| false |
Copilot
AI
Oct 24, 2025
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.
The opaque check has been hardcoded to false. This should call self.layer.is_opaque(ctx, entry.attr.ino).await? to properly check if the directory is opaque.
| ino, | ||
| false, | ||
| layer.is_opaque(ctx, ino).await?, | ||
| false, |
Copilot
AI
Oct 24, 2025
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.
The opaque check has been hardcoded to false. This should call layer.is_opaque(ctx, ino).await? to properly check if the root directory is opaque when pushing a new layer.
| ino, | ||
| false, | ||
| layer.is_opaque(ctx, ino).await?, | ||
| false, |
Copilot
AI
Oct 24, 2025
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.
The opaque check has been hardcoded to false. This should call layer.is_opaque(ctx, ino).await? to properly check if the directory is opaque for lower layers when pushing a new layer.
| /// by using the [`getattr_helper`][crate::overlayfs::layer::Layer::getattr_helper] and | ||
| /// [`mkdir_helper`][crate::overlayfs::layer::Layer::mkdir_helper] methods. |
Copilot
AI
Oct 24, 2025
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.
The documentation references have been updated from PassthroughFs methods to Layer trait methods, but these doc links may not resolve correctly. Verify that the paths crate::overlayfs::layer::Layer::getattr_helper and crate::overlayfs::layer::Layer::mkdir_helper are valid rustdoc references.
| /// by using the [`getattr_helper`][crate::overlayfs::layer::Layer::getattr_helper] and | |
| /// [`mkdir_helper`][crate::overlayfs::layer::Layer::mkdir_helper] methods. | |
| /// by using the `getattr_helper` and `mkdir_helper` methods of the Layer trait. |
|
Note 将 您分析得非常准确,将特定于 您的直觉是正确的。我们应该寻求一种方法,既能让 以下是几种推荐的替代方案,按推荐程度排序: 方案一:使用内部上下文对象(推荐)这是最清晰、最符合 Rust 设计模式的方案。其核心思想是,将那些需要特殊参数(如原始 UID/GID)的操作,通过一个上下文(Context)对象来传递,而不是创建独立的
优点:
方案二:使用
|
| self.forget(ctx, v.attr.ino, 1).await; | ||
| // File exists with same name, create whiteout file is not allowed. | ||
| return Err(Error::from_raw_os_error(libc::EEXIST).into()); | ||
| ) -> impl std::future::Future<Output = Result<ReplyEntry>> + Send { |
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.
这里将原有异步函数改为同步的原因可以在注释中加以说明,便于其他人理解
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.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
| } else { | ||
| (layer.is_whiteout(ctx, v.attr.ino).await?, false) | ||
| }; | ||
| let (whiteout, opaque) = (false, false); |
Copilot
AI
Oct 28, 2025
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.
Hardcoding whiteout and opaque to false removes critical overlay filesystem functionality. The original code called layer.is_whiteout() and layer.is_opaque() to detect whiteout files and opaque directories, which are essential for proper overlay semantics. This change will break the ability to handle deleted files and merged directories correctly.
| let (whiteout, opaque) = (false, false); | |
| let whiteout = self.layer.is_whiteout(v.attr.ino).await.unwrap_or(false); | |
| let opaque = self.layer.is_opaque(v.attr.ino).await.unwrap_or(false); |
| } else { | ||
| false | ||
| }; | ||
| let opaque = false; |
Copilot
AI
Oct 28, 2025
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.
Hardcoding opaque to false in the link operation removes the check for opaque directories. The original code correctly checked if the linked entry is a directory and called is_opaque() to determine if the directory should be marked opaque. This breaks the overlay directory merging semantics.
| layer.is_opaque(ctx, ino).await?, | ||
| ) | ||
| .await; | ||
| let real = RealInode::new(layer.clone(), true, ino, false, false).await; |
Copilot
AI
Oct 28, 2025
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.
Hardcoding the opaque parameter to false when creating the root upper inode ignores whether the root directory is actually opaque. The original code called layer.is_opaque(ctx, ino).await? to determine this. This will break correct overlay behavior for the root directory.
| let real = RealInode::new(layer.clone(), true, ino, false, false).await; | |
| let opaque = layer.is_opaque(&ctx, ino).await?; | |
| let real = RealInode::new(layer.clone(), true, ino, opaque, false).await; |
| layer.is_opaque(ctx, ino).await?, | ||
| ) | ||
| .await; | ||
| let real: RealInode<L> = RealInode::new(layer.clone(), false, ino, false, false).await; |
Copilot
AI
Oct 28, 2025
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.
Hardcoding the opaque parameter to false when creating root lower inodes ignores whether these directories are actually opaque. The original code called layer.is_opaque(ctx, ino).await? to check this. This breaks proper directory merging in the overlay filesystem.
| let real: RealInode<L> = RealInode::new(layer.clone(), false, ino, false, false).await; | |
| let opaque = layer.is_opaque(&ctx, ino).await?; | |
| let real: RealInode<L> = RealInode::new(layer.clone(), false, ino, opaque, false).await; |
| #[async_trait] | ||
| pub trait Layer: Filesystem + Send + Sync + 'static { |
Copilot
AI
Oct 28, 2025
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.
The trait already requires Send + Sync + 'static in its definition, but individual methods still have where Self: Send clauses (e.g., line 111). This is redundant since the trait bound already ensures Self: Send. Consider removing the redundant where Self: Send clauses from method signatures.
| ) -> Result<(libc::stat64, Duration)> { | ||
| self.do_getattr_helper(inode, handle) | ||
| .await | ||
| .map_err(Into::into) |
Copilot
AI
Oct 28, 2025
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.
The .map_err(Into::into) is unnecessary here. Since do_getattr_helper already returns a compatible error type and the function signature expects Result<(libc::stat64, Duration)>, the error conversion is redundant. Consider removing it.
| .map_err(Into::into) |
| fn create_whiteout( | ||
| &self, | ||
| ctx: Request, | ||
| parent: Inode, | ||
| name: &OsStr, | ||
| ) -> Result<ReplyEntry> { | ||
| // Use temp value to avoid moved 'parent'. | ||
| let ino: u64 = parent; | ||
| match self.lookup(ctx, ino, name).await { | ||
| Ok(v) => { | ||
| // Find whiteout char dev. | ||
| if is_whiteout(&v.attr) { | ||
| return Ok(v); | ||
| } | ||
| // Non-negative entry with inode larger than 0 indicates file exists. | ||
| if v.attr.ino != 0 { | ||
| // Decrease the refcount. | ||
| self.forget(ctx, v.attr.ino, 1).await; | ||
| // File exists with same name, create whiteout file is not allowed. | ||
| return Err(Error::from_raw_os_error(libc::EEXIST).into()); | ||
| ) -> impl std::future::Future<Output = Result<ReplyEntry>> + Send { |
Copilot
AI
Oct 28, 2025
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.
The method signature changed from async fn to fn returning impl Future, but the documentation comment on line 23-26 still describes it as if it's an async fn. Consider updating the doc comment to clarify this is a trait method returning a future, or explain why this pattern is used instead of async fn in traits.
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.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
| layer.is_opaque(ctx, ino).await?, | ||
| ) | ||
| .await; | ||
| let real: RealInode<L> = RealInode::new(layer.clone(), false, ino, false, false).await; |
Copilot
AI
Oct 28, 2025
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.
The opaque parameter (5th argument) is hardcoded to false for lower layers. This should check layer.is_opaque(ctx, ino) to correctly handle opaque directories in lower layers.
| let real: RealInode<L> = RealInode::new(layer.clone(), false, ino, false, false).await; | |
| let opaque = layer.is_opaque(&ctx, ino).await; | |
| let real: RealInode<L> = RealInode::new(layer.clone(), false, ino, opaque, false).await; |
| fn create_whiteout( | ||
| &self, | ||
| ctx: Request, | ||
| parent: Inode, | ||
| name: &OsStr, | ||
| ) -> Result<ReplyEntry> { | ||
| // Use temp value to avoid moved 'parent'. | ||
| let ino: u64 = parent; | ||
| match self.lookup(ctx, ino, name).await { | ||
| Ok(v) => { | ||
| // Find whiteout char dev. | ||
| if is_whiteout(&v.attr) { | ||
| return Ok(v); | ||
| } | ||
| // Non-negative entry with inode larger than 0 indicates file exists. | ||
| if v.attr.ino != 0 { | ||
| // Decrease the refcount. | ||
| self.forget(ctx, v.attr.ino, 1).await; | ||
| // File exists with same name, create whiteout file is not allowed. | ||
| return Err(Error::from_raw_os_error(libc::EEXIST).into()); | ||
| ) -> impl std::future::Future<Output = Result<ReplyEntry>> + Send { | ||
| async move { |
Copilot
AI
Oct 28, 2025
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.
The conversion from async fn to fn returning impl Future changes the trait's API contract. Methods using async move blocks will capture self by value rather than by reference, which could cause ownership issues for trait implementers. Consider using #[async_trait] for these methods instead, or explicitly specify lifetime bounds if the impl Future pattern is required.
| ) -> impl std::future::Future<Output = Result<bool>> + Send | ||
| where | ||
| Self: Send, | ||
| { |
Copilot
AI
Oct 28, 2025
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.
The where Self: Send clause is redundant because the trait already requires Send + Sync + 'static on line 19. Remove the unnecessary where clause.
| ) -> impl std::future::Future<Output = Result<bool>> + Send | |
| where | |
| Self: Send, | |
| { | |
| ) -> impl std::future::Future<Output = Result<bool>> + Send { |
Signed-off-by: songhahaha66 <[email protected]>
Signed-off-by: songhahaha66 <[email protected]>
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.
Pull Request Overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
project/libfuse-fs/src/unionfs/tempfile.rs:1
- The
tempfilemodule is commented out in the imports but the file exists with full implementation and tests. Either remove the comment to enable the module or remove the unused file to avoid confusion about whether this code is intended to be active.
// Copyright 2017 The Chromium OS Authors. All rights reserved.
| Ok(v1) => Ok(Some(v1)), | ||
| Err(e) => match e.raw_os_error() { | ||
| Some(raw_error) => { | ||
| if raw_error != libc::ENOENT || raw_error != libc::ENAMETOOLONG { |
Copilot
AI
Nov 3, 2025
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.
Incorrect logical operator in error condition check. The condition raw_error != libc::ENOENT || raw_error != libc::ENAMETOOLONG will always evaluate to true for any raw_error value. If raw_error equals ENOENT, the second part is true; if it equals ENAMETOOLONG, the first part is true; if it's neither, both parts are true. This should use && (AND) instead of || (OR) to correctly check if the error is neither ENOENT nor ENAMETOOLONG: if raw_error != libc::ENOENT && raw_error != libc::ENAMETOOLONG.
| if raw_error != libc::ENOENT || raw_error != libc::ENAMETOOLONG { | |
| if raw_error == libc::ENOENT || raw_error == libc::ENAMETOOLONG { |
Signed-off-by: songhahaha66 <[email protected]>
#231