Hard links Support Phase 1: Gofer Client dentry/inode refactor #11734
Conversation
![copybara-service[bot]](https://avatars.githubusercontent.com/in/44061?s=60&v=4){kind=small}
This commit does the following:
- Decouples dentry fields and the file's metadata (inode fields).
- Embeds the inode pointer into dentry struct {}
- Created createOrFindInode() stub.
- Refactor Gofer client fs to access the newly created inode pointer
- Move dentry.impl to inode.impl
- Test hard link support on Gofer/directFS
PiperOrigin-RevId: 750748327
There was a problem hiding this comment.
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.
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.
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.
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.
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. |
Hard links Support Phase 1: Gofer Client dentry/inode refactor
This commit does the following: