Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

os: O_SYNC not utilized in os.OpenFile() on Windows #35358

Closed
aschmahmann opened this issue Nov 4, 2019 · 23 comments
Closed

os: O_SYNC not utilized in os.OpenFile() on Windows #35358

aschmahmann opened this issue Nov 4, 2019 · 23 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Backlog

Comments

@aschmahmann
Copy link

While Linux systems are able to pass their low level flags directly into os.OpenFile on Windows os.OpenFile takes "invented values"

go/src/syscall/types_windows.go

Lines 34 to 48 in 5d1a951

const (
// Invented values to support what package os expects.
O_RDONLY = 0x00000
O_WRONLY = 0x00001
O_RDWR = 0x00002
O_CREAT = 0x00040
O_EXCL = 0x00080
O_NOCTTY = 0x00100
O_TRUNC = 0x00200
O_NONBLOCK = 0x00800
O_APPEND = 0x00400
O_SYNC = 0x01000
O_ASYNC = 0x02000
O_CLOEXEC = 0x80000
)

These invented values are then checked against a subset of the features that Windows actually supports in syscall.Open

go/src/syscall/syscall_windows.go

Line 273 in 03aca99

func Open(path string, mode int, perm uint32) (fd Handle, err error) {

Despite Windows supporting FILE_FLAG_WRITE_THROUGH (very close to O_SYNC on Linux) the Open function does not check for O_SYNC. This is unexpected behavior as developers would expect the O_SYNC flag on Windows to work since it can perform synchronous writes.

Some ways to resolve this unexpected behavior include:
@aschmahmann aschmahmann changed the title os: O_SYNC not utilized in os.OpenFile() os: O_SYNC not utilized in os.OpenFile() on Windows Nov 4, 2019
@networkimprov
Copy link

cc @alexbrainman @zx2c4 @mattn
@gopherbot add OS-Windows

@FiloSottile FiloSottile added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 5, 2019
@FiloSottile FiloSottile added this to the Go1.15 milestone Nov 5, 2019
@alexbrainman
Copy link
Member

Thank you for creating this issue.

I am not an expert here. But, I am not against mapping syscall.O_SYNC onto FILE_FLAG_WRITE_THROUGH.

Alex

@tsellers-r7
Copy link

Are there data loss concerns related to this issue. For example, expecting O_SYNC force writes to disk before returning but instead the OS is caching the writes?

@aschmahmann
Copy link
Author

@tsellers-r7 my understanding is the answer is "sort of".

go/src/os/file.go

Lines 68 to 81 in 03aca99

// Flags to OpenFile wrapping those of the underlying system. Not all
// flags may be implemented on a given system.
const (
// Exactly one of O_RDONLY, O_WRONLY, or O_RDWR must be specified.
O_RDONLY int = syscall.O_RDONLY // open the file read-only.
O_WRONLY int = syscall.O_WRONLY // open the file write-only.
O_RDWR int = syscall.O_RDWR // open the file read-write.
// The remaining values may be or'ed in to control behavior.
O_APPEND int = syscall.O_APPEND // append data to the file when writing.
O_CREATE int = syscall.O_CREAT // create a new file if none exists.
O_EXCL int = syscall.O_EXCL // used with O_CREATE, file must not exist.
O_SYNC int = syscall.O_SYNC // open for synchronous I/O.
O_TRUNC int = syscall.O_TRUNC // truncate regular writable file when opened.
)

The comment here states that not all flags are passed/utilized on every platform. However, if you assumed that O_SYNC worked on your platform (which wouldn't be unreasonable on Windows given the existence of FILE_FLAG_WRITE_THROUGH) and turned out to be incorrect you could experience unexpected data loss (as in dgraph-io/badger#1084).

@alexbrainman
Copy link
Member

However, if you assumed that O_SYNC worked on your platform (which wouldn't be unreasonable on Windows given the existence of FILE_FLAG_WRITE_THROUGH) and turned out to be incorrect you could experience unexpected data loss (as in dgraph-io/badger#1084).

I don't see any promise of data loss prevention in O_SYNC documentation.

It says

O_SYNC int = syscall.O_SYNC // open for synchronous I/O.

whatever that means.

Alex

@aschmahmann
Copy link
Author

aschmahmann commented Nov 15, 2019 •
edited

Fair enough, I guess I was referring to the Linux O_SYNC documentation http://man7.org/linux/man-pages/man2/open.2.html.

@alexbrainman you're absolutely correct that there aren't any Go contracts/documentation currently being violated. However, if a user isn't really sure what O_SYNC's synchronous I/O means they'll probably reach for something similar, like the link above.

@networkimprov
Copy link

The point of O_SYNC or write-through is to leave the file cache untouched, usually because the file to be written should not be added to the cache, for example in a backup scenario.

@ianlancetaylor
Copy link
Contributor

@networkimprov That sounds like like O_DIRECT. O_SYNC means that writes will not return until the data has been sent to the underlying hardware. (Although modern smart disks will themselves buffer the data, so even that is not necessarily a guarantee.)

@networkimprov
Copy link

Oh, yes, O_SYNC makes write() become write() + fsync().

@networkimprov networkimprov mentioned this issue Nov 20, 2019
Closed
@zx2c4
Copy link
Contributor

zx2c4 commented Nov 21, 2019

We have:

nix:

  • O_SYNC
  • O_DIRECT

win:

  • FILE_FLAG_NO_BUFFERING
  • FILE_FLAG_WRITE_THROUGH

Documentation on FILE_FLAG_NO_BUFFERING: https://docs.microsoft.com/en-us/windows/win32/fileio/file-buffering
Documentation on FILE_FLAG_WRITE_THROUGH: https://docs.microsoft.com/en-us/windows/win32/fileio/file-caching

Quick perusal makes me think that the closet approximation is actually O_SYNC=FILE_FLAG_WRITE_THROUGH, O_DIRECT=FILE_FLAG_NO_BUFFERING. Does this correspond with other folks' reading too?

Note that we'll probably never match the exact semantics, but we can find something that's reasonably close, I wouldn't mind making such a mapping in Go.

@networkimprov
Copy link

@ianlancetaylor is there a reason O_DIRECT wasn't included in pkg os?
As it happens, I now need it for a directory backup feature.

@ianlancetaylor
Copy link
Contributor

The os package is intended to be portable across operating systems, and not even all Unix-like systems support O_DIRECT. On systems that do support it you can use syscall.O_DIRECT with os.OpenFile.

@Stebalien Stebalien mentioned this issue Dec 1, 2019
Open
14 tasks
@networkimprov
Copy link

@gopherbot add "help wanted"

@jarifibrahim
Copy link

It should be easy to add the correct flags in the Open(..) syscall. @networkimprov @ianlancetaylor should I send a PR or is there a decision pending?

@jarifibrahim jarifibrahim mentioned this issue May 1, 2020
Closed
@ianlancetaylor
Copy link
Contributor

You can send a pull request. It should only change the syscall package to handle O_SYNC on Windows. Thanks.

(Note that we just entered a release freeze so any change will be for the 1.16 release.)

@bonedaddy bonedaddy mentioned this issue May 1, 2020
Merged
jarifibrahim added a commit to jarifibrahim/go that referenced this issue May 25, 2020
@jarifibrahim
Support O_SYNC flag for os.Open on windows
d8c49d6 
The current implementation of `os.Open` function does not use the O_SYNC
flag. This means that even if user has set the O_SYNC flag, the
`os.Open` call will ignore the `SYNC` flag. This commit fixes the issue
by adding the `FILE_FLAG_WRITE_THROUGH` which is the equivalent of
`O_SYNC` flag on linux.

Fixes golang#35358
@jarifibrahim jarifibrahim mentioned this issue May 25, 2020
Closed
@odeke-em
Copy link
Member

Thank you everyone, moving to Go1.16 as per Ian's last comment #35358 (comment) and given that the CL was sent 5 days ago and almost a month in the freeze. I've reviewed and posted feedback on the change though, so in August 2020 we'll hopefully merge it in.

@odeke-em odeke-em removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 30, 2020
@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 30, 2020
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 2, 2020
@networkimprov
Copy link

The CL doesn't appear on this issue because the Fixes line in the comment should be #35358, not a URL.

jarifibrahim added a commit to jarifibrahim/go that referenced this issue Jul 2, 2020
@jarifibrahim
Support O_SYNC flag for os.Open on windows
b0e0ab8 
The current implementation of `os.Open` function does not use the O_SYNC
flag. This means that even if user has set the O_SYNC flag, the
`os.Open` call will ignore the `SYNC` flag. This commit fixes the issue
by adding the `FILE_FLAG_WRITE_THROUGH` which is the equivalent of
`O_SYNC` flag on linux.

Fixes golang#35358
@jarifibrahim
Copy link

@networkimprov Thank you. I've fixed it.

@gopherbot
Copy link

Change https://golang.org/cl/235198 mentions this issue: syscall: support O_SYNC flag for os.OpenFile on windows

@odeke-em
Copy link
Member

Thank you @jarifibrahim, much appreciated CL. Unfortunately we didn’t land it for Go1.16, but we are definitely close to finishing it. Please take a look at @alexbrainman’s comments for the benchmark and tests, so that we can land it for Go1.17.

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 25, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Go1.17, Backlog Apr 21, 2021
@maddie
Copy link

maddie commented Oct 9, 2021

Is this change going to land anytime soon in a later release?

@ianlancetaylor
Copy link
Contributor

There are pending comments on the change (https://golang.org/cl/235198). Ideally the original author will address them, or someone else can create a new pull request.

@codefromthecrypt codefromthecrypt mentioned this issue Jan 13, 2023
Merged
@callthingsoff callthingsoff mentioned this issue Nov 9, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/541015 mentions this issue: syscall: support O_SYNC flag for os.OpenFile on windows

qiulaidongfeng pushed a commit to qiulaidongfeng/go that referenced this issue Nov 18, 2023
@callthingsoff @LetsUseGerrit
syscall: support O_SYNC flag for os.OpenFile on windows
68b555b 
os.OpenFile on windows did not use the O_SYNC flag. This meant
that even if the user set O_SYNC, os.OpenFile would ignore it.

This change adds a new flag FILE_FLAG_WRITE_THROUGH, which is
the equivalent of O_SYNC flag on Linux and is documented in
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea

Fixes golang#35358

Change-Id: Ib338caed5bb2f215723bfe30a2551a83998d92c9
GitHub-Last-Rev: 82c6275
GitHub-Pull-Request: golang#64027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Backlog