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.

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

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

Open
aschmahmann opened this issue Nov 4, 2019 · 19 comments · May be fixed by #39245
Open

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

aschmahmann opened this issue Nov 4, 2019 · 19 comments · May be fixed by #39245

Comments

@aschmahmann
Copy link

@aschmahmann aschmahmann commented Nov 4, 2019

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

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

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:

  • Adding a check for O_SYNC in syscall.Open in syscall_windows.go
  • Having syscall.Open perform on Windows the same way it does on Linux, by passing in file flags directly
    • Note: This solution would also help with #34681
@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

@networkimprov networkimprov commented Nov 5, 2019

@gopherbot gopherbot added the OS-Windows label Nov 5, 2019
@FiloSottile FiloSottile added this to the Go1.15 milestone Nov 5, 2019
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 14, 2019

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

@tsellers-r7 tsellers-r7 commented Nov 15, 2019

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

@aschmahmann aschmahmann commented Nov 15, 2019

@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

@alexbrainman alexbrainman commented Nov 15, 2019

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 aschmahmann commented Nov 15, 2019

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

@networkimprov networkimprov commented Nov 15, 2019

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

@ianlancetaylor ianlancetaylor commented Nov 15, 2019

@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

@networkimprov networkimprov commented Nov 15, 2019

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

@zx2c4
Copy link
Contributor

@zx2c4 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

@networkimprov networkimprov commented Nov 22, 2019

@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

@ianlancetaylor ianlancetaylor commented Nov 23, 2019

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.

@networkimprov
Copy link

@networkimprov networkimprov commented Dec 11, 2019

@gopherbot add "help wanted"

@jarifibrahim
Copy link

@jarifibrahim jarifibrahim commented May 1, 2020

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?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 1, 2020

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.)

jarifibrahim added a commit to jarifibrahim/go that referenced this issue May 25, 2020
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
@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 30, 2020

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 label May 30, 2020
@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 30, 2020
@dmitshur dmitshur added the NeedsFix label Jun 2, 2020
@networkimprov
Copy link

@networkimprov networkimprov commented Jul 2, 2020

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
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

@jarifibrahim jarifibrahim commented Jul 2, 2020

@networkimprov Thank you. I've fixed it.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 2, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.