-
Notifications
You must be signed in to change notification settings - Fork 5.3k
FileSystemWatcher.Linux: use a single inotify instance and refactor watch tracking. #117148
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
base: main
Are you sure you want to change the base?
Conversation
|
@dotnet/area-system-io @stephentoub ptal. |
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
…-Windows configurations.
… or we won't update _wdToWatch.
adamsitnik
left a comment
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.
@tmds big thanks for your contribution! For now I've reviewed 25% of this PR (I need to wrap my head around all the locks and it's going to take me a while), PTAL at my comments.
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
|
For the locking, this overview may be helpful:
To prevent deadlocks, the locks (as needed) are taken in this order: watchersLock, addLock, lock on Watcher, lock on Watch. |
|
@adamsitnik this is a challenge to review so feel free to ask any questions you have while looking at the code. I'm also going to make some time this week to look at the PR with my reviewer's hat on. |
|
@adamsitnik @jozkee can we target .NET 11 preview 1 for this PR? |
|
@jeffhandley @adamsitnik @jozkee it would be nice if we can work towards getting this merged. |
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 Linux implementation of FileSystemWatcher to use a single shared inotify instance across all FileSystemWatcher objects, addressing the low per-user limit of 128 inotify instances on Linux. The refactoring introduces a new architecture with improved watch management and event processing.
Key changes:
- Introduces a shared inotify instance pattern to reduce contention with other applications
- Implements a dedicated thread for reading inotify events with ThreadPool-based event handler invocation to prevent blocking
- Adds new internal classes (INotify, Watcher, WatchedDirectory, Watch) to track watch state and manage the relationship between paths and watch descriptors
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| System/IO/FileSystemWatcher.cs | Extracts CreateBufferOverflowException method for reuse in Linux implementation |
| System/IO/FileSystemWatcher.Linux.cs | Major refactoring: implements shared inotify instance, new watch tracking structures, improved event processing with Channel-based queuing |
| System.IO.FileSystem.Watcher.csproj | Adds required dependencies for System.Collections.Concurrent and System.Threading.Channels on non-Windows platforms |
| Interop.INotify.cs | Adds IN_MASK_ADD flag support needed for sharing inotify instances across multiple watchers |
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
adamsitnik
left a comment
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.
I've reviewed 30-40% of the code. Since I am not very familiar with this code and I know that historically we have disabled plenty of FSW tests (they were flaky) my next step will be to implement stress tests (and when the implementation passes them review the rest).
Big thanks for your contribution @tmds !
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…y and RemoveUnusedINotifyWatches.
adamsitnik
left a comment
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.
@tmds Could you please run these stress tests with just .NET 10 and with your new implementation and report back the results?
For the latter you should be able to do that with corerun $pathToCoreRun FswStressTests.dll
|
@adamsitnik, thanks for looking at this! I'll run the stress tests on Monday and report how it went. |
|
I built vmr's 10.0 branch with this implementation and ran the stress tests: To verify I used this implementation for the test run, I checked the |
Excellent! Our plan is that @jozkee is going to review the PR and I am going to merge it in the 2nd week of January. |
|
hi @jozkee, I look forward to get you review on this so we can get this merged. |
jozkee
left a comment
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.
I'm halfway through.
(By default) Linux allows 128 inotify instances per user. By sharing the inotify instance between the FileSystemWatchers we reduce contention with other applications.
Fixes #62869.