code review 6905057: os: Improve the accuracy of os.Chtimes

os: Improve the accuracy of os.Chtimes

I've been writing some code which involves syncing files (like
rsync) and it became apparent that under Linux I could read
modification times (os.Lstat) with nanosecond precision but
only write them with microsecond precision.  This difference
in precision is rather annoying when trying to discover
whether files need syncing or not!

I've patched syscall and os to increases the accuracy of of
os.Chtimes for Linux and Windows.  This involved exposing the
utimensat system call under Linux and a bit of extra code
under Windows.  I decided not to expose the "at" bit of the
system call as it is impossible to replicate under Windows, so
the patch adds syscall.Utimens() to all architectures along
with a ImplementsUtimens flag.

If the utimensat syscall isn't available (utimensat was added
to Linux in 2.6.22, Released, 8 July 2007) then it silently
falls back to the microsecond accuracy version it uses now.
The improved accuracy for Windows should be good for all
versions of Windows.

Unfortunately Darwin doesn't seem to have a utimensat system
call that I could find so I couldn't implement it there.  The
BSDs do, but since they share their syscall implementation
with Darwin I couldn't figure out how to define a syscall for
*BSD and not Darwin.  I've left this as a TODO in the code.

In the process I implemented the missing methods for Timespec
under Windows which I needed which just happened to round out
the Timespec API for all platforms!

------------------------------------------------------------

Test code: http://play.golang.org/p/1xnGuYOi4b

Linux Before (1000 ns precision)

$ ./utimetest.linux.before z
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 BST
Reading mtime 1344937903123457000: 2012-08-14 10:51:43.123457 +0100 BST

Linux After (1 ns precision)

$ ./utimetest.linux.after z
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 BST
Reading mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 BST

Windows Before (1000 ns precision)

X:\>utimetest.windows.before.exe c:\Test.txt
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 GMTDT
Reading mtime 1344937903123456000: 2012-08-14 10:51:43.123456 +0100 GMTDT

Windows After (100 ns precision)

X:\>utimetest.windows.after.exe c:\Test.txt
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 GMTDT
Reading mtime 1344937903123456700: 2012-08-14 10:51:43.1234567 +0100 GMTDT

[Nick Craig-Wood, 2012-12-08 22:47:42]

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

I'd like you to review this change to
https://code.google.com/p/go

[brainman, 2012-12-10 06:25:25]

It fails to build on windows with:

# GOOS=windows go install -v syscall
syscall
# syscall
./syscall_windows.go:460: undefined: utf16PtrFromString

But, please, wait for others to confirm that your change will be accepted in
principle first.

Thank you.

Alex

[rsc, 2012-12-10 06:49:56]

I haven't looked at the code but increasing the resolution of Chtimes is fine.

[bradfitz, 2012-12-10 17:44:31]

https://codereview.appspot.com/6905057/diff/3012/src/pkg/os/file_posix.go
File src/pkg/os/file_posix.go (right):

https://codereview.appspot.com/6905057/diff/3012/src/pkg/os/file_posix.go#new...
src/pkg/os/file_posix.go:156: if syscall.ImplementsUtimens {
I'm not sure I'm a fan of this Implements style, but I guess we do do it in one
other case, which I'd never seen before.

I would personally probably instead make a new nil-by-default func variable in
the os package like:

// chtimesNano if non-nil is an implementation of Chtimes
// that supports nanosecond resolution.  If nil or the return
// value is errNoSys, a portable microsecond resolution implementation
// is used instead.
var chtimesNano func(string, time.Time, time.Time) error

... and then Chtime is:

func Chtimes(...) {
     if chtimesNano != nil {
        if err := chtimesNano(name, atime, mtime); err != errNoSys {
             return err
        }
     }
     ... .
}

... and then make files like:

// +build linux,darwin

func init() {
   chtimesNano = chtimes
}

func chtimes(...) {

}

... then you don't have to have a dummy stub syscall.Utimens on operating
systems which don't support it.

[rsc, 2012-12-10 18:07:54]

It would be nice if we could avoid func init in package os if possible.
We're already passing nanoseconds to syscall.Utimes. Can we fix
syscall.Utimes on systems that support it to do something with the
nanoseconds?
Then package os doesn't need to change at all.

[Nick Craig-Wood, 2012-12-10 18:10:50]

On 2012/12/10 17:44:31, bradfitz wrote:
> https://codereview.appspot.com/6905057/diff/3012/src/pkg/os/file_posix.go
> File src/pkg/os/file_posix.go (right):
> 
>
https://codereview.appspot.com/6905057/diff/3012/src/pkg/os/file_posix.go#new...
> src/pkg/os/file_posix.go:156: if syscall.ImplementsUtimens {
> I'm not sure I'm a fan of this Implements style, but I guess we do do it in
one
> other case, which I'd never seen before.
> 
> I would personally probably instead make a new nil-by-default func variable in
> the os package like:
[snip]

A good plan - keeps the namespace clean with very little overhead.  I'll have a
go in a little while!

[Nick Craig-Wood, 2012-12-10 18:12:50]

On 2012/12/10 06:25:25, brainman wrote:
> It fails to build on windows with:
[snip]
> ./syscall_windows.go:460: undefined: utf16PtrFromString

Oops - a last minute update fail. Will fix shortly

[bradfitz, 2012-12-10 18:26:12]

On Mon, Dec 10, 2012 at 1:07 PM, Russ Cox <rsc@golang.org> wrote:

> It would be nice if we could avoid func init in package os if possible.
> We're already passing nanoseconds to syscall.Utimes.


No, looks like we're passing microseconds to syscall.Utimes.

We can't change that without breaking the Go 1 contract.

Can we fix
> syscall.Utimes on systems that support it to do something with the
> nanoseconds?
> Then package os doesn't need to change at all.
>

[rsc, 2012-12-10 18:38:15]

Oh, I missed the syscall.NsecToTimeval.
It seems fine to write a syscall.UtimesNano that just wraps
syscall.Utimes on the unimplemented systems.
syscall can do a little to be more regular across systems.

Russ

[Nick Craig-Wood, 2012-12-10 20:56:27]

On 2012/12/10 18:38:15, rsc wrote:
> Oh, I missed the syscall.NsecToTimeval.
> It seems fine to write a syscall.UtimesNano that just wraps
> syscall.Utimes on the unimplemented systems.
> syscall can do a little to be more regular across systems.

I like the idea of syscall.UtimesNano falling back to syscall.Utimes instead of
returning not implemented.  That then guarantees best accuracy and avoids the
pain for the users of making different builds for different OSes.

As far as I can see the whole `utimes` syscall was a mistake in POSIX which was
corrected later with the `utimensat` syscall.  We can do better in Go :-)

I'll have a go with Russ' suggestion.

[Nick Craig-Wood, 2012-12-10 23:28:54]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, rsc@golang.org,
bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[Nick Craig-Wood, 2012-12-10 23:32:34]

On 2012/12/10 23:28:54, Nick Craig-Wood wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:alex.brainman@gmail.com,
mailto:rsc@golang.org,
> mailto:bradfitz@golang.org (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

I hope I've addressed your concerns

  * removed syscall.ImplementsUtimens
  * rename syscall.Utimens to syscall.UtimesNano
  * implement UtimesNano on all architectures
  * If the underlying high resolution syscall isn't avalable then UtimesNano
falls back to Utimes (all BSD and very old Linux)

[brainman, 2012-12-10 23:41:23]

On 2012/12/10 18:38:15, rsc wrote:
> ...
> It seems fine to write a syscall.UtimesNano that just wraps
> syscall.Utimes on the unimplemented systems.
> syscall can do a little to be more regular across systems.
> 

I think, there is nothing "regular" about syscall.UtimesNano on windows - there
is no such windows syscall.

Also this approach will end up with new public interface (syscall.UtimesNano as
a minimum) and complicated windows logic (convert from time.Time to int64 then
to syscall.Timeval in os.Chtimes, then call to syscall.UtimesNano, then convert
to syscall.Timespec, then convert to int64, then convert to Filetime, then call
syscall).

Why can't we just move os.Chtimes from file_posix.go into file_unix.go and
file_windows.go, Then we could implement whatever needs to be done for windows
in file_windows.go without doing all conversion song and dance.

Alex

[Nick Craig-Wood, 2012-12-11 14:58:36]

On 2012/12/10 23:41:23, brainman wrote:
> On 2012/12/10 18:38:15, rsc wrote:
> > ...
> > It seems fine to write a syscall.UtimesNano that just wraps
> > syscall.Utimes on the unimplemented systems.
> > syscall can do a little to be more regular across systems.
> > 
> 
> I think, there is nothing "regular" about syscall.UtimesNano on windows -
there
> is no such windows syscall.
> 
> Also this approach will end up with new public interface (syscall.UtimesNano
as
> a minimum) and complicated windows logic (convert from time.Time to int64 then
> to syscall.Timeval in os.Chtimes, then call to syscall.UtimesNano, then
convert
> to syscall.Timespec, then convert to int64, then convert to Filetime, then
call
> syscall).
> 
> Why can't we just move os.Chtimes from file_posix.go into file_unix.go and
> file_windows.go, Then we could implement whatever needs to be done for windows
> in file_windows.go without doing all conversion song and dance.

I'd be happy to have a go at that if everyone is happy with the idea.

It certainly seems sensible to me, though I haven't looked at the details of how
hard it would be to divorce it from the posix code yet.

Also what do you think about making it part of this patch set?

[rsc, 2012-12-11 15:22:27]

I think this CL is fine as is. Please don't put any more syscall-specific stuff
in package os.

I would agree with Alex if Utimes wasn't already defined on Windows. If we're
going to have Utimes we might as well have Utimes and UtimesNano like the other
systems.

https://codereview.appspot.com/6905057/diff/13001/src/pkg/syscall/syscall_bsd.go
File src/pkg/syscall/syscall_bsd.go (right):

https://codereview.appspot.com/6905057/diff/13001/src/pkg/syscall/syscall_bsd...
src/pkg/syscall/syscall_bsd.go:615: //s*ys	utimensat(dirfd int, path string,
times *[2]Timespec) (err error)
Delete this line if you're not using it.

[Nick Craig-Wood, 2012-12-12 13:00:14]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, rsc@golang.org,
bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[Nick Craig-Wood, 2012-12-12 13:01:57]

On 2012/12/11 15:22:27, rsc wrote:
> I think this CL is fine as is. Please don't put any more syscall-specific
stuff
> in package os.
> 
> I would agree with Alex if Utimes wasn't already defined on Windows. If we're
> going to have Utimes we might as well have Utimes and UtimesNano like the
other
> systems.
> 
>
https://codereview.appspot.com/6905057/diff/13001/src/pkg/syscall/syscall_bsd.go
> File src/pkg/syscall/syscall_bsd.go (right):
> 
>
https://codereview.appspot.com/6905057/diff/13001/src/pkg/syscall/syscall_bsd...
> src/pkg/syscall/syscall_bsd.go:615: //s*ys	utimensat(dirfd int, path string,
> times *[2]Timespec) (err error)
> Delete this line if you're not using it.

