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] Enforce scatter/gather file I/O Windows API requirements et. al. #58423

Merged
merged 13 commits into from
Oct 19, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 31, 2021

Backport of #57424 to release/6.0

/cc @adamsitnik @teo-tsirpanis

Customer Impact

Without this fix, users of RandomAccess.ReadAsync(IReadOnlyList<Memory<byte>>) and RandomAccess.WriteAsync(IReadOnlyList<ReadOnlyMemory<byte>>) might get an IO exception for a valid input on Windows.

Testing

In #57717 (before the ported PR got merged) we have added new tests to extend the NO_BUFFERING tests and cover all scenarios.
In #57424 (what we are backporting here) a unit test that was failing before the fix has been added. It's now passing.
To ensure there is no perf regression new benchmarks have been added to performance repo: dotnet/performance#1967 and run manually on a Windows machine.

Risk

Low, as it's a bug fix for a feature that has been introduced in .NET 6 and has not been widely adopted yet.

@ghost
Copy link

ghost commented Aug 31, 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 #57424 to release/6.0

/cc @adamsitnik @teo-tsirpanis

Customer Impact

Testing

Risk

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

area-System.IO

Milestone: -

@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Aug 31, 2021

I suggest we wait until #58447 is merged, where I will subsequently submit a PR of these changes against the head of this PR's branch.

@danmoseley
Copy link
Member

Low, as it's a bug fix for a feature that has been introduced in .NET 6 and has not been widely adopted yet.

What is the risk to the feature itself though? This seems like a significant change to a somewhat complex/perf sensitive area.

@danmoseley
Copy link
Member

@dotnet/area-system-io is this backport something you support? we need a call one way or another. I haven't dug into it.

@adamsitnik
Copy link
Member

is this backport something you support? we need a call one way or another. I haven't dug into it.

It is something that we support and want as part of .NET 6, but we need to wait until #58447 is merged.

@jeffhandley jeffhandley added this to the 6.0.0 milestone Oct 10, 2021
@jeffhandley
Copy link
Member

@dotnet/area-system-io If we are still wanting to get this in for 6.0, it'll need to be presented to tactics this week.

* Allocate an array of memory handles only if needed.

* Remove an unnecessary variable in the multiple-syscall write gather.

* Actually verify the content read by the read scatter operation.

* Delay allocating native memory.

* Verify that the whole file was read in the scatter/gather test.

* Test the case when the scatter/gather buffers are acceptable by the Windows API.

* Avoid null pointer dereferences when passing an empty segment array.

* Test performing scatter/gather I/O with an empty segment array.

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@adamsitnik
Copy link
Member

@jeffhandley I've cherry-picked the changes from follow-up PR (#58447) and tested it locally. Once the CI is green we can merge it. It's not a release blocker as it's fixing a bug for a feature which very few know that we actually support (NO_BUFFERING).

@jozkee jozkee added the Servicing-consider Issue for next servicing release review label Oct 18, 2021
@jeffhandley
Copy link
Member

It's not a release blocker as it's fixing a bug for a feature which very few know that we actually support (NO_BUFFERING).

Thanks. To confirm, is the fix breaking in any way at all in the edge case usage?

@teo-tsirpanis
Copy link
Contributor

is the fix breaking in any way at all in the edge case usage?

No, the algorithm falls back to issuing many syscalls for each buffer segment.

@jeffhandley
Copy link
Member

Failing check is #48717.

@jeffhandley jeffhandley removed the Servicing-consider Issue for next servicing release review label Oct 19, 2021
@jeffhandley jeffhandley added the Servicing-approved Approved for servicing release label Oct 19, 2021
@Anipik Anipik merged commit 7907830 into release/6.0 Oct 19, 2021
@danmoseley danmoseley deleted the backport/pr-57424-to-release/6.0 branch October 19, 2021 17:21
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 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.

6 participants