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

Allow to specify file allocation size #51111

Merged
merged 42 commits into from
May 18, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Apr 12, 2021

Value provided as allocationSize argument has no effect unless it's positive and a regular file is being created, overwritten, or replaced. It means that:

  • if value is < 0 FileStream ctor throws ArgumentOutOfRangeException
  • if value is 0 it's just ignored
  • if the path points to a different type of file like pipe etc the setting is also just ignored
  • if the file system does not support such operation, the setting is also ignored

My main motivation was to make it easy to use it, without performing a lot of OS-specific checks on the caller side that would have to ensure that given path points to a regular file (it can be used for not just creating new files, but also replacing or truncating existing ones).

IOException is thrown when:

  • there is not enough available disk space (I was testing 1TB files on a < 1TB disk on Windows 10, Ubuntu and macOS)
  • the file is too large for given file system (I was testing 4GB+1 files created on FAT32 pendrive with Windows and Ubuntu)

Implementation details:

  • For Windows, I have used NtCreateFile which provides an atomic way to create|replace|truncate a file if there is enough free space. This works quite well, with the only exception of a misleading NT_STATUS_INVALID_PARAMETER returned for files that are too big for given file system. I've found a workaround for that, please see the code.
  • For Linux and other systems that implement posix_fallocate I've used this method. The difference with Windows is that it's an additional syscall (done after the file gets locked). It also changes the reported Length of given file (On Windows AllocationSize != EndOfFile so reported file length is not changed). The weird thing about posix_fallocate is that it's spec does not mention what should have happen when there is not enough space. In case of Ubuntu, the entire available disk space is allocated... This is why I've marked the WhenDiskIsFullTheErrorMessageContainsAllDetails test as [Outerloop] and disabled the parallelism for the type that defines it.
  • For macOS I've used fctnl with F_PREALLOCATE command. In contrary to posix_fallocate it's not modifying the reported file length, so that's why I've decided to also call ftruncate and "align the Unixes" (https://stackoverflow.com/a/29693383). I hate macOS because it's docs don't mention that fctnl can fail with ENOSPC (not enough space), but in reality, it can fail with ENOSPC (verified with my mac).
  • For other Unixes (like FreeBSD) that don't implement posix_fallocate but define F_ALLOCSP I've used it as a command for fctnl. Side note: I got inspired by some popular Torrent clients ;)

fixes #45946
fixes #52446

@adamsitnik adamsitnik added area-System.IO tenet-performance Performance related issue labels Apr 12, 2021
@adamsitnik adamsitnik added this to the 6.0.0 milestone Apr 12, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Apr 12, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #45946

this PR is not ready for review yet. Once I prove that it's useful from perf perspective on Windows, I am going to implement Unix support and then mark the PR as ready for review

Author: adamsitnik
Assignees: -
Labels:

area-System.IO, tenet-performance

Milestone: 6.0.0

# Conflicts:
#	src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamConformanceTests.cs
@KevinCathcart
Copy link
Contributor

Since a previous review comment thread showed some confusion as to why the \??\ prefix is needed and \\?\ does not work, let me explain what is going on.

It all stems from the fact that NtCreateFile needs native NT paths, and expects that any Win32 paths have converted to native ones already.

If you just want to know what the correct code to use in the general case is, you can skip over the background information, but reading it provides much needed background information. Do note, that I'm not sure this most general case is actually worth implementing here. It would add support for UNC paths (I've no idea if UNC paths support specifying allocation size), and would let you one use the \\.\ syntax, but that syntax is only supposed to be used for devices, and you won't be using this api to create a device of a specified size.

Background information

