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

proposal: os: add Touch to set access/mod times to current time #31880

Closed
sybrenstuvel opened this issue May 7, 2019 · 31 comments
Closed

proposal: os: add Touch to set access/mod times to current time #31880

sybrenstuvel opened this issue May 7, 2019 · 31 comments

Comments

@sybrenstuvel
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.12.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/flamencoadmin/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/flamencoadmin/go"
GOPROXY=""
GORACE=""
GOROOT="/opt/go"
GOTMPDIR=""
GOTOOLDIR="/opt/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/flamencoadmin/gotouch/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build400479428=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Call os.Chtimes(filepath, now, now) where filepath is poining to a regular file on an SMB share and now := time.Now().

What did you expect to see?

No error, and the mtime of the file changed to 'now'.

What did you see instead?

An os.PathError is returned as follows:

Error type: *os.PathError
Error msg: chtimes /mnt/flamenco-input/file-store/stored/d6/2d13ed80d57784d9c5113a4bb32541c2b23a1e7ef02117f49084985bee144d/8271557.blob: operation not permitted
    op  : chtimes
    path: /mnt/flamenco-input/file-store/stored/d6/2d13ed80d57784d9c5113a4bb32541c2b23a1e7ef02117f49084985bee144d/8271557.blob
    err : operation not permitted

The SMB share is served from a Microsoft SMB server (an Azure Files share), which apparently doesn't support setting the modification/access time to a specific timestamp. However, what it does support is setting the mtime to "now" using the Linux touch CLI application. By using strace touch thefile I found that it actually calls utimensat() with NULL as timestamp, rather than passing an explicit timestamp.

To test whether passing NULL would work, I added the following function to src/syscall/syscall_linux.go:

func UtimesNanoNow(path string) (err error) {
	err = utimensat(_AT_FDCWD, path, nil, 0)
	if err != ENOSYS {
		return err
	}
	return utimes(path, nil)
}

The following now works fine on the SMB share:

func ChtimesNow(name string) error {
	if e := syscall.UtimesNanoNow(name); e != nil {
		return &os.PathError{"chtimes", name, e}
	}
	return nil
}

I tested this on Linux (4.18.0-1014-azure) on Ubuntu (18.04.2). The SMB share was mounted with the following options, as per the Microsoft documentation:

//${ACCOUNT_NAME}.file.core.windows.net/flamenco-resources on /mnt/flamenco-resources type cifs (rw,relatime,vers=3.0,sec=ntlmssp,cache=strict,username=${ACCOUNT_NAME},password=${ACCOUNT_PASSWORD},dir_mode=0770,file_mode=0775,gid=flamenco,forcegid,sec=ntlmssp,mfsymlinks)
@ianlancetaylor ianlancetaylor changed the title os.Chtimes only accepts explicit timestamps, does not work on SMB shares os: Chtimes only accepts explicit timestamps, does not work on SMB shares May 7, 2019
@ianlancetaylor
Copy link
Contributor

I guess the question is whether we should add os.ChtimesNow to support this edge case, or whether we should expect programs to call unix.UtimesNanoAt, from the golang.org/x/sys/unix package, instead.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 7, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone May 7, 2019
@sybrenstuvel
Copy link
Author

Touching a file is a common operation. I suspect it is even more common than setting the timestamps to a different time, although that's just my gut feeling.

I vote for os.ChtimesNow (or simply call it os.Touch), as the operation is at the same level of abstraction as os.Chtimes. Just setting a file's mtime to "now" should IMHO not require any platform-specific branches in your code.

unix.UtimesNanoAt doesn't accept nil, so I doubt that it'll fix this issue. Or did you mean my UtimesNanoNow function @ianlancetaylor ?

@ianlancetaylor
Copy link
Contributor

When I look at the implementation of unix.UtimesNanoAt I see that it does accept nil as the []Timespec argument, and that it does the right thing. This dates back to https://golang.org/cl/12690.