I have updated the patch with this change and restested it on linux, os x and
windows

[bradfitz, 2012-12-12 19:52:38]

LGTM

but should wait on Alex for Windows.

https://codereview.appspot.com/6905057/diff/21001/src/pkg/syscall/syscall_bsd.go
File src/pkg/syscall/syscall_bsd.go (right):

https://codereview.appspot.com/6905057/diff/21001/src/pkg/syscall/syscall_bsd...
src/pkg/syscall/syscall_bsd.go:612: // TODO: The BSDs can do utimensat with
SYS_UTIMENSAT but it isn't supported by darwin
move the TODO inside the function.  where you have it now will make it render as
documentation on the function in godoc.

https://codereview.appspot.com/6905057/diff/21001/src/pkg/syscall/syscall_bsd...
src/pkg/syscall/syscall_bsd.go:614: func UtimesNano(path string, ts []Timespec)
(err error) {
drop the named result parameter stutter.  just "error" works.

[brainman, 2012-12-13 00:51:45]

LGTM

Leaving for Brad to submit once you have made his changes.

Alex

[Nick Craig-Wood, 2012-12-13 17:00:13]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, rsc@golang.org,
bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[Nick Craig-Wood, 2012-12-13 17:01:41]

On 2012/12/12 19:52:38, bradfitz wrote:
> LGTM
> 
> but should wait on Alex for Windows.
> 
>
https://codereview.appspot.com/6905057/diff/21001/src/pkg/syscall/syscall_bsd.go
> File src/pkg/syscall/syscall_bsd.go (right):
> 
>
https://codereview.appspot.com/6905057/diff/21001/src/pkg/syscall/syscall_bsd...
> src/pkg/syscall/syscall_bsd.go:612: // TODO: The BSDs can do utimensat with
> SYS_UTIMENSAT but it isn't supported by darwin
> move the TODO inside the function.  where you have it now will make it render
as
> documentation on the function in godoc.
> 
>
https://codereview.appspot.com/6905057/diff/21001/src/pkg/syscall/syscall_bsd...
> src/pkg/syscall/syscall_bsd.go:614: func UtimesNano(path string, ts
[]Timespec)
> (err error) {
> drop the named result parameter stutter.  just "error" works.

I've fixed both of those.

I note that there are lots of other instances of (err error) in that file which
could also be changed, but I haven't done so

[bradfitz, 2012-12-13 20:23:36]

Nick,

I don't see a CLA on file for you.

See http://golang.org/doc/contribute.html#copyright

Once a CLA is submitted, let me know, then I can submit this CL.

[bradfitz, 2012-12-13 21:02:43]

*** Submitted as https://code.google.com/p/go/source/detail?r=b7482cb67996 ***

os: Improve the accuracy of os.Chtimes

I've been writing some code which involves syncing files (like
rsync) and it became apparent that under Linux I could read
modification times (os.Lstat) with nanosecond precision but
only write them with microsecond precision.  This difference
in precision is rather annoying when trying to discover
whether files need syncing or not!

I've patched syscall and os to increases the accuracy of of
os.Chtimes for Linux and Windows.  This involved exposing the
utimensat system call under Linux and a bit of extra code
under Windows.  I decided not to expose the "at" bit of the
system call as it is impossible to replicate under Windows, so
the patch adds syscall.Utimens() to all architectures along
with a ImplementsUtimens flag.

If the utimensat syscall isn't available (utimensat was added
to Linux in 2.6.22, Released, 8 July 2007) then it silently
falls back to the microsecond accuracy version it uses now.
The improved accuracy for Windows should be good for all
versions of Windows.

