volume: reload a remote-tiered volume without re-entering the data lock (#10266)

LoadRemoteFile now takes dataFileAccessLock (so a live tier upload does
not race the heartbeat's DataBackend read). But load() also runs it, and
CommitCompact calls load() while already holding that lock, so reloading
a remote-tiered volume during a compaction commit re-enters the
non-reentrant lock and deadlocks.

Split the locked bodies out: swapDataBackendLocked and loadRemoteFileLocked
assume the caller holds dataFileAccessLock. load() uses loadRemoteFileLocked;
the public LoadRemoteFile keeps taking the lock for the live tier-upload
handler that does not hold it.

weed/storage/remote_tier_integration_test.go

@@ -294,6 +294,40 @@ func TestRemoteTier_LiveTierUpload_StillReportsToMaster(t *testing.T) {
 	require.NotEmpty(t, msg.RemoteStorageName, "reported volume must carry its remote backend name")
 }
 
+// TestRemoteTier_ReloadUnderDataLock_NoDeadlock guards the reload-under-lock
+// path: CommitCompact holds dataFileAccessLock and calls v.load(), which for a
+// remote-tiered volume swaps the data backend. That swap must go through the
+// lock-free loadRemoteFileLocked; if load() instead used the public
+// LoadRemoteFile (which takes dataFileAccessLock), it would re-enter the held
+// lock and deadlock.
+func TestRemoteTier_ReloadUnderDataLock_NoDeadlock(t *testing.T) {
+	b := newLocalDirBackend(t)
+	registerTestBackend(t, b)
+
+	dir := t.TempDir()
+	const vid = needle.VolumeId(68)
+	v, _ := tierUpVolumeLive(t, dir, vid, b)
+	v.location = &DiskLocation{Directory: dir, DiskType: types.HddType}
+	require.True(t, v.HasRemoteFile())
+
+	done := make(chan error, 1)
+	go func() {
+		// Mirror CommitCompact: hold the data lock across the reload.
+		v.dataFileAccessLock.Lock()
+		defer v.dataFileAccessLock.Unlock()
+		done <- v.load(true, false, v.needleMapKind, 0, v.Version())
+	}()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+		require.True(t, v.HasRemoteFile(), "volume must stay remote-tiered after reload")
+		v.Close()
+	case <-time.After(10 * time.Second):
+		t.Fatal("reload under dataFileAccessLock deadlocked: load() re-entered the held lock via LoadRemoteFile")
+	}
+}
+
 // TestRemoteTier_Move_KeepsRemoteObject simulates the move-on-source-after-copy
 // step of a balance: Destroy(onlyEmpty=false, keepRemoteData=true). The remote
 // object must survive — the destination's freshly-copied .vif points at it.

weed/storage/volume.go

@@ -323,6 +323,14 @@ func (v *Volume) Close() {
 func (v *Volume) SwapDataBackend(newBackend backend.BackendStorageFile, hasRemoteFile bool) {
 	v.dataFileAccessLock.Lock()
 	defer v.dataFileAccessLock.Unlock()
+	v.swapDataBackendLocked(newBackend, hasRemoteFile)
+}
+
+// swapDataBackendLocked is the body of SwapDataBackend for callers that already
+// hold dataFileAccessLock (e.g. load() reached while CommitCompact holds the
+// lock). Reusing it from those under-lock paths avoids re-entering the
+// non-reentrant lock, which would deadlock.
+func (v *Volume) swapDataBackendLocked(newBackend backend.BackendStorageFile, hasRemoteFile bool) {
 	if v.DataBackend != nil {
 		v.DataBackend.Close()
 	}

weed/storage/volume_loading.go

@@ -161,7 +161,10 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind
 		v.noWriteCanDelete = true
 		v.noWriteOrDelete = false
 		glog.V(0).Infof("loading volume %d from remote %v", v.Id, v.volumeInfo)
-		if err := v.LoadRemoteFile(); err != nil {
+		// loadRemoteFileLocked, not LoadRemoteFile: load() is reached from
+		// CommitCompact with dataFileAccessLock already held, and the locking
+		// variant would deadlock re-entering it.
+		if err := v.loadRemoteFileLocked(); err != nil {
 			return fmt.Errorf("load remote file %v: %w", v.volumeInfo, err)
 		}
 		// Set lastModifiedTsSeconds from remote file to prevent premature expiry on startup

weed/storage/volume_tier.go

@@ -60,20 +60,34 @@ func (v *Volume) HasRemoteFile() bool {
 	return v.hasRemoteFile.Load()
 }
 
+// LoadRemoteFile swaps the data backend to the remote tier object under
+// dataFileAccessLock. Call this from a context that does NOT already hold the
+// lock — the live tier-upload handler, where the heartbeat may be reading the
+// backend concurrently. load() must instead use loadRemoteFileLocked, since it
+// can be reached with the lock already held (CommitCompact).
 func (v *Volume) LoadRemoteFile() error {
+	v.dataFileAccessLock.Lock()
+	defer v.dataFileAccessLock.Unlock()
+	return v.loadRemoteFileLocked()
+}
+
+// loadRemoteFileLocked swaps the data backend to the remote tier object. The
+// caller must hold dataFileAccessLock or be single-threaded (load() during
+// construction or a compaction-commit reload). It marks the volume tiered in the
+// same locked step so a later heartbeat does not treat a removed local .dat as a
+// phantom volume and stop reporting it to the master.
+func (v *Volume) loadRemoteFileLocked() error {
+	// Callers only reach here for a tiered volume (HasRemoteFile / a just-appended
+	// remote file), but guard the index so a stray call is a clean error, not a panic.
+	if len(v.volumeInfo.GetFiles()) == 0 {
+		return fmt.Errorf("volume %d has no remote file to load", v.Id)
+	}
 	tierFile := v.volumeInfo.GetFiles()[0]
 	backendStorage, found := backend.BackendStorages[tierFile.BackendName()]
 	if !found {
 		return fmt.Errorf("backend storage %s not found", tierFile.BackendName())
 	}
-
-	// Swap under dataFileAccessLock (via SwapDataBackend) so the heartbeat's
-	// concurrent DataBackend read never races this reassignment, and mark the
-	// volume tiered in the same locked step so a later heartbeat does not treat
-	// the just-removed local .dat as a phantom volume and stop reporting it to
-	// the master. On disk-scan load this is already true; here it flips a volume
-	// that was tier-uploaded in-process without a reload.
-	v.SwapDataBackend(backendStorage.NewStorageFile(tierFile.Key, v.volumeInfo), true)
+	v.swapDataBackendLocked(backendStorage.NewStorageFile(tierFile.Key, v.volumeInfo), true)
 	return nil
 }