From c1c938387d9db41c45bf90be7a48d0518d66c8c1 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Wed, 23 Feb 2022 10:36:13 +0100 Subject: [PATCH 1/4] Keep the lock structs in a local map, protected by a mutex. There must be only one Flock struct per lock file. If that is existing, no other routine or process may get a lock. --- pkg/storage/utils/filelocks/filelocks.go | 104 ++++++++++++++++++++--- 1 file changed, 90 insertions(+), 14 deletions(-) diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go index 7a79b1abc0..02391ee259 100644 --- a/pkg/storage/utils/filelocks/filelocks.go +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -21,19 +21,64 @@ package filelocks import ( "errors" "os" + "sync" "time" "github.com/gofrs/flock" ) -// FlockFile returns the flock filename for a given file name -// it returns an empty string if the input is empty -func FlockFile(file string) string { - var n string - if len(file) > 0 { - n = file + ".flock" +// Locks stores the local Flock structs in a map by their file names. +// That is needed because each of the struct contains a mutex that the +// gofrs/flock module is using. Thus, there must only be one Flock struct +// per file. +type Locks struct { + mu sync.Mutex + // + _locks map[string]*flock.Flock +} + +var localLocks Locks + +func init() { + localLocks._locks = make(map[string]*flock.Flock) +} + +// getMutexedFlock returns a new Flock struct for the given file. +// If there is already one in the local store, it returns nil. +// The caller has to wait until it can get a new one out of this +// mehtod. +func getMutexedFlock(file string) *flock.Flock { + // the local data structure to keep the lock structs is mutex protected so that + // only one routine can access it. + localLocks.mu.Lock() + defer localLocks.mu.Unlock() + + // Is there lock already? + if _, ok := localLocks._locks[file]; ok { + // There is already a lock for this file, another can not be acquired + return nil + } + + // Acquire the write log on the target node first. + localLocks._locks[file] = flock.New(file) + return localLocks._locks[file] + +} + +// releaseMutexedFlock releases a Flock object that was acquired +// before by the getMutexedFlock function. +func releaseMutexedFlock(file string) { + if len(file) == 0 { + return + } + + localLocks.mu.Lock() + defer localLocks.mu.Unlock() + + _, ok := localLocks._locks[file] + if ok { + delete(localLocks._locks, file) } - return n } // acquireWriteLog acquires a lock on a file or directory. @@ -47,15 +92,27 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { if len(n) == 0 { return nil, errors.New("lock path is empty") } - // Acquire the write log on the target node first. - lock := flock.New(n) + + var flock *flock.Flock + for i := 1; i <= 10; i++ { + if flock = getMutexedFlock(n); flock != nil { + break + } + w := time.Duration(i*3) * time.Millisecond + // fmt.Printf("Waiting for lock to release %d\n", w) + + time.Sleep(w) + } + if flock == nil { + return nil, errors.New("unable to acquire a lock on the file") + } var ok bool for i := 1; i <= 10; i++ { if write { - ok, err = lock.TryLock() + ok, err = flock.TryLock() } else { - ok, err = lock.TryRLock() + ok, err = flock.TryRLock() } if ok { @@ -72,7 +129,18 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { if err != nil { return nil, err } - return lock, nil + // fmt.Printf("Returning flock for %s\n", flock.Path()) + return flock, nil +} + +// FlockFile returns the flock filename for a given file name +// it returns an empty string if the input is empty +func FlockFile(file string) string { + var n string + if len(file) > 0 { + n = file + ".flock" + } + return n } // AcquireReadLock tries to acquire a shared lock to read from the @@ -93,9 +161,17 @@ func ReleaseLock(lock *flock.Flock) error { // there is a probability that if the file can not be unlocked, // we also can not remove the file. We will only try to remove if it // was successfully unlocked. - err := lock.Unlock() + var err error + n := lock.Path() + // There is already a lock for this file + + err = lock.Unlock() if err == nil { - err = os.Remove(lock.Path()) + err = os.Remove(n) + // fmt.Printf("Removing flock for %s\n", n) + } + releaseMutexedFlock(n) + return err } From b1112e59d8c4b6a3c15c3ed09ccb2d8cf6c0a885 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Wed, 23 Feb 2022 10:42:26 +0100 Subject: [PATCH 2/4] Add Changelog --- changelog/unreleased/locksMutexed.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/locksMutexed.md diff --git a/changelog/unreleased/locksMutexed.md b/changelog/unreleased/locksMutexed.md new file mode 100644 index 0000000000..aeb2a90b5c --- /dev/null +++ b/changelog/unreleased/locksMutexed.md @@ -0,0 +1,6 @@ +Bugfix: keep lock structs in a local map protected by a mutex + +Make sure that only one go routine or process can get the +lock. + +https://github.com/cs3org/reva/pull/2582 From 0628ef1da01ce1d58a3625cb189c00158bbf0da5 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 24 Feb 2022 10:51:59 +0100 Subject: [PATCH 3/4] Use sync.Map for a thread save local storage of locks. Some more cleanups. --- pkg/storage/utils/filelocks/filelocks.go | 41 +++++------------------- 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go index 02391ee259..c5a7eae552 100644 --- a/pkg/storage/utils/filelocks/filelocks.go +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -27,20 +27,10 @@ import ( "github.com/gofrs/flock" ) -// Locks stores the local Flock structs in a map by their file names. -// That is needed because each of the struct contains a mutex that the -// gofrs/flock module is using. Thus, there must only be one Flock struct -// per file. -type Locks struct { - mu sync.Mutex - // - _locks map[string]*flock.Flock -} - -var localLocks Locks +var _localLocks sync.Map func init() { - localLocks._locks = make(map[string]*flock.Flock) + } // getMutexedFlock returns a new Flock struct for the given file. @@ -48,36 +38,25 @@ func init() { // The caller has to wait until it can get a new one out of this // mehtod. func getMutexedFlock(file string) *flock.Flock { - // the local data structure to keep the lock structs is mutex protected so that - // only one routine can access it. - localLocks.mu.Lock() - defer localLocks.mu.Unlock() // Is there lock already? - if _, ok := localLocks._locks[file]; ok { + if _, ok := _localLocks.Load(file); ok { // There is already a lock for this file, another can not be acquired return nil } // Acquire the write log on the target node first. - localLocks._locks[file] = flock.New(file) - return localLocks._locks[file] + l := flock.New(file) + _localLocks.Store(file, l) + return l } // releaseMutexedFlock releases a Flock object that was acquired // before by the getMutexedFlock function. func releaseMutexedFlock(file string) { - if len(file) == 0 { - return - } - - localLocks.mu.Lock() - defer localLocks.mu.Unlock() - - _, ok := localLocks._locks[file] - if ok { - delete(localLocks._locks, file) + if len(file) > 0 { + _localLocks.Delete(file) } } @@ -99,7 +78,6 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { break } w := time.Duration(i*3) * time.Millisecond - // fmt.Printf("Waiting for lock to release %d\n", w) time.Sleep(w) } @@ -129,7 +107,6 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { if err != nil { return nil, err } - // fmt.Printf("Returning flock for %s\n", flock.Path()) return flock, nil } @@ -168,8 +145,6 @@ func ReleaseLock(lock *flock.Flock) error { err = lock.Unlock() if err == nil { err = os.Remove(n) - // fmt.Printf("Removing flock for %s\n", n) - } releaseMutexedFlock(n) From 55000244b782e81f23030b547f0a2a630d1de251 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 24 Feb 2022 11:43:48 +0100 Subject: [PATCH 4/4] Remove unused empty init() function. --- pkg/storage/utils/filelocks/filelocks.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go index c5a7eae552..b392116f8a 100644 --- a/pkg/storage/utils/filelocks/filelocks.go +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -29,10 +29,6 @@ import ( var _localLocks sync.Map -func init() { - -} - // getMutexedFlock returns a new Flock struct for the given file. // If there is already one in the local store, it returns nil. // The caller has to wait until it can get a new one out of this