From f031d217ae3a2ce9cf22b576c8806d594bcdbd06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Thu, 4 May 2023 10:54:41 +0200 Subject: [PATCH 1/2] Fix propagation in concurrency scenarios With the previous code the propagation code read the parent node before grabbing the according write lock. That had the effect that when a directory was being updated concurrently by different operations the calculations were made on an outdated state, leading to incorrect numbers. After this change we grab the lock BEFORE actually reading the node which ensures that we always work with the proper data. --- pkg/storage/utils/decomposedfs/tree/tree.go | 178 ++++++++++---------- 1 file changed, 88 insertions(+), 90 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 2cb35524c6..d121252945 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -691,9 +691,9 @@ func (t *Tree) removeNode(path string, n *node.Node) error { // Propagate propagates changes to the root of the tree func (t *Tree) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) (err error) { sublog := appctx.GetLogger(ctx).With().Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Logger() - if !t.options.TreeTimeAccounting && !t.options.TreeSizeAccounting { + if !t.options.TreeTimeAccounting && (!t.options.TreeSizeAccounting || sizeDiff == 0) { // no propagation enabled - sublog.Debug().Msg("propagation disabled") + sublog.Debug().Msg("propagation disabled or nothing to propagate") return } @@ -707,11 +707,33 @@ func (t *Tree) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) (err for err == nil && n.ID != root.ID { sublog.Debug().Msg("propagating") - if n, err = n.Parent(); err != nil { - break + attrs := node.Attributes{} + + var f *lockedfile.File + // lock parent before reading treesize or tree time + switch t.lookup.MetadataBackend().(type) { + case metadata.MessagePackBackend: + f, err = lockedfile.OpenFile(t.lookup.MetadataBackend().MetadataPath(n.ParentPath()), 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 + f, err = lockedfile.OpenFile(n.ParentPath()+filelocks.LockFileSuffix, os.O_RDWR|os.O_CREATE, 0600) + } + if err != nil { + return err } + // always log error if closing node fails + defer func() { + // ignore already closed error + cerr := f.Close() + if err == nil && cerr != nil && !errors.Is(cerr, os.ErrClosed) { + err = cerr // only overwrite err with en error from close if the former was nil + } + }() - sublog = sublog.With().Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Logger() + if n, err = n.Parent(); err != nil { + return err + } // TODO none, sync and async? if !n.HasPropagation() { @@ -720,102 +742,78 @@ func (t *Tree) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) (err return nil } - if t.options.TreeTimeAccounting || (t.options.TreeSizeAccounting && sizeDiff != 0) { - attrs := node.Attributes{} + sublog = sublog.With().Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Logger() - var f *lockedfile.File - // lock node before reading treesize or tree time - switch t.lookup.MetadataBackend().(type) { - case metadata.MessagePackBackend: - f, err = lockedfile.OpenFile(t.lookup.MetadataBackend().MetadataPath(n.InternalPath()), 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 - f, err = lockedfile.OpenFile(n.InternalPath()+filelocks.LockFileSuffix, os.O_RDWR|os.O_CREATE, 0600) - } - if err != nil { - return err + if t.options.TreeTimeAccounting { + // update the parent tree time if it is older than the nodes mtime + updateSyncTime := false + + var tmTime time.Time + tmTime, err = n.GetTMTime() + switch { + case err != nil: + // missing attribute, or invalid format, overwrite + sublog.Debug().Err(err). + Msg("could not read tmtime attribute, overwriting") + updateSyncTime = true + case tmTime.Before(sTime): + sublog.Debug(). + Time("tmtime", tmTime). + Time("stime", sTime). + Msg("parent tmtime is older than node mtime, updating") + updateSyncTime = true + default: + sublog.Debug(). + Time("tmtime", tmTime). + Time("stime", sTime). + Dur("delta", sTime.Sub(tmTime)). + Msg("parent tmtime is younger than node mtime, not updating") } - // always log error if closing node fails - defer func() { - // ignore already closed error - cerr := f.Close() - if err == nil && cerr != nil && !errors.Is(cerr, os.ErrClosed) { - err = cerr // only overwrite err with en error from close if the former was nil - } - }() - - if t.options.TreeTimeAccounting { - // update the parent tree time if it is older than the nodes mtime - updateSyncTime := false - - var tmTime time.Time - tmTime, err = n.GetTMTime() - switch { - case err != nil: - // missing attribute, or invalid format, overwrite - sublog.Debug().Err(err). - Msg("could not read tmtime attribute, overwriting") - updateSyncTime = true - case tmTime.Before(sTime): - sublog.Debug(). - Time("tmtime", tmTime). - Time("stime", sTime). - Msg("parent tmtime is older than node mtime, updating") - updateSyncTime = true - default: - sublog.Debug(). - Time("tmtime", tmTime). - Time("stime", sTime). - Dur("delta", sTime.Sub(tmTime)). - Msg("parent tmtime is younger than node mtime, not updating") - } - - if updateSyncTime { - // update the tree time of the parent node - attrs.SetString(prefixes.TreeMTimeAttr, sTime.UTC().Format(time.RFC3339Nano)) - } - attrs.SetString(prefixes.TmpEtagAttr, "") + if updateSyncTime { + // update the tree time of the parent node + attrs.SetString(prefixes.TreeMTimeAttr, sTime.UTC().Format(time.RFC3339Nano)) } - // size accounting - if t.options.TreeSizeAccounting && sizeDiff != 0 { - var newSize uint64 + attrs.SetString(prefixes.TmpEtagAttr, "") + } - // read treesize - treeSize, err := n.GetTreeSize() - switch { - case metadata.IsAttrUnset(err): - // fallback to calculating the treesize - newSize, err = t.calculateTreeSize(ctx, n.InternalPath()) - if err != nil { - return err - } - case err != nil: + // size accounting + if t.options.TreeSizeAccounting && sizeDiff != 0 { + var newSize uint64 + + // read treesize + treeSize, err := n.GetTreeSize() + switch { + case metadata.IsAttrUnset(err): + // fallback to calculating the treesize + newSize, err = t.calculateTreeSize(ctx, n.InternalPath()) + if err != nil { return err - default: - if sizeDiff > 0 { - newSize = treeSize + uint64(sizeDiff) - } else { - newSize = treeSize - uint64(-sizeDiff) - } } - - // update the tree size of the node - attrs.SetString(prefixes.TreesizeAttr, strconv.FormatUint(newSize, 10)) - sublog.Debug().Uint64("newSize", newSize).Msg("updated treesize of parent node") - } - - if err = n.SetXattrs(attrs, false); err != nil { + case err != nil: return err + default: + if sizeDiff > 0 { + newSize = treeSize + uint64(sizeDiff) + } else { + newSize = treeSize - uint64(-sizeDiff) + } } - // Release node lock early, ignore already closed error - cerr := f.Close() - if cerr != nil && !errors.Is(cerr, os.ErrClosed) { - return cerr - } + // update the tree size of the node + attrs.SetString(prefixes.TreesizeAttr, strconv.FormatUint(newSize, 10)) + sublog.Debug().Uint64("newSize", newSize).Msg("updated treesize of parent node") + } + + if err = n.SetXattrs(attrs, false); err != nil { + return err + } + + // Release node lock early, ignore already closed error + cerr := f.Close() + if cerr != nil && !errors.Is(cerr, os.ErrClosed) { + return cerr } } if err != nil { From e106f3f6736def90aec85d25436b0530fe64f42d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Thu, 4 May 2023 11:13:34 +0200 Subject: [PATCH 2/2] Add changelog --- changelog/unreleased/fix-propagation.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/unreleased/fix-propagation.md diff --git a/changelog/unreleased/fix-propagation.md b/changelog/unreleased/fix-propagation.md new file mode 100644 index 0000000000..754125582c --- /dev/null +++ b/changelog/unreleased/fix-propagation.md @@ -0,0 +1,5 @@ +Bugfix: Fix propagation + +Fix propagation in concurrency scenarios + +https://github.com/cs3org/reva/pull/3845