From e502084cf8e4c4f7485cf4af84acb2b129d4318b Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 31 Aug 2021 19:31:05 +0300 Subject: [PATCH 1/9] Allocate an array of memory handles only if needed. --- .../src/System/IO/RandomAccess.Windows.cs | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 31adaca38fe5ee..6f6998afd3dd2e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -4,6 +4,7 @@ using System.Buffers; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.IO.Strategies; using System.Numerics; using System.Runtime.InteropServices; @@ -437,7 +438,7 @@ private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) // The pinned MemoryHandles and the pointer to the segments must be cleaned-up // with the CleanupScatterGatherBuffers method. private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnlyList buffers, - THandler handler, out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) + THandler handler, [NotNullWhen(true)] out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) where THandler: struct, IMemoryHandler { int pageSize = s_cachedPageSize; @@ -447,7 +448,7 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly long alignedAtPageSizeMask = pageSize - 1; int buffersCount = buffers.Count; - handlesToDispose = new MemoryHandle[buffersCount]; + handlesToDispose = null; segmentsPtr = IntPtr.Zero; totalBytes = 0; @@ -469,18 +470,23 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly return false; } - MemoryHandle handle = handlesToDispose[i] = handler.Pin(in buffer); + MemoryHandle handle = handler.Pin(in buffer); long ptr = segments[i] = (long)handle.Pointer; if ((ptr & alignedAtPageSizeMask) != 0) { + handle.Dispose(); return false; } + + // We avoid allocating an array if there are + // no buffers or the first one is unacceptable. + (handlesToDispose ??= new MemoryHandle[buffersCount])[i] = handle; } segmentsPtr = (IntPtr)segments; totalBytes = (int)totalBytes64; success = true; - return true; + return handlesToDispose != null; } finally { @@ -491,11 +497,14 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly } } - private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[] handlesToDispose, IntPtr segmentsPtr) + private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[]? handlesToDispose, IntPtr segmentsPtr) { - foreach (MemoryHandle handle in handlesToDispose) + if (handlesToDispose != null) { - handle.Dispose(); + foreach (MemoryHandle handle in handlesToDispose) + { + handle.Dispose(); + } } NativeMemory.Free((void*) segmentsPtr); @@ -510,7 +519,7 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I } if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) + && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { return ReadScatterAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); } @@ -607,7 +616,7 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn } if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) + && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { return WriteGatherAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); } From 3f802758666eb085afde65b8eaed9de72a77c54c Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 31 Aug 2021 19:33:13 +0300 Subject: [PATCH 2/9] Remove an unnecessary variable in the multiple-syscall write gather. --- .../src/System/IO/RandomAccess.Windows.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 6f6998afd3dd2e..e66673b02c49ac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -680,13 +680,12 @@ private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, IntP private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, CancellationToken cancellationToken) { - long bytesWritten = 0; int buffersCount = buffers.Count; for (int i = 0; i < buffersCount; i++) { ReadOnlyMemory rom = buffers[i]; - await WriteAtOffsetAsync(handle, rom, fileOffset + bytesWritten, cancellationToken).ConfigureAwait(false); - bytesWritten += rom.Length; + await WriteAtOffsetAsync(handle, rom, fileOffset, cancellationToken).ConfigureAwait(false); + fileOffset += rom.Length; } } From 6e0836bab650571f80a09f48ea87a43076bd15a8 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 31 Aug 2021 19:51:02 +0300 Subject: [PATCH 3/9] Actually verify the content read by the read scatter operation. --- .../tests/RandomAccess/NoBuffering.Windows.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index 8bb39e4b2893f5..0359f2c1f8990d 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -187,10 +187,10 @@ public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers() // The Windows scatter/gather APIs accept segments that are exactly one page long. // Combined with the FILE_FLAG_NO_BUFFERING's requirements, the segments must also // be aligned at page size boundaries and have a size of a multiple of the page size. - // Using segments with a length of twice the page size adheres to the second requirement - // but not the first. The RandomAccess implementation will see it and issue sequential - // read/write syscalls per segment, instead of one scatter/gather syscall. - // This test verifies that fallback behavior. + // Using segments with a length of twice the page size adheres to FILE_FLAG_NO_BUFFERING's + // requirements but not the scatter/gather APIs'. The RandomAccess implementation will + // see it and issue sequential read/write syscalls per segment, instead of one + // scatter/gather syscall. This test verifies that fallback behavior. int bufferSize = Environment.SystemPageSize * 2; int fileSize = bufferSize * 2; byte[] content = RandomNumberGenerator.GetBytes(fileSize); @@ -206,9 +206,9 @@ public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers() buffer.GetSpan().Clear(); await RandomAccess.ReadAsync(handle, new Memory[] { firstHalf, secondHalf }, 0); - } - Assert.Equal(content, await File.ReadAllBytesAsync(filePath)); + Assert.True(buffer.GetSpan().SequenceEqual(content.AsSpan()), "Unexpected file content."); + } } // when using FileOptions.Asynchronous we are testing Scatter&Gather APIs on Windows (FILE_FLAG_OVERLAPPED requirement) From e95d021582bd830ce02c3e31e191f92106b18eb3 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis <12659251+teo-tsirpanis@users.noreply.github.com> Date: Tue, 31 Aug 2021 22:00:47 +0300 Subject: [PATCH 4/9] Use AssertExtensions.SequenceEqual. Co-authored-by: Stephen Toub --- .../tests/RandomAccess/NoBuffering.Windows.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index 0359f2c1f8990d..b8d92e7186fa15 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -207,7 +207,7 @@ public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers() buffer.GetSpan().Clear(); await RandomAccess.ReadAsync(handle, new Memory[] { firstHalf, secondHalf }, 0); - Assert.True(buffer.GetSpan().SequenceEqual(content.AsSpan()), "Unexpected file content."); + AssertExtensions.SequenceEqual(buffer.GetSpan(), content.AsSpan()); } } From 8177b2329f92f7f0a81b16d38886ec52997c893c Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 31 Aug 2021 22:13:21 +0300 Subject: [PATCH 5/9] Delay allocating native memory. --- .../src/System/IO/RandomAccess.Windows.cs | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index e66673b02c49ac..46b84d080972ac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -452,9 +452,7 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly segmentsPtr = IntPtr.Zero; totalBytes = 0; - // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - long* segments = (long*) NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long)); - segments[buffersCount] = 0; + long* segments = null; bool success = false; try @@ -471,18 +469,27 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly } MemoryHandle handle = handler.Pin(in buffer); - long ptr = segments[i] = (long)handle.Pointer; + long ptr = (long)handle.Pointer; if ((ptr & alignedAtPageSizeMask) != 0) { handle.Dispose(); return false; } - // We avoid allocating an array if there are - // no buffers or the first one is unacceptable. + // We avoid allocations if there are no + // buffers or the first one is unacceptable. (handlesToDispose ??= new MemoryHandle[buffersCount])[i] = handle; + if (segments == null) + { + // "The array must contain enough elements to store nNumberOfBytesToWrite + // bytes of data, and one element for the terminating NULL." + segments = (long*)NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long)); + } + segments[i] = ptr; } + Debug.Assert(segments != null); + segments[buffersCount] = 0; segmentsPtr = (IntPtr)segments; totalBytes = (int)totalBytes64; success = true; @@ -507,7 +514,10 @@ private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[]? handlesTo } } - NativeMemory.Free((void*) segmentsPtr); + if (segmentsPtr != IntPtr.Zero) + { + NativeMemory.Free((void*) segmentsPtr); + } } private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList> buffers, From 14502904e95bcb92add977151729db657c399dd2 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 31 Aug 2021 22:26:51 +0300 Subject: [PATCH 6/9] Verify that the whole file was read in the scatter/gather test. --- .../tests/RandomAccess/NoBuffering.Windows.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index b8d92e7186fa15..d8e574b35dd269 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -205,8 +205,9 @@ public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers() await RandomAccess.WriteAsync(handle, new ReadOnlyMemory[] { firstHalf, secondHalf }, 0); buffer.GetSpan().Clear(); - await RandomAccess.ReadAsync(handle, new Memory[] { firstHalf, secondHalf }, 0); + long nRead = await RandomAccess.ReadAsync(handle, new Memory[] { firstHalf, secondHalf }, 0); + Assert.Equal(buffer.GetSpan().Length, nRead); AssertExtensions.SequenceEqual(buffer.GetSpan(), content.AsSpan()); } } From 1c74cdfe613775cd33d9c01efdc050cfdd4add32 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 1 Sep 2021 02:05:47 +0300 Subject: [PATCH 7/9] Test the case when the scatter/gather buffers are acceptable by the Windows API. --- .../tests/RandomAccess/NoBuffering.Windows.cs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index d8e574b35dd269..c1d2e6e64c7690 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -180,18 +180,17 @@ public async Task WriteAsyncUsingMultipleBuffers(bool asyncOperation, bool async Assert.Equal(content, File.ReadAllBytes(filePath)); } - [Fact] - public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ReadWriteAsyncUsingMultipleBuffers(bool memoryPageSized) { string filePath = GetTestFilePath(); - // The Windows scatter/gather APIs accept segments that are exactly one page long. - // Combined with the FILE_FLAG_NO_BUFFERING's requirements, the segments must also - // be aligned at page size boundaries and have a size of a multiple of the page size. - // Using segments with a length of twice the page size adheres to FILE_FLAG_NO_BUFFERING's - // requirements but not the scatter/gather APIs'. The RandomAccess implementation will - // see it and issue sequential read/write syscalls per segment, instead of one - // scatter/gather syscall. This test verifies that fallback behavior. - int bufferSize = Environment.SystemPageSize * 2; + // We test with buffers both one and two memory pages long. In the former case, + // the I/O operations will issue one scatter/gather API call, and in the latter + // case they will issue multiple calls; one per buffer. The buffers must still + // be aligned to comply with FILE_FLAG_NO_BUFFERING's requirements. + int bufferSize = Environment.SystemPageSize * (memoryPageSized ? 1 : 2); int fileSize = bufferSize * 2; byte[] content = RandomNumberGenerator.GetBytes(fileSize); From 4625c1659c28e7d8c0283f1cf898f3e210fa3852 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 13 Sep 2021 17:50:20 +0300 Subject: [PATCH 8/9] Avoid null pointer dereferences when passing an empty segment array. --- .../src/System/IO/RandomAccess.Windows.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 46b84d080972ac..01707557d8a783 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -439,7 +439,7 @@ private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) // with the CleanupScatterGatherBuffers method. private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnlyList buffers, THandler handler, [NotNullWhen(true)] out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) - where THandler: struct, IMemoryHandler + where THandler : struct, IMemoryHandler { int pageSize = s_cachedPageSize; Debug.Assert(BitOperations.IsPow2(pageSize), "Page size is not a power of two."); @@ -483,13 +483,12 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly { // "The array must contain enough elements to store nNumberOfBytesToWrite // bytes of data, and one element for the terminating NULL." - segments = (long*)NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long)); + segments = (long*)NativeMemory.Alloc((nuint)buffersCount + 1, sizeof(long)); + segments[buffersCount] = 0; } segments[i] = ptr; } - Debug.Assert(segments != null); - segments[buffersCount] = 0; segmentsPtr = (IntPtr)segments; totalBytes = (int)totalBytes64; success = true; @@ -499,7 +498,7 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly { if (!success) { - CleanupScatterGatherBuffers(handlesToDispose, (IntPtr) segments); + CleanupScatterGatherBuffers(handlesToDispose, (IntPtr)segments); } } } @@ -516,7 +515,7 @@ private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[]? handlesTo if (segmentsPtr != IntPtr.Zero) { - NativeMemory.Free((void*) segmentsPtr); + NativeMemory.Free((void*)segmentsPtr); } } From 0c0f64edfcd6918f2c74462314aee2d26a42c71d Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 13 Sep 2021 17:51:14 +0300 Subject: [PATCH 9/9] Test performing scatter/gather I/O with an empty segment array. --- .../tests/RandomAccess/NoBuffering.Windows.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index c1d2e6e64c7690..ef60a9f223fb28 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -211,6 +211,17 @@ public async Task ReadWriteAsyncUsingMultipleBuffers(bool memoryPageSized) } } + [Fact] + public async Task ReadWriteAsyncUsingEmptyBuffers() + { + string filePath = GetTestFilePath(); + using SafeFileHandle handle = File.OpenHandle(filePath, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.Asynchronous | NoBuffering); + + long nRead = await RandomAccess.ReadAsync(handle, Array.Empty>(), 0); + Assert.Equal(0, nRead); + await RandomAccess.WriteAsync(handle, Array.Empty>(), 0); + } + // when using FileOptions.Asynchronous we are testing Scatter&Gather APIs on Windows (FILE_FLAG_OVERLAPPED requirement) private static FileOptions GetFileOptions(bool asyncHandle) => (asyncHandle ? FileOptions.Asynchronous : FileOptions.None) | NoBuffering; }