@ianlancetaylor ianlancetaylor changed the title os: Chtimes only accepts explicit timestamps, does not work on SMB shares proposal: add os.Touch to set access/mod times to current time May 7, 2019
@ianlancetaylor
Copy link
Contributor

I turned this issue into a proposal for os.Touch.

@ianlancetaylor ianlancetaylor removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 7, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Proposal May 7, 2019
@rsc rsc changed the title proposal: add os.Touch to set access/mod times to current time proposal: os: add Touch to set access/mod times to current time May 7, 2019
@beoran
Copy link

beoran commented May 8, 2019

If this is named Touch(), then will it also create the file if it does not exist, as per the touch unix command?

@sybrenstuvel
Copy link
Author

sybrenstuvel commented May 8, 2019

@beoran for me personally that wouldn't be necessary, but I wouldn't object (unless it delays the availability of the function). All I need is a Chtimes(now) call that works for my use case.

@ianlancetaylor
Copy link
Contributor

It seems that there are two special cases for os.Chtimes: leave the time unchanged, and set the time to "now". Adding os.Touch will help with the latter but not the former.

@bradfitz
Copy link
Contributor

Other gross/weird idea: add sentinel time.Time values for UTIME_NOW and UTIME_OMIT, perhaps within a sentinel *time.Location.

@ianlancetaylor
Copy link
Contributor

I kind of like that, actually. We could add os.ChtimesUnchanged and os.ChtimesNow of type time.Time.

@sybrenstuvel
Copy link
Author

sybrenstuvel commented May 15, 2019

It seems that there are two special cases for os.Chtimes: leave the time unchanged, and set the time to "now".

Would you mind linking the source of that information?

We could add os.ChtimesUnchanged

What would the normal English interpretation be of a name like ChtimesUnchanged? To me, "Change time unchanged" sounds cryptic at best.

What is the use of calling a "change times" function with argument "do not change"?

@ianlancetaylor
Copy link
Contributor

The source for the observation that there are two special cases is 1) examination of actual calls to os.Chtimes in Google's code, and 2) the observation that the Linux kernel supports two special cases UTIME_NOW and UTIME_OMIT (see http://man7.org/linux/man-pages/man2/utimensat.2.html).

I agree that os.ChtimesUnchanged is not the best name, happy to hear alternatives. Note that it is not a function, but a value of type time.Time.

The use of the argument is that os.Chtimes takes two arguments for two different times, and some code wants to only change one of them. Currently that code must first call Stat to get the time to pass to os.Chtimes; an os.ChtimesUnchanged, whatever we call it, would simplify that code.

@bradfitz
Copy link
Contributor

We don't need Chtimes in the name. It'd look pretty stuttery with them:

os.Chtimes(path, os.ChtimesUnchanged, os.ChtimesNow)

How about:

package os

// Special time.Time values for os.Chtimes:
var (
    // KeepTime is recognized by Chtimes to mean that the access or modification
    // time should not be changed. It is not a valid Time value otherwise.
    KeepTime  = time.Unix(0, 1).In(utimeLocation)
    // TouchTime is recognized by Chtimes to mean that the access or modification
    // time should be set to the current time. It is not a valid Time value otherwise.
    TouchTime = time.Unix(0, 2).In(utimeLocation)
)

@sybrenstuvel
Copy link
Author

@bradfitz that looks good to me!

@rsc
Copy link
Contributor

rsc commented May 28, 2019

The special time location bothers me but a sentinel time seems OK.
We already have the zero time for 'don't update this', so really we just need a sentinel for "now".
Maybe time.Date(10000, 1, 1, 0, 0, 0, 0, time.UTC)?

@beoran
Copy link

beoran commented May 28, 2019 via email

@bradfitz
Copy link
Contributor

time.Now isn't a sentinel value. It's a function that returns a time value.

@ianlancetaylor
Copy link
Contributor

Also kind of the point of this issue is that there is at least one case where using time.Now() doesn't work.

@rsc
Copy link
Contributor

