From 9bf1e61bf745fa548885730a4985733ebb78b98b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 10 Feb 2022 16:23:33 +0000 Subject: [PATCH 1/9] xattrs: use unix flocks when writing multiple xattrs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../unreleased/flock-xattr-set-multiple.md | 5 +++++ .../utils/decomposedfs/xattrs/xattrs.go | 20 ++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 changelog/unreleased/flock-xattr-set-multiple.md diff --git a/changelog/unreleased/flock-xattr-set-multiple.md b/changelog/unreleased/flock-xattr-set-multiple.md new file mode 100644 index 0000000000..2cf0270669 --- /dev/null +++ b/changelog/unreleased/flock-xattr-set-multiple.md @@ -0,0 +1,5 @@ +Enhancement: use an exclcusive write lock when writing multiple attributes + +The xattr package can use an exclusive write lock when writing multiple extended attributes + +https://github.com/cs3org/reva/pull/2528 \ No newline at end of file diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index ea14aaa59f..fe730cbe93 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -19,11 +19,13 @@ package xattrs import ( + "os" "strconv" "strings" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/pkg/xattr" + "golang.org/x/sys/unix" ) // Declare a list of xattr keys @@ -142,12 +144,24 @@ func Set(filePath string, key string, val string) error { } // SetMultiple allows setting multiple key value pairs at once -// TODO the changes are protected with an flock +// the changes are protected with an flock func SetMultiple(filePath string, attribs map[string]string) error { - // FIXME: Lock here + // h, err := lockedfile.OpenFile(filePath, os.O_WRONLY, 0) // 0? Open File only workn for files ... but we want to lock dirs ... or symlinks + // or we append .lock to the file and use https://github.com/gofrs/flock + h, err := os.Open(filePath) + if err != nil { + return err + } + defer h.Close() + err = unix.Flock(int(h.Fd()), unix.LOCK_EX) + if err != nil { + return err + } + defer unix.Flock(int(h.Fd()), unix.LOCK_UN) + for key, val := range attribs { - if err := Set(filePath, key, val); err != nil { + if err := xattr.FSet(h, key, []byte(val)); err != nil { return err } } From d797d66b965851d0bc59d943f21a607b75f51282 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Mon, 14 Feb 2022 14:22:33 +0100 Subject: [PATCH 2/9] Implement locking of files when setting and reading xattrs To lock, a file with the ending .flock is created beside the to be locked node (which also can be a directory) which is locked with a Lock. Other ocis processes respect that. --- go.mod | 1 + go.sum | 2 + .../utils/decomposedfs/xattrs/xattrs.go | 129 ++++++++++++++---- 3 files changed, 107 insertions(+), 25 deletions(-) diff --git a/go.mod b/go.mod index e61ef61a4d..20ab718e7e 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( github.com/go-openapi/errors v0.20.1 // indirect github.com/go-openapi/strfmt v0.20.2 // indirect github.com/go-sql-driver/mysql v1.6.0 + github.com/gofrs/flock v0.8.1 // indirect github.com/golang-jwt/jwt v3.2.2+incompatible github.com/golang/protobuf v1.5.2 github.com/gomodule/redigo v1.8.8 diff --git a/go.sum b/go.sum index fea989cc02..f91f4bf868 100644 --- a/go.sum +++ b/go.sum @@ -419,6 +419,8 @@ github.com/gobwas/pool v0.2.1/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6Wezm github.com/gobwas/ws v1.0.4/go.mod h1:szmBTxLgaFppYjEmNtny/v3w89xOydFnnZMcgRRu/EM= github.com/godbus/dbus/v5 v5.0.3/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= github.com/godbus/dbus/v5 v5.0.4/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= +github.com/gofrs/flock v0.8.1 h1:+gYjHKf32LDeiEEFhQaotPbLuUXjY5ZqxKgXy7n59aw= +github.com/gofrs/flock v0.8.1/go.mod h1:F1TvTiK9OcQqauNUHlbJvyl9Qa1QvF/gOUDKA14jxHU= github.com/gofrs/uuid v3.2.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gofrs/uuid v4.0.0+incompatible h1:1SD/1F5pU8p29ybwgQSwpQk+mwdRrXCYuPhW6m+TnJw= github.com/gofrs/uuid v4.0.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index fe730cbe93..00ea65d44b 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -19,13 +19,13 @@ package xattrs import ( - "os" "strconv" "strings" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/gofrs/flock" + "github.com/pkg/errors" "github.com/pkg/xattr" - "golang.org/x/sys/unix" ) // Declare a list of xattr keys @@ -112,29 +112,97 @@ func refFromCS3(b []byte) (*provider.Reference, error) { }, 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 +} + +// acquireWriteLog acquires a lock on a file or directory. +// if the parameter write is true, it gets an exclusive write lock, otherwise a shared read lock. +// The function returns a Flock object, unlocking has to be done in the calling function. +func acquireLock(file string, write bool) (*flock.Flock, error) { + var err error + + // Create the a file to carry the log + n := flockFile(file) + if len(n) == 0 { + return nil, errors.New("Path empty") + } + // Acquire the write log on the target node first. + lock := flock.New(n) + + if write { + _, err = lock.TryLock() + } else { + _, err = lock.TryRLock() + } + + if err != nil { + return nil, err + } + return lock, nil +} + // CopyMetadata copies all extended attributes from source to target. // The optional filter function can be used to filter by attribute name, e.g. by checking a prefix -func CopyMetadata(s, t string, filter func(attributeName string) bool) error { - var attrs []string +// For the source file, a shared lock is acquired. For the target, an exclusive +// write lock is acquired. +func CopyMetadata(src, target string, filter func(attributeName string) bool) error { var err error - if attrs, err = xattr.List(s); err != nil { - return err + var writeLock, readLock *flock.Flock + + // Acquire the write log on the target node first. + writeLock, err = acquireLock(target, true) + + if err != nil { + return errors.Wrap(err, "xattrs: Unable to lock target to write") } - for i := range attrs { - if filter == nil || filter(attrs[i]) { - b, err := xattr.Get(s, attrs[i]) - if err != nil { - return err + defer func() { + err = writeLock.Unlock() + }() + + // now try to get a shared lock on the source + readLock, err = acquireLock(src, false) + + if err != nil { + return errors.Wrap(err, "xattrs: Unable to lock file for read") + } + defer func() { + err = readLock.Unlock() + }() + + // both locks are established. Copy. + var attrNameList []string + if attrNameList, err = xattr.List(src); err != nil { + return errors.Wrap(err, "Can not get xattr listing on src") + } + + // FIXME: this loop assumes that all reads and writes work. The case that one val + // is copied, but any others not is not handled properly. + for idx := range attrNameList { + attrName := attrNameList[idx] + if filter == nil || filter(attrName) { + var attrVal []byte + if attrVal, err = xattr.Get(src, attrName); err != nil { + return errors.Wrapf(err, "Can not read src attribute named %s", attrName) } - if err := xattr.Set(t, attrs[i], b); err != nil { - return err + if err = xattr.Set(target, attrName, attrVal); err != nil { + return errors.Wrapf(err, "Can not write target attribute named %s", attrName) } } } + return nil } // Set an extended attribute key to the given value +// No file locking is involved here as writing a single xattr is +// considered to be atomic. func Set(filePath string, key string, val string) error { if err := xattr.Set(filePath, key, []byte(val)); err != nil { @@ -144,24 +212,24 @@ func Set(filePath string, key string, val string) error { } // SetMultiple allows setting multiple key value pairs at once -// the changes are protected with an flock +// the changes are protected with an file lock +// If the file lock can not be acquired the function returns a +// lock error. func SetMultiple(filePath string, attribs map[string]string) error { // h, err := lockedfile.OpenFile(filePath, os.O_WRONLY, 0) // 0? Open File only workn for files ... but we want to lock dirs ... or symlinks // or we append .lock to the file and use https://github.com/gofrs/flock - h, err := os.Open(filePath) - if err != nil { - return err - } - defer h.Close() - err = unix.Flock(int(h.Fd()), unix.LOCK_EX) + fileLock, err := acquireLock(filePath, true) + if err != nil { - return err + return errors.Wrap(err, "xattrs: Can not acquired write log") } - defer unix.Flock(int(h.Fd()), unix.LOCK_UN) + defer func() { + err = fileLock.Unlock() + }() for key, val := range attribs { - if err := xattr.FSet(h, key, []byte(val)); err != nil { + if err := xattr.Set(filePath, key, []byte(val)); err != nil { return err } } @@ -169,8 +237,9 @@ func SetMultiple(filePath string, attribs map[string]string) error { } // Get an extended attribute value for the given key +// No file locking is involved here as reading a single xattr is +// considered to be atomic. func Get(filePath, key string) (string, error) { - v, err := xattr.Get(filePath, key) if err != nil { return "", err @@ -192,8 +261,18 @@ func GetInt64(filePath, key string) (int64, error) { return v, nil } -// All reads all extended attributes for a node +// All reads all extended attributes for a node, protected by a +// shared file lock func All(filePath string) (map[string]string, error) { + fileLock, err := acquireLock(filePath, false) + + if err != nil { + return nil, errors.Wrap(err, "xattrs: Unable to lock file for read") + } + defer func() { + err = fileLock.Unlock() + }() + attrNames, err := xattr.List(filePath) if err != nil { return nil, err From caa4f7479ad99d575e17505c1599438ddb8d1377 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 17 Feb 2022 10:52:44 +0100 Subject: [PATCH 3/9] Improve error handling of the xattr functions handling multiple data. --- go.mod | 2 +- .../utils/decomposedfs/xattrs/xattrs.go | 104 +++++++++++++----- 2 files changed, 80 insertions(+), 26 deletions(-) diff --git a/go.mod b/go.mod index 20ab718e7e..5b0eebcdb5 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/go-openapi/errors v0.20.1 // indirect github.com/go-openapi/strfmt v0.20.2 // indirect github.com/go-sql-driver/mysql v1.6.0 - github.com/gofrs/flock v0.8.1 // indirect + github.com/gofrs/flock v0.8.1 github.com/golang-jwt/jwt v3.2.2+incompatible github.com/golang/protobuf v1.5.2 github.com/gomodule/redigo v1.8.8 diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index 00ea65d44b..e5ba278d9d 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -19,6 +19,7 @@ package xattrs import ( + "os" "strconv" "strings" @@ -148,12 +149,22 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { return lock, nil } +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() + if err == nil { + err = os.Remove(lock.Path()) + } + return err +} + // CopyMetadata copies all extended attributes from source to target. // The optional filter function can be used to filter by attribute name, e.g. by checking a prefix // For the source file, a shared lock is acquired. For the target, an exclusive // write lock is acquired. -func CopyMetadata(src, target string, filter func(attributeName string) bool) error { - var err error +func CopyMetadata(src, target string, filter func(attributeName string) bool) (err error) { var writeLock, readLock *flock.Flock // Acquire the write log on the target node first. @@ -163,7 +174,12 @@ func CopyMetadata(src, target string, filter func(attributeName string) bool) er return errors.Wrap(err, "xattrs: Unable to lock target to write") } defer func() { - err = writeLock.Unlock() + rerr := releaseLock(writeLock) + + // if err is non nil we do not overwrite that + if err == nil { + err = rerr + } }() // now try to get a shared lock on the source @@ -173,7 +189,12 @@ func CopyMetadata(src, target string, filter func(attributeName string) bool) er return errors.Wrap(err, "xattrs: Unable to lock file for read") } defer func() { - err = readLock.Unlock() + rerr := releaseLock(readLock) + + // if err is non nil we do not overwrite that + if err == nil { + err = rerr + } }() // both locks are established. Copy. @@ -182,22 +203,27 @@ func CopyMetadata(src, target string, filter func(attributeName string) bool) er return errors.Wrap(err, "Can not get xattr listing on src") } - // FIXME: this loop assumes that all reads and writes work. The case that one val - // is copied, but any others not is not handled properly. + // error handling: We count errors of reads or writes of xattrs. + // if there were any read or write errors an error is returned. + var xerrs int = 0 + var xerr error for idx := range attrNameList { attrName := attrNameList[idx] if filter == nil || filter(attrName) { var attrVal []byte - if attrVal, err = xattr.Get(src, attrName); err != nil { - return errors.Wrapf(err, "Can not read src attribute named %s", attrName) + if attrVal, xerr = xattr.Get(src, attrName); xerr != nil { + xerrs++ } - if err = xattr.Set(target, attrName, attrVal); err != nil { - return errors.Wrapf(err, "Can not write target attribute named %s", attrName) + if xerr = xattr.Set(target, attrName, attrVal); xerr != nil { + xerrs++ } } } + if xerrs > 0 { + err = errors.Wrap(xerr, "Failed to copy all xattrs, last error returned.") + } - return nil + return err } // Set an extended attribute key to the given value @@ -215,25 +241,38 @@ func Set(filePath string, key string, val string) error { // the changes are protected with an file lock // If the file lock can not be acquired the function returns a // lock error. -func SetMultiple(filePath string, attribs map[string]string) error { +func SetMultiple(filePath string, attribs map[string]string) (err error) { // h, err := lockedfile.OpenFile(filePath, os.O_WRONLY, 0) // 0? Open File only workn for files ... but we want to lock dirs ... or symlinks // or we append .lock to the file and use https://github.com/gofrs/flock - fileLock, err := acquireLock(filePath, true) + var fileLock *flock.Flock + fileLock, err = acquireLock(filePath, true) if err != nil { return errors.Wrap(err, "xattrs: Can not acquired write log") } defer func() { - err = fileLock.Unlock() + rerr := releaseLock(fileLock) + + // if err is non nil we do not overwrite that + if err == nil { + err = rerr + } }() + // error handling: Count if there are errors while setting the attribs. + // if there were any, return an error. + var xerrs int = 0 for key, val := range attribs { - if err := xattr.Set(filePath, key, []byte(val)); err != nil { - return err + if xerr := xattr.Set(filePath, key, []byte(val)); xerr != nil { + // log + xerrs++ } } - return nil + if xerrs > 0 { + err = errors.New("Failed to set all xattrs") + } + return err } // Get an extended attribute value for the given key @@ -263,14 +302,21 @@ func GetInt64(filePath, key string) (int64, error) { // All reads all extended attributes for a node, protected by a // shared file lock -func All(filePath string) (map[string]string, error) { - fileLock, err := acquireLock(filePath, false) +func All(filePath string) (attribs map[string]string, err error) { + var fileLock *flock.Flock + + fileLock, err = acquireLock(filePath, false) if err != nil { return nil, errors.Wrap(err, "xattrs: Unable to lock file for read") } defer func() { - err = fileLock.Unlock() + rerr := releaseLock(fileLock) + + // if err is non nil we do not overwrite that + if err == nil { + err = rerr + } }() attrNames, err := xattr.List(filePath) @@ -278,13 +324,21 @@ func All(filePath string) (map[string]string, error) { return nil, err } - attribs := make(map[string]string, len(attrNames)) + var xerrs int = 0 + + // error handling: Count if there are errors while reading all attribs. + // if there were any, return an error. + attribs = make(map[string]string, len(attrNames)) for _, name := range attrNames { - val, err := xattr.Get(filePath, name) - if err != nil { - return nil, err + if val, xerr := xattr.Get(filePath, name); xerr != nil { + xerrs++ + } else { + attribs[name] = string(val) } - attribs[name] = string(val) + } + + if xerrs > 0 { + err = errors.New("Failed to read all xattrs") } return attribs, nil From e35c94b537d37b8a9deb50ca2ca162c97fdc3849 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 17 Feb 2022 11:22:07 +0100 Subject: [PATCH 4/9] Move file lock functions to their own module. --- .../utils/decomposedfs/xattrs/xattrs.go | 65 ++------------ pkg/storage/utils/filelocks/filelocks.go | 85 +++++++++++++++++++ 2 files changed, 94 insertions(+), 56 deletions(-) create mode 100644 pkg/storage/utils/filelocks/filelocks.go diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index e5ba278d9d..3d4a0ab481 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -19,11 +19,11 @@ package xattrs import ( - "os" "strconv" "strings" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/cs3org/reva/pkg/storage/utils/filelocks" "github.com/gofrs/flock" "github.com/pkg/errors" "github.com/pkg/xattr" @@ -113,53 +113,6 @@ func refFromCS3(b []byte) (*provider.Reference, error) { }, 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 -} - -// acquireWriteLog acquires a lock on a file or directory. -// if the parameter write is true, it gets an exclusive write lock, otherwise a shared read lock. -// The function returns a Flock object, unlocking has to be done in the calling function. -func acquireLock(file string, write bool) (*flock.Flock, error) { - var err error - - // Create the a file to carry the log - n := flockFile(file) - if len(n) == 0 { - return nil, errors.New("Path empty") - } - // Acquire the write log on the target node first. - lock := flock.New(n) - - if write { - _, err = lock.TryLock() - } else { - _, err = lock.TryRLock() - } - - if err != nil { - return nil, err - } - return lock, nil -} - -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() - if err == nil { - err = os.Remove(lock.Path()) - } - return err -} - // CopyMetadata copies all extended attributes from source to target. // The optional filter function can be used to filter by attribute name, e.g. by checking a prefix // For the source file, a shared lock is acquired. For the target, an exclusive @@ -168,13 +121,13 @@ func CopyMetadata(src, target string, filter func(attributeName string) bool) (e var writeLock, readLock *flock.Flock // Acquire the write log on the target node first. - writeLock, err = acquireLock(target, true) + writeLock, err = filelocks.AcquireWriteLock(target) if err != nil { return errors.Wrap(err, "xattrs: Unable to lock target to write") } defer func() { - rerr := releaseLock(writeLock) + rerr := filelocks.ReleaseLock(writeLock) // if err is non nil we do not overwrite that if err == nil { @@ -183,13 +136,13 @@ func CopyMetadata(src, target string, filter func(attributeName string) bool) (e }() // now try to get a shared lock on the source - readLock, err = acquireLock(src, false) + readLock, err = filelocks.AcquireReadLock(src) if err != nil { return errors.Wrap(err, "xattrs: Unable to lock file for read") } defer func() { - rerr := releaseLock(readLock) + rerr := filelocks.ReleaseLock(readLock) // if err is non nil we do not overwrite that if err == nil { @@ -246,13 +199,13 @@ func SetMultiple(filePath string, attribs map[string]string) (err error) { // h, err := lockedfile.OpenFile(filePath, os.O_WRONLY, 0) // 0? Open File only workn for files ... but we want to lock dirs ... or symlinks // or we append .lock to the file and use https://github.com/gofrs/flock var fileLock *flock.Flock - fileLock, err = acquireLock(filePath, true) + fileLock, err = filelocks.AcquireWriteLock(filePath) if err != nil { return errors.Wrap(err, "xattrs: Can not acquired write log") } defer func() { - rerr := releaseLock(fileLock) + rerr := filelocks.ReleaseLock(fileLock) // if err is non nil we do not overwrite that if err == nil { @@ -305,13 +258,13 @@ func GetInt64(filePath, key string) (int64, error) { func All(filePath string) (attribs map[string]string, err error) { var fileLock *flock.Flock - fileLock, err = acquireLock(filePath, false) + fileLock, err = filelocks.AcquireReadLock(filePath) if err != nil { return nil, errors.Wrap(err, "xattrs: Unable to lock file for read") } defer func() { - rerr := releaseLock(fileLock) + rerr := filelocks.ReleaseLock(fileLock) // if err is non nil we do not overwrite that if err == nil { diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go new file mode 100644 index 0000000000..dc58857b81 --- /dev/null +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -0,0 +1,85 @@ +// Copyright 2018-2021 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package filelocks + +import ( + "errors" + "os" + + "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" + } + return n +} + +// acquireWriteLog acquires a lock on a file or directory. +// if the parameter write is true, it gets an exclusive write lock, otherwise a shared read lock. +// The function returns a Flock object, unlocking has to be done in the calling function. +func acquireLock(file string, write bool) (*flock.Flock, error) { + var err error + + // Create the a file to carry the log + n := FlockFile(file) + 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) + + if write { + _, err = lock.TryLock() + } else { + _, err = lock.TryRLock() + } + + if err != nil { + return nil, err + } + return lock, nil +} + +// AcquireReadLock tries to acquire a shared lock to read from the +// file and returns a lock object or an error accordingly. +func AcquireReadLock(file string) (*flock.Flock, error) { + return acquireLock(file, false) +} + +// AcquireWriteLock tries to acquire a shared lock to write from the +// file and returns a lock object or an error accordingly. +func AcquireWriteLock(file string) (*flock.Flock, error) { + return acquireLock(file, true) +} + +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() + if err == nil { + err = os.Remove(lock.Path()) + } + return err +} From 627aab59dc114c9b998c103190e8c0b2c6dd3445 Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 17 Feb 2022 12:13:57 +0100 Subject: [PATCH 5/9] Fix doc comments of exported funcs --- pkg/storage/utils/filelocks/filelocks.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go index dc58857b81..90f7daa421 100644 --- a/pkg/storage/utils/filelocks/filelocks.go +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -25,7 +25,7 @@ import ( "github.com/gofrs/flock" ) -// flockFile returns the flock filename for a given file name +// 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 @@ -73,6 +73,8 @@ func AcquireWriteLock(file string) (*flock.Flock, error) { return acquireLock(file, true) } +// ReleaseLock releases a lock from a file that was previously created +// by AcquireReadLock or AcquireWriteLock. 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 From 5d403e0fbbc7d27fe1926902d57194eed5dd90da Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 17 Feb 2022 18:05:36 +0100 Subject: [PATCH 6/9] Return err instead of nil to get error from the defer func. Co-authored-by: David Christofas --- pkg/storage/utils/decomposedfs/xattrs/xattrs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index 3d4a0ab481..28b0695ade 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -294,5 +294,5 @@ func All(filePath string) (attribs map[string]string, err error) { err = errors.New("Failed to read all xattrs") } - return attribs, nil + return attribs, err } From 59a37c326a5b2ccdcbfb95a5174665017b7ea0bb Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Thu, 17 Feb 2022 22:02:29 +0100 Subject: [PATCH 7/9] Wait if a file is locked and make up to ten attempts. --- .../utils/decomposedfs/xattrs/xattrs.go | 16 +++++++------ pkg/storage/utils/filelocks/filelocks.go | 24 +++++++++++++++---- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index 28b0695ade..b351776753 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -202,7 +202,7 @@ func SetMultiple(filePath string, attribs map[string]string) (err error) { fileLock, err = filelocks.AcquireWriteLock(filePath) if err != nil { - return errors.Wrap(err, "xattrs: Can not acquired write log") + return errors.Wrap(err, "xattrs: Can not acquire write log") } defer func() { rerr := filelocks.ReleaseLock(fileLock) @@ -216,16 +216,17 @@ func SetMultiple(filePath string, attribs map[string]string) (err error) { // error handling: Count if there are errors while setting the attribs. // if there were any, return an error. var xerrs int = 0 + var xerr error for key, val := range attribs { - if xerr := xattr.Set(filePath, key, []byte(val)); xerr != nil { + if xerr = xattr.Set(filePath, key, []byte(val)); xerr != nil { // log xerrs++ } } if xerrs > 0 { - err = errors.New("Failed to set all xattrs") + err = errors.Wrap(xerr, "Failed to set all xattrs") } - return err + return nil } // Get an extended attribute value for the given key @@ -278,12 +279,13 @@ func All(filePath string) (attribs map[string]string, err error) { } var xerrs int = 0 - + var xerr error // error handling: Count if there are errors while reading all attribs. // if there were any, return an error. attribs = make(map[string]string, len(attrNames)) for _, name := range attrNames { - if val, xerr := xattr.Get(filePath, name); xerr != nil { + var val []byte + if val, xerr = xattr.Get(filePath, name); xerr != nil { xerrs++ } else { attribs[name] = string(val) @@ -291,7 +293,7 @@ func All(filePath string) (attribs map[string]string, err error) { } if xerrs > 0 { - err = errors.New("Failed to read all xattrs") + err = errors.Wrap(xerr, "Failed to read all xattrs") } return attribs, err diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go index 90f7daa421..2bcf622562 100644 --- a/pkg/storage/utils/filelocks/filelocks.go +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -21,6 +21,7 @@ package filelocks import ( "errors" "os" + "time" "github.com/gofrs/flock" ) @@ -41,7 +42,7 @@ func FlockFile(file string) string { func acquireLock(file string, write bool) (*flock.Flock, error) { var err error - // Create the a file to carry the log + // Create a file to carry the log n := FlockFile(file) if len(n) == 0 { return nil, errors.New("lock path is empty") @@ -49,10 +50,23 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { // Acquire the write log on the target node first. lock := flock.New(n) - if write { - _, err = lock.TryLock() - } else { - _, err = lock.TryRLock() + var ok bool + for i := 0; i < 10; i++ { + if write { + ok, err = lock.TryLock() + } else { + ok, err = lock.TryRLock() + } + + if ok { + break + } + + time.Sleep(time.Duration(i*3) * time.Millisecond) + } + + if !ok { + err = errors.New("could not acquire lock after wait") } if err != nil { From 832700d8cd7cfb61e031f58aa0c59430e84a60ae Mon Sep 17 00:00:00 2001 From: Klaas Freitag Date: Fri, 18 Feb 2022 08:02:35 +0100 Subject: [PATCH 8/9] start for loop with i=1 to have a proper factor for the wait period. --- pkg/storage/utils/filelocks/filelocks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/filelocks/filelocks.go b/pkg/storage/utils/filelocks/filelocks.go index 2bcf622562..7a79b1abc0 100644 --- a/pkg/storage/utils/filelocks/filelocks.go +++ b/pkg/storage/utils/filelocks/filelocks.go @@ -51,7 +51,7 @@ func acquireLock(file string, write bool) (*flock.Flock, error) { lock := flock.New(n) var ok bool - for i := 0; i < 10; i++ { + for i := 1; i <= 10; i++ { if write { ok, err = lock.TryLock() } else { From f43c5cfb2eb206d9d25062001d147d2674dedaf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 18 Feb 2022 10:37:57 +0100 Subject: [PATCH 9/9] return error Co-authored-by: David Christofas --- pkg/storage/utils/decomposedfs/xattrs/xattrs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go index b351776753..4168d80ff0 100644 --- a/pkg/storage/utils/decomposedfs/xattrs/xattrs.go +++ b/pkg/storage/utils/decomposedfs/xattrs/xattrs.go @@ -226,7 +226,7 @@ func SetMultiple(filePath string, attribs map[string]string) (err error) { if xerrs > 0 { err = errors.Wrap(xerr, "Failed to set all xattrs") } - return nil + return err } // Get an extended attribute value for the given key