Fix wrong drive format on Unix #107365

MojtabaTajik · 2024-09-04T17:37:54Z

Add reading filesystem type from /proc/self/mountinfo on Linux and use sys call as the fallback approach.

Prioritize reading the filesystem type from /proc/self/mountinfo on Linux.
Retain the existing P/Invoke fallback for non-Linux platforms and as a fallback on Linux.
Added try/catch block to handle potential but unlikely access issues or unrecognized data.
Although /proc/self/mountinfo is process-specific and should generally be accessible,
this ensures robustness in case of unexpected errors (e.g., temporary unavailability, corrupted data).
Updated documentation to reflect the reasoning and kernel behaviour regarding procfs on different platforms.

No breaking changes are expected. The refactor should be functionally equivalent but more reliable and efficient.

Change GetFormatInfoForMountPoint method to "use /proc/self/mountinfo" file - Replaced system calls (`statfs`/`statvfs`) with a simplified parsing of `/proc/self/mountinfo`. - Removed native SystemNative_GetFormatInfoForMountPoint as we don't need anymore - Parsing the mount info file allows for efficient retrieval of filesystem format and type. - No functional changes; the method continues to return the correct filesystem format and drive type for the given mount point. FIX dotnet#95099

am11 · 2024-09-05T05:50:11Z

with a more simplified approach that parses /proc/self/mountinfo to retrieve filesystem information

procfs can be unavailable on linux. It can also be misbehaving and we have made runtime implementation resilient for such cases in number of places using fallbacks.
procfs is also optional in FreeBSD. It is not enabled by default.
On SunOS et al. procfs is different; provides binary interface instead of text-based one.
there is no procfs on macOS.

Given all that, we should not remove this common P/Invoke call, but keep it as primary source for non-linux and as a fallback on linux.

MojtabaTajik · 2024-09-05T05:55:39Z

@am11 Should we go by the "mountinfo" first and go for that statfs syscall if that failed?
In that case, I think checking against "/proc/self/mountinfo" file exists is enough for a fallback mechanism, yes?

In the case of something like SunOS, where the symlink is represented as a binary format, how should we handle it? We may need to check the operating system or implement a mechanism for the parser method to fail gracefully, triggering a fallback syscall as a recovery option.

am11 · 2024-09-05T07:12:05Z

@am11 Should we go by the "mountinfo" first and go for that statfs syscall if that failed? In that case, I think checking against "/proc/self/mountinfo" file exists is enough for a fallback mechanism, yes?

Problem with File.Exists is that we still may not be able to read the file due to access issues. The reliable way is to actually read the file and TryParse whatever comes back. If it doesn't exist or the data is unavailable or unrecognized, fallback. We could use one of the existing patterns: #62513 (simplest would be: try/catch { /* ignore */ }).

In the case of something like SunOS, where the symlink is represented as a binary format, how should we handle it? We may need to check the operating system or implement a mechanism for the parser method to fail gracefully, triggering a fallback syscall as a recovery option.

We can keep P/Invoking for non-linux:
e.g.

#if TARGET_LINUX
try
{
  // read from procfs
  // return;
}
catch { /* ignored */ }
#endif

// existing GetFormatInfoForMountPoint logic

MojtabaTajik · 2024-09-05T07:44:20Z

Problem with File.Exists is that we still may not be able to read the file due to access issues.

Based on the Kernel documentation, There is one such file per process because not all processes see the same mount points.

Since /proc/self/mountinfo refers to the process that is reading the file, you should have no issues accessing it within your process. You can simply open and read it like any other file in the /proc directory.

Btw, I agree that using try-catch is a good approach to handle any unexpected behaviour, as there could be other unknown issues.

…Linux Add support for reading filesystem type from /proc/self/mountinfo on Linux - Prioritize reading the filesystem type from `/proc/self/mountinfo` on Linux. - Retain the existing P/Invoke fallback for non-Linux platforms and as a fallback on Linux. - Added `try/catch` block to handle potential but unlikely access issues or unrecognized data. Although `/proc/self/mountinfo` is process-specific and should generally be accessible, this ensures robustness in case of unexpected errors (e.g., temporary unavailability, corrupted data). - Updated documentation to reflect the reasoning and kernel behavior regarding procfs on different platforms.