rsc commented Jun 4, 2019

Last week we talked about a sentinel for meaning "tell the FS 'now'". One problem with this approach is that it only works when people think to use it, and more importantly not doing it only breaks on Azure Files or other SMB servers. So most code will work everywhere except Azure and it will be hard to get everyone to fix their code for a use case they may not directly need.

One possible alternative, following a bit @beoran's suggestion, is to say that if the time passed to Chtimes is "within epsilon" of the current time, then Chtimes just automatically tells the file system to use the special "now" form instead of an explicit time. For example you could define "within epsilon" to mean "has monotonic time at most 100ms earlier than now". That would have an unfortunate effect if you did os.Chtimes(time.Now()) and the code was interrupted between those calls. But it would work most of the time without requiring everyone to learn a new way to spell that call.

On balance it is probably still better to have the sentinel instead?

@rsc
Copy link
Contributor

rsc commented Jun 4, 2019

Does anyone know more about the underlying issue with Azure Files / SMB? Is this a fundamental limitation of the SMB protocol that there is no "set mtime" RPC, or is it a problem specific to Azure Files not implementing that call?

/cc @ashleymcnamara

@sybrenstuvel
Copy link
Author

sybrenstuvel commented Jun 5, 2019

Last week we talked about a sentinel for meaning "tell the FS 'now'". One problem with this approach is that it only works when people think to use it, and more importantly not doing it only breaks on Azure Files or other SMB servers. So most code will work everywhere except Azure and it will be hard to get everyone to fix their code for a use case they may not directly need.

This is a good point. I think it can be partially solved by good documentation, making the correct choice (using the sentinel value) more obvious than using the probably-works choice. This is also partially why my initial suggestion had a different, parameterless function; when ChTimes() and ChTimesNow() appear next to each other in a module function list, it's easier to just pick the ChTimesNow() and harder to think "I can use time.Now() and pass its return value to ChTimes()".

I would certainly add a warning to ChTimes() that states that setting an arbitrary mtime/atime isn't supported on all filesystems, and that the sentinel value for 'now' has wider filesystem support than passing an explicit time.Now().

One possible alternative, following a bit @beoran's suggestion, is to say that if the time passed to Chtimes is "within epsilon" of the current time, then Chtimes just automatically tells the file system to use the special "now" form instead of an explicit time. For example you could define "within epsilon" to mean "has monotonic time at most 100ms earlier than now". That would have an unfortunate effect if you did os.Chtimes(time.Now()) and the code was interrupted between those calls. But it would work most of the time without requiring everyone to learn a new way to spell that call.

I wouldn't like this approach; I don't like code that should work most of the time. Sure, in the point you make above we also have code that works most of the time. The distribution of failure is different though: it reliably never works on those Microsoft SMB servers, and it reliably always works on other filesystems. The "epsilon approach" would unreliably work most of the time, which I think is much worse.

@rsc
Copy link
Contributor

rsc commented Jun 11, 2019

Right now we have:

  • os.Chtimes(a, m) - sets a, m

We might want:

  • os.Chtimes(_, m) - set only m
  • os.Chtimes(a, _) - set only a
  • os.Chtimes(_, NOW) - set only m to NOW
  • os.Chtimes(NOW, _) - set only a to NOW
  • os.Chtimes(NOW, NOW) - set both to NOW

We can handle the "don't set this" case with time.Time{}. That seems unobjectionable. What's left is the NOW cases.

We could add os.ChtimesNow but with no arguments it would only cover one of those three. To cover all three we'd need arguments, like os.Chtimes(false, true), which is mysterious. We could add os.ChtimesModNow and os.ChtimesAccessNow but that would leave no way to do the "NOW, NOW" and guarantee they got the same time. None of these sound good. We need a better idea.

I am still confused about why it is not possible to set the times to anything at all. I looked briefly at the latest CIFS spec and can't see where it says "you can't set atime/mtime to anything but right now" nor do I even see "here is how to set them to an abstract right now". Or is Azure imposing the constraint? If so, where is that documented? I'd really like to hear something authoritative from Microsoft before we start adding workarounds that show up in package os's exported, long-term API. (/cc @erikstmartin)

