windows.DeleteFile: Use FileDispositionInformationEx if possible, but fallback if not #16499
Conversation
… fallback if not Using FileDispositionInformationEx (and therefore flags like FILE_DISPOSITION_POSIX_SEMANTICS and FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE) is only supported on NTFS, so the comptime Windows version range check is not enough to determine whether or not the NtSetInformationFile will succeed. This commit makes DeleteFile always try using FileDispositionInformationEx first, but if INVALID_PARAMETER is received (which is the status that occurs when the filesystem doesn't support FileDispositionInformationEx), then it will fallback and try calling NtSetInformationFile with FileDispositionInformation. This keeps NTFS as fast as it was before, since it will do at most 1 NtSetInformationFile call, but on non-NTFS filesystems (e.g. FAT32), DeleteFile may need to do 2 NtSetInformationFile calls. Closes ziglang#16497
These two tests can't be disambiguated at comptime, since the filesystem that the test is running on also matters for whether or not POSIX_SEMANTICS / IGNORE_READONLY_ATTRIBUTE can actually be used (since they are only supported on NTFS).
There was a problem hiding this comment.
nit: test with file pending semantics does not check, if file has been deleted.
other nit: commit message could mention that windows version < 10 SP1 now use always 2 syscalls instead of 1 as those version dont support posix semantics. so not only fat file systems.
| if (delete_result) { | ||
| try testing.expectError(error.FileNotFound, tmp.dir.deleteFile("test_file")); | ||
| } else |err| { | ||
| try testing.expectEqual(@as(anyerror, error.AccessDenied), err); |
There was a problem hiding this comment.
This does not strictly test that the deleteFile with file pending semantics succeeds (file does not exist anymore).
The file must be closed for the deletion to be successful (and it should be checked).
This is not something either of the previous tests did, but I agree that it might be a nice addition to the test.
This isn't true, check the code again. The version check is still there, so on versions < |
Also add optimization for happy path with early return. No idea, why this worked before.
Also add optimization for happy path with early return. No idea, why this worked before.
Also add optimization for happy path with early return. No idea, why this worked before.
Mitigates #14978. Uses the same strategy as in #16499 suggested by @squeek502
Using
FileDispositionInformationEx(and therefore flags likeFILE_DISPOSITION_POSIX_SEMANTICSandFILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE) is only supported on NTFS, so the comptime Windows version range check is not enough to determine whether or not theNtSetInformationFilewill succeed.This commit makes
DeleteFilealways try usingFileDispositionInformationExfirst, but ifINVALID_PARAMETERis received (which is the status that occurs when the filesystem doesn't supportFileDispositionInformationEx), then it will fallback and try callingNtSetInformationFilewithFileDispositionInformation.This keeps NTFS as fast as it was before, since it will do at most 1 NtSetInformationFile call, but on non-NTFS filesystems (e.g. FAT32), DeleteFile will need to do 2 NtSetInformationFile calls.
Closes #16497
As mentioned in #16497, the alternative would be to use a
NtQueryVolumeInformationFileupfront to check if the filesystem of the file supports posix semantics.I've implemented the alternative here and benchmarked them using the
deleteTreebenchmark from #13073:(fallback is this PR, query is the alternative implementation with
NtQueryVolumeInformationFile; benchmark was run on NTFS)Not a huge/any difference, and I figured the version that keeps it to 1
NtSetInformationFilecall on NTFS would be preferable.