src/native/libs/System.Native/entrypoints.c

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

am11 · 2024-09-05T10:08:09Z

Btw, I agree that using try-catch is a good approach to handle any unexpected behaviour, as there could be other unknown issues.

Yup, we have actually seen those issues in chroot type environments (qemu, unikernels like nanos (which is the implementation of Linux interface and not the stock kernel), scratch containers).

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

…untPoints.FormatInfo.cs Co-authored-by: Huo Yaoyuan <[email protected]>

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

This reverts commit 5da14ac.

This reverts commit eba6e78.

This reverts commit c8c0a84.

…terop.MountPoints.FormatInfo.cs" This reverts commit 3505c12.

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

tmds · 2024-09-16T11:57:24Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+                    fields.MoveNext(); // Skip Parent ID
+                    fields.MoveNext(); // Skip Major:Minor
+                    fields.MoveNext(); // Skip Root
+                    fields.MoveNext(); // Skip to MountPoint field


This PR should include additional changes so we use the MountPoint field as the Name/RootDirectory for DriveInfo.

That is: for the instances returned by DriveInfo.GetDrives.

tmds · 2024-09-16T12:01:48Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+                    format = line[fields.Current].ToString();
+
+                    fields.MoveNext();
+                    type = GetDriveType(line[fields.Current].ToString());


Shouldn't we be passing format to GetDriveType instead of reading the next field?

man proc has this example line:

36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue

tmds · 2024-09-16T12:10:45Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+
+                    return 0;
+            }
+            catch { /* ignored */ }


Since the format is well documented, we shouldn't need a catch all.

We can place this under if (File.Exists("/proc/self/mountinfo")) to handle the case where proc is unavailable.

I see this was discussed in #107365 (comment).

Personally, I think the File.Exists should be fine, but I defer to the maintainers' preferences.

