Skip to content

Commit

Permalink
objstorage: fix race in vfsSync
Browse files Browse the repository at this point in the history
We have a race in `vfsSync`: if two goroutines Sync at the same time,
one might see the flag=false and return immediately, before the other
goroutine actually runs/completes the Sync.

The fix is to store a change counter and wait for any in-progress
syncs as necessary.

Informs cockroachdb/cockroach#124845
  • Loading branch information
RaduBerinde committed Jul 17, 2024
1 parent 17a2c98 commit cbd534e
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 11 deletions.
15 changes: 10 additions & 5 deletions objstorage/objstorageprovider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,14 @@ type provider struct {

remote remoteLockedState

// localObjectsChanged is set if non-remote objects were created or deleted
// but Sync was not yet called.
localObjectsChanged bool
// TODO(radu): move these fields to a localLockedState struct.
// localObjectsChanged is incremented whenever non-remote objects are created.
// The purpose of this counter is to avoid syncing the local filesystem when
// only remote objects are changed.
localObjectsChangeCounter uint64
// localObjectsChangeCounterSynced is the value of localObjectsChangeCounter
// value at the time the last completed sync was launched.
localObjectsChangeCounterSynced uint64

// knownObjects maintains information about objects that are known to the provider.
// It is initialized with the list of files in the manifest when we open a DB.
Expand Down Expand Up @@ -583,7 +588,7 @@ func (p *provider) addMetadataLocked(meta objstorage.ObjectMetadata) {
p.mu.remote.addExternalObject(meta)
}
} else {
p.mu.localObjectsChanged = true
p.mu.localObjectsChangeCounter++
}
}

Expand All @@ -602,7 +607,7 @@ func (p *provider) removeMetadata(fileNum base.DiskFileNum) {
if meta.IsRemote() {
p.mu.remote.catalogBatch.DeleteObject(fileNum)
} else {
p.mu.localObjectsChanged = true
p.mu.localObjectsChangeCounter++
}
}

Expand Down
16 changes: 10 additions & 6 deletions objstorage/objstorageprovider/vfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,19 +86,23 @@ func (p *provider) vfsInit() error {

func (p *provider) vfsSync() error {
p.mu.Lock()
shouldSync := p.mu.localObjectsChanged
p.mu.localObjectsChanged = false
counterVal := p.mu.localObjectsChangeCounter
lastSynced := p.mu.localObjectsChangeCounterSynced
p.mu.Unlock()

if !shouldSync {
if lastSynced >= counterVal {
return nil
}
if err := p.fsDir.Sync(); err != nil {
p.mu.Lock()
defer p.mu.Unlock()
p.mu.localObjectsChanged = true
return err
}

p.mu.Lock()
if p.mu.localObjectsChangeCounterSynced < counterVal {
p.mu.localObjectsChangeCounterSynced = counterVal
}
p.mu.Unlock()

return nil
}

Expand Down

0 comments on commit cbd534e

Please sign in to comment.