Prevent 0-byte metadata files

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

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=

pkg/storage/utils/decomposedfs/metadata/messagepack_backend.go

@@ -20,6 +20,7 @@ package metadata
 
 import (
 	"context"
+	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -29,6 +30,7 @@ import (
 
 	"github.com/cs3org/reva/v2/pkg/appctx"
 	"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"
@@ -146,14 +148,15 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set
 		span.End()
 	}()
 
+	lockPath := b.lockFilePath(path)
 	metaPath := b.MetadataPath(path)
 	if acquireLock {
 		_, subspan := appctx.GetTracerProvider(ctx).Tracer(tracerName).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 := appctx.GetTracerProvider(ctx).Tracer(tracerName).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 {
@@ -162,53 +165,42 @@ func (b MessagePackBackend) saveAttributes(ctx context.Context, path string, set
 	defer f.Close()
 
 	// Read current state
-	_, subspan := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "io.ReadAll")
+	_, subspan := appctx.GetTracerProvider(ctx).Tracer(tracerName).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 = appctx.GetTracerProvider(ctx).Tracer(tracerName).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 = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "f.Write")
-	_, err = f.Write(d)
+
+	// overwrite file atomically
+	_, subspan = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "renameio.Writefile")
+	err = renameio.WriteFile(metaPath, d, 0600)
 	subspan.End()
-	if err != nil {
-		return err
-	}
 
 	go func() { _ = b.metaCache.PushToCache(b.cacheKey(path), attribs) }()
 
@@ -229,8 +221,9 @@ func (b MessagePackBackend) loadAttributes(ctx context.Context, path string, sou
 
 	if source == nil {
 		// // No cached entry found. Read from storage and store in cache
-		_, subspan := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "lockedfile.Open")
-		source, err = lockedfile.Open(metaPath)
+		_, subspan := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "os.OpenFile")
+		// source, err = lockedfile.Open(metaPath)
+		source, err = os.Open(metaPath)
 		subspan.End()
 		if err != nil {
 			if os.IsNotExist(err) {
@@ -248,7 +241,8 @@ func (b MessagePackBackend) loadAttributes(ctx context.Context, path string, sou
 		}
 		_, subspan = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "io.ReadAll")
 		msgBytes, err = io.ReadAll(source)
-		source.(*lockedfile.File).Close()
+		// source.(*lockedfile.File).Close()
+		source.(*os.File).Close()
 		subspan.End()
 	} else {
 		_, subspan := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "io.ReadAll")
@@ -303,6 +297,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

pkg/storage/utils/decomposedfs/tree/tree.go

@@ -748,7 +748,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