From a2a6bcde0715edff78253efdd0e704ae69e7e405 Mon Sep 17 00:00:00 2001 From: Erik Mavrinac Date: Mon, 4 Nov 2024 09:31:10 -0800 Subject: [PATCH 1/4] Loosen volume search logic to simply ignore volumes that have errors. This is in reaction to continued updates to the list of error codes to ignore, causing a high error rate from build machines. --- lib/Windows/NativeMethods.cs | 2 +- lib/Windows/VolumeInfo.cs | 2 +- lib/Windows/VolumeInfoCache.cs | 34 +++++++++------------------------- 3 files changed, 11 insertions(+), 27 deletions(-) diff --git a/lib/Windows/NativeMethods.cs b/lib/Windows/NativeMethods.cs index 8f3ced5..b533235 100644 --- a/lib/Windows/NativeMethods.cs +++ b/lib/Windows/NativeMethods.cs @@ -114,7 +114,7 @@ public struct BY_HANDLE_FILE_INFORMATION public uint FileIndexHigh; public uint FileIndexLow; - public long FileSize => ((long)FileSizeHigh << 32) | FileSizeLow; + public readonly long FileSize => ((long)FileSizeHigh << 32) | FileSizeLow; } [DllImport("Kernel32.dll", SetLastError = true, CharSet = CharSet.Auto)] diff --git a/lib/Windows/VolumeInfo.cs b/lib/Windows/VolumeInfo.cs index 80e5dbe..b29f725 100644 --- a/lib/Windows/VolumeInfo.cs +++ b/lib/Windows/VolumeInfo.cs @@ -3,7 +3,7 @@ namespace Microsoft.CopyOnWrite.Windows; -internal class VolumeInfo +internal sealed class VolumeInfo { public VolumeInfo(string primaryDriveLetterRoot, string volumeName, bool supportsCoW, long clusterSize) { diff --git a/lib/Windows/VolumeInfoCache.cs b/lib/Windows/VolumeInfoCache.cs index 1af8d26..c930faf 100644 --- a/lib/Windows/VolumeInfoCache.cs +++ b/lib/Windows/VolumeInfoCache.cs @@ -45,7 +45,7 @@ internal VolumeInfoCache(IList<(VolumePaths, VolumeInfo?)> volumesAndMountedPath _driveLetterSubPathsSortedInReverseOrder = new SubPathAndVolume[26][]; for (int i = 0; i < _driveLetterSubPathsSortedInReverseOrder.Length; i++) { - _driveLetterSubPathsSortedInReverseOrder[i] = Array.Empty(); + _driveLetterSubPathsSortedInReverseOrder[i] = []; } foreach ((VolumePaths volumePaths, VolumeInfo? volumeInfo) in volumesAndMountedPaths) @@ -65,7 +65,7 @@ internal VolumeInfoCache(IList<(VolumePaths, VolumeInfo?)> volumesAndMountedPath // Typical case is for one volume to be mounted at one drive letter root path. if (existingPaths.Length == 0) { - _driveLetterSubPathsSortedInReverseOrder[letterIndex] = new[] { newRef }; + _driveLetterSubPathsSortedInReverseOrder[letterIndex] = [newRef]; } else { @@ -103,11 +103,6 @@ public VolumeInfo GetVolumeForPath(string path) "If the drive was added recently you may need to recreate the filesystem cache."); } - private const int ERROR_NOT_READY = 21; - private const int ERROR_INVALID_PARAMETER = 87; - private const int ERROR_UNRECOGNIZED_VOLUME = 1005; - private const int FVE_E_LOCKED_VOLUME = unchecked((int)0x80310000); - private static VolumeInfo? GetVolumeInfo(VolumePaths volumePaths) { bool result = NativeMethods.GetVolumeInformation( @@ -121,29 +116,18 @@ public VolumeInfo GetVolumeForPath(string path) 0); if (!result) { - int lastErr = Marshal.GetLastWin32Error(); - - // Ignore: + // Ignore volumes that have errors. + // Caveat: We don't have a callback to inform the caller that we cannot enumerate a volume, + // Which means CoW will simply invisibly not work. However, attempts to list individual + // error codes proved difficult and prone to creating tons of new PRs after various deployments. + // + // Examples of things we are ignoring here: // - Some SD Card readers show a drive letter even when empty. // - BitLocker can have a volume locked. // - Access denied can imply a volume needing escalated privilege to get its metadata, // sometimes indicating a Windows container volume. // - Not found can occur as a timing issue on some machines. - if (lastErr == ERROR_UNRECOGNIZED_VOLUME || - lastErr == ERROR_NOT_READY || - lastErr == ERROR_INVALID_PARAMETER || - lastErr == FVE_E_LOCKED_VOLUME || - lastErr == NativeMethods.ERROR_ACCESS_DENIED || - lastErr == NativeMethods.ERROR_FILE_NOT_FOUND || - lastErr == NativeMethods.ERROR_DEV_NOT_EXIST || - lastErr == NativeMethods.ERROR_NO_SUCH_DEVICE || - lastErr == NativeMethods.STATUS_VHD_INVALID_STATE) - { - return null; - } - - NativeMethods.ThrowSpecificIoException(lastErr, - $"Failed retrieving volume information for {volumePaths.PrimaryDriveRootPath} with winerror {lastErr}"); + return null; } result = NativeMethods.GetDiskFreeSpace( From 4b2c3ecbd153cd9b1c7f2432476e2f18f17a10c4 Mon Sep 17 00:00:00 2001 From: Erik Mavrinac Date: Mon, 4 Nov 2024 09:32:19 -0800 Subject: [PATCH 2/4] step --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8d13ca6..4216163 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ File clones on Windows do not actually allocate space on-drive for the clone. Th [![NuGet version (CopyOnWrite)](https://img.shields.io/nuget/v/CopyOnWrite?style=plastic)](https://www.nuget.org/packages/CopyOnWrite) -* 0.4.2 November 2024: Add STATUS_VHD_INVALID_STATE and ERROR_NO_SUCH_DEVICE handling on getting volume information +* 0.4.2 November 2024: Loosened error handling getting volume information, any volume resulting in an error is ignored. This aids continued failures finding new unique error types based on al lthe various disk subsystems and drivers out there. * 0.4.1 October 2024: Add ERROR_DEV_NOT_EXIST handling on volume enumeration * 0.4.0 October 2024: Remove async versions of `CloneFile` as the implementation did not use overlapped I/O anyway. CoW support is releasing in the Server 2025 and Win11 24H2 wave, built into the `CopyFile` API suite and on by default for Dev Drive and ReFS, so overlapped I/O in this library will never be implemented. Resolves https://github.com/microsoft/CopyOnWrite/issues/50 * 0.3.12 October 2024: Add ERROR_DEV_NOT_EXIST handling on getting free disk space From ef3d3a0c9d8e24d6a05bd4a46551971450a80425 Mon Sep 17 00:00:00 2001 From: Erik Mavrinac Date: Mon, 4 Nov 2024 09:39:37 -0800 Subject: [PATCH 3/4] step --- lib/Windows/VolumeInfoCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Windows/VolumeInfoCache.cs b/lib/Windows/VolumeInfoCache.cs index c930faf..188a194 100644 --- a/lib/Windows/VolumeInfoCache.cs +++ b/lib/Windows/VolumeInfoCache.cs @@ -118,7 +118,7 @@ public VolumeInfo GetVolumeForPath(string path) { // Ignore volumes that have errors. // Caveat: We don't have a callback to inform the caller that we cannot enumerate a volume, - // Which means CoW will simply invisibly not work. However, attempts to list individual + // which means CoW will simply invisibly not work. However, attempts to list individual // error codes proved difficult and prone to creating tons of new PRs after various deployments. // // Examples of things we are ignoring here: From ef92eeee66890b3dbfc32cc23181fc9e5061017f Mon Sep 17 00:00:00 2001 From: Erik Mavrinac Date: Mon, 4 Nov 2024 09:45:13 -0800 Subject: [PATCH 4/4] step --- lib/Windows/VolumeEnumerator.cs | 6 +++--- lib/Windows/VolumeInfoCache.cs | 6 +++--- lib/Windows/WindowsCopyOnWriteFilesystem.cs | 16 ++++++++-------- tests/utils/DisposableTempDirectory.cs | 2 +- tests/utils/WindowsReFsDriveSession.cs | 2 +- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/Windows/VolumeEnumerator.cs b/lib/Windows/VolumeEnumerator.cs index caf447e..ed48d24 100644 --- a/lib/Windows/VolumeEnumerator.cs +++ b/lib/Windows/VolumeEnumerator.cs @@ -86,7 +86,7 @@ public IEnumerable GetVolumesAndVolumePaths() NativeMethods.QueryDosDevice(drive, lpszDosDeviceNames, singleDriveMappingSize); string? mappedToVolume = Marshal.PtrToStringUni(lpszDosDeviceNames); - if (mappedToVolume != null) + if (mappedToVolume is not null) { if (mappedToVolume.StartsWith(@"\??\", StringComparison.Ordinal)) { @@ -219,10 +219,10 @@ private bool GetNextVolume(out string? volumeName) var sb = new StringBuilder(bufferLen); volumeName = null; - if (_findHandle == null) + if (_findHandle is null) { _findHandle = NativeMethods.FindFirstVolume(sb, bufferLen); - if (_findHandle != null && !_findHandle.IsInvalid) + if (_findHandle is not null && !_findHandle.IsInvalid) { volumeName = sb.ToString(); return true; diff --git a/lib/Windows/VolumeInfoCache.cs b/lib/Windows/VolumeInfoCache.cs index 188a194..f06cdc9 100644 --- a/lib/Windows/VolumeInfoCache.cs +++ b/lib/Windows/VolumeInfoCache.cs @@ -82,7 +82,7 @@ internal VolumeInfoCache(IList<(VolumePaths, VolumeInfo?)> volumesAndMountedPath } } - public VolumeInfo GetVolumeForPath(string path) + public VolumeInfo? GetVolumeForPath(string path) { // Look up paths by drive letter to reduce the size of the resulting path array to search. int driveLetterIndex = IndexFromDriveLetter(path[0]); @@ -91,11 +91,11 @@ public VolumeInfo GetVolumeForPath(string path) // Paths are sorted in reverse order to get longer paths ahead of shorter paths for prefix matching. // For cases where volumes are mounted under other volumes, e.g. a D: ReFS drive mounted // under D:\ReFS, we want to match the deeper path. - foreach (SubPathAndVolume spv in subPathsAndVolumes.Where(spv => spv.Volume is not null)) + foreach (SubPathAndVolume spv in subPathsAndVolumes) { if (path.IsSubpathOf(spv.SubPath)) { - return spv.Volume!; + return spv.Volume; } } diff --git a/lib/Windows/WindowsCopyOnWriteFilesystem.cs b/lib/Windows/WindowsCopyOnWriteFilesystem.cs index 2951204..0de225f 100644 --- a/lib/Windows/WindowsCopyOnWriteFilesystem.cs +++ b/lib/Windows/WindowsCopyOnWriteFilesystem.cs @@ -43,9 +43,9 @@ public bool CopyOnWriteLinkSupportedBetweenPaths(string source, string destinati } // Must be in the same volume. - VolumeInfo sourceVolume = _volumeInfoCache.GetVolumeForPath(resolvedSource); - VolumeInfo destVolume = _volumeInfoCache.GetVolumeForPath(resolvedDestination); - if (!ReferenceEquals(sourceVolume, destVolume)) + VolumeInfo? sourceVolume = _volumeInfoCache.GetVolumeForPath(resolvedSource); + VolumeInfo? destVolume = _volumeInfoCache.GetVolumeForPath(resolvedDestination); + if (sourceVolume is null || destVolume is null || !ReferenceEquals(sourceVolume, destVolume)) { return false; } @@ -62,8 +62,8 @@ public bool CopyOnWriteLinkSupportedInDirectoryTree(string rootDirectory, bool p return false; } - VolumeInfo volumeInfo = _volumeInfoCache.GetVolumeForPath(resolvedSource); - return volumeInfo.SupportsCoW; + VolumeInfo? volumeInfo = _volumeInfoCache.GetVolumeForPath(resolvedSource); + return volumeInfo?.SupportsCoW ?? false; } /// @@ -84,10 +84,10 @@ public void CloneFile(string source, string destination, CloneFlags cloneFlags) throw new NotSupportedException($"Source path '{source}' is not in the correct format"); } - VolumeInfo sourceVolume = _volumeInfoCache.GetVolumeForPath(resolvedSource); - if (!sourceVolume.SupportsCoW) + VolumeInfo? sourceVolume = _volumeInfoCache.GetVolumeForPath(resolvedSource); + if (sourceVolume is null || !sourceVolume.SupportsCoW) { - throw new NotSupportedException($@"Drive volume {sourceVolume.PrimaryDriveLetterRoot} does not support copy-on-write clone links, i.e. is not formatted with ReFS"); + throw new NotSupportedException($@"Drive volume {sourceVolume?.PrimaryDriveLetterRoot} does not support copy-on-write clone links, i.e. is not formatted with ReFS"); } // Get an open file handle to the source file. We use FILE_FLAGS_NO_BUFFERING here diff --git a/tests/utils/DisposableTempDirectory.cs b/tests/utils/DisposableTempDirectory.cs index 42ede24..c33ce71 100644 --- a/tests/utils/DisposableTempDirectory.cs +++ b/tests/utils/DisposableTempDirectory.cs @@ -78,7 +78,7 @@ public void Dispose() } // Restore the previous current directory if necessary - if (_previousCurrentDirectory != null) + if (_previousCurrentDirectory is not null) { Environment.CurrentDirectory = _previousCurrentDirectory; } diff --git a/tests/utils/WindowsReFsDriveSession.cs b/tests/utils/WindowsReFsDriveSession.cs index 953d012..1b22580 100644 --- a/tests/utils/WindowsReFsDriveSession.cs +++ b/tests/utils/WindowsReFsDriveSession.cs @@ -105,7 +105,7 @@ public static WindowsReFsDriveSession Create(string relativeTestRootDir, bool fo public void Dispose() { Directory.Delete(Path.GetDirectoryName(TestRootDir)!, recursive: true); - if (_removeVhdScriptPath != null) + if (_removeVhdScriptPath is not null) { RunPowershellScript(_removeVhdScriptPath, $"{_vhdDriveLetter}"); }