Improve performance of decomposedfs (#3729)

* Allow for reusing existing spaceroots/parent information in ReadNode

* Do not resolve the parent just to transform the parent id

* Fetch child node information concurrently

* Transform child nodes concurrently

* Add changelog

changelog/unreleased/improve-decomposedfs-performance.md

@@ -0,0 +1,5 @@
+Enhancement: Improve decomposedfs performance, esp. with network fs/cache
+
+We improved the performance of decomposedfs, esp. in scenarios where network storage and caches are involed.
+
+https://github.com/cs3org/reva/pull/3729

pkg/storage/utils/decomposedfs/decomposedfs.go

@@ -59,6 +59,7 @@ import (
 	"github.com/cs3org/reva/v2/pkg/storagespace"
 	"github.com/cs3org/reva/v2/pkg/utils"
 	"github.com/pkg/errors"
+	"golang.org/x/sync/errgroup"
 )
 
 // name is the Tracer name used to identify this instrumentation library.
@@ -203,7 +204,7 @@ func (fs *Decomposedfs) Postprocessing(ch <-chan events.Event) {
 				keepUpload bool
 			)
 
-			n, err := node.ReadNode(ctx, fs.lu, up.Info.Storage["SpaceRoot"], up.Info.Storage["NodeId"], false)
+			n, err := node.ReadNode(ctx, fs.lu, up.Info.Storage["SpaceRoot"], up.Info.Storage["NodeId"], false, nil, true)
 			if err != nil {
 				log.Error().Err(err).Str("uploadID", ev.UploadID).Msg("could not read node")
 				continue
@@ -228,7 +229,7 @@ func (fs *Decomposedfs) Postprocessing(ch <-chan events.Event) {
 			}
 
 			now := time.Now()
-			if p, err := node.ReadNode(ctx, fs.lu, up.Info.Storage["SpaceRoot"], n.ParentID, false); err != nil {
+			if p, err := node.ReadNode(ctx, fs.lu, up.Info.Storage["SpaceRoot"], n.ParentID, false, nil, true); err != nil {
 				log.Error().Err(err).Str("uploadID", ev.UploadID).Msg("could not read parent")
 			} else {
 				// update parent tmtime to propagate etag change
@@ -756,18 +757,17 @@ func (fs *Decomposedfs) GetMD(ctx context.Context, ref *provider.Reference, mdKe
 }
 
 // ListFolder returns a list of resources in the specified folder
-func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (finfos []*provider.ResourceInfo, err error) {
-	var n *node.Node
-	if n, err = fs.lu.NodeFromResource(ctx, ref); err != nil {
-		return
+func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) ([]*provider.ResourceInfo, error) {
+	n, err := fs.lu.NodeFromResource(ctx, ref)
+	if err != nil {
+		return nil, err
 	}
 
 	ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "ListFolder")
 	defer span.End()
 
 	if !n.Exists {
-		err = errtypes.NotFound(filepath.Join(n.ParentID, n.Name))
-		return
+		return nil, errtypes.NotFound(filepath.Join(n.ParentID, n.Name))
 	}
 
 	rp, err := fs.p.AssemblePermissions(ctx, n)
@@ -785,22 +785,30 @@ func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference,
 	var children []*node.Node
 	children, err = fs.tp.ListFolder(ctx, n)
 	if err != nil {
-		return
+		return nil, err
 	}
-
+	finfos := make([]*provider.ResourceInfo, len(children))
+	eg, ctx := errgroup.WithContext(ctx)
 	for i := range children {
-		np := rp
-		// add this childs permissions
-		pset, _ := n.PermissionSet(ctx)
-		node.AddPermissions(&np, &pset)
-		ri, err := children[i].AsResourceInfo(ctx, &np, mdKeys, fieldMask, utils.IsRelativeReference(ref))
-		if err != nil {
-			return nil, errtypes.InternalError(err.Error())
-		}
-		finfos = append(finfos, ri)
+		pos := i
+		eg.Go(func() error {
+			np := rp
+			// add this childs permissions
+			pset, _ := n.PermissionSet(ctx)
+			node.AddPermissions(&np, &pset)
+			ri, err := children[pos].AsResourceInfo(ctx, &np, mdKeys, fieldMask, utils.IsRelativeReference(ref))
+			if err != nil {
+				return errtypes.InternalError(err.Error())
+			}
+			finfos[pos] = ri
+			return nil
+		})
+	}
+	if err := eg.Wait(); err != nil {
+		return nil, err
 	}
 
-	return
+	return finfos, nil
 }
 
 // Delete deletes the specified resource

pkg/storage/utils/decomposedfs/lookup/lookup.go

@@ -143,7 +143,7 @@ func (lu *Lookup) NodeFromID(ctx context.Context, id *provider.ResourceId) (n *n
 		// The Resource references the root of a space
 		return lu.NodeFromSpaceID(ctx, id)
 	}
-	return node.ReadNode(ctx, lu, id.SpaceId, id.OpaqueId, false)
+	return node.ReadNode(ctx, lu, id.SpaceId, id.OpaqueId, false, nil, false)
 }
 
 // Pathify segments the beginning of a string into depth segments of width length
@@ -164,7 +164,7 @@ func Pathify(id string, depth, width int) string {
 
 // NodeFromSpaceID converts a resource id without an opaque id into a Node
 func (lu *Lookup) NodeFromSpaceID(ctx context.Context, id *provider.ResourceId) (n *node.Node, err error) {
-	node, err := node.ReadNode(ctx, lu, id.SpaceId, id.OpaqueId, false)
+	node, err := node.ReadNode(ctx, lu, id.SpaceId, id.OpaqueId, false, nil, false)
 	if err != nil {
 		return nil, err
 	}

pkg/storage/utils/decomposedfs/node/node.go

@@ -231,33 +231,35 @@ func (n *Node) SpaceOwnerOrManager(ctx context.Context) *userpb.UserId {
 }
 
 // ReadNode creates a new instance from an id and checks if it exists
-func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canListDisabledSpace bool) (*Node, error) {
+func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canListDisabledSpace bool, spaceRoot *Node, skipParentCheck bool) (*Node, error) {
 	var err error
 
-	// read space root
-	r := &Node{
-		SpaceID: spaceID,
-		lu:      lu,
-		ID:      spaceID,
-	}
-	r.SpaceRoot = r
-	r.owner, err = r.readOwner()
-	switch {
-	case metadata.IsNotExist(err):
-		return r, nil // swallow not found, the node defaults to exists = false
-	case err != nil:
-		return nil, err
-	}
-	r.Exists = true
+	if spaceRoot == nil {
+		// read space root
+		spaceRoot = &Node{
+			SpaceID: spaceID,
+			lu:      lu,
+			ID:      spaceID,
+		}
+		spaceRoot.SpaceRoot = spaceRoot
+		spaceRoot.owner, err = spaceRoot.readOwner()
+		switch {
+		case metadata.IsNotExist(err):
+			return spaceRoot, nil // swallow not found, the node defaults to exists = false
+		case err != nil:
+			return nil, err
+		}
+		spaceRoot.Exists = true
 
-	// lookup name in extended attributes
-	r.Name, err = r.XattrString(prefixes.NameAttr)
-	if err != nil {
-		return nil, err
+		// lookup name in extended attributes
+		spaceRoot.Name, err = spaceRoot.XattrString(prefixes.NameAttr)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// TODO ReadNode should not check permissions
-	if !canListDisabledSpace && r.IsDisabled() {
+	if !canListDisabledSpace && spaceRoot.IsDisabled() {
 		// no permission = not found
 		return nil, errtypes.NotFound(spaceID)
 	}
@@ -267,7 +269,7 @@ func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canLis
 
 	// check if this is a space root
 	if spaceID == nodeID {
-		return r, nil
+		return spaceRoot, nil
 	}
 
 	// are we reading a revision?
@@ -288,7 +290,7 @@ func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canLis
 		SpaceID:   spaceID,
 		lu:        lu,
 		ID:        nodeID,
-		SpaceRoot: r,
+		SpaceRoot: spaceRoot,
 	}
 	nodePath := n.InternalPath()
 
@@ -340,12 +342,14 @@ func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canLis
 	//     - can be made more robust with a journal
 	//     - same recursion mechanism can be used to purge items? sth we still need to do
 	//   - flag the two above options with dtime
-	_, err = os.Stat(n.ParentPath())
-	if err != nil {
-		if errors.Is(err, fs.ErrNotExist) {
-			return nil, errtypes.NotFound(err.Error())
+	if !skipParentCheck {
+		_, err = os.Stat(n.ParentPath())
+		if err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				return nil, errtypes.NotFound(err.Error())
+			}
+			return nil, err
 		}
-		return nil, err
 	}
 
 	if revisionSuffix == "" {
@@ -417,11 +421,10 @@ func (n *Node) Child(ctx context.Context, name string) (*Node, error) {
 	}
 
 	var c *Node
-	c, err = ReadNode(ctx, n.lu, spaceID, nodeID, false)
+	c, err = ReadNode(ctx, n.lu, spaceID, nodeID, false, n.SpaceRoot, true)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not read child node")
 	}
-	c.SpaceRoot = n.SpaceRoot
 
 	return c, nil
 }
@@ -655,14 +658,6 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi
 		}
 	}
 
-	var parentID *provider.ResourceId
-	if p, err := n.Parent(); err == nil {
-		parentID = &provider.ResourceId{
-			SpaceId:  p.SpaceID,
-			OpaqueId: p.ID,
-		}
-	}
-
 	ri = &provider.ResourceInfo{
 		Id:            id,
 		Path:          fn,
@@ -672,8 +667,11 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi
 		Target:        target,
 		PermissionSet: rp,
 		Owner:         n.Owner(),
-		ParentId:      parentID,
-		Name:          n.Name,
+		ParentId: &provider.ResourceId{
+			SpaceId:  n.SpaceID,
+			OpaqueId: n.ParentID,
+		},
+		Name: n.Name,
 	}
 
 	if n.IsProcessing() {

pkg/storage/utils/decomposedfs/node/node_test.go

@@ -74,7 +74,7 @@ var _ = Describe("Node", func() {
 			})
 			Expect(err).ToNot(HaveOccurred())
 
-			n, err := node.ReadNode(env.Ctx, env.Lookup, lookupNode.SpaceID, lookupNode.ID, false)
+			n, err := node.ReadNode(env.Ctx, env.Lookup, lookupNode.SpaceID, lookupNode.ID, false, nil, false)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(n.BlobID).To(Equal("file1-blobid"))
 		})
@@ -327,7 +327,7 @@ var _ = Describe("Node", func() {
 			})
 			Expect(err).ToNot(HaveOccurred())
 			// checking that the path "subpath" is denied properly
-			subfolder, err = node.ReadNode(env.Ctx, env.Lookup, subfolder.SpaceID, subfolder.ID, false)
+			subfolder, err = node.ReadNode(env.Ctx, env.Lookup, subfolder.SpaceID, subfolder.ID, false, nil, false)
 			Expect(err).ToNot(HaveOccurred())
 			subfolderActual, denied := subfolder.PermissionSet(env.Ctx)
 			subfolderExpected := ocsconv.NewDeniedRole().CS3ResourcePermissions()

pkg/storage/utils/decomposedfs/revisions.go

@@ -124,7 +124,7 @@ func (fs *Decomposedfs) DownloadRevision(ctx context.Context, ref *provider.Refe
 
 	spaceID := ref.ResourceId.SpaceId
 	// check if the node is available and has not been deleted
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false)
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false, nil, false)
 	if err != nil {
 		return nil, err
 	}
@@ -178,7 +178,7 @@ func (fs *Decomposedfs) RestoreRevision(ctx context.Context, ref *provider.Refer
 
 	spaceID := ref.ResourceId.SpaceId
 	// check if the node is available and has not been deleted
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false)
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false, nil, false)
 	if err != nil {
 		return err
 	}
@@ -315,7 +315,7 @@ func (fs *Decomposedfs) getRevisionNode(ctx context.Context, ref *provider.Refer
 
 	spaceID := ref.ResourceId.SpaceId
 	// check if the node is available and has not been deleted
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false)
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false, nil, false)
 	if err != nil {
 		return nil, err
 	}

