[Support] Avoid a VirtualBox shared folders mmap bug #78597
Conversation
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-lld Author: Martin Storsjö (mstorsjo) ChangesThis reverts the functional parts of commit The conditional call to FlushFileBuffers was added to work around a rare kernel bug, see acd8791 and 3b9b4d2. However, closing a memory mapping that has been written to, can also need a call to FlushFileBuffers in other cases. When operating on a VirtualBox Shared Folder file system, the output file ends up unwritten/unflushed (with the right size but all zeros), if written via a memory mapping, then the file is renamed before the output handle is closed (this is what is done in lld-link). The sequence to trigger the issue on the VirtualBox Shared Folders is this, summarized: With this sequence, the output file always ends up with all zeros. See mstorsjo/llvm-mingw#393 (comment) for a full reproduction example. The issue can seem elusive, as we conditionally do the call to FlushFileBuffers() to work around the rare kernel bug, whenever running on versions of Windows older than 10.0.0.17763. This fixes downstream bug Full diff: https://github.com/llvm/llvm-project/pull/78597.diff 1 Files Affected:
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 2bf68b7972e746f..204dc412972d63d 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -936,30 +936,11 @@ mapped_file_region::mapped_file_region(sys::fs::file_t fd, mapmode mode,
copyFrom(mapped_file_region());
}
-static bool hasFlushBufferKernelBug() {
- static bool Ret{GetWindowsOSVersion() < llvm::VersionTuple(10, 0, 0, 17763)};
- return Ret;
-}
-
-static bool isEXE(StringRef Magic) {
- static const char PEMagic[] = {'P', 'E', '\0', '\0'};
- if (Magic.starts_with(StringRef("MZ")) && Magic.size() >= 0x3c + 4) {
- uint32_t off = read32le(Magic.data() + 0x3c);
- // PE/COFF file, either EXE or DLL.
- if (Magic.substr(off).starts_with(StringRef(PEMagic, sizeof(PEMagic))))
- return true;
- }
- return false;
-}
-
void mapped_file_region::unmapImpl() {
if (Mapping) {
-
- bool Exe = isEXE(StringRef((char *)Mapping, Size));
-
::UnmapViewOfFile(Mapping);
- if (Mode == mapmode::readwrite && Exe && hasFlushBufferKernelBug()) {
+ if (Mode == mapmode::readwrite) {
// There is a Windows kernel bug, the exact trigger conditions of which
// are not well understood. When triggered, dirty pages are not properly
// flushed and subsequent process's attempts to read a file can return
@@ -967,6 +948,11 @@ void mapped_file_region::unmapImpl() {
// sufficient to ensure that this bug is not triggered.
// The bug only occurs when writing an executable and executing it right
// after, under high I/O pressure.
+ //
+ // Separately, on VirtualBox Shared Folder mounts, writes via memory maps
+ // always end up unflushed (regardless of version of Windows, unless flushed
+ // with this explicit call, if they are renamed with
+ // SetFileInformationByHandle(FileRenameInfo) before closing the output handle.
::FlushFileBuffers(FileHandle);
}
|
|
Also CC @cjacek who might have some more insights into the internal workings of these Windows functions and their interactions. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@mstorsjo Are you able to test linking a large PDB by chance? 4GB+ PDB are becoming quite common nowadays, and since the PDB isn’t needed just after link, the previous optimization I did was saving several seconds of link time. Corectness is needed before performance, but this PR would impact cases which don’t need the flush (I assume most of LLD usages on Windows). Can we rather detect problematic cases on LLD startup, like writing to a virtual/network drive, and only disable the optimization there? |
|
In general, I think that if a memory map is involved, then it makes sense to flush the data. I don't think that the default behaviour is to do write through for performance reasons. |
I don't have any such scenarios available to test off the bat at least. To understand - what bit is it that takes the extra time in that case? Does
I'm not sure how we would detect those problematic cases. We could use the same heuristic that we use elsewhere, |
|
I think that instead of |
|
I'm pretty hesitant about the idea of flushing files to disk on shutdown. My understanding is that the vast majority of software improvements to filesystem performance are attributable to caching data in memory and deferring disk writes. I can't find a quotation, but I believe I picked this up from Ted T'so somewhere (maybe here), but reading that again, I wonder if he's trying to make the opposite point. Maybe the way forward here is to land the flush operation under a flag so folks can measure performance in different scenarios, since it doesn't seem reasonable to put the burden of proving there is no performance regression on Martin. |
|
What would be the right way to test this? We produce PDB files bigger than 4GB so it's not a huge deal for me to test. |
Try |
We had the same discussion on this. I'd prefer auto-detection of the problematic target drive because new flags are not easily discoverable by users and we will end with this same bug report. But if that's not possible we can also do a flag. In that case what about making things correct by default and a flag for disabling the flush? |
I agree that the flag is not a long term solution for the users affected by the virtualbox issue. It would be a way to commit the code change, allow unaffected users to measure the cost of additional flushing with low effort (flag flips instead of local patches), and then we can refine the auto-detection logic for which devices require the flush, or decide how important it is to have a narrow criteria. |
If you can do that, and also measure the performance impact of this change (on top of this one), that'd be great: diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 204dc412972d..9c75fafc5fdf 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -938,6 +938,7 @@ mapped_file_region::mapped_file_region(sys::fs::file_t fd, mapmode mode,
void mapped_file_region::unmapImpl() {
if (Mapping) {
+ ::FlushViewOfFile(Mapping, Size);
::UnmapViewOfFile(Mapping);
if (Mode == mapmode::readwrite) {
@@ -953,7 +954,7 @@ void mapped_file_region::unmapImpl() {
// always end up unflushed (regardless of version of Windows, unless flushed
// with this explicit call, if they are renamed with
// SetFileInformationByHandle(FileRenameInfo) before closing the output handle.
- ::FlushFileBuffers(FileHandle);
+// ::FlushFileBuffers(FileHandle);
}
::CloseHandle(FileHandle); |
I took a shot at measuring this now after all. I don't have any case where I build binaries that big, but in building a statically linked The difference in speed is indeed notable. Originally (with no flush calls, i.e. running on a new enough version of Windows), linking takes 4.2 seconds. By always calling With a breakdown with So this is clearly a regression wrt performance for these cases, while it improves correctness. I'll try to do a PoC for hooking up heuristics via the |
In acd8791, a call to FlushFileBuffers was added to work around a rare kernel bug. In 3b9b4d2, the scope of that workaround was limited, for performance reasons, as the flushes are quite expensive. On VirtualBox shared folders, closing a memory mapping that has been written to, also needs to be explicitly flushed, if renaming the output file before it is closed. Contrary to the kernel bug, this always happens on such mounts. In these cases, the output ends up as a file of the right size, but the contents are all zeros. The sequence to trigger the issue on the VirtualBox Shared Folders is this, summarized: file = CreateFile() mapping = CreateFileMapping(file) mem = MapViewOfFile() CloseHandle(mapping) write(mem) UnmapViewOfFile(mem) SetFileInformationByHandle(file, FileRenameInfo) CloseHandle(file) With this sequence, the output file always ends up with all zeros. See mstorsjo/llvm-mingw#393 for a full reproduction example. To avoid this issue, call FlushFileBuffers() when the file may reside on a VitualBox shared folder. As the flushes are expensive, only do them when the output isn't on a local file system. The issue with VirtualBox shared folders could also be fixed by calling FlushViewOfFile before UnmapViewOfFile, and doing that could be slightly less expensive than FlushFileBuffers. Empirically, the difference between the two is very small though, and as it's not easy to verify whether switching FlushFileBuffers to FlushViewOfFile helps with the rare kernel bug, keep using FlushFileBuffers for both cases, for code simplicity. This fixes downstream bug mstorsjo/llvm-mingw#393.
mstorsjo
changed the title
|
I updated the patch to call For the rare kernel bug case, we don't know if calling This keeps the usual much faster performance for writing large PDB files on normal storage, while fixing the correctness in the VirtualBox case. |
@aganea Does this seem like a reasonable compromise, performance wise? |
|
I think flushing network output files is actually reasonable behavior in the general case. For local files, it seems reasonable to trust the kernel to flush the file contents to disk "soon", but network filesystems are less reliable, and it is probably better to wait for the bytes to get pushed out over the network. |
Yes, thank you for doing this! I would assume the |
It shouldn't affect the timings notably, no. In https://reviews.llvm.org/D155579 we concluded that |
In acd8791, a call to FlushFileBuffers was added to work around a rare kernel bug. In 3b9b4d2, the scope of that workaround was limited, for performance reasons, as the flushes are quite expensive.
On VirtualBox shared folders, closing a memory mapping that has been written to, also needs to be explicitly flushed, if renaming the output file before it is closed. Contrary to the kernel bug, this always happens on such mounts. In these cases, the output ends up as a file of the right size, but the contents are all zeros.
The sequence to trigger the issue on the VirtualBox Shared Folders is this, summarized:
With this sequence, the output file always ends up with all zeros. See mstorsjo/llvm-mingw#393 for a full reproduction example.
To avoid this issue, call FlushFileBuffers() when the file may reside on a VitualBox shared folder. As the flushes are expensive, only do them when the output isn't on a local file system.
The issue with VirtualBox shared folders could also be fixed by calling FlushViewOfFile before UnmapViewOfFile, and doing that could be slightly less expensive than FlushFileBuffers.
Empirically, the difference between the two is very small though, and as it's not easy to verify whether switching FlushFileBuffers to FlushViewOfFile helps with the rare kernel bug, keep using FlushFileBuffers for both cases, for code simplicity.
This fixes downstream bug mstorsjo/llvm-mingw#393.