[go] cmd/link: call syscall.FlushFileBuffers on outbuf Unmap
Than McIntosh (Gerrit)
Than McIntosh has uploaded this change for review.
cmd/link: call syscall.FlushFileBuffers on outbuf Unmap
DO NOT SUBMIT
In the windows version of OutBuf.munmap, call syscall.FlushFileBuffers
after the call to syscall.FlushViewOfFile, on the theory that this
will help flush all associated meta-data for the file the linker is
writing.
Note: at the moment doesn't pass bootstrap on windows, failing with
C:\workdir\go\pkg\tool\windows_amd64\link: FlushFileBuffers failed: The handle is invalid.
Not sure what the issue is.
Change-Id: Ibff7d05008a91eeed7634d2760153851e15e1c18
---
M src/cmd/link/internal/ld/outbuf_windows.go
1 file changed, 58 insertions(+), 4 deletions(-)
diff --git a/src/cmd/link/internal/ld/outbuf_windows.go b/src/cmd/link/internal/ld/outbuf_windows.go
index 915c72b..3023715 100644
--- a/src/cmd/link/internal/ld/outbuf_windows.go
+++ b/src/cmd/link/internal/ld/outbuf_windows.go
@@ -5,11 +5,16 @@
package ld
import (
+ "fmt"
"reflect"
+ "sync"
"syscall"
"unsafe"
)
+var addrToHandleLock sync.Mutex
+var addrToHandle = map[uintptr]syscall.Handle{}
+
// Mmap maps the output file with the given size. It unmaps the old mapping
// if it is already mapped. It also flushes any in-heap data to the new
// mapping.
@@ -29,12 +34,20 @@
if err != nil {
return err
}
- defer syscall.CloseHandle(fmap)
-
ptr, err := syscall.MapViewOfFile(fmap, syscall.FILE_MAP_READ|syscall.FILE_MAP_WRITE, 0, 0, uintptr(filesize))
if err != nil {
return err
}
+
+ var addr uintptr
+ addr = ptr
+ addrToHandleLock.Lock()
+ if _, ok := addrToHandle[addr]; ok {
+ return fmt.Errorf("unexpected existing mapping %x in Mmap", addr)
+ }
+ addrToHandle[addr] = fmap
+ addrToHandleLock.Unlock()
+
bufHdr := (*reflect.SliceHeader)(unsafe.Pointer(&out.buf))
bufHdr.Data = ptr
bufHdr.Len = int(filesize)
@@ -55,11 +68,30 @@
}
// Apparently unmapping without flush may cause ACCESS_DENIED error
// (see issue 38440).
- err := syscall.FlushViewOfFile(uintptr(unsafe.Pointer(&out.buf[0])), 0)
+ addr := uintptr(unsafe.Pointer(&out.buf[0]))
+ err := syscall.FlushViewOfFile(addr, 0)
if err != nil {
Exitf("FlushViewOfFile failed: %v", err)
}
- err = syscall.UnmapViewOfFile(uintptr(unsafe.Pointer(&out.buf[0])))
+
+ addrToHandleLock.Lock()
+ defer addrToHandleLock.Unlock()
+ handle, ok := addrToHandle[addr]
+ if !ok {
+ Exitf("no record of mapping in addrToHandle")
+ }
+ delete(addrToHandle, addr)
+ err = syscall.FlushFileBuffers(handle)
+ if err != nil {
+ Exitf("FlushFileBuffers failed: %v", err)
+ }
+
+ err = syscall.CloseHandle(syscall.Handle(handle))
+ if err != nil {
+ Exitf("CloseHandle: %v", err)
+ }
+
+ err = syscall.UnmapViewOfFile(addr)
out.buf = nil
if err != nil {
Exitf("UnmapViewOfFile failed: %v", err)
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Than McIntosh uploaded patch set #2 to this change.
cmd/link: call syscall.FlushFileBuffers on outbuf Unmap
In the windows version of OutBuf.munmap, call syscall.FlushFileBuffers
after the call to syscall.FlushViewOfFile, on the theory that this
will help flush all associated meta-data for the file the linker is
writing.
Updates #44817.
Change-Id: Ibff7d05008a91eeed7634d2760153851e15e1c18
---
M src/cmd/link/internal/ld/outbuf_windows.go
1 file changed, 20 insertions(+), 0 deletions(-)
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Russ Cox.
Patch set 2:Run-TryBot +1
1 comment:
Patchset:
TRY=windows-amd64-longtest
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox, Than McIntosh.
2 comments:
Patchset:
Code looks good.
Have you been able to reproduce the failure, and do you know if this CL makes a difference? Thanks.
File src/cmd/link/internal/ld/outbuf_windows.go:
Patch Set #2, Line 62:
err = syscall.FlushFileBuffers(syscall.Handle(out.f.Fd()))Maybe add a comment like the one above (or update that one)?
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Russ Cox.
2 comments:
Patchset:
Code looks good. […]
Well, so far I haven't actually been able to reproduce with goswarm over the last couple of days. This seems like an especially fleeting bug. I'll do some more swarming this coming week to see if I can trigger it.
File src/cmd/link/internal/ld/outbuf_windows.go:
Patch Set #2, Line 62:
err = syscall.FlushFileBuffers(syscall.Handle(out.f.Fd()))Maybe add a comment like the one above (or update that one)?
Done
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Russ Cox, Than McIntosh.
Than McIntosh uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Than McIntosh, TryBot-Result+1 by Gopher Robot
cmd/link: call syscall.FlushFileBuffers on outbuf Unmap
In the windows version of OutBuf.munmap, call syscall.FlushFileBuffers
after the call to syscall.FlushViewOfFile, on the theory that this
will help flush all associated meta-data for the file the linker is
writing.
Updates #44817.
Change-Id: Ibff7d05008a91eeed7634d2760153851e15e1c18
---
M src/cmd/link/internal/ld/outbuf_windows.go
1 file changed, 28 insertions(+), 0 deletions(-)
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox, Than McIntosh.
Patch set 3:Code-Review +2
1 comment:
Patchset:
If you prefer we probably could check this in and watch the dashboard. If it doesn't make any difference we can back it off.
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
If you prefer we probably could check this in and watch the dashboard. […]
Thanks, but I'd rather wait (for now) and see what the swarming turns up.
I also still have concerns about the business of "The caller must have administrative privileges" for the FlushFileBuffers call.
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox, Than McIntosh.
Patch set 3:Code-Review +2
2 comments:
Patchset:
Thanks, but I'd rather wait (for now) and see what the swarming turns up. […]
If you refer to
```
To flush all open files on a volume, call FlushFileBuffers with a handle to the volume. The caller must have administrative privileges.
```at
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-flushfilebuffers
then your situation is different. They are talking about case where they write to the disk directly. See
```
When opening a volume with CreateFile, the lpFileName string should be the following form: \.\x: or \?\Volume{GUID}. Do not use a trailing backslash in the volume name, because that indicates the root directory of a drive.
```at the same help web page.
LGTM.
I agree, we need this change if we read Microsoft documentation.
And regardless I don't see how this can hurt anywhere.
I also found this similar issue
https://github.com/edsrzf/mmap-go/issues/5
Alex
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Patchset:
LGTM. […]
Thanks Alex.
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Than McIntosh submitted this change.
cmd/link: call syscall.FlushFileBuffers on outbuf Unmap
In the windows version of OutBuf.munmap, call syscall.FlushFileBuffers
after the call to syscall.FlushViewOfFile, on the theory that this
will help flush all associated meta-data for the file the linker is
writing.
Updates #44817.
Change-Id: Ibff7d05008a91eeed7634d2760153851e15e1c18
Reviewed-on: https://go-review.googlesource.com/c/go/+/406814
Reviewed-by: Cherry Mui <cher...@google.com>
Reviewed-by: Alex Brainman <alex.b...@gmail.com>
---
M src/cmd/link/internal/ld/outbuf_windows.go
1 file changed, 31 insertions(+), 0 deletions(-)
diff --git a/src/cmd/link/internal/ld/outbuf_windows.go b/src/cmd/link/internal/ld/outbuf_windows.go
index 915c72b..a568a17 100644
--- a/src/cmd/link/internal/ld/outbuf_windows.go
+++ b/src/cmd/link/internal/ld/outbuf_windows.go
@@ -59,6 +59,18 @@
if err != nil {
Exitf("FlushViewOfFile failed: %v", err)
}
+ // Issue 44817: apparently the call below may be needed (according
+ // to the Windows docs) in addition to the FlushViewOfFile call
+ // above, " ... to flush all the dirty pages plus the metadata for
+ // the file and ensure that they are physically written to disk".
+ // Windows DOC links:
+ //
+ // https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-flushviewoffile
+ // https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-flushfilebuffers
+ err = syscall.FlushFileBuffers(syscall.Handle(out.f.Fd()))
+ if err != nil {
+ Exitf("FlushFileBuffers failed: %v", err)
+ }
err = syscall.UnmapViewOfFile(uintptr(unsafe.Pointer(&out.buf[0])))
out.buf = nil
if err != nil {
To view, visit change 406814. To unsubscribe, or for help writing mail filters, visit settings.