os: File.Sync() for Darwin should use the F_FULLFSYNC fcntl

What version of Go?

1.10 or tip

What operating system and processor architecture?

darwin, *

What is the matter?

According to this and other sites, fcntl(fd, F_FULLFSYNC) is necessary for proper fsync on MacOS (OS X): https://danluu.com/file-consistency/

Github search finds no uses of "F_FULLFSYNC" in the src tree.

If there is a rationale for the present implementation, it should be documented in File.Sync() with directions on how to invoke the fcntl variant.

Since this issue threatens data loss/corruption, please consider fixing it in 1.11.x and 1.10.x

@networkimprov

Thank you for reporting this issue @networkimprov! It is triaged for 1.12 as 1.11 is scheduled to be released in just a few days. It has also been like this since forever. However we'll definitely consider it as an early candidate for Go1.12 when the tree opens up in the next few weeks. If you are interested in submitting a CL, please feel free, it'd be great to have your contribution in the Go project and community, otherwise we'll get it in for Go1.12.

I'm not proposing 1.11.0 as a target; rather 1.11.1 & 1.10.4 or the minor releases that appear after a fix lands on tip.

Do you have guidance on where fcntl should be called for Darwin File.Sync()?

The question is whether we should do it in syscall.Fsync or os.File.Fsync. I guess I would lean slightly toward the former, but happy to hear comments.

@ianlancetaylor

I'm not proposing 1.11.0 as a target; rather 1.11.1 & 1.10.4 or the minor releases that appear after a fix lands on tip.

A backport I believe warrants something that has plagued previous versions or results for memory corruption, but I'll defer to others on whether this qualifies too -- in this case potential file corruption might qualify :)

The question is whether we should do it in syscall.Fsync or os.File.Fsync. I guess I would lean slightly toward the former, but happy to hear comments.

@ianlancetaylor I too was leaning towards it going in to syscall.Fsync since perhaps we can minimally make it

// As per /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk/usr/include/sys/fcntl.h
const F_FULLFSYNC = 51
func Fsync(fd int) error {
     _, err := fcntl(fd, F_FULLFSYNC, 1)
    return err
}

so in full