Unfortunately Darwin doesn't seem to have a utimensat system
call that I could find so I couldn't implement it there.  The
BSDs do, but since they share their syscall implementation
with Darwin I couldn't figure out how to define a syscall for
*BSD and not Darwin.  I've left this as a TODO in the code.

In the process I implemented the missing methods for Timespec
under Windows which I needed which just happened to round out
the Timespec API for all platforms!

------------------------------------------------------------

Test code: http://play.golang.org/p/1xnGuYOi4b

Linux Before (1000 ns precision)

$ ./utimetest.linux.before z
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 BST
Reading mtime 1344937903123457000: 2012-08-14 10:51:43.123457 +0100 BST

Linux After (1 ns precision)

$ ./utimetest.linux.after z
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 BST
Reading mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 BST

Windows Before (1000 ns precision)

X:\>utimetest.windows.before.exe c:\Test.txt
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 GMTDT
Reading mtime 1344937903123456000: 2012-08-14 10:51:43.123456 +0100 GMTDT

Windows After (100 ns precision)

X:\>utimetest.windows.after.exe c:\Test.txt
Setting mtime 1344937903123456789: 2012-08-14 10:51:43.123456789 +0100 GMTDT
Reading mtime 1344937903123456700: 2012-08-14 10:51:43.1234567 +0100 GMTDT

R=golang-dev, alex.brainman, rsc, bradfitz
CC=golang-dev
https://codereview.appspot.com/6905057

Committer: Brad Fitzpatrick <bradfitz@golang.org>