The reason why all/most procfs usage in libs are using try-catch pattern is because anything could go wrong (#62513). Also see #74316 (comment) and #52753 to read on unreliability issues attached with FileSystemInfo.Exists out in the wild.

There are some reasons why we don't expect these to occur for /proc/self/mountinfo

Anyway, my main concern here is that a generic catch all masks failures in the parsing.

Perhaps we can filter for IOException/UnauthorizedAccessException. These are the exceptions we might expect when not being able to read the procfs file.

BTW, this approach is similar to what has been used in networking info https://github.com/dotnet/runtime/pull/63696/files#diff-deb3e580b54bccfac2148f60c5d90c6e88b55b508948052e9cf2d1e5421f6ff5R21. Maybe adding a Debug.Fail in catch body for unexpected parsing issues during the development/testing is enough so implementation bugs don't go undetected?

tmds · 2024-09-16T12:13:22Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+        /// <param name="name">The mount point name to query.</param>
+        /// <param name="format">Output parameter for the filesystem format.</param>
+        /// <param name="type">Output parameter for the drive type.</param>
+        /// <returns>0 if successful, otherwise an error code.</returns>
        private static unsafe int GetFormatInfoForMountPoint(string name, out string format, out DriveType type)
        {


It would be nice to have some parsing tests for this, similar to

runtime/src/libraries/Common/tests/Tests/Interop/procfsTests.cs

Line 36 in 0154a2f

public void ParseValidStatFiles_Success(

.

tmds · 2024-09-16T12:32:12Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+                    MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');
+
+                    // Skip fields we don't care about (Fields 1-4)
+                    fields.MoveNext(); // Skip Mount ID


nit: I find the Skip in this comment misleading. The call to MoveNext puts us at the Mount ID, not past it, right?

Maybe you can drop the Skip.

fields.MoveNext(); // Mount ID ...

tmds · 2024-09-16T12:44:03Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+
+                    fields.MoveNext();
+                    type = GetDriveType(line[fields.Current].ToString());
+


There doesn't seem to be a matching closing } for the while loop. If CI doesn't complain, then TARGET_LINUX may not be set.

tmds · 2024-09-16T12:45:56Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+                    type = GetDriveType(line[fields.Current].ToString());
+
+                    return 0;
+            }


If we went over the lines of mountinfo and didn't find a match, there's no need to do the PInvoke. We can just return immediately.

am11 · 2024-09-23T14:06:35Z

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

+#if TARGET_LINUX
+            try
+            {
+                using StreamReader reader = new("/proc/self/mountinfo");
+
+                string rawLine;
+                while ((rawLine = reader.ReadLine()) is not null)
+                {
+                    ReadOnlySpan<char> line = rawLine.AsSpan();
+                    MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');
+
+                    // Skip fields we don't care about (Fields 1-4)
+                    fields.MoveNext(); // Skip Mount ID
+                    fields.MoveNext(); // Skip Parent ID
+                    fields.MoveNext(); // Skip Major:Minor
+                    fields.MoveNext(); // Skip Root
+                    fields.MoveNext(); // Skip to MountPoint field
+
+                    if (!MemoryExtensions.Equals(line[fields.Current], name, StringComparison.Ordinal)) continue;
+
+                    // Skip to the separator which is end of optional fields (Field 8)
+                    while (fields.MoveNext() && !MemoryExtensions.Equals(line[fields.Current], "-", StringComparison.Ordinal));
+
+                    fields.MoveNext();
+                    format = line[fields.Current].ToString();
+
+                    fields.MoveNext();
+                    type = GetDriveType(line[fields.Current].ToString());
+
+                    return 0;
+            }
+            catch { /* ignored */ }
+#endif


Suggested change

#if TARGET_LINUX

try

{

using StreamReader reader = new("/proc/self/mountinfo");

string rawLine;

while ((rawLine = reader.ReadLine()) is not null)

{

ReadOnlySpan<char> line = rawLine.AsSpan();

MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');

// Skip fields we don't care about (Fields 1-4)

fields.MoveNext(); // Skip Mount ID

fields.MoveNext(); // Skip Parent ID

fields.MoveNext(); // Skip Major:Minor

fields.MoveNext(); // Skip Root

fields.MoveNext(); // Skip to MountPoint field

if (!MemoryExtensions.Equals(line[fields.Current], name, StringComparison.Ordinal)) continue;

// Skip to the separator which is end of optional fields (Field 8)

while (fields.MoveNext() && !MemoryExtensions.Equals(line[fields.Current], "-", StringComparison.Ordinal));

fields.MoveNext();

format = line[fields.Current].ToString();

fields.MoveNext();

type = GetDriveType(line[fields.Current].ToString());

return 0;

}

catch { /* ignored */ }

#endif

if (OperatingSystem.IsLinux())

{

try

{

using StreamReader reader = new("/proc/self/mountinfo");

string? rawLine;

while ((rawLine = reader.ReadLine()) is not null)

{

ReadOnlySpan<char> line = rawLine.AsSpan();

MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');

// Skip fields we don't care about (Fields 1-4)

fields.MoveNext(); // Skip Mount ID

fields.MoveNext(); // Skip Parent ID

fields.MoveNext(); // Skip Major:Minor

fields.MoveNext(); // Skip Root

fields.MoveNext(); // Skip to MountPoint field

if (!MemoryExtensions.Equals(line[fields.Current], name, StringComparison.Ordinal)) continue;

// Skip to the separator which is end of optional fields (Field 8)

while (fields.MoveNext() && !MemoryExtensions.Equals(line[fields.Current], "-", StringComparison.Ordinal));

fields.MoveNext();

format = line[fields.Current].ToString();

fields.MoveNext();

type = GetDriveType(line[fields.Current].ToString());

return 0;

}

}

catch { /* ignored */ }

}

I just noticed that this wasn't building on Linux because only CoreLib has TARGET_LINUX type of constants defined. Also, there is a syntax error try and while blocks have only one closing }.

OperatingSystem.IsLinux() block will be optimized out on non-Linux by the JIT (since it returns a constant bool).

I had noticed the syntax error and based on that assumed TARGET_LINUX must not be set (#107365 (comment)).

if (OperatingSystem.IsLinux())

Yes, this is the way!

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 4, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 4, 2024

Merge branch 'main' into FixWrongDriveFormatOnUnix

9fe1985

huoyaoyuan added area-System.IO and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 5, 2024

am11 reviewed Sep 5, 2024

View reviewed changes

src/native/libs/System.Native/entrypoints.c Outdated Show resolved Hide resolved

am11 reviewed Sep 5, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

am11 reviewed Sep 5, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

am11 reviewed Sep 5, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

MojtabaTajik added 3 commits September 5, 2024 12:17

Fix whitespaces and replace tabs with 4 spaces

080825d

Remove pre-processor directives indention

1171819

Fix whitespaces

d43dd7b

am11 reviewed Sep 5, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

MojtabaTajik and others added 2 commits September 5, 2024 12:38

Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…

3542fd8

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

fix indention

1607833

am11 reviewed Sep 5, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

am11 reviewed Sep 5, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

MojtabaTajik and others added 2 commits September 6, 2024 05:23

Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…

2fd5836

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…

49ac57e

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

huoyaoyuan reviewed Sep 6, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

MojtabaTajik and others added 2 commits September 6, 2024 05:57

Use actual type instead of var

eafc91b

Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…

4a17349

…untPoints.FormatInfo.cs Co-authored-by: Huo Yaoyuan <[email protected]>

am11 reviewed Sep 6, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

MojtabaTajik and others added 11 commits September 6, 2024 10:11

Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…

3505c12

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

Simplify checks

c8c0a84

add new line

eba6e78

fix spaces

5da14ac

Revert "fix spaces"

ab1e2f4

This reverts commit 5da14ac.

Revert "add new line"

d52fe86

This reverts commit eba6e78.

Revert "Simplify checks"

4bbceac

This reverts commit c8c0a84.

Revert "Update src/libraries/Common/src/Interop/Unix/System.Native/In…

459d1a6

…terop.MountPoints.FormatInfo.cs" This reverts commit 3505c12.

Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…

b5e2873

…untPoints.FormatInfo.cs Co-authored-by: Adeel Mujahid <[email protected]>

Skip to mount point

542c244

Update comment

171c068

am11 reviewed Sep 6, 2024

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs Outdated Show resolved Hide resolved

Use MemoryExtensions.Equals

1ea7097

am11 requested a review from a team September 7, 2024 08:53

tmds reviewed Sep 16, 2024

View reviewed changes

am11 reviewed Sep 23, 2024

View reviewed changes

build-analysis bot mentioned this pull request Sep 6, 2024

Microsoft.Extensions.SourceGeneration.Configuration.Binder.Tests.ConfigurationBindingGeneratorTests fails on CI #107381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong drive format on Unix #107365

Fix wrong drive format on Unix #107365

MojtabaTajik commented Sep 4, 2024 •

edited

Loading

am11 commented Sep 5, 2024 •

edited

Loading

MojtabaTajik commented Sep 5, 2024 •

edited

Loading

am11 commented Sep 5, 2024

MojtabaTajik commented Sep 5, 2024 •

edited

Loading

am11 commented Sep 5, 2024

tmds Sep 16, 2024

tmds Sep 16, 2024

tmds Sep 16, 2024

tmds Sep 16, 2024

tmds Sep 16, 2024

am11 Sep 16, 2024

tmds Sep 16, 2024

am11 Sep 16, 2024

tmds Sep 16, 2024

tmds Sep 16, 2024 •

edited

Loading

tmds Sep 16, 2024

tmds Sep 16, 2024

am11 Sep 23, 2024

tmds Sep 24, 2024 •

edited

Loading


		fields.MoveNext();
		type = GetDriveType(line[fields.Current].ToString());

Fix wrong drive format on Unix #107365

Are you sure you want to change the base?

Fix wrong drive format on Unix #107365

Conversation

MojtabaTajik commented Sep 4, 2024 • edited Loading

am11 commented Sep 5, 2024 • edited Loading

MojtabaTajik commented Sep 5, 2024 • edited Loading

am11 commented Sep 5, 2024

MojtabaTajik commented Sep 5, 2024 • edited Loading

am11 commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

MojtabaTajik commented Sep 4, 2024 •

edited

Loading

am11 commented Sep 5, 2024 •

edited

Loading

MojtabaTajik commented Sep 5, 2024 •

edited

Loading

MojtabaTajik commented Sep 5, 2024 •

edited

Loading

tmds Sep 16, 2024 •

edited

Loading

tmds Sep 24, 2024 •

edited

Loading