fuse: remove race conditions on fsh #53
Conversation
btwotch
force-pushed
the
filesystemhost_race_conditions
branch
4 times, most recently
from
21feca1
to
3ee7e5b
Compare
fuse/host.go
Outdated
| @@ -577,13 +582,17 @@ func NewFileSystemHost(fsop FileSystemInterface) *FileSystemHost { | |||
| // SetCapCaseInsensitive informs the host that the hosted file system is case insensitive | |||
| // [OSX and Windows only]. | |||
| func (host *FileSystemHost) SetCapCaseInsensitive(value bool) { | |||
| host.Lock() | |||
There was a problem hiding this comment.
This is not necessary.
The SetCapCaseInsensitive method is (should be) called before Mount happens. The Mount method may (implicitly or explicitly) create new file system threads, and the value of host.capCaseInsensitive will become available to them regardless of locking. Note that the value of host.capCaseInsensitive is not supposed to change after Mount.
For this reason it is not necessary to wrap the host.capCaseInsensitive = value statement in a lock.
There was a problem hiding this comment.
I removed it, although I think that it would make the code more uniform to lock it here as well.
fuse/host.go
Outdated
| host.capCaseInsensitive = value | ||
| } | ||
|
|
||
| // SetCapReaddirPlus informs the host that the hosted file system has the readdir-plus | ||
| // capability [Windows only]. A file system that has the readdir-plus capability can send | ||
| // full stat information during Readdir, thus avoiding extraneous Getattr calls. | ||
| func (host *FileSystemHost) SetCapReaddirPlus(value bool) { | ||
| host.Lock() |
There was a problem hiding this comment.
This is not necessary.
The SetCapReaddirPlus method is (should be) called before Mount happens. The Mount method may (implicitly or explicitly) create new file system threads, and the value of host.capReaddirPlus will become available to them regardless of locking. Note that the value of host.capReaddirPlus is not supposed to change after Mount.
For this reason it is not necessary to wrap the host.capReaddirPlus = value statement in a lock.
fuse/host.go
Outdated
| @@ -651,6 +660,7 @@ func (host *FileSystemHost) Mount(mountpoint string, opts []string) bool { | |||
| * We need to determine the mountpoint that FUSE is going (to try) to use, so that we | |||
| * can unmount later. | |||
| */ | |||
| host.Lock() | |||
There was a problem hiding this comment.
This is not necessary.
The host.mntp variable is read-only after Mount. The Mount method may (implicitly or explicitly) create new file system threads, and the value of host.mntp will become available to them regardless of locking.
fuse/host.go
Outdated
| @@ -685,10 +698,15 @@ func (host *FileSystemHost) Mount(mountpoint string, opts []string) bool { | |||
| defer func() { | |||
| <-done | |||
| }() | |||
| host.Lock() | |||
There was a problem hiding this comment.
This is not necessary.
The host.sigc variable is read-only after Mount. The Mount method may (implicitly or explicitly) create new file system threads, and the value of host.sigc will become available to them regardless of locking.
| @@ -708,6 +726,8 @@ func (host *FileSystemHost) Mount(mountpoint string, opts []string) bool { | |||
| // Unmount may be called at any time after the Init() method has been called | |||
| // and before the Destroy() method has been called. | |||
| func (host *FileSystemHost) Unmount() bool { | |||
| host.RLock() | |||
There was a problem hiding this comment.
The host.fuse variable is modified in hostInit and hostDestroy. The Unmount method is documented as being safe only between Init and Destroy, which means that the file system should provide its own synchronization method to ensure that Unmount is not called at the wrong time.
However I agree that it might be better if Unmount is improved to catch cases where the file system calls it before Init or after Destroy.
There was a problem hiding this comment.
If I remove the lock here, I get into a race condition when executing go test -race; I had the same problem with my fs and I don't know how to fix it on the side of my fs.
| return 0 != c_hostUnmount(host.fuse, mntp) | ||
| } | ||
|
|
||
| // Notify notifies the operating system about a file change. | ||
| // The action is a combination of the fuse.NOTIFY_* constants. | ||
| func (host *FileSystemHost) Notify(path string, action uint32) bool { | ||
| host.RLock() |
There was a problem hiding this comment.
My comments in Unmount apply here as well (especially because I failed to document that is only safe between Init and Destroy).
There was a problem hiding this comment.
In comparison to func Unmount, it doesn't run into a race condition when running go test -race.
btwotch
force-pushed
the
filesystemhost_race_conditions
branch
from
3ee7e5b
to
92831e3
Compare
remove race conditions as seen with `go test -race` on FileSystemHost members
btwotch
force-pushed
the
filesystemhost_race_conditions
branch
from
92831e3
to
c6e3169
Compare
remove race conditions as seen with
go test -raceon FileSystemHostmembers