-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Trim UnixFileSystemTypes to the values .NET runtime cares about. #122964
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
src/libraries/Common/src/Interop/Unix/System.Native/Interop.UnixFileSystemTypes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
Outdated
Show resolved
Hide resolved
| default: | ||
| return true; // in all other situations it should be OK | ||
| } | ||
| return Interop.Sys.FileSystemSupportsLocking(this); |
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.
The behavior here is changing. Previously, if we didn't recognize the type as one of the values in UnixFileSystemTypes then no locking was performed. Now, we'll do locking unless the type is nfs/smb/cifs.
I assume we're ok to do this as return true also happens in earlier checks.
This does mean that we rely on the string names in PAL to be correct for identifying nfs/smb/cifs.
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.
This does mean that we rely on the string names in PAL to be correct for identifying nfs/smb/cifs.
Isn't it the same as main branch? f_type on linux (comparison with magic integers) and f_fstypename / f_basetype on other Unix (string comparison with strcmp)
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.
Suppose we have the names wrong.
On the main branch, MapFileSystemNameToEnum returns zero, then TryGetFileSystemType return false and then CanLockTheFile will return false.
With the new implementation, FileSystemNameSupportsLocking returns 1 and then FileSystemSupportsLocking will return true.
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.
The comment above says "// LOCK_SH is not OK when writing to NFS, CIFS or SMB". Why is it not OK? What's the failure more?
I am trying to better understand the downside of enabling locking by default for all file systems except the few we explicitly check for.
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.
Locking is enabled by default to match Windows behavior.
(For example: you get an exception when the SDK tries to write the same file simultaneously.)
The code is filtering for a combination that doesn't work well.
The combination seems to be: LOCK_SH + FileAccess.Write + nfs/smb/cifs.
And the referenced issues mention silently discarded writes and throwing on dispose (#44546, #53182).
am11
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.
Looks great. Thanks! :)
|
Thanks for the suggestions/review, @am11! |
@am11 @stephentoub @jkotas @adamsitnik ptal.