Skip to content
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

[release/7.0] Implement proper File System detection for Apple OSes #74560

Merged
merged 4 commits into from
Aug 28, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 25, 2022

Backport of #74493 to release/7.0

/cc @adamsitnik

Customer Impact

Customers using macOS can't write to files located on NFS shares. We have fixed this bug in 6.0 for Linux (#55256), but it was not tested for maOS. It turned out that file system detection was not working properly on macOS, as data populated by fstatfs was containing an OS version specific file system id.

Example taken from StackOverflow: autofs was recognized as 25 for macOS 10.12 and as 24 for macOS 10.13.

Official Apple documentation refers to it as:

Filesystem type numbers are an old construct; most filesystems just get a number assigned based on the order in which they are registered with the system.

The fix was to rely on the string populated by the sys-call. The mapping logic already existed and was used by BSD-like systems.

Testing

An assert that verifies that File System is always recognized was added.

Risk

The risk is very low:

if (!Interop.Sys.TryGetFileSystemType(this, out Interop.Sys.UnixFileSystemTypes unixFileSystemType))
{
return false; // assume we should not acquire the lock if we don't know the File System
}

@ghost
Copy link

ghost commented Aug 25, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #74493 to release/7.0

/cc @adamsitnik

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.IO

Milestone: -

@adamsitnik adamsitnik added the os-mac-os-x macOS aka OSX label Aug 25, 2022
@adamsitnik adamsitnik added this to the 7.0.0 milestone Aug 25, 2022
@adamsitnik adamsitnik requested a review from jozkee August 25, 2022 09:23
@adamsitnik
Copy link
Member

cc @danmoseley

@adamsitnik
Copy link
Member

The build failure is #74179

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean backport, LGTM.

@carlossanlop
Copy link
Member

@danmoseley can we get an approval for this backport?

@danmoseley
Copy link
Member

danmoseley commented Aug 25, 2022

wrt risk -- on Linux, I see the codepath is unchanged.

On Apple, is it possible there are other cases where statfsArgs.f_type is correct today, but statfsArgs.f_fstypename (which we now use) is not?

If we do introducing a bug here, it looks like the only effect will be to potentially change whether we take an advisory lock or not. That's not good, but perhaps not a disaster either. would it break any functionality? (other than potentially not fixing this NFS copying)?

typically we would not take a fix at this stage that wasn't a regression, or at least serious. However, I think we would potentially service for this. So I'm open to taking it but interested in answers to qs above.

@jozkee
Copy link
Member

jozkee commented Aug 25, 2022

is it possible there are other cases where statfsArgs.f_type is correct today

f_type is unreliable on macOS, as metioned here. I think this is better than what we currently have.
But in the case where we would break something, yes, it would affect only the advisory locks, which was already broken, as reported in #72786.

@jozkee
Copy link
Member

jozkee commented Aug 25, 2022

@adamsitnik are there any tests that were disabled for macOS due to this issue and that now can be re-enabled? I couldn't find any but you probably know better.

@danmoseley
Copy link
Member

which was already broken, as reported in #72786.

Right, but if hypothetically macOS reported the correct statfsArgs.f_type but wrong statfsArgs.f_fstypename for smb, smb2, cifs (since those are the other 4 we will not lock for) we will start locking, when we did not before. And reverse for other file systems, but in that case not locking when we did, which is less significant.

                case Interop.Sys.UnixFileSystemTypes.nfs: // #44546
                case Interop.Sys.UnixFileSystemTypes.smb:
                case Interop.Sys.UnixFileSystemTypes.smb2: // #53182
                case Interop.Sys.UnixFileSystemTypes.cifs:

This is hypothetical, I'm just trying to think through all possibilities.

@adamsitnik
Copy link
Member

On Apple, is it possible there are other cases where statfsArgs.f_type is correct today, but statfsArgs.f_fstypename (which we now use) is not?

I very much doubt that. Based on the research I did it's exactly the opposite (statfsArgs.f_type is always wrong, statfsArgs.f_fstypename should be used instead).

That's not good, but perhaps not a disaster either. would it break any functionality?

In theory it's possible that so far, we were not able to recognize the file system on some apple OSes and we were not locking the files even when we should have:

if (!Interop.Sys.TryGetFileSystemType(this, out Interop.Sys.UnixFileSystemTypes unixFileSystemType))
{
return false; // assume we should not acquire the lock if we don't know the File System
}

And now we are going to start locking them, as expected. Which might fix a silent error (users using files thinking they have exclusive access, while they don't).

Even if in theory we miss some case users can still request no locking via config switch:

internal static bool DisableFileLocking { get; } = OperatingSystem.IsBrowser() // #40065: Emscripten does not support file locking
|| AppContextConfigHelper.GetBooleanConfig("System.IO.DisableFileLocking", "DOTNET_SYSTEM_IO_DISABLEFILELOCKING", defaultValue: false);

are there any tests that were disabled for macOS due to this issue and that now can be re-enabled?

there are none, as we never had any NFS tests: #54495

@danmoseley
Copy link
Member

OK, approved, as we would potentially service for this as it's a fairly basic scenario, no workaround, customer reported. Thanks for the info above.

@carlossanlop
Copy link
Member

Thanks for the approval.
Signed off.
CI failures are unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit fe99c44 into release/7.0 Aug 28, 2022
@carlossanlop carlossanlop deleted the backport/pr-74493-to-release/7.0 branch August 28, 2022 21:24
@mikkeljohnsen
Copy link

Will this be backported to 6.0 ?

@ghost ghost locked as resolved and limited conversation to collaborators Oct 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants