From 7907830305cef30c35e4f85b6bb01969571302b4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 19 Oct 2021 10:02:56 -0700 Subject: [PATCH] [release/6.0] Enforce scatter/gather file I/O Windows API requirements et. al. (#58423) * Move checking and pinning Windows vectored I/O buffers to a dedicated method. * Refactor the scatter/gather APIs to use the common checking method. And use pinned GCHandles and IntPtrs instead of MemoryHandles when passing the segment array to the bottom-most method. * Shorten the name of the buffer-checking method. * Directly get the pinned array's address instead of calling GCHandle.AddrOfPinnedObject. * Refactor the error handling logic in TryPrepareScatterGatherBuffers. * Allocate the segment array from native memory and at TryPrepareScatterGatherBuffers. * Cache the page size on a static readonly field and add a couple of TODOs. * Make the memory handlers readonly structs. * Add a test. * Reorder some methods with PR feedback taken into consideration. * Stop special-casing scatter/gather operations with zero or one buffer. * Factor the cleaning-up of the segment buffers into a separate method. * Follow up on Scatter/Gather API changes (#58447) * 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 Co-authored-by: Theodore Tsirpanis Co-authored-by: Theodore Tsirpanis <12659251+teo-tsirpanis@users.noreply.github.com> Co-authored-by: Stephen Toub --- .../tests/RandomAccess/NoBuffering.Windows.cs | 42 +++ .../src/System/IO/RandomAccess.Windows.cs | 267 ++++++++++-------- 2 files changed, 195 insertions(+), 114 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 d897cc7b8be25..bde3863d5b1df 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -176,5 +176,47 @@ public async Task WriteAsyncUsingMultipleBuffers(bool async) Assert.Equal(content, File.ReadAllBytes(filePath)); } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ReadWriteAsyncUsingMultipleBuffers(bool memoryPageSized) + { + string filePath = GetTestFilePath(); + // 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); + + using (SafeFileHandle handle = File.OpenHandle(filePath, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.Asynchronous | NoBuffering)) + using (SectorAlignedMemory buffer = SectorAlignedMemory.Allocate(fileSize)) + { + Memory firstHalf = buffer.Memory.Slice(0, bufferSize); + Memory secondHalf = buffer.Memory.Slice(bufferSize); + + content.AsSpan().CopyTo(buffer.GetSpan()); + await RandomAccess.WriteAsync(handle, new ReadOnlyMemory[] { firstHalf, secondHalf }, 0); + + buffer.GetSpan().Clear(); + long nRead = await RandomAccess.ReadAsync(handle, new Memory[] { firstHalf, secondHalf }, 0); + + Assert.Equal(buffer.GetSpan().Length, nRead); + AssertExtensions.SequenceEqual(buffer.GetSpan(), content.AsSpan()); + } + } + + [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); + } } } 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 d367abb8fb973..01707557d8a78 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,7 +4,9 @@ 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; using System.Threading; using System.Threading.Tasks; @@ -16,6 +18,9 @@ public static partial class RandomAccess { private static readonly IOCompletionCallback s_callback = AllocateCallback(); + // TODO: Use SystemPageSize directly when #57442 is fixed. + private static readonly int s_cachedPageSize = Environment.SystemPageSize; + internal static unsafe long GetFileLength(SafeFileHandle handle) { Interop.Kernel32.FILE_STANDARD_INFO info; @@ -413,80 +418,137 @@ internal static void WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyList ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList> buffers, - long fileOffset, CancellationToken cancellationToken) + // From https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfilescatter: + // "The file handle must be created with [...] the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." + private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) + => handle.IsAsync && ((handle.GetFileOptions() & SafeFileHandle.NoBuffering) != 0); + + // From the same source: + // "Each buffer must be at least the size of a system memory page and must be aligned on a system + // memory page size boundary. The system reads/writes one system memory page of data into/from each buffer." + // This method returns true if the buffers can be used by + // the Windows scatter/gather API, which happens when they are: + // 1. aligned at page size boundaries + // 2. exactly one page long each (our own requirement to prevent partial reads) + // 3. not bigger than 2^32 - 1 in total + // This function is also responsible for pinning the buffers if they + // are suitable and they must be unpinned after the I/O operation completes. + // It also returns a pointer with the segments to be passed to the + // Windows API, and the total size of the buffers that is needed as well. + // 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, [NotNullWhen(true)] out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) + where THandler : struct, IMemoryHandler { - if (!handle.IsAsync) - { - return ScheduleSyncReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); - } + int pageSize = s_cachedPageSize; + Debug.Assert(BitOperations.IsPow2(pageSize), "Page size is not a power of two."); + // We take advantage of the fact that the page size is + // a power of two to avoid an expensive modulo operation. + long alignedAtPageSizeMask = pageSize - 1; + + int buffersCount = buffers.Count; + handlesToDispose = null; + segmentsPtr = IntPtr.Zero; + totalBytes = 0; + + long* segments = null; - if (CanUseScatterGatherWindowsAPIs(handle)) + bool success = false; + try { - long totalBytes = 0; - int buffersCount = buffers.Count; + long totalBytes64 = 0; for (int i = 0; i < buffersCount; i++) { - totalBytes += buffers[i].Length; + T buffer = buffers[i]; + int length = handler.GetLength(in buffer); + totalBytes64 += length; + if (length != pageSize || totalBytes64 > int.MaxValue) + { + return false; + } + + MemoryHandle handle = handler.Pin(in buffer); + long ptr = (long)handle.Pointer; + if ((ptr & alignedAtPageSizeMask) != 0) + { + handle.Dispose(); + return false; + } + + // 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[buffersCount] = 0; + } + segments[i] = ptr; } - if (totalBytes <= int.MaxValue) // the ReadFileScatter API uses int, not long + segmentsPtr = (IntPtr)segments; + totalBytes = (int)totalBytes64; + success = true; + return handlesToDispose != null; + } + finally + { + if (!success) { - return ReadScatterAtOffsetSingleSyscallAsync(handle, buffers, fileOffset, (int)totalBytes, cancellationToken); + CleanupScatterGatherBuffers(handlesToDispose, (IntPtr)segments); } } - - return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); } - // From https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfilescatter: - // "The file handle must be created with the GENERIC_READ right, and the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." - private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) - => handle.IsAsync && ((handle.GetFileOptions() & SafeFileHandle.NoBuffering) != 0); - - private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[]? handlesToDispose, IntPtr segmentsPtr) { - int buffersCount = buffers.Count; - if (buffersCount == 1) + if (handlesToDispose != null) { - // we have to await it because we can't cast a VT to VT - return await ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken).ConfigureAwait(false); + foreach (MemoryHandle handle in handlesToDispose) + { + handle.Dispose(); + } } - // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - long[] fileSegments = new long[buffersCount + 1]; - fileSegments[buffersCount] = 0; + if (segmentsPtr != IntPtr.Zero) + { + NativeMemory.Free((void*)segmentsPtr); + } + } - MemoryHandle[] memoryHandles = new MemoryHandle[buffersCount]; - MemoryHandle pinnedSegments = fileSegments.AsMemory().Pin(); + private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList> buffers, + long fileOffset, CancellationToken cancellationToken) + { + if (!handle.IsAsync) + { + return ScheduleSyncReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); + } - try + if (CanUseScatterGatherWindowsAPIs(handle) + && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { - for (int i = 0; i < buffersCount; i++) - { - Memory buffer = buffers[i]; - MemoryHandle memoryHandle = buffer.Pin(); - memoryHandles[i] = memoryHandle; + return ReadScatterAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); + } - unsafe // awaits can't be in an unsafe context - { - fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64(); - } - } + return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); + } - return await ReadFileScatterAsync(handle, pinnedSegments, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, CancellationToken cancellationToken) + { + try + { + return await ReadFileScatterAsync(handle, segmentsPtr, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } finally { - foreach (MemoryHandle memoryHandle in memoryHandles) - { - memoryHandle.Dispose(); - } - pinnedSegments.Dispose(); + CleanupScatterGatherBuffers(handlesToDispose, segmentsPtr); } } - private static unsafe ValueTask ReadFileScatterAsync(SafeFileHandle handle, MemoryHandle pinnedSegments, int bytesToRead, long fileOffset, CancellationToken cancellationToken) + private static unsafe ValueTask ReadFileScatterAsync(SafeFileHandle handle, IntPtr segmentsPtr, int bytesToRead, long fileOffset, CancellationToken cancellationToken) { handle.EnsureThreadPoolBindingInitialized(); @@ -494,9 +556,9 @@ private static unsafe ValueTask ReadFileScatterAsync(SafeFileHandle handle, try { NativeOverlapped* nativeOverlapped = vts.PrepareForOperation(Memory.Empty, fileOffset); - Debug.Assert(pinnedSegments.Pointer != null); + Debug.Assert(segmentsPtr != IntPtr.Zero); - if (Interop.Kernel32.ReadFileScatter(handle, (long*)pinnedSegments.Pointer, bytesToRead, IntPtr.Zero, nativeOverlapped) == 0) + if (Interop.Kernel32.ReadFileScatter(handle, (long*)segmentsPtr, bytesToRead, IntPtr.Zero, nativeOverlapped) == 0) { // The operation failed, or it's pending. int errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); @@ -562,82 +624,28 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn return ScheduleSyncWriteGatherAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); } - if (CanUseScatterGatherWindowsAPIs(handle)) + if (CanUseScatterGatherWindowsAPIs(handle) + && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { - long totalBytes = 0; - for (int i = 0; i < buffers.Count; i++) - { - totalBytes += buffers[i].Length; - } - - if (totalBytes <= int.MaxValue) // the ReadFileScatter API uses int, not long - { - return WriteGatherAtOffsetSingleSyscallAsync(handle, buffers, fileOffset, (int)totalBytes, cancellationToken); - } + return WriteGatherAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); } return WriteGatherAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); } - private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, CancellationToken cancellationToken) + private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, 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; - } - } - - private static ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) - { - if (buffers.Count == 1) + try { - return WriteAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken); + await WriteFileGatherAsync(handle, segmentsPtr, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } - - return Core(handle, buffers, fileOffset, totalBytes, cancellationToken); - - static async ValueTask Core(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + finally { - // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - int buffersCount = buffers.Count; - long[] fileSegments = new long[buffersCount + 1]; - fileSegments[buffersCount] = 0; - - MemoryHandle[] memoryHandles = new MemoryHandle[buffersCount]; - MemoryHandle pinnedSegments = fileSegments.AsMemory().Pin(); - - try - { - for (int i = 0; i < buffersCount; i++) - { - ReadOnlyMemory buffer = buffers[i]; - MemoryHandle memoryHandle = buffer.Pin(); - memoryHandles[i] = memoryHandle; - - unsafe // awaits can't be in an unsafe context - { - fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64(); - } - } - - await WriteFileGatherAsync(handle, pinnedSegments, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); - } - finally - { - foreach (MemoryHandle memoryHandle in memoryHandles) - { - memoryHandle.Dispose(); - } - pinnedSegments.Dispose(); - } + CleanupScatterGatherBuffers(handlesToDispose, segmentsPtr); } } - private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, MemoryHandle pinnedSegments, int bytesToWrite, long fileOffset, CancellationToken cancellationToken) + private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, IntPtr segmentsPtr, int bytesToWrite, long fileOffset, CancellationToken cancellationToken) { handle.EnsureThreadPoolBindingInitialized(); @@ -645,10 +653,10 @@ private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, Memo try { NativeOverlapped* nativeOverlapped = vts.PrepareForOperation(ReadOnlyMemory.Empty, fileOffset); - Debug.Assert(pinnedSegments.Pointer != null); + Debug.Assert(segmentsPtr != IntPtr.Zero); // Queue an async WriteFile operation. - if (Interop.Kernel32.WriteFileGather(handle, (long*)pinnedSegments.Pointer, bytesToWrite, IntPtr.Zero, nativeOverlapped) == 0) + if (Interop.Kernel32.WriteFileGather(handle, (long*)segmentsPtr, bytesToWrite, IntPtr.Zero, nativeOverlapped) == 0) { // The operation failed, or it's pending. int errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); @@ -679,6 +687,17 @@ private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, Memo return new ValueTask(vts, vts.Version); } + private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, CancellationToken cancellationToken) + { + int buffersCount = buffers.Count; + for (int i = 0; i < buffersCount; i++) + { + ReadOnlyMemory rom = buffers[i]; + await WriteAtOffsetAsync(handle, rom, fileOffset, cancellationToken).ConfigureAwait(false); + fileOffset += rom.Length; + } + } + private static unsafe NativeOverlapped* GetNativeOverlappedForAsyncHandle(ThreadPoolBoundHandle threadPoolBinding, long fileOffset, CallbackResetEvent resetEvent) { // After SafeFileHandle is bound to ThreadPool, we need to use ThreadPoolBinding @@ -750,5 +769,25 @@ internal unsafe void FreeNativeOverlapped(NativeOverlapped* pOverlapped) } } } + + // Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory. + // TODO: Use abstract static methods when they become stable. + private interface IMemoryHandler + { + int GetLength(in T memory); + MemoryHandle Pin(in T memory); + } + + private readonly struct MemoryHandler : IMemoryHandler> + { + public int GetLength(in Memory memory) => memory.Length; + public MemoryHandle Pin(in Memory memory) => memory.Pin(); + } + + private readonly struct ReadOnlyMemoryHandler : IMemoryHandler> + { + public int GetLength(in ReadOnlyMemory memory) => memory.Length; + public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); + } } }