From 0ae1fdcad2fbc9f4719dd04d4c0de7cfc17f42c3 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 8 Jul 2026 01:27:10 -0700 Subject: [PATCH] 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 | 34 ++++++++++++++++++++ weed/storage/volume.go | 8 +++++ weed/storage/volume_loading.go | 5 ++- weed/storage/volume_tier.go | 30 ++++++++++++----- 4 files changed, 68 insertions(+), 9 deletions(-) diff --git a/weed/storage/remote_tier_integration_test.go b/weed/storage/remote_tier_integration_test.go index ec08130c3..e0dc733b5 100644 --- a/weed/storage/remote_tier_integration_test.go +++ b/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. diff --git a/weed/storage/volume.go b/weed/storage/volume.go index 25c8bd038..5c6f3f913 100644 --- a/weed/storage/volume.go +++ b/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() } diff --git a/weed/storage/volume_loading.go b/weed/storage/volume_loading.go index 6c5cf2284..3ede17c75 100644 --- a/weed/storage/volume_loading.go +++ b/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 diff --git a/weed/storage/volume_tier.go b/weed/storage/volume_tier.go index 21c442ee9..1f0214b9e 100644 --- a/weed/storage/volume_tier.go +++ b/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 }