+// As per /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk/usr/include/sys/fcntl.h
+const F_FULLFSYNC = 51
+
+func Fsync(fd int) error {
+       _, err := fcntl(fd, F_FULLFSYNC, 0)
+       return err
+}
+
 // SockaddrDatalink implements the Sockaddr interface for AF_LINK type sockets.
 type SockaddrDatalink struct {
        Len    uint8
@@ -332,7 +340,6 @@ func Uname(uname *Utsname) error {
 //sys  Fstat(fd int, stat *Stat_t) (err error) = SYS_FSTAT64
 //sys  Fstatat(fd int, path string, stat *Stat_t, flags int) (err error) = SYS_FSTATAT64
 //sys  Fstatfs(fd int, stat *Statfs_t) (err error) = SYS_FSTATFS64
-//sys  Fsync(fd int) (err error)
 //sys  Ftruncate(fd int, length int64) (err error)
 //sys  Getdirentries(fd int, buf []byte, basep *uintptr) (n int, err error) = SYS_GETDIRENTRIES64
 //sys  Getdtablesize() (size int)

Also happy to hear what comments others have.

On the other hand, thinking about it further, most of the syscall functions simply implement that operation on the system, rather than trying to present a unified cross-system API. So that suggests that maybe syscall.Fsync on MacOS should just implement the fsync system call, and that we should make the fcntl call in os.File.Sync, or, really, internal/poll/Fd.Fsync. I'm not sure.

I'm not proposing 1.11.0 as a target; rather 1.11.1 & 1.10.4 ...

A backport I believe warrants something that has plagued previous versions or results for memory corruption, but I'll defer to others on whether this qualifies too -- in this case potential file corruption might qualify :)

Note that this can hurt end-users without the Go developer ever realizing it: User does xyz in app, OS crashes (or hw fails), user sees odd behavior in app after restart and restores from backup (losing today's work), user assumes problem was due to hw or os.

+const FULLFSYNC = 51

It's already defined in tree:
https://github.com/golang/go/search?q=f_fullfsync&unscoped_q=f_fullfsync

Agreed you should leave MacOS fsync available in case anyone needs it.

Perhaps surface MacOS fsync as syscall.FsyncDarwin and implement syscall.Fsync with fcntl?

@networkimprov

Cool, thanks for the discussion @networkimprov and @ianlancetaylor!

@odeke-em

@odeke-em the 1.12 tree is open; can we get this CL posted?

Change https://golang.org/cl/155038 mentions this issue: doc: clarify change to File.Sync on macOS

What do you think of https://golang.org/cl/155038?

(I really think we are spending too much time on this. It's a bug fix.)

@ianlancetaylor

LGTM @ianlancetaylor. Thanks for rewording it.

That would imply that calling File.Sync could in itself cause data loss/corruption

True. Maybe "File.Sync() prior to 1.12 is ineffective on MacOS, and cannot prevent data loss/corruption on system crash/power-off."

It's a bug fix with broad impact among database packages, including etcd bbolt, syndtr goleveldb, dgraph badger, and bleve.

Thanks, I'm going with what I've got.

doc: clarify change to File.Sync on macOS

I'm definitely late to the conversation, but I wanted to document my frustration with the decision of not backporting this fix, and how it's affecting https://github.com/dgraph-io/badger.

We now need to replace all the calls to os.File.Sync to instead call a FileSync function for which we provide two implementations.

Once is very straight forward:

file_sync.go

// +build !darwin go1.12

package y

import "os"

// FileSync calls os.File.Sync with the right parameters.
// This function can be removed once we stop supporting Go 1.11
// on MacOS.
func FileSync(f *os.File) error {return f.Sync()}

The second one for now simply makes the corresponding syscall.

file_sync_darwin.go

// +build darwin,!go1.12

package y

import (
	"os"
	"syscall"
)

func FileSync(f *os.File) error {
	_, _, err := syscall.Syscall(syscall.SYS_FCNTL, f.Fd(), syscall.F_FULLFSYNC, 0)
	if err == 0 {
		return nil
	}
	return err
}

Making this function do the right thing seems to require interacting with syscall package in ways it seems it's not designed to (according to this change). Before calling the syscall.Syscall(syscall.SYS_FCNTL, ...) we need to first call incref which is not exported.

Could you, at least, provide a clear way of how to implement this function correctly?
Is my code correct?
I'm afraid I'll hack something that works but eventually will figure out something is missing.

Thanks

UPDATE: I already found a way my code was wrong, added the check to see whether errno is zero.

@campoy

@campoy I understand your frustration, but there was no good solution here. Making this change has led to multiple bug reports about significant performance changes, changes due to now using the correct behavior. It would have been inappropriate to make a change that led to multiple bug reports in a minor release.

The code you show above looks correct and fine for a file on local disk. I'm not sure there is a fully correct to call fsync for an NFS file in Go 1.11 on Darwin. The incref is not an issue because you are calling the Fd method.

@ianlancetaylor

Thanks for the confirmation, @ianlancetaylor!

I think that just keeping my code in this issue should be enough to help others wondering how to fix it on their own codebases.

The PR fixing this issue for badger is dgraph-io/badger#871.

don't os.File.Sync() on mac CAS items

Navigation Menu

Repository navigation

os: File.Sync() for Darwin should use the F_FULLFSYNC fcntl #26650

What version of Go?

What operating system and processor architecture?

What is the matter?

15 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

os: File.Sync() for Darwin should use the F_FULLFSYNC fcntl #26650

Description

What version of Go?

What operating system and processor architecture?

What is the matter?

Activity

networkimprov commented on Jul 27, 2018

odeke-em commented on Jul 27, 2018

networkimprov commented on Jul 27, 2018

ianlancetaylor commented on Jul 27, 2018

odeke-em commented on Jul 27, 2018

ianlancetaylor commented on Jul 27, 2018

networkimprov commented on Jul 28, 2018

networkimprov commented on Jul 28, 2018

odeke-em commented on Jul 28, 2018

networkimprov commented on Aug 22, 2018

15 remaining items

gopherbot commented on Dec 19, 2018

ianlancetaylor commented on Dec 19, 2018

mark-rushakoff commented on Dec 19, 2018

networkimprov commented on Dec 19, 2018

ianlancetaylor commented on Dec 20, 2018

campoy commented on Jun 17, 2019

ianlancetaylor commented on Jun 19, 2019

campoy commented on Jun 21, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions