Merge pull request #3397 from aduffeck/improve-lock-contention2

Further improve lock contention

changelog/unreleased/reduce-lock-contention.md

@@ -0,0 +1,5 @@
+Enhancement: Reduce lock contention issues
+
+We reduced lock contention during high load by caching the extended attributes of a file for the duration of a request.
+
+https://github.com/cs3org/reva/pull/3397

pkg/storage/utils/decomposedfs/decomposedfs.go

@@ -341,7 +341,7 @@ func (fs *Decomposedfs) CreateDir(ctx context.Context, ref *provider.Reference)
 
  if fs.o.TreeTimeAccounting || fs.o.TreeSizeAccounting {
  // mark the home node as the end of propagation
- if err = n.SetMetadata(xattrs.PropagationAttr, "1"); err != nil {
+ if err = n.SetXattr(xattrs.PropagationAttr, "1"); err != nil {
  appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not mark node to propagate")
 
  // FIXME: This does not return an error at all, but results in a severe situation that the

pkg/storage/utils/decomposedfs/grants.go

@@ -135,15 +135,21 @@ func (fs *Decomposedfs) ListGrants(ctx context.Context, ref *provider.Reference)
  }
  log := appctx.GetLogger(ctx)
  np := node.InternalPath()
- var attrs []string
- if attrs, err = xattrs.List(np); err != nil {
+ var attrs map[string]string
+ if attrs, err = node.Xattrs(); err != nil {
  log.Error().Err(err).Msg("error listing attributes")
  return nil, err
  }
+ attrNames := make([]string, len(attrs))
+ i := 0
+ for attr := range attrs {
+ attrNames[i] = attr
+ i++
+ }
 
- log.Debug().Interface("attrs", attrs).Msg("read attributes")
+ log.Debug().Interface("attrs", attrNames).Msg("read attributes")
 
- aces := extractACEsFromAttrs(ctx, np, attrs)
+ aces := extractACEsFromAttrs(ctx, np, attrNames)
 
  uid := ctxpkg.ContextMustGetUser(ctx).GetId()
  grants = make([]*provider.Grant, 0, len(aces))
@@ -303,7 +309,7 @@ func (fs *Decomposedfs) storeGrant(ctx context.Context, n *node.Node, g *provide
  // set the grant
  e := ace.FromGrant(g)
  principal, value := e.Marshal()
- if err := n.SetMetadata(xattrs.GrantPrefix+principal, string(value)); err != nil {
+ if err := n.SetXattr(xattrs.GrantPrefix+principal, string(value)); err != nil {
  appctx.GetLogger(ctx).Error().Err(err).
  Str("principal", principal).Msg("Could not set grant for principal")
  return err

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

@@ -134,7 +134,7 @@ func (lu *Lookup) WalkPath(ctx context.Context, r *node.Node, p string, followRe
  }
 
  if followReferences {
- if attrBytes, err := r.GetMetadata(xattrs.ReferenceAttr); err == nil {
+ if attrBytes, err := r.Xattr(xattrs.ReferenceAttr); err == nil {
  realNodeID := attrBytes
  ref, err := xattrs.ReferenceFromAttr([]byte(realNodeID))
  if err != nil {
@@ -147,7 +147,7 @@ func (lu *Lookup) WalkPath(ctx context.Context, r *node.Node, p string, followRe
  }
  }
  }
- if node.IsSpaceRoot(r) {
+ if r.IsSpaceRoot() {
  r.SpaceRoot = r
  }
 

pkg/storage/utils/decomposedfs/metadata.go

@@ -33,7 +33,6 @@ import (
  "github.com/cs3org/reva/v2/pkg/storagespace"
  "github.com/cs3org/reva/v2/pkg/utils"
  "github.com/pkg/errors"
- "github.com/pkg/xattr"
 )
 
 // SetArbitraryMetadata sets the metadata on a resource
@@ -69,8 +68,6 @@ func (fs *Decomposedfs) SetArbitraryMetadata(ctx context.Context, ref *provider.
  return err
  }
 
- nodePath := n.InternalPath()
-
  errs := []error{}
  // TODO should we really continue updating when an error occurs?
  if md.Metadata != nil {
@@ -115,7 +112,7 @@ func (fs *Decomposedfs) SetArbitraryMetadata(ctx context.Context, ref *provider.
  }
  for k, v := range md.Metadata {
  attrName := xattrs.MetadataPrefix + k
- if err = xattr.Set(nodePath, attrName, []byte(v)); err != nil {
+ if err = n.SetXattr(attrName, v); err != nil {
  errs = append(errs, errors.Wrap(err, "Decomposedfs: could not set metadata attribute "+attrName+" to "+k))
  }
  }
@@ -166,7 +163,6 @@ func (fs *Decomposedfs) UnsetArbitraryMetadata(ctx context.Context, ref *provide
  return err
  }
 
- nodePath := n.InternalPath()
  errs := []error{}
  for _, k := range keys {
  switch k {
@@ -189,7 +185,7 @@ func (fs *Decomposedfs) UnsetArbitraryMetadata(ctx context.Context, ref *provide
  continue
  }
  fa := fmt.Sprintf("%s:%s:%s@%s", xattrs.FavPrefix, utils.UserTypeToString(uid.GetType()), uid.GetOpaqueId(), uid.GetIdp())
- if err := xattrs.Remove(nodePath, fa); err != nil {
+ if err := n.RemoveXattr(fa); err != nil {
  if xattrs.IsAttrUnset(err) {
  continue // already gone, ignore
  }
@@ -200,7 +196,7 @@ func (fs *Decomposedfs) UnsetArbitraryMetadata(ctx context.Context, ref *provide
  errs = append(errs, errors.Wrap(err, "could not unset favorite flag"))
  }
  default:
- if err = xattrs.Remove(nodePath, xattrs.MetadataPrefix+k); err != nil {
+ if err = n.RemoveXattr(xattrs.MetadataPrefix + k); err != nil {
  if xattrs.IsAttrUnset(err) {
  continue // already gone, ignore
  }

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

@@ -81,8 +81,12 @@ var _ = Describe("Node locks", func() {
  AppName: "app2",
  LockId: uuid.New().String(),
  }
- n = node.New("u-s-e-r-id", "tobelockedid", "", "tobelocked", 10, "", env.Owner.Id, env.Lookup)
- n2 = node.New("u-s-e-r-id", "neverlockedid", "", "neverlocked", 10, "", env.Owner.Id, env.Lookup)
+ spaceResID, err := env.CreateTestStorageSpace("project", &provider.Quota{QuotaMaxBytes: 2000})
+ Expect(err).ToNot(HaveOccurred())
+ n, err = env.CreateTestFile("tobelockedid", "blob", spaceResID.OpaqueId, spaceResID.OpaqueId, 10)
+ Expect(err).ToNot(HaveOccurred())
+ n2, err = env.CreateTestFile("neverlockedlockedid", "blob", spaceResID.OpaqueId, spaceResID.OpaqueId, 10)
+ Expect(err).ToNot(HaveOccurred())
  })
 
  AfterEach(func() {