From 4c9e249e3ac1fc2995e01eb1ed24799c3a3bc66b Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Mon, 25 Jan 2016 00:51:28 +0100 Subject: Convert fdLock to an RWMutex and protect the whole transaction ...against concurrent closes. The testcase (set -e; while true; do truncate -s $RANDOM b; done) & (set -e; while true; do truncate -s $RANDOM b; done) & uncovered lots of unnecessary RMW failures that were the result of concurrent closes. With this patch, the only remaining error is "Truncate on forgotten file" that is probably caused by a problem in the go-fuse lib ( https://github.com/hanwen/go-fuse/issues/95 ) --- cryptfs/names_diriv.go | 2 +- pathfs_frontend/file.go | 106 ++++++++++++++++++++++++++---------------------- 2 files changed, 58 insertions(+), 50 deletions(-) diff --git a/cryptfs/names_diriv.go b/cryptfs/names_diriv.go index 9519ebc..45abfce 100644 --- a/cryptfs/names_diriv.go +++ b/cryptfs/names_diriv.go @@ -1,9 +1,9 @@ package cryptfs import ( - "os" "fmt" "io/ioutil" + "os" "path/filepath" "strings" "sync" diff --git a/pathfs_frontend/file.go b/pathfs_frontend/file.go index ceb9216..33ad0c7 100644 --- a/pathfs_frontend/file.go +++ b/pathfs_frontend/file.go @@ -20,12 +20,11 @@ import ( type file struct { fd *os.File - // os.File is not threadsafe. Although fd themselves are - // constant during the lifetime of an open file, the OS may - // reuse the fd number after it is closed. When open races - // with another close, they may lead to confusion as which - // file gets written in the end. - fdLock sync.Mutex + // fdLock prevents the fd to be closed while we are in the middle of + // an operation. + // Every FUSE entrypoint should RLock(). The only user of Lock() is + // Release(), which closes the fd, hence has to acquire an exclusive lock. + fdLock sync.RWMutex // Was the file opened O_WRONLY? writeOnly bool @@ -38,6 +37,8 @@ type file struct { // File header header *cryptfs.FileHeader + + forgotten bool } func NewFile(fd *os.File, writeOnly bool, cfs *cryptfs.CryptFS) nodefs.File { @@ -53,6 +54,12 @@ func NewFile(fd *os.File, writeOnly bool, cfs *cryptfs.CryptFS) nodefs.File { } } +// intFd - return the backing file descriptor as an integer. Used for debug +// messages. +func (f *file) intFd() int { + return int(f.fd.Fd()) +} + func (f *file) InnerFile() nodefs.File { return nil } @@ -84,11 +91,9 @@ func (f *file) createHeader() error { buf := h.Pack() // Prevent partially written (=corrupt) header by preallocating the space beforehand - f.fdLock.Lock() - defer f.fdLock.Unlock() err := prealloc(int(f.fd.Fd()), 0, cryptfs.HEADER_LEN) if err != nil { - cryptfs.Warn.Printf("createHeader: fallocateRetry failed: %s\n", err.Error()) + cryptfs.Warn.Printf("ino%d: createHeader: prealloc failed: %s\n", f.ino, err.Error()) return err } @@ -133,9 +138,7 @@ func (f *file) doRead(off uint64, length uint64) ([]byte, fuse.Status) { skip := blocks[0].Skip cryptfs.Debug.Printf("JointCiphertextRange(%d, %d) -> %d, %d, %d", off, length, alignedOffset, alignedLength, skip) ciphertext := make([]byte, int(alignedLength)) - f.fdLock.Lock() n, err := f.fd.ReadAt(ciphertext, int64(alignedOffset)) - f.fdLock.Unlock() if err != nil && err != io.EOF { cryptfs.Warn.Printf("read: ReadAt: %s", err.Error()) return nil, fuse.ToStatus(err) @@ -173,6 +176,8 @@ func (f *file) doRead(off uint64, length uint64) ([]byte, fuse.Status) { // Read - FUSE call func (f *file) Read(buf []byte, off int64) (resultData fuse.ReadResult, code fuse.Status) { + f.fdLock.RLock() + defer f.fdLock.RUnlock() cryptfs.Debug.Printf("ino%d: FUSE Read: offset=%d length=%d", f.ino, len(buf), off) @@ -231,7 +236,7 @@ func (f *file) doWrite(data []byte, off int64) (uint32, fuse.Status) { o, _ := b.PlaintextRange() oldData, status := f.doRead(o, f.cfs.PlainBS()) if status != fuse.OK { - cryptfs.Warn.Printf("RMW read failed: %s", status.String()) + cryptfs.Warn.Printf("ino%d fh%d: RMW read failed: %s", f.ino, f.intFd(), status.String()) return written, status } // Modify @@ -246,19 +251,15 @@ func (f *file) doWrite(data []byte, off int64) (uint32, fuse.Status) { f.ino, uint64(len(blockData))-f.cfs.BlockOverhead(), b.BlockNo) // Prevent partially written (=corrupt) blocks by preallocating the space beforehand - f.fdLock.Lock() err := prealloc(int(f.fd.Fd()), int64(blockOffset), int64(blockLen)) - f.fdLock.Unlock() if err != nil { - cryptfs.Warn.Printf("doWrite: fallocateRetry failed: %s", err.Error()) + cryptfs.Warn.Printf("ino%d fh%d: doWrite: prealloc failed: %s", f.ino, f.intFd(), err.Error()) status = fuse.ToStatus(err) break } // Write - f.fdLock.Lock() _, err = f.fd.WriteAt(blockData, int64(blockOffset)) - f.fdLock.Unlock() if err != nil { cryptfs.Warn.Printf("doWrite: Write failed: %s", err.Error()) @@ -272,10 +273,13 @@ func (f *file) doWrite(data []byte, off int64) (uint32, fuse.Status) { // Write - FUSE call func (f *file) Write(data []byte, off int64) (uint32, fuse.Status) { - cryptfs.Debug.Printf("ino%d: FUSE Write: offset=%d length=%d", f.ino, off, len(data)) + f.fdLock.RLock() + defer f.fdLock.RUnlock() wlock.lock(f.ino) defer wlock.unlock(f.ino) + cryptfs.Debug.Printf("ino%d: FUSE Write: offset=%d length=%d", f.ino, off, len(data)) + fi, err := f.fd.Stat() if err != nil { cryptfs.Warn.Printf("Write: Fstat failed: %v", err) @@ -292,23 +296,24 @@ func (f *file) Write(data []byte, off int64) (uint32, fuse.Status) { return f.doWrite(data, off) } -// Release - FUSE call, forget file +// Release - FUSE call, close file func (f *file) Release() { f.fdLock.Lock() + defer f.fdLock.Unlock() + f.fd.Close() - f.fdLock.Unlock() - wlock.unregister(f.ino) + f.forgotten = true } // Flush - FUSE call func (f *file) Flush() fuse.Status { - f.fdLock.Lock() + f.fdLock.RLock() + defer f.fdLock.RUnlock() // Since Flush() may be called for each dup'd fd, we don't // want to really close the file, we just want to flush. This // is achieved by closing a dup'd fd. newFd, err := syscall.Dup(int(f.fd.Fd())) - f.fdLock.Unlock() if err != nil { return fuse.ToStatus(err) @@ -318,25 +323,28 @@ func (f *file) Flush() fuse.Status { } func (f *file) Fsync(flags int) (code fuse.Status) { - f.fdLock.Lock() - r := fuse.ToStatus(syscall.Fsync(int(f.fd.Fd()))) - f.fdLock.Unlock() + f.fdLock.RLock() + defer f.fdLock.RUnlock() - return r + return fuse.ToStatus(syscall.Fsync(int(f.fd.Fd()))) } // Truncate - FUSE call func (f *file) Truncate(newSize uint64) fuse.Status { + f.fdLock.RLock() + defer f.fdLock.RUnlock() wlock.lock(f.ino) defer wlock.unlock(f.ino) + if f.forgotten { + cryptfs.Warn.Printf("ino%d fh%d: Truncate on forgotten file", f.ino, f.intFd()) + } + // Common case first: Truncate to zero if newSize == 0 { - f.fdLock.Lock() err := syscall.Ftruncate(int(f.fd.Fd()), 0) - f.fdLock.Unlock() if err != nil { - cryptfs.Warn.Printf("ino%d: Ftruncate(fd, 0) returned error: %v", f.ino, err) + cryptfs.Warn.Printf("ino%d fh%d: Ftruncate(fd, 0) returned error: %v", f.ino, f.intFd(), err) return fuse.ToStatus(err) } // Truncate to zero kills the file header @@ -348,7 +356,7 @@ func (f *file) Truncate(newSize uint64) fuse.Status { // the file fi, err := f.fd.Stat() if err != nil { - cryptfs.Warn.Printf("ino%d: Truncate: Fstat failed: %v", f.ino, err) + cryptfs.Warn.Printf("ino%d fh%d: Truncate: Fstat failed: %v", f.ino, f.intFd(), err) return fuse.ToStatus(err) } oldSize := f.cfs.CipherSizeToPlainSize(uint64(fi.Size())) @@ -358,6 +366,11 @@ func (f *file) Truncate(newSize uint64) fuse.Status { cryptfs.Debug.Printf("ino%d: FUSE Truncate from %.2f to %.2f blocks (%d to %d bytes)", f.ino, oldB, newB, oldSize, newSize) } + // File size stays the same - nothing to do + if newSize == oldSize { + return fuse.OK + } + // File grows if newSize > oldSize { @@ -381,9 +394,7 @@ func (f *file) Truncate(newSize uint64) fuse.Status { } } else { off, length := b.CiphertextRange() - f.fdLock.Lock() err := syscall.Ftruncate(int(f.fd.Fd()), int64(off+length)) - f.fdLock.Unlock() if err != nil { cryptfs.Warn.Printf("grow Ftruncate returned error: %v", err) return fuse.ToStatus(err) @@ -407,9 +418,7 @@ func (f *file) Truncate(newSize uint64) fuse.Status { } } // Truncate down to last complete block - f.fdLock.Lock() err = syscall.Ftruncate(int(f.fd.Fd()), int64(cipherOff)) - f.fdLock.Unlock() if err != nil { cryptfs.Warn.Printf("shrink Ftruncate returned error: %v", err) return fuse.ToStatus(err) @@ -424,27 +433,26 @@ func (f *file) Truncate(newSize uint64) fuse.Status { } func (f *file) Chmod(mode uint32) fuse.Status { - f.fdLock.Lock() - r := fuse.ToStatus(f.fd.Chmod(os.FileMode(mode))) - f.fdLock.Unlock() + f.fdLock.RLock() + defer f.fdLock.RUnlock() - return r + return fuse.ToStatus(f.fd.Chmod(os.FileMode(mode))) } func (f *file) Chown(uid uint32, gid uint32) fuse.Status { - f.fdLock.Lock() - r := fuse.ToStatus(f.fd.Chown(int(uid), int(gid))) - f.fdLock.Unlock() + f.fdLock.RLock() + defer f.fdLock.RUnlock() - return r + return fuse.ToStatus(f.fd.Chown(int(uid), int(gid))) } func (f *file) GetAttr(a *fuse.Attr) fuse.Status { + f.fdLock.RLock() + defer f.fdLock.RUnlock() + cryptfs.Debug.Printf("file.GetAttr()") st := syscall.Stat_t{} - f.fdLock.Lock() err := syscall.Fstat(int(f.fd.Fd()), &st) - f.fdLock.Unlock() if err != nil { return fuse.ToStatus(err) } @@ -469,6 +477,9 @@ func (f *file) Allocate(off uint64, sz uint64, mode uint32) fuse.Status { const _UTIME_OMIT = ((1 << 30) - 2) func (f *file) Utimens(a *time.Time, m *time.Time) fuse.Status { + f.fdLock.RLock() + defer f.fdLock.RUnlock() + ts := make([]syscall.Timespec, 2) if a == nil { @@ -483,9 +494,6 @@ func (f *file) Utimens(a *time.Time, m *time.Time) fuse.Status { ts[1].Sec = m.Unix() } - f.fdLock.Lock() fn := fmt.Sprintf("/proc/self/fd/%d", f.fd.Fd()) - err := syscall.UtimesNano(fn, ts) - f.fdLock.Unlock() - return fuse.ToStatus(err) + return fuse.ToStatus(syscall.UtimesNano(fn, ts)) } -- cgit v1.2.3