code review 3887042: os: add Sync to *File, wraps syscall.Fsync

os: add Sync to *File, wraps syscall.Fsync

[adg, 2011-01-10 00:19:48]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[adg, 2011-01-10 00:31:02]

On further consideration, maybe Sync is a better name for this method.
Go types that implement Flush tend to be memory-buffered, whereas this
is about blocking for hardware synchronization.

On 10 January 2011 11:19,  <adg@golang.org> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> os: add Flush to *File, calls fsync
>
> Please review this at http://codereview.appspot.com/3887042/
>
> Affected files:
>  M src/pkg/os/file_unix.go
>
>
> Index: src/pkg/os/file_unix.go
> ===================================================================
> --- a/src/pkg/os/file_unix.go
> +++ b/src/pkg/os/file_unix.go
> @@ -94,6 +94,22 @@
>        return
>  }
>
> +// Flush transfers all modified file data to the storage device where that
> +// file resides. Flush should block until the device reports that the
> transfer
> +// has completed.
> +//
> +// See your operating system's fsync documentation for more details.
> +func (file *File) Flush() (err Error) {
> +       if file == nil {
> +               return EINVAL
> +       }
> +       e := syscall.Fsync(file.fd)
> +       if e != 0 {
> +               return NewSyscallError("fsync", e)
> +       }
> +       return nil
> +}
> +
>  // Truncate changes the size of the named file.
>  // If the file is a symbolic link, it changes the size of the link's
> target.
>  func Truncate(name string, size int64) Error {
>
>
>

[brainman, 2011-01-10 01:31:37]

http://codereview.appspot.com/3887042/diff/1/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

http://codereview.appspot.com/3887042/diff/1/src/pkg/os/file_unix.go#newcode102
src/pkg/os/file_unix.go:102: func (file *File) Flush() (err Error) {
You might as well put in the common file (file.go), just add this:

func Fsync(fd int) (errno int) {
	// TODO(brainman): use FlushFileBuffers Windows api to implement it.
	return EWINDOWS
}

to syscall/syscall_windows.go.

Thank you.

[adg, 2011-01-10 02:23:13]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-01-10 02:55:00]

Sorry, I didn't make myself clear. You also need to add windows version of
syscall.Fsync() - please add this:

func Fsync(fd int) (errno int) {
  // TODO(brainman): use FlushFileBuffers Windows api to implement it.
  return EWINDOWS
}

to syscall/syscall_windows.go. Thank you.

Alex

http://codereview.appspot.com/3887042/diff/6001/src/pkg/os/file.go
File src/pkg/os/file.go (right):

http://codereview.appspot.com/3887042/diff/6001/src/pkg/os/file.go#newcode416
src/pkg/os/file.go:416: func (file *File) Sync() (err Error) {
I like Flush() better, but it just me.

[adg, 2011-01-10 03:11:44]

On 10 January 2011 13:55,  <alex.brainman@gmail.com> wrote:
> Sorry, I didn't make myself clear. You also need to add windows version

Ah, apologies. I thought it already existed.

> I like Flush() better, but it just me.

I'm in two minds about it, with no strong feelings either way.

Andrew

[brainman, 2011-01-10 03:15:50]

LGTM. Thank you.

[r, 2011-01-10 06:24:27]

http://codereview.appspot.com/3887042/diff/12001/src/pkg/os/file.go
File src/pkg/os/file.go (right):

http://codereview.appspot.com/3887042/diff/12001/src/pkg/os/file.go#newcode415
src/pkg/os/file.go:415: // See your operating system's fsync documentation for
more details.
i'm not very happy about this. the os package, which is supposed to capture
portable behavior, is acquiring a workaround for a bug in unix.

if i get outvoted, at least:
1) explain what it is. the comment above is bizarre.  what modified file data?
what storage device?  i'm writing on a pipe.
2) "Sync should block": why single out sync's blocking?
3) "should block": well, does it? if so, no need to say. if not, fix it.
4) if you're doing fsync, you need to provide sync. but...
5) my operating system might not have an fsync - what then?

i believe this is just the sort of thing package syscall is for. however, i
expect to be argued with

[adg, 2011-01-10 06:41:50]

On 10 January 2011 17:24,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/3887042/diff/12001/src/pkg/os/file.go
> File src/pkg/os/file.go (right):
>
> http://codereview.appspot.com/3887042/diff/12001/src/pkg/os/file.go#newcode415
> src/pkg/os/file.go:415: // See your operating system's fsync
> documentation for more details.
> i'm not very happy about this. the os package, which is supposed to
> capture portable behavior, is acquiring a workaround for a bug in unix.
>
> if i get outvoted, at least:
> 1) explain what it is. the comment above is bizarre.  what modified file
> data? what storage device?  i'm writing on a pipe.
> 2) "Sync should block": why single out sync's blocking?
> 3) "should block": well, does it? if so, no need to say. if not, fix it.

Agree the comment should be better. I based it on the preamble to
linux's fsync manpage. (A bad idea, in general.)

Unfortunately, "should" is deliberate here. Fsync does behave weirdly
on different platforms, as there are many layers between the fsync
call and the bits being written to disk. I believe that
[controversially] some platforms will return from an fsync without
having actually written the bits to disk. My opinion is that we should
give programmers the option to request an fsync, no matter how it
works underneath.

A better comment might be:

// Flush requests that the operating system flush any unwritten data
to the underlying block device.

> 4) if you're doing fsync, you need to provide sync. but...
> 5) my operating system might not have an fsync - what then?
>
> i believe this is just the sort of thing package syscall is for.
> however, i expect to be argued with

That there is an equivalent function in Windows is an argument for its
inclusion here.

Andrew

[rsc, 2011-01-11 15:43:44]

Please make the change Alex suggested.

http://codereview.appspot.com/3887042/diff/12001/src/pkg/os/file.go
File src/pkg/os/file.go (right):

http://codereview.appspot.com/3887042/diff/12001/src/pkg/os/file.go#newcode415
src/pkg/os/file.go:415: // See your operating system's fsync documentation for
more details.
On 2011/01/10 06:24:27, r wrote:
> i'm not very happy about this. the os package, which is supposed to capture
> portable behavior, is acquiring a workaround for a bug in unix.
> 
> if i get outvoted, at least:
> 1) explain what it is. the comment above is bizarre.  what modified file data?
> what storage device?  i'm writing on a pipe.
> 2) "Sync should block": why single out sync's blocking?
> 3) "should block": well, does it? if so, no need to say. if not, fix it.
> 4) if you're doing fsync, you need to provide sync. but...
> 5) my operating system might not have an fsync - what then?
> 
> i believe this is just the sort of thing package syscall is for. however, i
> expect to be argued with

Syscall is okay if you want to tie yourself to a particular OS.
Each OS has a slightly different struct stat, and yet package os
presents a common interface to it across systems.
I think of it this way:
Package syscall is the interface that the kernel provides.
Package os is the interface we want Go to provide.

I am not aware of any mainstream operating systems that don't have fsync.
Even Plan 9 has fsync (nulldir(&d); dirfwstat(fd, &d)).

The comment is admittedly awkward: it reads like an interface spec
instead of a description of an implementation.  I suggest:

// Sync commits the current contents of the file to stable storage.
// Typically, this means flushing the file system's in-memory copy
// of recently written data to disk.

[rsc, 2011-01-11 15:44:10]

Never mind, I see you made that change.

LGTM but wait for r.

[r2, 2011-01-11 19:03:27]

i like rsc's comment better. please also drop the reference to the other
documentation we're not responsible for

[adg, 2011-01-12 00:02:31]

On 12 January 2011 06:02, Rob 'Commander' Pike <r@google.com> wrote:
> i like rsc's comment better. please also drop the reference to the other
documentation we're not responsible for

Okay. So are we satisfied by the choice of Sync over Flush?

[r2, 2011-01-12 00:05:47]

On Jan 11, 2011, at 4:01 PM, Andrew Gerrand wrote:

> On 12 January 2011 06:02, Rob 'Commander' Pike <r@google.com> wrote:
>> i like rsc's comment better. please also drop the reference to the other
documentation we're not responsible for
> 
> Okay. So are we satisfied by the choice of Sync over Flush?

yes

[adg, 2011-01-12 00:08:46]

*** Submitted as http://code.google.com/p/go/source/detail?r=7e072383c57d ***

os: add Sync to *File, wraps syscall.Fsync

R=rsc, brainman, r, r2
CC=golang-dev
http://codereview.appspot.com/3887042