From 353f2d3a6e30f595a721abcd9cf1b108b041ee21 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 13 Jun 2017 08:29:07 -0700 Subject: [PATCH] fs: Hold `format.json` readLock ref to avoid GC. (#4532) Looks like if we follow pattern such as ``` _ = rlk ``` Go can potentially kick in GC and close the fd when the reference is lost, only speculation is that the cause here is `SetFinalizer` which is set on `os.close()` internally in `os` stdlib. This is unexpected and unsual endeavour for Go, but we have to make sure the reference is never lost and always dies with the server. Fixes #4530 --- cmd/fs-v1-metadata.go | 48 +++++++++++++++------------------------- cmd/fs-v1.go | 18 +++++++++++---- pkg/lock/lock_solaris.go | 2 +- 3 files changed, 33 insertions(+), 35 deletions(-) diff --git a/cmd/fs-v1-metadata.go b/cmd/fs-v1-metadata.go index 29c595ed2..935783a78 100644 --- a/cmd/fs-v1-metadata.go +++ b/cmd/fs-v1-metadata.go @@ -290,17 +290,23 @@ func checkLockedValidFormatFS(fsPath string) (*lock.RLockedFile, error) { return rlk, traceError(err) } -// Writes the new format.json if unformatted, -// otherwise closes the input locked file -// and returns any error. -func writeFormatFS(lk *lock.LockedFile) error { +// Creates a new format.json if unformatted. +func createFormatFS(fsPath string) error { + fsFormatPath := pathJoin(fsPath, minioMetaBucket, formatConfigFile) + + // Attempt a write lock on formatConfigFile `format.json` + // file stored in minioMetaBucket(.minio.sys) directory. + lk, err := lock.TryLockedOpenFile(preparePath(fsFormatPath), os.O_RDWR|os.O_CREATE, 0600) + if err != nil { + return traceError(err) + } // Close the locked file upon return. defer lk.Close() // Load format on disk, checks if we are unformatted // writes the new format.json var format = &formatConfigV1{} - err := format.LoadFormat(lk) + err = format.LoadFormat(lk) if errorCause(err) == errUnformattedDisk { _, err = newFSFormat().WriteTo(lk) return err @@ -308,20 +314,12 @@ func writeFormatFS(lk *lock.LockedFile) error { return err } -func initFormatFS(fsPath, fsUUID string) (err error) { - fsFormatPath := pathJoin(fsPath, minioMetaBucket, formatConfigFile) - - // Once the filesystem has initialized hold the read lock for - // the life time of the server. This is done to ensure that under - // shared backend mode for FS, remote servers do not migrate - // or cause changes on backend format. - +func initFormatFS(fsPath string) (rlk *lock.RLockedFile, err error) { // This loop validates format.json by holding a read lock and // proceeds if disk unformatted to hold non-blocking WriteLock // If for some reason non-blocking WriteLock fails and the error // is lock.ErrAlreadyLocked i.e some other process is holding a // lock we retry in the loop again. - var rlk *lock.RLockedFile for { // Validate the `format.json` for expected values. rlk, err = checkLockedValidFormatFS(fsPath) @@ -330,34 +328,24 @@ func initFormatFS(fsPath, fsUUID string) (err error) { // Holding a read lock ensures that any write lock operation // is blocked if attempted in-turn avoiding corruption on // the backend disk. - _ = rlk // Hold the lock on `format.json` until server dies. - return nil + return rlk, nil case errorCause(err) == errUnformattedDisk: - // Attempt a write lock on formatConfigFile `format.json` - // file stored in minioMetaBucket(.minio.sys) directory. - var lk *lock.LockedFile - lk, err = lock.TryLockedOpenFile(preparePath(fsFormatPath), os.O_RDWR|os.O_CREATE, 0600) - if err != nil { + if err = createFormatFS(fsPath); err != nil { // Existing write locks detected. - if err == lock.ErrAlreadyLocked { + if errorCause(err) == lock.ErrAlreadyLocked { // Lock already present, sleep and attempt again. time.Sleep(100 * time.Millisecond) continue } // Unexpected error, return. - return traceError(err) + return nil, err } - // Write new format. - if err = writeFormatFS(lk); err != nil { - return err - } - // Loop will continue to attempt a - // read-lock on `format.json` . + // Loop will continue to attempt a read-lock on `format.json`. default: // Unhandled error return. - return err + return nil, err } } } diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go index 6f9d0a9f1..c8d0db1d6 100644 --- a/cmd/fs-v1.go +++ b/cmd/fs-v1.go @@ -42,6 +42,9 @@ type fsObjects struct { // temporary transactions. fsUUID string + // This value shouldn't be touched, once initialized. + fsFormatRlk *lock.RLockedFile // Is a read lock on `format.json`. + // FS rw pool. rwPool *fsIOPool @@ -109,6 +112,12 @@ func newFSObjectLayer(fsPath string) (ObjectLayer, error) { return nil, fmt.Errorf("Unable to initialize '.minio.sys' meta volume, %s", err) } + // Initialize `format.json`, this function also returns. + rlk, err := initFormatFS(fsPath) + if err != nil { + return nil, err + } + // Initialize fs objects. fs := &fsObjects{ fsPath: fsPath, @@ -122,10 +131,11 @@ func newFSObjectLayer(fsPath string) (ObjectLayer, error) { }, } - // Initialize `format.json`. - if err = initFormatFS(fsPath, fsUUID); err != nil { - return nil, err - } + // Once the filesystem has initialized hold the read lock for + // the life time of the server. This is done to ensure that under + // shared backend mode for FS, remote servers do not migrate + // or cause changes on backend format. + fs.fsFormatRlk = rlk // Initialize and load bucket policies. err = initBucketPolicies(fs) diff --git a/pkg/lock/lock_solaris.go b/pkg/lock/lock_solaris.go index d4a4fbe42..ba1f61816 100644 --- a/pkg/lock/lock_solaris.go +++ b/pkg/lock/lock_solaris.go @@ -58,7 +58,7 @@ func lockedOpenFile(path string, flag int, perm os.FileMode, rlockType int) (*Lo if err = syscall.FcntlFlock(f.Fd(), rlockType, &lock); err != nil { f.Close() if err == syscall.EAGAIN { - err = ErrLocked + err = ErrAlreadyLocked } return nil, err }