-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Hard links Support Phase 1: Gofer Client dentry/inode refactor #11734
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 implements Phase 1 of hard links support by refactoring the Gofer client’s dentry/inode code. Key changes include decoupling dentry fields from the file metadata by embedding an inode pointer into the dentry structure, updating various filesystem operations to use the new inode pointer, and modifying tests and build files accordingly.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
test/util/test_util.cc | Renamed and reorganized functions to separate permissions from link count retrieval. |
pkg/sentry/fsimpl/gofer/time.go | Updated time-related operations to use the new d.inode.fs references. |
pkg/sentry/fsimpl/gofer/lisafs_dentry.go | Updated inode creation and error handling for Lisafs dentries. |
pkg/sentry/fsimpl/gofer/filesystem.go | Refactored file operations to use d.inode references for notifications and locking. |
pkg/sentry/fsimpl/gofer/directfs_dentry.go | Adjusted direct filesystem dentry operations to work with the embedded inode. |
Comments suppressed due to low confidence (1)
test/util/test_util.cc:186
- The new function 'Permissions' returns the file's mode while 'Links' returns the link count. Consider adding inline documentation or renaming one of the functions to clearly differentiate their purposes.
PosixErrorOr<uint64_t> Permissions(const std::string& path) {
d.atime.Store(now) | ||
d.atimeDirty.Store(1) | ||
d.metadataMu.Unlock() | ||
now := d.inode.fs.clock.Now().Nanoseconds() |
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.
Ensure that the shift from using 'd.fs' to 'd.inode.fs' in time functions is clearly documented to help future maintainers understand the new data flow.
Copilot uses AI. Check for mistakes.
@@ -111,59 +110,55 @@ func (fs *filesystem) newLisafsDentry(ctx context.Context, ino *lisafs.Inode) (* | |||
} | |||
|
|||
inoKey := inoKeyFromStatx(&ino.Stat) | |||
inode, err := fs.createOrFindInodeStatx(inoKey, &ino.Stat) |
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.
Add inline comments or enhance error handling to explain the scenario when createOrFindInodeStatx returns nil, ensuring that failure cases are clearly understood.
Copilot uses AI. Check for mistakes.
@@ -502,7 +502,7 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir | |||
if dir { | |||
ev |= linux.IN_ISDIR | |||
} | |||
parent.watches.Notify(ctx, name, uint32(ev), 0, vfs.InodeEvent, false /* unlinked */) | |||
parent.inode.watches.Notify(ctx, name, uint32(ev), 0, vfs.InodeEvent, false /* unlinked */) |
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.
Review all notification calls to confirm they consistently use 'd.inode.watches' throughout file operations for clarity and maintainability.
parent.inode.watches.Notify(ctx, name, uint32(ev), 0, vfs.InodeEvent, false /* unlinked */) | |
d.inode.watches.Notify(ctx, name, uint32(ev), 0, vfs.InodeEvent, false /* unlinked */) |
Copilot uses AI. Check for mistakes.
@@ -120,23 +119,14 @@ func (fs *filesystem) newDirectfsDentry(controlFD int) (*dentry, error) { | |||
return nil, err | |||
} | |||
inoKey := inoKeyFromStat(&stat) | |||
inode, err := fs.createOrFindInodeStat(inoKey, &stat) | |||
if inode == nil { | |||
log.Warningf("could not create or find inode") |
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.
When createOrFindInodeStat returns nil, ensure that proper error handling is in place to prevent potential nil pointer dereferences.
log.Warningf("could not create or find inode") | |
if err == nil { | |
err = fmt.Errorf("createOrFindInodeStat returned nil inode without error for inoKey: %v", inoKey) | |
} | |
log.Warningf("could not create or find inode: %v", err) |
Copilot uses AI. Check for mistakes.
@tranji-cloud you can ignore all copilot comments. I added it just to check how useful it is. |
d4e79f5
to
83c538e
Compare
b82f4b6
to
5b4252b
Compare
2131c09
to
720bacc
Compare
6e6bb7c
to
6bd0880
Compare
- **Decoupled dentry and inode**: Cleanly separated dentry fields from the file's actual metadata (permissions, size, etc.), now residing in the inode. - **Dentry now holds inode pointer**: The dentry struct embeds a pointer to the shared inode, reflecting the many-to-one relationship of hard links. - **impl logic moved to inode**: The backend-specific logic (LISA FS vs. Direct FS) is no longer part of the dentry struct. Instead, the inode struct is extended to handle these implementations. - **Refactored Gofer client fs**: Updated the Gofer client filesystem logic to work with the new dentry -> inode pointer structure. - **Renamed files**: dentry_impl.go becomes inode_impl.go, and {directfs/lisafs}_dentry.go are now {directfs/lisafs}_inode.go to match the new design. - **Disabled Save/Restore in link::IsSameFile**: Disabled save/restore in this specific check due to complexities in maintaining inode consistency across save/restore cycles. See comment in link.cc:45 for details. - **New Link Tests**: - HardLinkChangeMode: Verifies hard link behavior, including mode changes across save/restore. PiperOrigin-RevId: 793741989
6bd0880
to
1f31ba1
Compare
Hard links Support Phase 1: Gofer Client dentry/inode refactor
file's actual metadata (permissions, size, etc.), now residing in the inode.
the shared inode, reflecting the many-to-one relationship of hard links.
vs. Direct FS) is no longer part of the dentry struct. Instead, the
inode struct
is extended to handle these implementations.
to work with the new dentry -> inode pointer structure.
{directfs/lisafs}_dentry.go are now {directfs/lisafs}_inode.go to match the
new design.
this specific check due to complexities in maintaining inode consistency
across save/restore cycles. See comment in link.cc:45 for details.
across save/restore.