File preallocationSize: align Windows and Unix behavior. #59338

tmds · 2021-09-20T11:50:53Z

This aligns Windows and Unix behavior of preallocationSize for the
intended use-case of specifing the size of a file that will be written.

For this use-case, the expected FileAccess is Write, and the file should be
a new one (FileMode.Create*) or a truncated file (FileMode.Truncate).
Specifing a preallocationSize for other modes, or non-writable files throws ArgumentException.

The opened file will have a length of zero, and is ready to be written to by the user.

If the requested size cannot be allocated, an IOException is thrown.

When the OS/filesystem does not support pre-allocating, preallocationSize is ignored.

@stephentoub @danmoseley @jozkee @carlossanlop ptal.

cc @adamsitnik

edit: relates to #59078

This aligns Windows and Unix behavior of preallocationSize for the intended use-case of specifing the size of a file that will be written. For this use-case, the expected FileAccess is Write, and the file should be a new one (FileMode.Create*) or a truncated file (FileMode.Truncate). Specifing a preallocationSize for other modes, or non-writable files throws ArgumentException. The opened file will have a length of zero, and is ready to be written to by the user. If the requested size cannot be allocated, an IOException is thrown. When the OS/filesystem does not support pre-allocating, preallocationSize is ignored.

ghost · 2021-09-20T11:51:00Z

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

Issue Details

This aligns Windows and Unix behavior of preallocationSize for the
intended use-case of specifing the size of a file that will be written.

For this use-case, the expected FileAccess is Write, and the file should be
a new one (FileMode.Create*) or a truncated file (FileMode.Truncate).
Specifing a preallocationSize for other modes, or non-writable files throws ArgumentException.

The opened file will have a length of zero, and is ready to be written to by the user.

If the requested size cannot be allocated, an IOException is thrown.

When the OS/filesystem does not support pre-allocating, preallocationSize is ignored.

@stephentoub @danmoseley @jozkee @carlossanlop ptal.

cc @adamsitnik

Author:	tmds
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

tmds · 2021-09-20T11:52:23Z

I've only run this on my Linux machine. I'll have to make some updates based on CI.

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FILE_ALLOCATION_INFO.cs

src/libraries/Native/Unix/System.Native/pal_io.c

src/libraries/System.IO.FileSystem/tests/File/Open.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

stephentoub · 2021-09-20T15:12:06Z

@danmoseley, @jeffhandley, we're currently in a poor state with regards to this preallocationSize thing. main contains a breaking change from release/6.0, and neither has the behavior we should be shipping. The goal of this PR is to get us back to something that makes sense and is consistent across platforms, but for that to be successful we need to be willing to take this to release/6.0.

For reference, this is about the preallocationSize argument added to FileStream in .NET 6. If you end up writing fewer bytes than you preallocated, in release/6.0 Windows will truncate the file to the number of bytes actually written whereas Linux will end up with a file padded with zeros at the end to meet the preallocatedSize. In main, both Windows and Linux will end up with said zeros at the end. This PR tries to bring it to a state where preallocatedSize is just a (non-observable) hint purely for performance, such that the resulting file size isn't dependent on the preallocatedSize, and the latter is just there to help the system optimize.

jeffhandley · 2021-09-20T15:21:47Z

Thanks for the helpful background and explanation, @stephentoub. The behavior of this just being a hint instead of a fixed file size (let alone the consistency) sounds intuitive to me. This has my support for acceptance into RC2.

tmds · 2021-09-20T15:55:51Z

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs

+
+            if (mode != FileMode.Create &&
+                mode != FileMode.CreateNew &&
+                mode != FileMode.Truncate)


I'm going to leave out Truncate here as I think it requires some special attention in case the allocation fails to ensure the file remains but doesn't take up space.

For Create and CreateNew we'll delete the file.

This is missing the case of OpenOrCreate when the file is seekable and its length is zero, was that made on purpose?
Is there any other case that this PR is switching to "not support" and it was previously supported?

This is missing the case of OpenOrCreate when the file is seekable and its length is zero, was that made on purpose?

Yes, the behavior changes depending on the length of the file, which the user typically won't know.
Leaving out OpenOrCreate is more deterministic.

Is there any other case that this PR is switching to "not support" and it was previously supported?

The files must to be opened with write file access and a mode that creates.

src/libraries/Native/Unix/System.Native/pal_io.c

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs

jozkee · 2021-09-20T20:37:51Z

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs

+
+            if (mode != FileMode.Create &&
+                mode != FileMode.CreateNew &&
+                mode != FileMode.Truncate)


This is missing the case of OpenOrCreate when the file is seekable and its length is zero, was that made on purpose?
Is there any other case that this PR is switching to "not support" and it was previously supported?

tmds · 2021-09-21T18:32:46Z

@stephentoub @jozkee I've addressed your feedback and CI is passing, can you take another look?

jozkee

@tmds I still need to take a look at the tests but otherwise, LGTM.

src/libraries/Native/Unix/System.Native/pal_io.c

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

src/libraries/System.IO.FileSystem/src/Resources/Strings.resx

Co-authored-by: David Cantú <[email protected]>