pkg/storage/utils/decomposedfs/spaces.go

@@ -83,7 +83,7 @@ func (fs *Decomposedfs) CreateStorageSpace(ctx context.Context, req *provider.Cr
 		alias = templates.WithSpacePropertiesAndUser(u, req.Type, req.Name, fs.o.PersonalSpaceAliasTemplate)
 	}
 
-	root, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true) // will fall into `Exists` case below
+	root, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true, nil, false) // will fall into `Exists` case below
 	switch {
 	case err != nil:
 		return nil, err
@@ -268,7 +268,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 
 	if spaceID != spaceIDAny && nodeID != spaceIDAny {
 		// try directly reading the node
-		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true) // permission to read disabled space is checked later
+		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true, nil, false) // permission to read disabled space is checked later
 		if err != nil {
 			appctx.GetLogger(ctx).Error().Err(err).Str("id", nodeID).Msg("could not read node")
 			return nil, err
@@ -356,7 +356,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 			continue
 		}
 
-		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true)
+		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true, nil, true)
 		if err != nil {
 			appctx.GetLogger(ctx).Error().Err(err).Str("id", nodeID).Msg("could not read node, skipping")
 			continue
@@ -396,7 +396,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 	// if there are no matches (or they happened to be spaces for the owner) and the node is a child return a space
 	if len(matches) <= numShares && nodeID != spaceID {
 		// try node id
-		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true) // permission to read disabled space is checked in storageSpaceFromNode
+		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true, nil, false) // permission to read disabled space is checked in storageSpaceFromNode
 		if err != nil {
 			return nil, err
 		}
@@ -484,7 +484,7 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up
 	}
 
 	// check which permissions are needed
-	spaceNode, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true)
+	spaceNode, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true, nil, false)
 	if err != nil {
 		return nil, err
 	}
@@ -573,7 +573,7 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De
 		return err
 	}
 
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true) // permission to read disabled space is checked later
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true, nil, false) // permission to read disabled space is checked later
 	if err != nil {
 		return err
 	}

pkg/storage/utils/decomposedfs/testhelpers/helpers.go

@@ -285,7 +285,7 @@ func (t *TestEnv) CreateTestStorageSpace(typ string, quota *providerv1beta1.Quot
 	if err != nil {
 		return nil, err
 	}
-	h, err := node.ReadNode(t.Ctx, t.Lookup, sid.SpaceId, sid.OpaqueId, false)
+	h, err := node.ReadNode(t.Ctx, t.Lookup, sid.SpaceId, sid.OpaqueId, false, nil, false)
 	if err != nil {
 		return nil, err
 	}