From cbd534e661cc0f293532d03a31038d72aba4990f Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Fri, 12 Jul 2024 08:30:27 -0700
Subject: [PATCH] objstorage: fix race in vfsSync

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 https://github.com/cockroachdb/cockroach/issues/124845
---
 objstorage/objstorageprovider/provider.go | 15 ++++++++++-----
 objstorage/objstorageprovider/vfs.go      | 16 ++++++++++------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/objstorage/objstorageprovider/provider.go b/objstorage/objstorageprovider/provider.go
index e50f2c82b6..953a9d33dd 100644
--- a/objstorage/objstorageprovider/provider.go
+++ b/objstorage/objstorageprovider/provider.go
@@ -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.
@@ -583,7 +588,7 @@ func (p *provider) addMetadataLocked(meta objstorage.ObjectMetadata) {
 			p.mu.remote.addExternalObject(meta)
 		}
 	} else {
-		p.mu.localObjectsChanged = true
+		p.mu.localObjectsChangeCounter++
 	}
 }
 
@@ -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++
 	}
 }
 
diff --git a/objstorage/objstorageprovider/vfs.go b/objstorage/objstorageprovider/vfs.go
index b3d2326f7b..6e842dcef8 100644
--- a/objstorage/objstorageprovider/vfs.go
+++ b/objstorage/objstorageprovider/vfs.go
@@ -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
 }