Skip to content

Layered file system#27

Merged
jaybosamiya-ms merged 16 commits intomainfrom
jayb/push-pnsltkkypous
Mar 7, 2025
Merged

Layered file system#27
jaybosamiya-ms merged 16 commits intomainfrom
jayb/push-pnsltkkypous

Conversation

@jaybosamiya-ms
Copy link
Copy Markdown
Member

@jaybosamiya-ms jaybosamiya-ms commented Mar 6, 2025

This PR implements a layered file system, roughly layered::FileSystem<Upper, Lower> (closes #5).

image

Essentially, a layered filesystem itself doesn't carry or store any of the files, but delegates to each of the the layers. Specifically, this implementation will look for and work with files in the upper layer, unless they don't exist, in which case the lower layer is looked at.

The current design of layering treats the lower layer as read-only and performs copy-on-write semantics for moving to upper layer.

The biggest complexity of this implementation comes from needing to support the deletion of files from the lower layer (without actually changing the lower layer), as well as supporting aliasing (same file opened twice) under the presence of writing. There are tests added for ensuring that the behavior of these are as expected.

Currently, the implementation's largest downside is in its handling of directories and permissions, each of which would be improved by having support for stat in the core FileSystem trait, but I decided that should be a separate PR and not be part of this one. There are relevant TODOs in the code for this. We also would need to handle correct positioning of the "migrated" CoW files, but that would require having support for lseek in the core FileSystem trait; similarly, future update.

@jaybosamiya-ms jaybosamiya-ms requested a review from Copilot March 6, 2025 22:57
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request implements a layered file system that delegates file operations to an upper or lower layer. It introduces tests for layered behavior (e.g. copy‐on‐write and file deletion), adds an auxiliary method to compute path ancestors, and adjusts helper routines to support these changes.

Reviewed Changes

File Description
litebox/src/fs/tests.rs Added tests for layered file system behavior including read, write, and deletion scenarios.
litebox/src/path.rs Added an increasing_ancestors method for computing path components.
litebox/src/fs/shared.rs Modified remove() to return the removed descriptor and added an iter_mut() method.
litebox/src/fs/mod.rs Registered the new layered module and updated doc comments accordingly.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

litebox/src/fs/shared.rs:37

  • Changing the return type of remove() from () to Descriptor might break existing callers. Ensure that this change is intentional and update related documentation where necessary.
pub(crate) fn remove(&mut self, mut fd: FileFd) -> Descriptor {

litebox/src/path.rs:119

  • [nitpick] The logic for appending "/" when the last ancestor has a length > 1 is not immediately clear. Consider adding a clarifying comment to explain its purpose.
if res.last().unwrap().len() > 1 {

@jaybosamiya-ms jaybosamiya-ms requested a review from wdcui March 6, 2025 22:58
Comment thread litebox/src/path.rs
Comment thread litebox/src/fs/layered.rs Outdated
Comment thread litebox/src/fs/layered.rs
Comment thread litebox/src/fs/layered.rs
Comment thread litebox/src/fs/layered.rs Outdated
Comment thread litebox/src/fs/layered.rs Outdated
Comment thread litebox/src/fs/layered.rs
Comment thread litebox/src/fs/layered.rs
Comment thread litebox/src/fs/layered.rs Outdated
Comment thread litebox/src/fs/layered.rs
Comment thread litebox/src/fs/layered.rs Outdated
@jaybosamiya-ms
Copy link
Copy Markdown
Member Author

jaybosamiya-ms commented Mar 7, 2025

Note: CI failures are due to #29 being merged into main which changed the interfaces. For now, the migration from lower to upper does not account for offsets, which will be fixed up in a future PR. I'm adding a TODO item for this as a comment.

PS: reads/writes on either lower or upper layer would handle offsets correctly in this implementation: the issue only shows up for migrated files, which is a rarer scenario and requires lseek support, thus I am postponing to a separate PR.

Will merge once CI goes green again.

@jaybosamiya-ms jaybosamiya-ms merged commit 9a27131 into main Mar 7, 2025
@jaybosamiya-ms jaybosamiya-ms deleted the jayb/push-pnsltkkypous branch March 7, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support a parametric layered FileSystem

4 participants