From 6e10201cc2d11fd6ba084fcfd72eddd2a2b24bed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Thu, 29 Jun 2023 09:02:21 +0200 Subject: [PATCH 01/15] Prevent 0-byte metadata files --- go.mod | 1 + go.sum | 2 + .../metadata/messagepack_backend.go | 74 ++++++++++--------- pkg/storage/utils/decomposedfs/tree/tree.go | 2 +- 4 files changed, 42 insertions(+), 37 deletions(-) diff --git a/go.mod b/go.mod index b586864a4a..538837166b 100644 --- a/go.mod +++ b/go.mod @@ -138,6 +138,7 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect + github.com/google/renameio/v2 v2.0.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.13.0 // indirect github.com/hashicorp/consul/api v1.15.2 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect diff --git a/go.sum b/go.sum index ea902352db..76f735197f 100644 --- a/go.sum +++ b/go.sum @@ -781,6 +781,8 @@ github.com/google/pprof v0.0.0-20210609004039-a478d1d731e9/go.mod h1:kpwsk12EmLe github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 h1:K6RDEckDVWvDI9JAJYCmNdQXq6neHJOYx3V6jnqNEec= github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= +github.com/google/renameio/v2 v2.0.0 h1:UifI23ZTGY8Tt29JbYFiuyIU3eX+RNFtUwefq9qAhxg= +github.com/google/renameio/v2 v2.0.0/go.mod h1:BtmJXm5YlszgC+TD4HOEEUFgkJP3nLxehU6hfe7jRt4= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.2.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 7e2137c2c4..21bdfd667c 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -20,6 +20,7 @@ package metadata import ( "context" + "errors" "io" "os" "path/filepath" @@ -28,6 +29,7 @@ import ( "time" "github.com/cs3org/reva/v2/pkg/storage/cache" + "github.com/google/renameio/v2" "github.com/pkg/xattr" "github.com/rogpeppe/go-internal/lockedfile" "github.com/shamaton/msgpack/v2" @@ -142,14 +144,15 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set span.End() }() + lockPath := b.lockFilePath(path) metaPath := b.MetadataPath(path) if acquireLock { _, subspan := tracer.Start(ctx, "lockedfile.OpenFile") - f, err = lockedfile.OpenFile(metaPath, os.O_RDWR|os.O_CREATE, 0600) + f, err = lockedfile.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, 0600) subspan.End() } else { _, subspan := tracer.Start(ctx, "os.OpenFile") - f, err = os.OpenFile(metaPath, os.O_RDWR|os.O_CREATE, 0600) + f, err = os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, 0600) subspan.End() } if err != nil { @@ -163,53 +166,42 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set subspan.End() // Read current state - _, subspan = tracer.Start(ctx, "io.ReadAll") + _, subspan = tracer.Start(ctx, "os.ReadFile") var msgBytes []byte - msgBytes, err = io.ReadAll(f) + msgBytes, err = os.ReadFile(metaPath) subspan.End() if err != nil { return err } attribs := map[string][]byte{} - if len(msgBytes) > 0 { - err = msgpack.Unmarshal(msgBytes, &attribs) - if err != nil { - return err - } + + if len(msgBytes) == 0 { + // ugh. an empty file? bail out + return errors.New("encountered empty metadata file") } - // set new metadata + err = msgpack.Unmarshal(msgBytes, &attribs) + if err != nil { + return err + } + + // prepare metadata for key, val := range setAttribs { attribs[key] = val } for _, key := range deleteAttribs { delete(attribs, key) } - - // Truncate file - _, err = f.Seek(0, io.SeekStart) - if err != nil { - return err - } - _, subspan = tracer.Start(ctx, "f.Truncate") - err = f.Truncate(0) - subspan.End() - if err != nil { - return err - } - - // Write new metadata to file var d []byte d, err = msgpack.Marshal(attribs) if err != nil { return err } - _, subspan = tracer.Start(ctx, "f.Write") - _, err = f.Write(d) + + // overwrite file atomically + _, subspan = tracer.Start(ctx, "renameio.Writefile") + err = renameio.WriteFile(metaPath, d, 0600) subspan.End() - if err != nil { - return err - } _, subspan = tracer.Start(ctx, "metaCache.PushToCache") err = b.metaCache.PushToCache(b.cacheKey(path), attribs) @@ -227,9 +219,13 @@ func (b MessagePackBackend) loadAttributes(ctx context.Context, path string, sou } metaPath := b.MetadataPath(path) + var msgBytes []byte + if source == nil { - _, subspan := tracer.Start(ctx, "lockedfile.Open") - source, err = lockedfile.Open(metaPath) + // // No cached entry found. Read from storage and store in cache + _, subspan := tracer.Start(ctx, "os.OpenFile") + // source, err = lockedfile.Open(metaPath) + source, err = os.Open(metaPath) subspan.End() // // No cached entry found. Read from storage and store in cache if err != nil { @@ -246,12 +242,16 @@ func (b MessagePackBackend) loadAttributes(ctx context.Context, path string, sou return attribs, nil // no attributes set yet } } - defer source.(*lockedfile.File).Close() + _, subspan = tracer.Start(ctx, "io.ReadAll") + msgBytes, err = io.ReadAll(source) + source.(*os.File).Close() + subspan.End() + } else { + _, subspan := tracer.Start(ctx, "io.ReadAll") + msgBytes, err = io.ReadAll(source) + subspan.End() } - _, subspan := tracer.Start(ctx, "io.ReadAll") - msgBytes, err := io.ReadAll(source) - subspan.End() if err != nil { return nil, err } @@ -262,7 +262,7 @@ func (b MessagePackBackend) loadAttributes(ctx context.Context, path string, sou } } - _, subspan = tracer.Start(ctx, "metaCache.PushToCache") + _, subspan := tracer.Start(ctx, "metaCache.PushToCache") err = b.metaCache.PushToCache(b.cacheKey(path), attribs) subspan.End() if err != nil { @@ -304,6 +304,8 @@ func (b MessagePackBackend) Rename(oldPath, newPath string) error { // MetadataPath returns the path of the file holding the metadata for the given path func (MessagePackBackend) MetadataPath(path string) string { return path + ".mpk" } +func (MessagePackBackend) lockFilePath(path string) string { return path + ".mpk.lock" } + func (b MessagePackBackend) cacheKey(path string) string { // rootPath is guaranteed to have no trailing slash // the cache key shouldn't begin with a slash as some stores drop it which can cause diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index f809eec856..551668213e 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -753,7 +753,7 @@ func (t *Tree) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) (err var parentFilename string switch t.lookup.MetadataBackend().(type) { case metadata.MessagePackBackend: - parentFilename = t.lookup.MetadataBackend().MetadataPath(n.ParentPath()) + parentFilename = t.lookup.MetadataBackend().MetadataPath(n.ParentPath()) + ".lock" f, err = lockedfile.OpenFile(parentFilename, os.O_RDWR|os.O_CREATE, 0600) case metadata.XattrsBackend: // we have to use dedicated lockfiles to lock directories From 800226ebea5835902b16471b17b31d1dc2049356 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Sat, 1 Jul 2023 00:33:25 +0200 Subject: [PATCH 02/15] ignore reading not existing file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../metadata/messagepack_backend.go | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 21bdfd667c..032a5e4d84 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -22,6 +22,7 @@ import ( "context" "errors" "io" + "io/fs" "os" "path/filepath" "strconv" @@ -170,19 +171,21 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set var msgBytes []byte msgBytes, err = os.ReadFile(metaPath) subspan.End() - if err != nil { - return err - } attribs := map[string][]byte{} - - if len(msgBytes) == 0 { + switch { + case err != nil: + if _, ok := err.(*fs.PathError); !ok { + return err + } + case len(msgBytes) == 0: // ugh. an empty file? bail out return errors.New("encountered empty metadata file") - } - - err = msgpack.Unmarshal(msgBytes, &attribs) - if err != nil { - return err + default: + // only unmarshal if we read data + err = msgpack.Unmarshal(msgBytes, &attribs) + if err != nil { + return err + } } // prepare metadata From b30757117d5329e726ede1a8f3f5f00a4fcc4626 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Sat, 1 Jul 2023 01:05:45 +0200 Subject: [PATCH 03/15] actually, we are the ones creating empty files on purpose :( MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 032a5e4d84..42d4a86327 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -20,7 +20,6 @@ package metadata import ( "context" - "errors" "io" "io/fs" "os" @@ -179,7 +178,7 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set } case len(msgBytes) == 0: // ugh. an empty file? bail out - return errors.New("encountered empty metadata file") + // return errors.New("encountered empty metadata file") // actually this is normal when we write new files, because initNewNode creates an empty file for the mtime default: // only unmarshal if we read data err = msgpack.Unmarshal(msgBytes, &attribs) From 5ff29070dfade38a96ff4019af0a82605796ede6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 3 Jul 2023 10:53:02 +0200 Subject: [PATCH 04/15] Lock the proper file --- pkg/storage/utils/decomposedfs/lookup/lookup.go | 2 +- .../utils/decomposedfs/metadata/messagepack_backend.go | 6 +----- pkg/storage/utils/decomposedfs/upload/processing.go | 4 ++-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/lookup/lookup.go b/pkg/storage/utils/decomposedfs/lookup/lookup.go index 1bf151449e..53f72bb3ed 100644 --- a/pkg/storage/utils/decomposedfs/lookup/lookup.go +++ b/pkg/storage/utils/decomposedfs/lookup/lookup.go @@ -312,7 +312,7 @@ func (lu *Lookup) CopyMetadataWithSourceLock(ctx context.Context, sourcePath, ta switch { case lockedSource == nil: return errors.New("no lock provided") - case lockedSource.File.Name() != lu.MetadataBackend().MetadataPath(sourcePath): + case lockedSource.File.Name() != lu.MetadataBackend().MetadataPath(sourcePath)+".lock": return errors.New("lockpath does not match filepath") } diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 42d4a86327..df17706f85 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -150,15 +150,11 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set _, subspan := tracer.Start(ctx, "lockedfile.OpenFile") f, err = lockedfile.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, 0600) subspan.End() - } else { - _, subspan := tracer.Start(ctx, "os.OpenFile") - f, err = os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, 0600) - subspan.End() + defer f.Close() } if err != nil { return err } - defer f.Close() // Invalidate cache early _, subspan := tracer.Start(ctx, "metaCache.RemoveMetadata") diff --git a/pkg/storage/utils/decomposedfs/upload/processing.go b/pkg/storage/utils/decomposedfs/upload/processing.go index 41ac0e4345..bfb5156a93 100644 --- a/pkg/storage/utils/decomposedfs/upload/processing.go +++ b/pkg/storage/utils/decomposedfs/upload/processing.go @@ -329,7 +329,7 @@ func initNewNode(upload *Upload, n *node.Node, fsize uint64) (*lockedfile.File, } // create and write lock new node metadata - f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().MetadataPath(n.InternalPath()), os.O_RDWR|os.O_CREATE, 0600) + f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().MetadataPath(n.InternalPath())+".lock", os.O_RDWR|os.O_CREATE, 0600) if err != nil { return nil, err } @@ -403,7 +403,7 @@ func updateExistingNode(upload *Upload, n *node.Node, spaceID string, fsize uint targetPath := n.InternalPath() // write lock existing node before reading treesize or tree time - f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().MetadataPath(targetPath), os.O_RDWR, 0600) + f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().MetadataPath(targetPath)+".lock", os.O_RDWR|os.O_CREATE, 0600) if err != nil { return nil, err } From 8be2465d1e73384e3447dbf93431e02ccd0bb3d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 3 Jul 2023 14:33:36 +0200 Subject: [PATCH 05/15] Do not tolerate existing 0-byte files. We do not generate them anymore --- pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index df17706f85..30aec85538 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -174,7 +174,7 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set } case len(msgBytes) == 0: // ugh. an empty file? bail out - // return errors.New("encountered empty metadata file") // actually this is normal when we write new files, because initNewNode creates an empty file for the mtime + return errors.New("encountered empty metadata file") default: // only unmarshal if we read data err = msgpack.Unmarshal(msgBytes, &attribs) From 3f63ab3aa82603f7f7012078d08e68209ae68c3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 3 Jul 2023 14:34:47 +0200 Subject: [PATCH 06/15] Only tolerate ErrNotExist errors --- pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 30aec85538..e0121dfa84 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -169,7 +169,7 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set attribs := map[string][]byte{} switch { case err != nil: - if _, ok := err.(*fs.PathError); !ok { + if !errors.Is(err, fs.ErrNotExist) { return err } case len(msgBytes) == 0: From 823aaa412474138cab5fecc996b2e99fae9b3128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 3 Jul 2023 14:37:59 +0200 Subject: [PATCH 07/15] Fix rebase error --- pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index e0121dfa84..49b0b97aaf 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -20,6 +20,7 @@ package metadata import ( "context" + "errors" "io" "io/fs" "os" From a467326467f8bc71494d33b91a942d070fc4c6a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 3 Jul 2023 15:03:07 +0200 Subject: [PATCH 08/15] Handle error --- pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 49b0b97aaf..a958d1f373 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -200,6 +200,9 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set // overwrite file atomically _, subspan = tracer.Start(ctx, "renameio.Writefile") err = renameio.WriteFile(metaPath, d, 0600) + if err != nil { + return err + } subspan.End() _, subspan = tracer.Start(ctx, "metaCache.PushToCache") From 38bccfdb480bb918c4f08abc00d001669a934fb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 4 Jul 2023 12:18:26 +0200 Subject: [PATCH 09/15] Add changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/fix-0-byte-msgpack.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/unreleased/fix-0-byte-msgpack.md diff --git a/changelog/unreleased/fix-0-byte-msgpack.md b/changelog/unreleased/fix-0-byte-msgpack.md new file mode 100644 index 0000000000..da0cf11125 --- /dev/null +++ b/changelog/unreleased/fix-0-byte-msgpack.md @@ -0,0 +1,5 @@ +Bugfix: fix writing 0 byte msgpack metadata + +File metadata is now written atomically to be more resilient during timeouts + +https://github.com/cs3org/reva/pull/4033 From 6424169de910a3799602516941c0d22d580a3027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Tue, 4 Jul 2023 08:52:34 +0200 Subject: [PATCH 10/15] Adapt IsMetaFile to new locking strategy --- .../utils/decomposedfs/metadata/messagepack_backend.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index a958d1f373..32a36a5c61 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -275,7 +275,9 @@ func (b MessagePackBackend) loadAttributes(ctx context.Context, path string, sou } // IsMetaFile returns whether the given path represents a meta file -func (MessagePackBackend) IsMetaFile(path string) bool { return strings.HasSuffix(path, ".mpk") } +func (MessagePackBackend) IsMetaFile(path string) bool { + return strings.HasSuffix(path, ".mpk") || strings.HasSuffix(path, ".mpk.lock") +} // Purge purges the data of a given path func (b MessagePackBackend) Purge(path string) error { From b0b9166e0442d3b1617cc8fd9844a1c6b20bd5e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 4 Jul 2023 12:36:08 +0200 Subject: [PATCH 11/15] always touch metadata file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../utils/decomposedfs/upload/processing.go | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/upload/processing.go b/pkg/storage/utils/decomposedfs/upload/processing.go index bfb5156a93..4138caebbb 100644 --- a/pkg/storage/utils/decomposedfs/upload/processing.go +++ b/pkg/storage/utils/decomposedfs/upload/processing.go @@ -38,7 +38,6 @@ import ( "github.com/cs3org/reva/v2/pkg/logger" "github.com/cs3org/reva/v2/pkg/storage/utils/chunking" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/lookup" - "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/metadata" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/options" @@ -334,18 +333,12 @@ func initNewNode(upload *Upload, n *node.Node, fsize uint64) (*lockedfile.File, return nil, err } - switch upload.lu.MetadataBackend().(type) { - case metadata.MessagePackBackend: - // for the ini and metadata backend we also need to touch the actual node file here. - // it stores the mtime of the resource, which must not change when we update the ini file - h, err := os.OpenFile(n.InternalPath(), os.O_CREATE, 0600) - if err != nil { - return f, err - } - h.Close() - case metadata.XattrsBackend: - // nothing to do + // we also need to touch the actual node file here it stores the mtime of the resource + h, err := os.OpenFile(n.InternalPath(), os.O_CREATE, 0600) + if err != nil { + return f, err } + h.Close() if _, err := node.CheckQuota(upload.Ctx, n.SpaceRoot, false, 0, fsize); err != nil { return f, err From 02f5f19b17c3b45701b282e0fc0baa89c0405cfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Tue, 4 Jul 2023 12:42:50 +0200 Subject: [PATCH 12/15] Fix unit tests --- .../decomposedfs/metadata/metadata_test.go | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/metadata/metadata_test.go b/pkg/storage/utils/decomposedfs/metadata/metadata_test.go index eb9741ff4e..4cd0dad629 100644 --- a/pkg/storage/utils/decomposedfs/metadata/metadata_test.go +++ b/pkg/storage/utils/decomposedfs/metadata/metadata_test.go @@ -31,9 +31,8 @@ import ( var _ = Describe("Backend", func() { var ( - tmpdir string - file string - metafile string + tmpdir string + file string backend metadata.Backend ) @@ -46,9 +45,6 @@ var _ = Describe("Backend", func() { JustBeforeEach(func() { file = path.Join(tmpdir, "file") - metafile = backend.MetadataPath(file) - _, err := os.Create(metafile) - Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { @@ -147,10 +143,9 @@ var _ = Describe("Backend", func() { Expect(v["bar"]).To(Equal([]byte("baz"))) }) - It("returns an empty map", func() { - v, err := backend.All(context.Background(), file) - Expect(err).ToNot(HaveOccurred()) - Expect(v).To(Equal(map[string][]byte{})) + It("fails when the metafile does not exist", func() { + _, err := backend.All(context.Background(), file) + Expect(err).To(HaveOccurred()) }) }) @@ -165,10 +160,9 @@ var _ = Describe("Backend", func() { Expect(v).To(ConsistOf("foo", "bar")) }) - It("returns an empty list", func() { - v, err := backend.List(context.Background(), file) - Expect(err).ToNot(HaveOccurred()) - Expect(v).To(Equal([]string{})) + It("fails when the metafile does not exist", func() { + _, err := backend.List(context.Background(), file) + Expect(err).To(HaveOccurred()) }) }) From 312f65e36a31b5d00c3b45666f3b9447a12aa4f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Tue, 4 Jul 2023 13:50:39 +0200 Subject: [PATCH 13/15] Do not try to read metadata from the lock file --- pkg/storage/utils/decomposedfs/lookup/lookup.go | 8 ++++---- pkg/storage/utils/decomposedfs/revisions.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/lookup/lookup.go b/pkg/storage/utils/decomposedfs/lookup/lookup.go index 53f72bb3ed..816150efc6 100644 --- a/pkg/storage/utils/decomposedfs/lookup/lookup.go +++ b/pkg/storage/utils/decomposedfs/lookup/lookup.go @@ -284,7 +284,7 @@ func refFromCS3(b []byte) (*provider.Reference, error) { func (lu *Lookup) CopyMetadata(ctx context.Context, src, target string, filter func(attributeName string) bool) (err error) { // Acquire a read log on the source node // write lock existing node before reading treesize or tree time - f, err := lockedfile.Open(lu.MetadataBackend().MetadataPath(src)) + lock, err := lockedfile.OpenFile(lu.MetadataBackend().MetadataPath(src)+".lock", os.O_RDONLY|os.O_CREATE, 0600) if err != nil { return err } @@ -293,7 +293,7 @@ func (lu *Lookup) CopyMetadata(ctx context.Context, src, target string, filter f return errors.Wrap(err, "xattrs: Unable to lock source to read") } defer func() { - rerr := f.Close() + rerr := lock.Close() // if err is non nil we do not overwrite that if err == nil { @@ -301,7 +301,7 @@ func (lu *Lookup) CopyMetadata(ctx context.Context, src, target string, filter f } }() - return lu.CopyMetadataWithSourceLock(ctx, src, target, filter, f) + return lu.CopyMetadataWithSourceLock(ctx, src, target, filter, lock) } // CopyMetadataWithSourceLock copies all extended attributes from source to target. @@ -316,7 +316,7 @@ func (lu *Lookup) CopyMetadataWithSourceLock(ctx context.Context, sourcePath, ta return errors.New("lockpath does not match filepath") } - attrs, err := lu.metadataBackend.AllWithLockedSource(ctx, sourcePath, lockedSource) + attrs, err := lu.metadataBackend.All(ctx, sourcePath) if err != nil { return err } diff --git a/pkg/storage/utils/decomposedfs/revisions.go b/pkg/storage/utils/decomposedfs/revisions.go index 6bde18bd06..42289465a1 100644 --- a/pkg/storage/utils/decomposedfs/revisions.go +++ b/pkg/storage/utils/decomposedfs/revisions.go @@ -237,7 +237,7 @@ func (fs *Decomposedfs) RestoreRevision(ctx context.Context, ref *provider.Refer attributeName == prefixes.BlobsizeAttr }) if err != nil { - return errtypes.InternalError("failed to copy blob xattrs to version node") + return errtypes.InternalError("failed to copy blob xattrs to version node: " + err.Error()) } // remember mtime from node as new revision mtime @@ -256,7 +256,7 @@ func (fs *Decomposedfs) RestoreRevision(ctx context.Context, ref *provider.Refer attributeName == prefixes.BlobsizeAttr }) if err != nil { - return errtypes.InternalError("failed to copy blob xattrs to old revision to node") + return errtypes.InternalError("failed to copy blob xattrs to old revision to node: " + err.Error()) } revisionSize, err := fs.lu.MetadataBackend().GetInt64(ctx, restoredRevisionPath, prefixes.BlobsizeAttr) From 4ac1f2484e34f3548bc8f4a622a4fd15143ed8f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Tue, 4 Jul 2023 13:56:38 +0200 Subject: [PATCH 14/15] Filter .lock files when listing revisions --- pkg/storage/utils/decomposedfs/revisions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/revisions.go b/pkg/storage/utils/decomposedfs/revisions.go index 42289465a1..1475483994 100644 --- a/pkg/storage/utils/decomposedfs/revisions.go +++ b/pkg/storage/utils/decomposedfs/revisions.go @@ -70,7 +70,7 @@ func (fs *Decomposedfs) ListRevisions(ctx context.Context, ref *provider.Referen np := n.InternalPath() if items, err := filepath.Glob(np + node.RevisionIDDelimiter + "*"); err == nil { for i := range items { - if fs.lu.MetadataBackend().IsMetaFile(items[i]) { + if fs.lu.MetadataBackend().IsMetaFile(items[i]) || strings.HasSuffix(items[i], ".lock") { continue } From 8d7ed4d174124f75f321f2b145a7c843c768ca01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Tue, 4 Jul 2023 15:12:33 +0200 Subject: [PATCH 15/15] Prevent lockfile name clashes --- pkg/storage/utils/decomposedfs/lookup/lookup.go | 4 ++-- .../decomposedfs/metadata/messagepack_backend.go | 5 +++-- .../utils/decomposedfs/metadata/metadata.go | 4 ++++ .../utils/decomposedfs/metadata/xattrs_backend.go | 6 +++++- pkg/storage/utils/decomposedfs/tree/tree.go | 14 ++------------ .../utils/decomposedfs/upload/processing.go | 4 ++-- 6 files changed, 18 insertions(+), 19 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/lookup/lookup.go b/pkg/storage/utils/decomposedfs/lookup/lookup.go index 816150efc6..273e094f9d 100644 --- a/pkg/storage/utils/decomposedfs/lookup/lookup.go +++ b/pkg/storage/utils/decomposedfs/lookup/lookup.go @@ -284,7 +284,7 @@ func refFromCS3(b []byte) (*provider.Reference, error) { func (lu *Lookup) CopyMetadata(ctx context.Context, src, target string, filter func(attributeName string) bool) (err error) { // Acquire a read log on the source node // write lock existing node before reading treesize or tree time - lock, err := lockedfile.OpenFile(lu.MetadataBackend().MetadataPath(src)+".lock", os.O_RDONLY|os.O_CREATE, 0600) + lock, err := lockedfile.OpenFile(lu.MetadataBackend().LockfilePath(src), os.O_RDONLY|os.O_CREATE, 0600) if err != nil { return err } @@ -312,7 +312,7 @@ func (lu *Lookup) CopyMetadataWithSourceLock(ctx context.Context, sourcePath, ta switch { case lockedSource == nil: return errors.New("no lock provided") - case lockedSource.File.Name() != lu.MetadataBackend().MetadataPath(sourcePath)+".lock": + case lockedSource.File.Name() != lu.MetadataBackend().LockfilePath(sourcePath): return errors.New("lockpath does not match filepath") } diff --git a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go index 32a36a5c61..9cfa629c10 100644 --- a/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go @@ -145,7 +145,7 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set span.End() }() - lockPath := b.lockFilePath(path) + lockPath := b.LockfilePath(path) metaPath := b.MetadataPath(path) if acquireLock { _, subspan := tracer.Start(ctx, "lockedfile.OpenFile") @@ -308,7 +308,8 @@ func (b MessagePackBackend) Rename(oldPath, newPath string) error { // MetadataPath returns the path of the file holding the metadata for the given path func (MessagePackBackend) MetadataPath(path string) string { return path + ".mpk" } -func (MessagePackBackend) lockFilePath(path string) string { return path + ".mpk.lock" } +// LockfilePath returns the path of the lock file +func (MessagePackBackend) LockfilePath(path string) string { return path + ".mpk.lock" } func (b MessagePackBackend) cacheKey(path string) string { // rootPath is guaranteed to have no trailing slash diff --git a/pkg/storage/utils/decomposedfs/metadata/metadata.go b/pkg/storage/utils/decomposedfs/metadata/metadata.go index c3e37ea074..243895b364 100644 --- a/pkg/storage/utils/decomposedfs/metadata/metadata.go +++ b/pkg/storage/utils/decomposedfs/metadata/metadata.go @@ -52,6 +52,7 @@ type Backend interface { Rename(oldPath, newPath string) error IsMetaFile(path string) bool MetadataPath(path string) string + LockfilePath(path string) string AllWithLockedSource(ctx context.Context, path string, source io.Reader) (map[string][]byte, error) } @@ -110,6 +111,9 @@ func (NullBackend) Rename(oldPath, newPath string) error { return errUnconfigure // MetadataPath returns the path of the file holding the metadata for the given path func (NullBackend) MetadataPath(path string) string { return "" } +// LockfilePath returns the path of the lock file +func (NullBackend) LockfilePath(path string) string { return "" } + // AllWithLockedSource reads all extended attributes from the given reader // The path argument is used for storing the data in the cache func (NullBackend) AllWithLockedSource(ctx context.Context, path string, source io.Reader) (map[string][]byte, error) { diff --git a/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go b/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go index 5a402f6817..92462f89d2 100644 --- a/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go +++ b/pkg/storage/utils/decomposedfs/metadata/xattrs_backend.go @@ -24,6 +24,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "github.com/cs3org/reva/v2/pkg/storage/utils/filelocks" "github.com/pkg/errors" @@ -156,7 +157,7 @@ func (XattrsBackend) Remove(ctx context.Context, filePath string, key string) (e } // IsMetaFile returns whether the given path represents a meta file -func (XattrsBackend) IsMetaFile(path string) bool { return false } +func (XattrsBackend) IsMetaFile(path string) bool { return strings.HasSuffix(path, ".meta.lock") } // Purge purges the data of a given path func (XattrsBackend) Purge(path string) error { return nil } @@ -167,6 +168,9 @@ func (XattrsBackend) Rename(oldPath, newPath string) error { return nil } // MetadataPath returns the path of the file holding the metadata for the given path func (XattrsBackend) MetadataPath(path string) string { return path } +// LockfilePath returns the path of the lock file +func (XattrsBackend) LockfilePath(path string) string { return path + ".meta.lock" } + func cleanupLockfile(f *lockedfile.File) { _ = f.Close() _ = os.Remove(f.Name()) diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 551668213e..2613033a1e 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -40,7 +40,6 @@ import ( "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/options" - "github.com/cs3org/reva/v2/pkg/storage/utils/filelocks" "github.com/cs3org/reva/v2/pkg/utils" "github.com/google/uuid" "github.com/pkg/errors" @@ -750,17 +749,8 @@ func (t *Tree) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) (err // lock parent before reading treesize or tree time _, subspan := tracer.Start(ctx, "lockedfile.OpenFile") - var parentFilename string - switch t.lookup.MetadataBackend().(type) { - case metadata.MessagePackBackend: - parentFilename = t.lookup.MetadataBackend().MetadataPath(n.ParentPath()) + ".lock" - f, err = lockedfile.OpenFile(parentFilename, os.O_RDWR|os.O_CREATE, 0600) - case metadata.XattrsBackend: - // we have to use dedicated lockfiles to lock directories - // this only works because the xattr backend also locks folders with separate lock files - parentFilename = n.ParentPath() + filelocks.LockFileSuffix - f, err = lockedfile.OpenFile(parentFilename, os.O_RDWR|os.O_CREATE, 0600) - } + parentFilename := t.lookup.MetadataBackend().LockfilePath(n.ParentPath()) + f, err = lockedfile.OpenFile(parentFilename, os.O_RDWR|os.O_CREATE, 0600) subspan.End() if err != nil { sublog.Error().Err(err). diff --git a/pkg/storage/utils/decomposedfs/upload/processing.go b/pkg/storage/utils/decomposedfs/upload/processing.go index 4138caebbb..08bba8f996 100644 --- a/pkg/storage/utils/decomposedfs/upload/processing.go +++ b/pkg/storage/utils/decomposedfs/upload/processing.go @@ -328,7 +328,7 @@ func initNewNode(upload *Upload, n *node.Node, fsize uint64) (*lockedfile.File, } // create and write lock new node metadata - f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().MetadataPath(n.InternalPath())+".lock", os.O_RDWR|os.O_CREATE, 0600) + f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().LockfilePath(n.InternalPath()), os.O_RDWR|os.O_CREATE, 0600) if err != nil { return nil, err } @@ -396,7 +396,7 @@ func updateExistingNode(upload *Upload, n *node.Node, spaceID string, fsize uint targetPath := n.InternalPath() // write lock existing node before reading treesize or tree time - f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().MetadataPath(targetPath)+".lock", os.O_RDWR|os.O_CREATE, 0600) + f, err := lockedfile.OpenFile(upload.lu.MetadataBackend().LockfilePath(targetPath), os.O_RDWR|os.O_CREATE, 0600) if err != nil { return nil, err }