It seems like maybe Go is not using the right Windows call inside Chtimes. If that's the case, let's fix that.

/cc @spf13

@rsc
Copy link
Contributor

rsc commented Jun 11, 2019

Started #32558 for the _ (don't set me) case. This issue will be about time.Now() vs NOW vs Azure.

@spf13
Copy link
Contributor

spf13 commented Jun 11, 2019

I was working on a program recently that encountered a similar issue. Specifically that from Go, SMB operations don't respect calls to change the mtime of files.

@beoran
Copy link

beoran commented Jun 11, 2019 via email

@ianlancetaylor
Copy link
Contributor

@beoran That doesn't provide a clear way to set both times to "now" and to the same "now". Also it's worth noting that on Unix and Windows the underlying system call takes both times. And also there is really only one flag--set the time to "now"--and it's hard to see why there would ever be another one.

@beoran
Copy link

beoran commented Jun 12, 2019

Yes, you are right. I checked the system calls SetFileTime() and utimens(), and I realized that now. It makes sense because if two system calls were needed, it would be impossible to synchronize both times. But I do notice that on both OS, the system calls do no take a wall clock time like LPSYSTEMTIME or struct time, but a specific file-related time, FILETIME or struct timespec, which allows more easily to specify special values than time.Time does.

So, I would say that this is then the problem with os.Chtimes, it takes plain time.Time, but it should probably take a file system specific time, e.g, os.FileTime. For backwards compatibility, os.Chtimes can be left as it it, and the new function that takes the os.FileTime could be named something like os.Chfiletimes. The os.FileTime could be something like type FileTime struct { time.Time ; Now bool ; Ignore bool }.

I think this would be better than making an arbitrary time.Time{} "magical" in the sense that it will set the time to NOW, since that arbitrary time.Time risks to have already being used in existing code for other purposes.

@sybrenstuvel
Copy link
Author

os.FileTime could be something like type FileTime struct { time.Time ; Now bool ; Ignore bool }

To me this looks like a hassle, compared to just passing a time.Time instance. Furthermore, it allows for the invalid combination Now=true; Ignore=true; those two are mutually exclusive.

@rsc
Copy link
Contributor

rsc commented Jun 25, 2019

I don't think we should make any decisions here without some kind of definitive clarification about what the Azure- or SMB-imposed limitations are. We are designing around something and we don't even know what shape it is. Let's not kick around any new APIs until we understand that better.

See #31880 (comment) and #31880 (comment).

/cc @ashleymcnamara @erikstmartin

@rsc
Copy link
Contributor

rsc commented Aug 27, 2019

The original report claims that using Chtimes with any specific time fails on Linux talking to a Microsoft Azure Files server over SMB. We do not understand why this restriction would exist - the CIFS protocol clearly supports sending a time in the various calls that change file modification times.

We asked for details about what the specific CIFS/SMB/WinAPI/Azure restrictions might be, in #31880 (comment) and #31880 (comment) and #31880 (comment). No answers here.

In the absence of information about what the underlying constraints are, it is essentially impossible to decide what an appropriate Go API change would be. Without new information, this issue seems like a likely decline (not enough information).

Leaving open for a week for final comments.

@ianlancetaylor
Copy link
Contributor

Marked this last week as likely decline w/ call for last comments (#31880 (comment)).
Declining now.

@sybrenstuvel
Copy link
Author

I couldn't answer earlier as I was on holiday.

We asked for details about what the specific CIFS/SMB/WinAPI/Azure restrictions might be, in #31880 (comment) and #31880 (comment) and #31880 (comment). No answers here.

I'm far from a CIFS/SMB expert, I just know that one thing worked and another thing didn't. I was hoping someone else from the Golang community could chip in.

@golang golang locked and limited conversation to collaborators Sep 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants