feat(fs): implement inotify filesystem event notification - #2164
Conversation
|
needs to rebase |
8fad46f to
05ce40d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05ce40d4de
ℹ️ 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".
…5 P1 + 4 P2) P1 fixes: - Composite mark index key (inode_id, dev_id): prevents FUSE cross-mount event leakage when multiple mounts reuse same inode number. - Remove has_any_watch() gate on File::inotify_parent resolution: watches added after open now correctly receive content events. - Move inode-death (DELETE_SELF/UNMOUNT) destroy before mask subscription filter: prevents watch leak when watch doesn't subscribe to the event. - Cache nlinks before unlink/rename-over: avoids FUSE GETATTR failure after namespace change; uses pre-operation link count for DELETE_SELF. - Rename-over checks displaced nlinks: multiple hardlinks survive. P2 fixes: - Sync IN_ONESHOT field on IN_MASK_ADD (OR semantics). - Dynamic O_NONBLOCK: fcntl(F_SETFL) now works on inotify fds. - EOF reads (len==0) no longer deliver IN_ACCESS. - Reject mask==0 and IN_MASK_ADD|IN_MASK_CREATE as EINVAL. - Skip MOVED events on no-op rename (same dir + same name). Verified: make kernel clean, inotify_dir_watch 2/2 PASS, fuse_core 6/6 PASS. Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
Implements inotify (issue DragonOS-Community#2151): the fsnotify core notification layer, the inotify pseudo-device, 4 syscalls (init/init1/add_watch/rm_watch), and VFS write-path hooks for all standard events (create/delete/move/ modify/access/close/attrib/self events). Architecture: - fsnotify/ unified dispatch layer: global (inode_id, dev_id) index, TOTAL_WATCHES atomic fast-path (zero cost when no watches), lock-family separation (global index lock / events lock / wd lock never nested). - inotify.rs device: InotifyInode implements IndexNode + PollableInode, epoll-integrated via LockedEPItemLinkedList, exact inotify_event layout (name field aligned to sizeof(inotify_event)=16, matching Linux ABI). - VFS hooks placed in syscall-core layer (vcore/open/rename_utils/...), NOT per-filesystem: single anchor covers ext4/tmpfs/overlayfs/fuse. Hooks fire only after success and never alter syscall return values. Review fixes incorporated: - Directory watches receive child content events (issue B): IN_MODIFY/ ACCESS/OPEN/CLOSE delivered to parent dir watch with child name. MountFSInode::as_any_ref() returns the inner inode's Any, so use downcast_arc instead of downcast_ref for parent resolution. - Guard DELETE_SELF on hardlink unlink/rename-over: only emit when i_nlink reaches 0, matching Linux fsnotify_link_count() semantics. - Composite mark index key (inode_id, dev_id): prevents FUSE cross-mount event leakage when multiple mounts reuse same inode number. - Remove has_any_watch() gate on File::inotify_parent resolution so watches added after open receive content events. - Tolerate metadata failure after unlink/rename-over (FUSE GETATTR can return ENOENT); cache nlinks before the namespace operation. - SYS_INOTIFY_INIT only registered on x86_64 (generic syscall ABI uses inotify_init1); riscv64/loongarch64 lack the legacy init syscall. - Skip MOVED events on no-op rename; EOF reads no longer deliver IN_ACCESS; reject mask==0 and IN_MASK_ADD|IN_MASK_CREATE. Test: dunitest inotify_dir_watch (2 tests) + inotify_events (6 tests) covering content/namespace/self events, multi-instance, and poll. Design doc: docs/kernel/filesystem/inotify.md Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
fslongjin
left a comment
There was a problem hiding this comment.
Thank you for implementing inotify. This feature is important for skillfs, agent-memory, and configuration hot reload. The overall layering—VFS operations produce events, fsnotify dispatches them, and the inotify backend queues and exposes them—is a good direction and avoids duplicating hooks in every filesystem.
However, this review found several issues that can directly cause missing events, incorrect events, leaked watches, or significant system-wide overhead. I recommend addressing these before merging.
Required changes
1. Normal rename does not produce move events
Location: kernel/src/filesystem/vfs/syscall/rename_utils.rs:149
The event code for a normal rename(a, b) is inside the RENAME_EXCHANGE branch. As a result, normal rename does not produce:
IN_MOVED_FROMIN_MOVED_TOIN_MOVE_SELF
Conversely, RENAME_EXCHANGE sends the two expected groups and then sends an extra duplicate group for the old inode with a different cookie.
Please split normal rename and exchange rename into separate branches. A normal rename should emit one paired set of events. An exchange should emit one set for each of the two exchanged files.
2. A removed watch can still receive later events
Location: kernel/src/filesystem/fsnotify/mod.rs:282
Dispatch first takes a snapshot of the watch list and then queues events after releasing the global lock. Another thread can call inotify_rm_watch(), queue IN_IGNORED, and remove the watch while the first thread still holds a reference to it. The first thread can then queue another normal event for the removed wd.
Users can therefore observe:
IN_IGNORED;- then another
IN_MODIFYor similar event for the same wd.
Two concurrent events can also both pass the IN_ONESHOT check and be queued.
Please add an explicit active/removed state to each watch. Removing a watch must mark it removed before cleanup. Event delivery must check and claim that state before enqueueing. For IN_ONESHOT, only one concurrent event may succeed, and IN_IGNORED must be the final event for the wd.
3. A watch may not be found or released after the file is deleted
Locations:
kernel/src/filesystem/fsnotify/mod.rs:185kernel/src/filesystem/fsnotify/mark.rs:36
After unlink, rmdir, or an overwriting rename succeeds, fsnotify calls metadata() again to obtain the inode identity. A deleted FUSE file can return ENOENT here. Other code in this PR already recognizes that post-delete FUSE metadata may fail, but dispatch and watch removal still depend on it.
This can cause:
- missing
IN_DELETE_SELFandIN_IGNORED; - failure to remove the watch from the global index;
- the watch retaining the inode and its quota until the inotify fd is closed.
Please save a stable file identity when the watch is created. Destructive operations should also capture that identity and file type before changing the namespace. Notification and cleanup must not depend on querying metadata after the file has already been deleted.
4. Events use the old directory and old name after rename
Locations: kernel/src/filesystem/vfs/file.rs:688, :1282
File stores the parent directory and file name at open time. If A/old is opened and later renamed to B/new, reads, writes, and close through the original fd still:
- notify the watch on A;
- report the name
old; - fail to notify the watch on B.
The code comments already acknowledge this limitation, but it makes directory-watch results incorrect. When a file moves between directories, the old directory's watcher may also continue seeing activity for a file that is no longer there.
Please do not store a fixed copy of the open-time parent and name. File should retain path information that follows rename, and event delivery should obtain the current parent and name when the event occurs. VFS path and rename handling should maintain this consistently.
5. A directory watch misses child attribute changes
Locations: kernel/src/filesystem/vfs/open.rs:139, :250, :700
chmod, chown, and utimensat send IN_ATTRIB without a parent directory. Only a watch placed directly on the file receives the event; a watch on the parent directory does not.
Linux notifies both the file watch and the parent directory's child watch. These operations should provide the current parent and child name. Other attribute-changing paths, including xattr operations, should be covered consistently.
6. IN_EXCL_UNLINK is not updated on an existing watch
Location: kernel/src/filesystem/inotify.rs:447
When a watch already exists, the code updates only the event mask and IN_ONESHOT. IN_EXCL_UNLINK remains fixed at the value used when the watch was first created:
- adding
IN_EXCL_UNLINKwithIN_MASK_ADDhas no effect; - replacing the mask cannot clear an old
IN_EXCL_UNLINK.
Please update the event mask, IN_ONESHOT, and IN_EXCL_UNLINK together. Add mode should merge them; replace mode should replace them.
7. O_PATH incorrectly produces open and close events
Locations:
kernel/src/filesystem/vfs/open.rs:494kernel/src/filesystem/vfs/file.rs:2508
O_PATH creates a path handle; it does not open the file for normal I/O. Linux does not generate IN_OPEN or IN_CLOSE_NOWRITE for it, but the current implementation generates both.
Please exclude FMODE_PATH in both the open and close notification paths.
8. Hard-link count changes are missing IN_ATTRIB
Locations:
kernel/src/filesystem/vfs/syscall/link_utils.rs:137kernel/src/filesystem/vfs/vcore.rs:726
Creating a hard link currently sends only IN_CREATE to the new parent directory. Removing one name while other hard links remain sends only IN_DELETE to the parent. The inode link count has changed, but a watch on the file does not receive IN_ATTRIB.
Please send IN_ATTRIB to the target inode after successful link and unlink operations. Whether to send IN_DELETE_SELF should remain a separate decision based on whether this was the final link.
9. IN_UNMOUNT is declared as supported but is not implemented
Locations:
kernel/src/filesystem/fsnotify/mod.rs:257kernel/src/filesystem/vfs/mount/mod.rs:3541
The PR description and design document claim the full standard event set, but there is no path that produces IN_UNMOUNT. On unmount:
- watches do not receive
IN_UNMOUNT; - they do not receive the following
IN_IGNORED; - references held by the watches are not released immediately.
The design document postpones this because the current ANOLISA use case does not trigger it. That does not match the claim of complete support. Please notify and remove affected watches during unmount. If this PR intentionally does not implement it, narrow the feature claim and track the missing behavior separately.
10. One watch adds substantial overhead to unrelated file operations
Locations:
kernel/src/filesystem/fsnotify/mod.rs:175kernel/src/filesystem/vfs/file.rs:1290
The implementation has only a global watch-count shortcut. Once any watch exists anywhere in the system, every unrelated read, write, open, and close performs metadata queries, takes the same global interrupt-disabling lock, and searches the global map. FUSE metadata queries may be particularly expensive.
Even when no watch exists, every normal open still resolves the parent and allocates a saved file-name copy.
Please maintain a small per-file/per-directory indication of whether the file itself or its parent has relevant watches. Enter the global lookup only when an event can actually match. Also avoid parent/name allocation when no watch exists.
Additional compatibility and performance issues
IN_DELETE_SELFandIN_MOVE_SELFfor a directory should not includeIN_ISDIR. Linux explicitly removes that bit for compatibility. Location:kernel/src/filesystem/fsnotify/mod.rs:277.- Reading a large event queue keeps an interrupt-disabling lock while serializing and zero-padding the entire result. Pop one complete event under the lock and copy it to the read buffer after releasing the lock. Location:
kernel/src/filesystem/inotify.rs:607. - When the queue is already full and an overflow event is already present, every dropped event still wakes all waiters and walks the epoll list. Do not wake when the queue state did not change. Location:
kernel/src/filesystem/inotify.rs:240. - Instance and watch limits are system-wide. One unprivileged user can exhaust them and prevent other users from using inotify. Please account these limits per user. A separate global count may remain as a fast “no watches anywhere” check.
Suggested test coverage
Please add dunitest coverage for at least:
- normal rename, cross-directory rename, overwrite, two hard links to the same inode, and
RENAME_EXCHANGE; - open a file, rename it, then read/write/close through the original fd;
- concurrent event delivery with
rm_watch; - concurrent event delivery with
IN_ONESHOT; - changing
IN_EXCL_UNLINKon an existing watch; - a directory watch while chmod, chown, utimensat, and xattr are applied to a child;
- no open/close events for
O_PATH; IN_ATTRIBwhen the hard-link count changes;IN_UNMOUNTfollowed byIN_IGNORED;- queue overflow, a too-small read buffer, and changing
O_NONBLOCKat runtime.
Overall recommendation
The layering can be kept, but I do not recommend adding more special cases on top of the current approach of saving the open-time parent/name, querying metadata after deletion, and using one global lookup lock.
Please first settle these three foundations:
- a stable file identity that remains usable after deletion;
- path information for an open file that follows rename;
- correct ordering between watch removal, one-shot watches, and event enqueueing.
Once those are in place, completing the event set and optimizing dispatch should be much safer and closer to Linux behavior.
Local make kernel passes. At the time of review, the GitHub Dunitest, Integration Test, x86_64 Build, and build-current-version checks are failing. After the changes above, please run the expanded tests inside the DragonOS guest before requesting another review.
Summary
Implements inotify (issue #2151): filesystem event notification with
inotify_init1/inotify_add_watch/inotify_rm_watch/read, the full standard event set, epoll integration, and exactinotify_eventABI.Design doc:
docs/kernel/filesystem/inotify.md. Behavior targets Linux 6.6.Architecture (3 layers)
fs/notify/-style): globalinode_id -> Weak<mark>index,TOTAL_WATCHESatomic fast-path (zero cost when no watches), lock-family separation (global index lock / events lock / wd lock never nested). Hooks fire only after success and never alter syscall return values.inotify.rs):InotifyInodeimplementsIndexNode + PollableInode, epoll-integrated viaLockedEPItemLinkedList, exactinotify_eventlayout (name field aligned tosizeof(inotify_event)=16, matching the Linux 6.6 ABI).vcore/open/rename_utils/...), not per-filesystem: a single anchor covers ext4/tmpfs/overlayfs/fuse.What this PR adds beyond the initial implementation
The initial implementation was put through a 3-way adversarial review (independent reviewers) + independent verification. This PR includes both the feature and the resulting fixes:
Directory watches now receive child content events (the primary inotify use case, e.g.
inotifywait -m /dir). Previously only namespace events (create/delete/move) reached directory watches;IN_MODIFY/ACCESS/OPEN/CLOSEon children were silently dropped. Resolved by snapshotting the parent dir + child name once atFileconstruction (gated byhas_any_watchfor zero cost when unused) and routing content events to both the parent (with name) and self watch. This also makesIN_EXCL_UNLINKeffective.Other review fixes (all verified):
TOTAL_WATCHESdouble-count onadd_watch(broke the no-watch fast path).add_watchTOCTOU: concurrent same-inode adds created duplicate marks.IN_EXCL_UNLINKpolarity was inverted.fallocate: restored theoffset+lenoverflow guard removed during theIN_MODIFYrefactor.renameoverwriting an existing target now emitsIN_DELETE/IN_DELETE_SELF(was a ghost-watch leak).RENAME_EXCHANGEnow emits the full 4 namespace events + 2MOVE_SELFwith two cookies.Testing
normal/inotify_dir_watch(directory-watch child content events + self-watch sanity), added to the whitelist so CI runs it.make kernel).docs/impl-notes/): rename-after-open yields a stale parent snapshot (EXCL_UNLINK covers unlink);IN_ATTRIB-to-parent via fd-based setattr is not yet wired; unmount does not emitIN_UNMOUNT(watch reclaims on fd close).Checklist
make kernelcompiles