jozkee

My only concern with the proposed change here is that it is dropping support of various FileModes to only support Create and CreateNew, and it is switching to throw exceptions instead of just "ignoring the hint". I think is not a big deal since we can change in the future and it wouldn't be a breaking change to do so.

Other than my comments related to tests, LGTM.

src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options.cs

src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options.Windows.cs

src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options.Unix.cs

stephentoub · 2021-09-23T03:54:07Z

src/libraries/System.IO.FileSystem/tests/File/OpenHandle.cs

    {
        protected override string GetExpectedParamName(string paramName) => paramName;

        protected override FileStream CreateFileStream(string path, FileMode mode)
        {
            FileAccess access = mode == FileMode.Append ? FileAccess.Write : FileAccess.ReadWrite;
-            return new FileStream(File.OpenHandle(path, mode, access, preallocationSize: PreallocationSize), access);
+            return new FileStream(File.OpenHandle(path, mode, access), access);


What was the verdict on testing coverage with all of these tests being updated to not pass in a preallocation size?

Coverage is good now.

I refactored tests to follow the pattern that was already used by adding an overload of CreateFileStream to the base class FileStream_ctor_options that accepts a preallocationSize and add the preallocationSize related tests in this class. The derived classes use it from different public APIs.

stephentoub

Thanks, Tom.

tmds · 2021-09-23T08:27:32Z

CI failures are not related.

This is good to merge.

jozkee · 2021-09-23T16:27:21Z

/backport to release/6.0

github-actions · 2021-09-23T16:27:35Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1266655869

github-actions · 2021-09-23T16:29:20Z

@jozkee backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: File preallocationSize: align Windows and Unix behavior.
Using index info to reconstruct a base tree...
M	src/libraries/Native/Unix/System.Native/entrypoints.c
M	src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.Windows.cs
M	src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs
M	src/libraries/System.IO.FileSystem/tests/Net5CompatTests/System.IO.FileSystem.Net5Compat.Tests.csproj
M	src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
M	src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
M	src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
M	src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
M	src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs
Auto-merging src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Auto-merging src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
CONFLICT (content): Merge conflict in src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
Auto-merging src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
CONFLICT (content): Merge conflict in src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
Auto-merging src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
CONFLICT (content): Merge conflict in src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
Auto-merging src/libraries/System.IO.FileSystem/tests/Net5CompatTests/System.IO.FileSystem.Net5Compat.Tests.csproj
Auto-merging src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs
CONFLICT (content): Merge conflict in src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs
Auto-merging src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.Windows.cs
CONFLICT (content): Merge conflict in src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.Windows.cs
CONFLICT (add/add): Merge conflict in src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.Unix.cs
Auto-merging src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.Unix.cs
Auto-merging src/libraries/Native/Unix/System.Native/entrypoints.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 File preallocationSize: align Windows and Unix behavior.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

* File preallocationSize: align Windows and Unix behavior. This aligns Windows and Unix behavior of preallocationSize for the intended use-case of specifing the size of a file that will be written. For this use-case, the expected FileAccess is Write, and the file should be a new one (FileMode.Create*) or a truncated file (FileMode.Truncate). Specifing a preallocationSize for other modes, or non-writable files throws ArgumentException. The opened file will have a length of zero, and is ready to be written to by the user. If the requested size cannot be allocated, an IOException is thrown. When the OS/filesystem does not support pre-allocating, preallocationSize is ignored. * fix pal_io preprocessor checks * pal_io more fixes * ctor_options_as.Windows.cs: fix compilation * Update tests * tests: use preallocationSize from all public APIs * pal_io: add back FreeBSD, fix OSX * tests: check allocated is zero when preallocation is not supported. * Only throw for not enough space errors * Fix compilation * Add some more tests * Fix ExtendedPathsAreSupported test * Apply suggestions from code review Co-authored-by: David Cantú <[email protected]> * Update System.Private.CoreLib Strings.resx * PR feedback * Remove GetPathToNonExistingFile * Fix compilation * Skip checking allocated size on mobile platforms. Co-authored-by: David Cantú <[email protected]>

…vior. (#59532) * Align preallocationSize behavior (#58726) Co-authored-by: Stephen Toub <[email protected]> * File preallocationSize: align Windows and Unix behavior. (#59338) * File preallocationSize: align Windows and Unix behavior. This aligns Windows and Unix behavior of preallocationSize for the intended use-case of specifing the size of a file that will be written. For this use-case, the expected FileAccess is Write, and the file should be a new one (FileMode.Create*) or a truncated file (FileMode.Truncate). Specifing a preallocationSize for other modes, or non-writable files throws ArgumentException. The opened file will have a length of zero, and is ready to be written to by the user. If the requested size cannot be allocated, an IOException is thrown. When the OS/filesystem does not support pre-allocating, preallocationSize is ignored. * fix pal_io preprocessor checks * pal_io more fixes * ctor_options_as.Windows.cs: fix compilation * Update tests * tests: use preallocationSize from all public APIs * pal_io: add back FreeBSD, fix OSX * tests: check allocated is zero when preallocation is not supported. * Only throw for not enough space errors * Fix compilation * Add some more tests * Fix ExtendedPathsAreSupported test * Apply suggestions from code review Co-authored-by: David Cantú <[email protected]> * Update System.Private.CoreLib Strings.resx * PR feedback * Remove GetPathToNonExistingFile * Fix compilation * Skip checking allocated size on mobile platforms. Co-authored-by: David Cantú <[email protected]> * Fix unused fileDescriptor * void fd when unused * Address feedback Co-authored-by: Adam Sitnik <[email protected]> Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: Tom Deseyn <[email protected]>

ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 20, 2021

dotnet-issue-labeler bot added area-System.IO and removed community-contribution Indicates that the PR has been added by a community member labels Sep 20, 2021

tmds added 4 commits September 20, 2021 14:55

fix pal_io preprocessor checks

3245d91

pal_io more fixes

ff29b43

ctor_options_as.Windows.cs: fix compilation

c2aef89

Update tests

d3db729

stephentoub reviewed Sep 20, 2021

View reviewed changes

tmds commented Sep 20, 2021

View reviewed changes

am11 reviewed Sep 20, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_io.c Show resolved Hide resolved

jozkee reviewed Sep 20, 2021

View reviewed changes

tmds added 7 commits September 21, 2021 06:04

tests: use preallocationSize from all public APIs

a5fde03

pal_io: add back FreeBSD, fix OSX

31de574

tests: check allocated is zero when preallocation is not supported.

ec6e98f

Only throw for not enough space errors

a62b89c

Fix compilation

fc75560

Add some more tests

a58c0e2

Fix ExtendedPathsAreSupported test

77d1978

jozkee reviewed Sep 21, 2021

View reviewed changes

tmds and others added 2 commits September 21, 2021 21:14

Apply suggestions from code review

7decfba

Co-authored-by: David Cantú <[email protected]>

Update System.Private.CoreLib Strings.resx

7482a40

jozkee approved these changes Sep 22, 2021

View reviewed changes

tmds added 2 commits September 22, 2021 10:48

PR feedback

3ea664a

Remove GetPathToNonExistingFile

9f4e556

Fix compilation

26282f3

stephentoub reviewed Sep 23, 2021

View reviewed changes

stephentoub approved these changes Sep 23, 2021

View reviewed changes

Skip checking allocated size on mobile platforms.

ec9412b

jozkee merged commit 44b4450 into dotnet:main Sep 23, 2021

jozkee mentioned this pull request Sep 23, 2021

[release/6.0-rc2] File preallocationSize: align Windows and Unix behavior. #59532

Merged

runfoapp bot mentioned this pull request Sep 23, 2021

System.Text.RegularExpressions.Tests.RegexMatchTests.Match_ExcessPrefix Failing in CI #59541

Closed

runfoapp bot mentioned this pull request Sep 24, 2021

emsdk failure: emcmake not found #59556

Closed

kunalspathak mentioned this pull request Sep 28, 2021

Regression in File.PreallocationSize #59705

Closed

adamsitnik mentioned this pull request Oct 8, 2021

fallocate sets errno on error, posix_fallocate did not #60185

Merged

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File preallocationSize: align Windows and Unix behavior. #59338

File preallocationSize: align Windows and Unix behavior. #59338

tmds commented Sep 20, 2021 •

edited by danmoseley

Loading

ghost commented Sep 20, 2021

tmds commented Sep 20, 2021

stephentoub commented Sep 20, 2021 •

edited

Loading

jeffhandley commented Sep 20, 2021

tmds Sep 20, 2021

jozkee Sep 20, 2021 •

edited

Loading

tmds Sep 21, 2021

jozkee Sep 20, 2021 •

edited

Loading

tmds commented Sep 21, 2021

jozkee left a comment

jozkee left a comment •

edited

Loading

stephentoub Sep 23, 2021

tmds Sep 23, 2021

stephentoub left a comment

tmds commented Sep 23, 2021

jozkee commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

File preallocationSize: align Windows and Unix behavior. #59338

File preallocationSize: align Windows and Unix behavior. #59338

Conversation

tmds commented Sep 20, 2021 • edited by danmoseley Loading

ghost commented Sep 20, 2021

tmds commented Sep 20, 2021

stephentoub commented Sep 20, 2021 • edited Loading

jeffhandley commented Sep 20, 2021

tmds Sep 20, 2021

Choose a reason for hiding this comment

jozkee Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

tmds Sep 21, 2021

Choose a reason for hiding this comment

jozkee Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

tmds commented Sep 21, 2021

jozkee left a comment

Choose a reason for hiding this comment

jozkee left a comment • edited Loading

Choose a reason for hiding this comment

stephentoub Sep 23, 2021

Choose a reason for hiding this comment

tmds Sep 23, 2021

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

tmds commented Sep 23, 2021

jozkee commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

tmds commented Sep 20, 2021 •

edited by danmoseley

Loading

stephentoub commented Sep 20, 2021 •

edited

Loading

jozkee Sep 20, 2021 •

edited

Loading

jozkee Sep 20, 2021 •

edited

Loading

jozkee left a comment •

edited

Loading