Paths that begin with \??\ are actually not valid Win32 paths, and in theory should never be used with Win32 APIs, although apparently some like CreateFile will tolerate those paths. (I'm not really sure if that has always been tolerated, or if it was added more recently.) The paths beginning with \??\ are actually native NT paths, although not all native NT paths begin with such prefix, which is why it is questionable to pass these ones to Win32 APIs, since other native NT paths won't work if passed to those APIs.

Beyond the normal absolute, relative, drive relative path types, and UNC path types that everybody knows, Win32 has a few more path types. One is \\.\ (a Win32 Device path) which is intended to be used to access devices. Then there is also the \\?\ path type that ntdll calls a "Win32Nt" path. That path type skips normalization, and also skips the path length limitation (historically only with the Unicode versions of functions, but later it also skipped the path limits with the ANSI versions too). In fact the only difference between those two formats is the normalization, and length limit bypass, although they were intended for different scenarios, as reflected by the official documentation, which tries to guide users to using those as they were intended.

Both of those prefixes are resolved against the native NT path that was originally named \DosDevices\, but later, when per session device support was added it got renamed to \??\, with the existing name kept around as a symlink for backwards compatibility. I assume this renaming is partially because the new name is shorter, but also because the old one was starting to feel inaccurate. Device names like Volume{b935930a-783c-4270-9144-a7e696ade62f} were never valid for actual DOS after all.

Before calling NtCreateFile you are "supposed" to convert Win32 paths to native NT paths with the undocumented RtlDosPathNameToNtPathName_U function (or the RtlDosPathNameToRelativeNtPathName_U function, or their status code returning versions.) The intent appears to have been that only Windows itself would ever need to do this. It was intended that if anybody else was was calling NtCreateFile they would be constructing native paths directly, most likely for devices, but potentially for other kernel object too.

So what do those functions do? Well in terms of actual path conversion, for Win32Nt paths (the ones that start \\?\) it simply replaces that prefix with \??\, and is finished. For all other paths, it first normalizes, and then maps to native NT format. For normal drive letter paths it prepends \??\. For Device paths it replaces \\.\ with \??\. For UNC paths it replaces \\ with \??\UNC\.

Basically EnsureExtendedPrefix(Path.GetFullPath(path)) does everything that the undocumented function does except for changing \\.\ and \\?\ to \??\, except in irrelevant edge cases, like paths containing NUL characters which .NET refuses to handle anyway. You can probably now guess my recommendation for the general case.

The General Case

In the general case, if you want maximum compatibility when directly calling NtCreateFile, you would ideally would want to do the equivalent of: EnsureExtendedPrefix(Path.GetFullPath(path)) followed by replacing any leading \\?\ or \\.\ with \??\.

Of course, if something higher up has already ensured Path.GetFullPath has been run, you can skip that part and just do EnsureExtendedPrefix, and the prefix replacement. And of course, if allocations are important, a zero allocation equivalent could be used.

More Info

Besides reading the source code of ntdll.dll (or asking you favorite NT Kernel developer), the best resource for all of this is this post from Google's Project Zero that goes into even more detail than anybody had ever previously written down.

As an aside: I'd have loved if that post delved a bit more into some of how the kernel processes the path afterwards (for example the per session overlays on top of \Global??\ are interesting), but other resources like the Windows Internals books do cover that topic in more depth, while the user mode part of path translation is not well covered by most other sources.

Hope this helps, or at the very least was interesting.

@adamsitnik
Copy link
Member Author

Of course, if something higher up has already ensured Path.GetFullPath has been run, you can skip that part and just do EnsureExtendedPrefix, and the prefix replacement.

In this particular case Path.GetFullPath is already ensured:

internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options)
{
string fullPath = Path.GetFullPath(path);
_path = fullPath;
_access = access;
_share = share;
_fileHandle = FileStreamHelpers.OpenHandle(fullPath, mode, access, share, options);

And of course, if allocations are important, a zero allocation equivalent could be used

This is ensured by ValueStringBuilder that uses stack-allocated memory for small buffers and ArrayPool for larger buffers.

@KevinCathcart big thanks for providing all the information! It helped me to understand why this mapping is needed.

@adamsitnik
Copy link
Member Author

@JeremyKuhne do you have any idea about how we could map the following security flags to NtCreateFile

int flagsAndAttributes = (int)options;
// For mitigating local elevation of privilege attack through named pipes
// make sure we always call CreateFile with SECURITY_ANONYMOUS so that the
// named pipe server can't impersonate a high privileged client security context
// (note that this is the effective default on CreateFile2)
flagsAndAttributes |= (Interop.Kernel32.SecurityOptions.SECURITY_SQOS_PRESENT | Interop.Kernel32.SecurityOptions.SECURITY_ANONYMOUS);

currently, it's the only thing that blocks me from switching to NtCreateFile by default and removing the call to CreateFileW.

Other mappings that we already have in place can be found here: https://github.com/adamsitnik/runtime/blob/c2f995efd5a26b2501e71df173894da7d0a707ae/src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs#L85-L176

@KevinCathcart
Copy link
Contributor

KevinCathcart commented May 10, 2021

The third argument to NtCreateFile is the object attributes. One of the fields of that is PSECURITY_QUALITY_OF_SERVICE SecurityQualityOfService, which is a pointer to yet another structure.

Passing the SECURITY_SQOS_PRESENT flag means that this structure is filled in using the remaining SECURITY_ prefixed attributes passed, instead of being set to a default. With that flag set, all fields in the SECURITY_QUALITY_OF_SERVICE structure default to zero (except, of course, the Length field, per the normal expandable structure pattern), unless a relevant flag is passed. The impersonation level attributes maps to the ImpersonationLevel field, where the enum value for Anonymous is zero. The SECURITY_CONTEXT_TRACKING flag maps to setting the ContextTrackingMode field (which is effectively a strongly typed BOOLEAN) to SECURITY_DYNAMIC_TRACK (1 / TRUE). The SECURITY_EFFECTIVE_ONLY flag maps to setting the EffectiveOnly boolean to true.

Hope this helps.

@JeremyKuhne
Copy link
Member

@adamsitnik

SECURITY_SQOS_PRESENT is a little odd in that it overlaps FILE_FLAG_OPEN_NO_RECALL. The security flags are shifted (>> 16) to map to the ImpersonationLevel in the SECURITY_QUALITY_OF_SERVICE struct (also see 2.2.3.7 in the MS-LSAD open spec). So SECURITY_ANONYMOUS -> SECURITY_IMPERSONATION_LEVEL.Anonymous.

As we don't allow anything else, there is no SECURITY_CONTEXT_TRACKING so SECURITY_IMPERSONATION_LEVEL.ContextTrackingMode should be set to SECURITY_STATIC_TRACKING, which you'll see is false. If the flag was there this would be SECURITY_DYNAMIC_TRACKING.

In a similar vein we don't allow SECURITY_EFFECTIVE_ONLY so SECURITY_QUALITY_OF_SERVICE.EffectiveOnly would be false.

Without the SECURITY_SQOS_PRESENT flag SECURITY_QUALITY_OF_SERVICE should be set to:

  • SECURITY_DYNAMIC_TRACKING
  • SECURITY_IMPERSONATION_LEVEL.SecurityImpersonation
  • TRUE

Notes:

  • The SQOS flags only have meaning if the file is the client side of a named pipe.
  • Set the size of struct in Length, don't infer that you shouldn't because the MS-LSAD spec says it should be ignored.

I put a fair amount of comments on mappings in the various parameters in my experimental library. For example:

https://github.com/JeremyKuhne/WInterop/blob/main/src/WInterop.Desktop/Storage/CreateDisposition.cs
https://github.com/JeremyKuhne/WInterop/blob/main/src/WInterop.Desktop/Storage/DesiredAccess.cs
https://github.com/JeremyKuhne/WInterop/blob/main/src/WInterop.Desktop/Storage/ShareModes.cs

If you haven't looked through my various types around CreateFile and NtCreateFile in the library it would probably be worth looking into it further.

@adamsitnik
Copy link
Member Author

these changes look good to me but this PR is only adding pre-allocation support to FileStream, are you planning on adding the respective overloads for File/FileInfo/StreamWriter in a separate one?

@jozkee yes, my plan is to add them in a separate PR, after we get an approval to change the old proposal from adding a new ctor argument (long allocationSize) to use FileStreamOptions

@adamsitnik adamsitnik requested review from carlossanlop and jozkee May 14, 2021 21:57
@adamsitnik
Copy link
Member Author

@jozkee @carlossanlop I've switched to use FileStreamOptions and addressed all the feedback. PTAL

// For mitigating local elevation of privilege attack through named pipes
// make sure we always call NtCreateFile with SECURITY_ANONYMOUS so that the
// named pipe server can't impersonate a high privileged client security context
SECURITY_QUALITY_OF_SERVICE securityQualityOfService = new SECURITY_QUALITY_OF_SERVICE(
Copy link
Member Author

Choose a reason for hiding this comment

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

@carlossanlop @jozkee this is to mimic the following logic:

// For mitigating local elevation of privilege attack through named pipes
// make sure we always call CreateFile with SECURITY_ANONYMOUS so that the
// named pipe server can't impersonate a high privileged client security context
// (note that this is the effective default on CreateFile2)
flagsAndAttributes |= (Interop.Kernel32.SecurityOptions.SECURITY_SQOS_PRESENT | Interop.Kernel32.SecurityOptions.SECURITY_ANONYMOUS);

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.

Otherwise LGTM, thanks.

Comment on lines 7325 to 7334
public FileStream(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share = System.IO.FileShare.Read, int bufferSize = 4096, System.IO.FileOptions options = System.IO.FileOptions.None, long allocationSize = 0) { }
public FileStream(string path, System.IO.FileStreamOptions options) { }
Copy link
Member

Choose a reason for hiding this comment

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

If the FileStream API approved in #45946 (comment) will not be needed after all, make sure to communicate that in the thread.
Also consider not closing #45946 when this PR gets merged since there's still work to do related to the rest of other types.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jozkee my plan is to just add FileStreamOptions overloads everywhere: #24698 (comment)

@adamsitnik adamsitnik merged commit 4e4b8bf into dotnet:main May 18, 2021
@adamsitnik adamsitnik deleted the fileAllocationSize branch May 18, 2021 07:52
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new option bag for FileStream ctor FileStream file preallocation performance
9 participants