Skip to content

Commit e77dd61

Browse files
kobergjkobergj
authored
Merge pull request #4924 from kobergj/FixSyncDefers
Fix SyncPropagation Defer
2 parents e4191d8 + 5076d64 commit e77dd61

File tree

2 files changed

+123
-117
lines changed

2 files changed

+123
-117
lines changed

‎changelog/unreleased/fix-sync-propagation.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Bugfix: Fix sync propagation
2+
3+
Fixes the defers in the sync propagation.
4+
5+
https://github.com/cs3org/reva/pull/4924

‎pkg/storage/utils/decomposedfs/tree/propagator/sync.go

+118-117
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes"
3131
"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node"
3232
"github.com/rogpeppe/go-internal/lockedfile"
33+
"github.com/rs/zerolog"
3334
)
3435

3536
// SyncPropagator implements synchronous treetime & treesize propagation
@@ -72,137 +73,137 @@ func (p SyncPropagator) Propagate(ctx context.Context, n *node.Node, sizeDiff in
7273
sTime := time.Now().UTC()
7374

7475
// we loop until we reach the root
75-
var err error
76-
for err == nil && n.ID != root.ID {
77-
sublog.Debug().Msg("propagating")
78-
79-
attrs := node.Attributes{}
80-
81-
var f *lockedfile.File
82-
// lock parent before reading treesize or tree time
83-
84-
_, subspan := tracer.Start(ctx, "lockedfile.OpenFile")
85-
parentFilename := p.lookup.MetadataBackend().LockfilePath(n.ParentPath())
86-
f, err = lockedfile.OpenFile(parentFilename, os.O_RDWR|os.O_CREATE, 0600)
87-
subspan.End()
88-
if err != nil {
89-
sublog.Error().Err(err).
90-
Str("parent filename", parentFilename).
91-
Msg("Propagation failed. Could not open metadata for parent with lock.")
92-
return err
93-
}
94-
// always log error if closing node fails
95-
defer func() {
96-
// ignore already closed error
97-
cerr := f.Close()
98-
if err == nil && cerr != nil && !errors.Is(cerr, os.ErrClosed) {
99-
err = cerr // only overwrite err with en error from close if the former was nil
100-
}
101-
_ = os.Remove(f.Name())
102-
}()
76+
var (
77+
err error
78+
stop bool
79+
)
10380

104-
if n, err = n.Parent(ctx); err != nil {
105-
sublog.Error().Err(err).
106-
Msg("Propagation failed. Could not read parent node.")
107-
return err
108-
}
81+
for err == nil && !stop && n.ID != root.ID {
82+
n, stop, err = p.propagateItem(ctx, n, sTime, sizeDiff, sublog)
83+
}
10984

110-
if !n.HasPropagation(ctx) {
111-
sublog.Debug().Str("attr", prefixes.PropagationAttr).Msg("propagation attribute not set or unreadable, not propagating")
112-
// if the attribute is not set treat it as false / none / no propagation
113-
return nil
114-
}
85+
if err != nil {
86+
sublog.Error().Err(err).Msg("error propagating")
87+
return err
88+
}
89+
return nil
90+
}
11591

116-
sublog = sublog.With().Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Logger()
117-
118-
if p.treeTimeAccounting {
119-
// update the parent tree time if it is older than the nodes mtime
120-
updateSyncTime := false
121-
122-
var tmTime time.Time
123-
tmTime, err = n.GetTMTime(ctx)
124-
switch {
125-
case err != nil:
126-
// missing attribute, or invalid format, overwrite
127-
sublog.Debug().Err(err).
128-
Msg("could not read tmtime attribute, overwriting")
129-
updateSyncTime = true
130-
case tmTime.Before(sTime):
131-
sublog.Debug().
132-
Time("tmtime", tmTime).
133-
Time("stime", sTime).
134-
Msg("parent tmtime is older than node mtime, updating")
135-
updateSyncTime = true
136-
default:
137-
sublog.Debug().
138-
Time("tmtime", tmTime).
139-
Time("stime", sTime).
140-
Dur("delta", sTime.Sub(tmTime)).
141-
Msg("parent tmtime is younger than node mtime, not updating")
142-
}
92+
func (p SyncPropagator) propagateItem(ctx context.Context, n *node.Node, sTime time.Time, sizeDiff int64, log zerolog.Logger) (*node.Node, bool, error) {
93+
log.Debug().Msg("propagating")
14394

144-
if updateSyncTime {
145-
// update the tree time of the parent node
146-
attrs.SetString(prefixes.TreeMTimeAttr, sTime.UTC().Format(time.RFC3339Nano))
147-
}
95+
attrs := node.Attributes{}
96+
97+
var f *lockedfile.File
98+
// lock parent before reading treesize or tree time
14899

149-
attrs.SetString(prefixes.TmpEtagAttr, "")
100+
_, subspan := tracer.Start(ctx, "lockedfile.OpenFile")
101+
parentFilename := p.lookup.MetadataBackend().LockfilePath(n.ParentPath())
102+
f, err := lockedfile.OpenFile(parentFilename, os.O_RDWR|os.O_CREATE, 0600)
103+
subspan.End()
104+
if err != nil {
105+
log.Error().Err(err).
106+
Str("parent filename", parentFilename).
107+
Msg("Propagation failed. Could not open metadata for parent with lock.")
108+
return nil, true, err
109+
}
110+
// always log error if closing node fails
111+
defer func() {
112+
// ignore already closed error
113+
cerr := f.Close()
114+
if err == nil && cerr != nil && !errors.Is(cerr, os.ErrClosed) {
115+
err = cerr // only overwrite err with en error from close if the former was nil
150116
}
117+
}()
151118

152-
// size accounting
153-
if p.treeSizeAccounting && sizeDiff != 0 {
154-
var newSize uint64
155-
156-
// read treesize
157-
treeSize, err := n.GetTreeSize(ctx)
158-
switch {
159-
case metadata.IsAttrUnset(err):
160-
// fallback to calculating the treesize
161-
sublog.Warn().Msg("treesize attribute unset, falling back to calculating the treesize")
162-
newSize, err = calculateTreeSize(ctx, p.lookup, n.InternalPath())
163-
if err != nil {
164-
return err
165-
}
166-
case err != nil:
167-
sublog.Error().Err(err).
168-
Msg("Faild to propagate treesize change. Error when reading the treesize attribute from parent")
169-
return err
170-
case sizeDiff > 0:
171-
newSize = treeSize + uint64(sizeDiff)
172-
case uint64(-sizeDiff) > treeSize:
173-
// The sizeDiff is larger than the current treesize. Which would result in
174-
// a negative new treesize. Something must have gone wrong with the accounting.
175-
// Reset the current treesize to 0.
176-
sublog.Error().Uint64("treeSize", treeSize).Int64("sizeDiff", sizeDiff).
177-
Msg("Error when updating treesize of parent node. Updated treesize < 0. Reestting to 0")
178-
newSize = 0
179-
default:
180-
newSize = treeSize - uint64(-sizeDiff)
181-
}
119+
if n, err = n.Parent(ctx); err != nil {
120+
log.Error().Err(err).
121+
Msg("Propagation failed. Could not read parent node.")
122+
return n, true, err
123+
}
124+
125+
if !n.HasPropagation(ctx) {
126+
log.Debug().Str("attr", prefixes.PropagationAttr).Msg("propagation attribute not set or unreadable, not propagating")
127+
// if the attribute is not set treat it as false / none / no propagation
128+
return n, true, nil
129+
}
182130

183-
// update the tree size of the node
184-
attrs.SetString(prefixes.TreesizeAttr, strconv.FormatUint(newSize, 10))
185-
sublog.Debug().Uint64("newSize", newSize).Msg("updated treesize of parent node")
131+
log = log.With().Str("spaceid", n.SpaceID).Str("nodeid", n.ID).Logger()
132+
133+
if p.treeTimeAccounting {
134+
// update the parent tree time if it is older than the nodes mtime
135+
updateSyncTime := false
136+
137+
var tmTime time.Time
138+
tmTime, err = n.GetTMTime(ctx)
139+
switch {
140+
case err != nil:
141+
// missing attribute, or invalid format, overwrite
142+
log.Debug().Err(err).
143+
Msg("could not read tmtime attribute, overwriting")
144+
updateSyncTime = true
145+
case tmTime.Before(sTime):
146+
log.Debug().
147+
Time("tmtime", tmTime).
148+
Time("stime", sTime).
149+
Msg("parent tmtime is older than node mtime, updating")
150+
updateSyncTime = true
151+
default:
152+
log.Debug().
153+
Time("tmtime", tmTime).
154+
Time("stime", sTime).
155+
Dur("delta", sTime.Sub(tmTime)).
156+
Msg("parent tmtime is younger than node mtime, not updating")
186157
}
187158

188-
if err = n.SetXattrsWithContext(ctx, attrs, false); err != nil {
189-
sublog.Error().Err(err).Msg("Failed to update extend attributes of parent node")
190-
return err
159+
if updateSyncTime {
160+
// update the tree time of the parent node
161+
attrs.SetString(prefixes.TreeMTimeAttr, sTime.UTC().Format(time.RFC3339Nano))
191162
}
192163

193-
// Release node lock early, ignore already closed error
194-
_, subspan = tracer.Start(ctx, "f.Close")
195-
cerr := f.Close()
196-
subspan.End()
197-
if cerr != nil && !errors.Is(cerr, os.ErrClosed) {
198-
sublog.Error().Err(cerr).Msg("Failed to close parent node and release lock")
199-
return cerr
164+
attrs.SetString(prefixes.TmpEtagAttr, "")
165+
}
166+
167+
// size accounting
168+
if p.treeSizeAccounting && sizeDiff != 0 {
169+
var newSize uint64
170+
171+
// read treesize
172+
treeSize, err := n.GetTreeSize(ctx)
173+
switch {
174+
case metadata.IsAttrUnset(err):
175+
// fallback to calculating the treesize
176+
log.Warn().Msg("treesize attribute unset, falling back to calculating the treesize")
177+
newSize, err = calculateTreeSize(ctx, p.lookup, n.InternalPath())
178+
if err != nil {
179+
return n, true, err
180+
}
181+
case err != nil:
182+
log.Error().Err(err).
183+
Msg("Faild to propagate treesize change. Error when reading the treesize attribute from parent")
184+
return n, true, err
185+
case sizeDiff > 0:
186+
newSize = treeSize + uint64(sizeDiff)
187+
case uint64(-sizeDiff) > treeSize:
188+
// The sizeDiff is larger than the current treesize. Which would result in
189+
// a negative new treesize. Something must have gone wrong with the accounting.
190+
// Reset the current treesize to 0.
191+
log.Error().Uint64("treeSize", treeSize).Int64("sizeDiff", sizeDiff).
192+
Msg("Error when updating treesize of parent node. Updated treesize < 0. Reestting to 0")
193+
newSize = 0
194+
default:
195+
newSize = treeSize - uint64(-sizeDiff)
200196
}
197+
198+
// update the tree size of the node
199+
attrs.SetString(prefixes.TreesizeAttr, strconv.FormatUint(newSize, 10))
200+
log.Debug().Uint64("newSize", newSize).Msg("updated treesize of parent node")
201201
}
202-
if err != nil {
203-
sublog.Error().Err(err).Msg("error propagating")
204-
return err
202+
203+
if err = n.SetXattrsWithContext(ctx, attrs, false); err != nil {
204+
log.Error().Err(err).Msg("Failed to update extend attributes of parent node")
205+
return n, true, err
205206
}
206-
return nil
207207

208+
return n, false, nil
208209
}

0 commit comments

Comments
 (0)