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/6.0-rc2] File preallocationSize: align Windows and Unix behavior. #59532

Merged
merged 5 commits into from
Sep 24, 2021

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Sep 23, 2021

Backport of #59338 to release/6.0

Customer Impact

As per @stephentoub in #59338 (comment):

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 preallocationSize. In main (before this change was merged 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 preallocationSize is just a (non-observable) hint purely for performance, such that the resulting file size isn't dependent on the preallocationSize, and the latter is just there to help the system optimize.

The downside is that some OSes/platforms (e.g: browser) don't have the hint behavior and therefore will ignore preallocationSize.

Testing

Tests are being updated accordingly.

Risk

Low

Note

@ghost
Copy link

ghost commented Sep 23, 2021

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 #59338 to release/6.0

Customer Impact

Author: Jozkee
Assignees: -
Labels:

area-System.IO

Milestone: -

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Sep 23, 2021
@danmoseley
Copy link
Member

not ready yet, but marking so tactics aware. last day is essentially tomorrow.

@jozkee jozkee added this to the 6.0.0 milestone Sep 23, 2021
@jozkee
Copy link
Member Author

jozkee commented Sep 23, 2021

Will send a commit to fix the browser error, strange that we didn't got it in the PR for main.

src/libraries/Native/Unix/System.Native/pal_io.c(1015,9): error G2D4C7688: (NETCORE_ENGINEERING_TELEMETRY=Build) unused variable 'fileDescriptor' [-Werror,-Wunused-variable]

@jozkee
Copy link
Member Author

jozkee commented Sep 23, 2021

@danmoseley it should be ready now.

@jamshedd jamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 23, 2021
@danmoseley
Copy link
Member

@jozkee we can merge as soon as it's green. @Anipik

@jeffhandley
Copy link
Member

not ready yet, but marking so tactics aware. last day is essentially tomorrow.

For RC2, right? Is the intent/expectation to backport this PR into release/6.0-rc2 after it's merged into release/6.0?

@danmoseley
Copy link
Member

Ah yes I didn't realize this wasn't targeted at RC2

@danmoseley danmoseley changed the base branch from release/6.0 to release/6.0-rc2 September 24, 2021 00:35
@danmoseley
Copy link
Member

Rebased, hopefully it works. Still need a review

<PreReleaseVersionLabel>rc</PreReleaseVersionLabel>
<PreReleaseVersionIteration>2</PreReleaseVersionIteration>
<PreReleaseVersionLabel>rtm</PreReleaseVersionLabel>
<PreReleaseVersionIteration></PreReleaseVersionIteration>
Copy link
Member

@stephentoub stephentoub Sep 24, 2021

Choose a reason for hiding this comment

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

@danmoseley, these rebases aren't doing the right things...

Copy link
Member

Choose a reason for hiding this comment

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

I suspect it would be easier/cleaner to back out (with a force push) the rebase commit, and change this back to target release/6.0, merge this as it was into that branch, and then use the backport bot to port from release/6.0 to release/6.0-rc2.

Copy link
Member Author

@jozkee jozkee Sep 24, 2021

Choose a reason for hiding this comment

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

I think we could cherry-pick Adam and Tom's commits to release/6.0-rc2 then force-push that to this branch; then the bot that syncs rc2 with release/6.0 will port automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow. I should have noticed what this did. I wonder how many others...

Yes into 6.0 first is fine it just needs to be in RC2 tomorrow. I'm hesitant to attempt to fix anything again..

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, I rebased to include only the relevant commits and is already targeting RC2. No need to manually port to 6.0 as that is made automatically AFAIK.

adamsitnik and others added 4 commits September 23, 2021 19:08
* 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]>
@jozkee jozkee changed the title [release/6.0] File preallocationSize: align Windows and Unix behavior. [release/6.0-rc2] File preallocationSize: align Windows and Unix behavior. Sep 24, 2021
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Looks like a good retargeting to me; thanks, @jozkee. Either @tmds or @stephentoub should take another look through the changes to verify too.

Copy link
Member

@tmds tmds left a comment

Choose a reason for hiding this comment

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

lgtm

@danmoseley
Copy link
Member

Want me to merge now @jozkee ?

}
}

[OuterLoop("Might allocate 1 TB file if there is enough space on the disk")]
Copy link
Member

Choose a reason for hiding this comment

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

Remind me never to run this suite on outerloop ;-)

@danmoseley
Copy link
Member

I will be unavailable this afternoon (recruiting) so please rely on someone else to merge when ready.

@Anipik Anipik merged commit 157d591 into dotnet:release/6.0-rc2 Sep 24, 2021
@jozkee jozkee deleted the prealloc_6.0 branch September 24, 2021 21:57
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants