From 254c2a1024466affc840867f1af02341725a9f82 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 7 Jul 2026 23:03:17 -0700 Subject: [PATCH] volume: clear remote flag when tiering a volume back to local (#10262) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VolumeTierMoveDatFromRemote downloads the .dat, trims the .vif, and swaps the data backend to the local file, but left hasRemoteFile set. The volume.tier.download command masks this by unmounting and remounting right after, which reloads the flag from the trimmed .vif — but in the window before the remount the in-memory flag is wrong: doDeleteRequest would skip appending the tombstone to the freshly local .dat, and the phantom-.dat guard stays disabled. Give SwapDataBackend a hasRemoteFile argument so the backend swap and the flag move together under one lock, and route both tier directions through it: the tier-down download passes false, LoadRemoteFile passes true. The flag can no longer disagree with the live backend. --- weed/server/volume_grpc_tier_download.go | 13 +++++++------ weed/server/volume_grpc_tier_download_test.go | 14 +++++++++++--- weed/storage/volume.go | 15 ++++++++++----- weed/storage/volume_tier.go | 13 ++++++------- 4 files changed, 34 insertions(+), 21 deletions(-) diff --git a/weed/server/volume_grpc_tier_download.go b/weed/server/volume_grpc_tier_download.go index af07f80d6..81ef87505 100644 --- a/weed/server/volume_grpc_tier_download.go +++ b/weed/server/volume_grpc_tier_download.go @@ -85,10 +85,10 @@ func (vs *VolumeServer) VolumeTierMoveDatFromRemote(req *volume_server_pb.Volume } // Trim the remote file reference and persist the .vif (util.WriteFile fsyncs it) - // BEFORE deleting the remote object. After this point hasRemoteFile is false, so a - // crash before DeleteFile merely leaks the remote object while the volume reloads - // its local .dat. The volume must NEVER be left with a .vif referencing the remote - // object while that object is deleted. + // BEFORE deleting the remote object. After this the .vif no longer references the + // remote object, so a crash before DeleteFile merely leaks the remote object while + // the volume reloads its local .dat as non-remote. The volume must NEVER be left + // with a .vif referencing the remote object while that object is deleted. v.GetVolumeInfo().Files = v.GetVolumeInfo().Files[1:] if err := v.SaveVolumeInfo(); err != nil { return fmt.Errorf("volume %d failed to save remote file info: %v", v.Id, err) @@ -127,8 +127,9 @@ func swapToLocalDatBackend(v *storage.Volume, datFileName string) error { return err } // Swap under the volume's data lock so concurrent reads never see a closed - // or half-swapped backend. - v.SwapDataBackend(backend.NewDiskFile(dataFile)) + // or half-swapped backend, clearing hasRemoteFile in the same step since the + // volume now serves from the local .dat. + v.SwapDataBackend(backend.NewDiskFile(dataFile), false) return nil } diff --git a/weed/server/volume_grpc_tier_download_test.go b/weed/server/volume_grpc_tier_download_test.go index 43401ca26..f346b5257 100644 --- a/weed/server/volume_grpc_tier_download_test.go +++ b/weed/server/volume_grpc_tier_download_test.go @@ -121,8 +121,8 @@ func (f *tierTestBackendFile) ReadAt(p []byte, off int64) (int, error) { func (f *tierTestBackendFile) WriteAt(p []byte, off int64) (int, error) { panic("not implemented") } func (f *tierTestBackendFile) Truncate(off int64) error { panic("not implemented") } func (f *tierTestBackendFile) Close() error { return nil } -func (f *tierTestBackendFile) Name() string { return f.key } -func (f *tierTestBackendFile) Sync() error { return nil } +func (f *tierTestBackendFile) Name() string { return f.key } +func (f *tierTestBackendFile) Sync() error { return nil } func (f *tierTestBackendFile) GetStat() (int64, time.Time, error) { files := f.tierInfo.GetFiles() if len(files) == 0 { @@ -136,7 +136,9 @@ type fakeTierStream struct { grpc.ServerStream } -func (s *fakeTierStream) Send(*volume_server_pb.VolumeTierMoveDatFromRemoteResponse) error { return nil } +func (s *fakeTierStream) Send(*volume_server_pb.VolumeTierMoveDatFromRemoteResponse) error { + return nil +} func newTierTestStore(t *testing.T, dir string) *storage.Store { t.Helper() @@ -276,6 +278,12 @@ func TestTierMoveDatFromRemote_KeepRemote_LeavesReplicaLocal(t *testing.T) { if _, ok := v.DataBackend.(*backend.DiskFile); !ok { t.Fatalf("after keep-remote download the data backend must be local DiskFile, got %T", v.DataBackend) } + // The live volume must also report non-remote immediately, without waiting for + // a remount: a delete arriving before the remount would otherwise skip the .dat + // tombstone, and the phantom-.dat guard would stay disabled. + if v.HasRemoteFile() { + t.Fatal("after download the live volume must no longer be remote-backed") + } // Remount and confirm the volume is no longer remote-backed and reads come // from the local .dat (matching the bytes uploaded before tiering). diff --git a/weed/storage/volume.go b/weed/storage/volume.go index e2b81fb17..25c8bd038 100644 --- a/weed/storage/volume.go +++ b/weed/storage/volume.go @@ -312,17 +312,22 @@ func (v *Volume) Close() { v.doClose() } -// SwapDataBackend atomically replaces the data backend (e.g. swapping a -// remote-tier backend for a freshly downloaded local .dat), closing the old -// one. Held under dataFileAccessLock so a concurrent read/write never observes -// a half-swapped or closed backend. -func (v *Volume) SwapDataBackend(newBackend backend.BackendStorageFile) { +// SwapDataBackend atomically replaces the data backend and updates the +// remote-tier flag under dataFileAccessLock, closing the old backend. Both tier +// directions go through here so hasRemoteFile always matches the live backend: +// tier-down passes hasRemoteFile=false (now serving a local .dat), tier-up +// passes true. Keeping the swap and the flag under one lock means the heartbeat +// never observes a half-swapped backend or a flag that disagrees with it — a +// stale-false flag would make doDeleteRequest skip the new .dat's tombstones and +// disable the phantom-.dat guard. +func (v *Volume) SwapDataBackend(newBackend backend.BackendStorageFile, hasRemoteFile bool) { v.dataFileAccessLock.Lock() defer v.dataFileAccessLock.Unlock() if v.DataBackend != nil { v.DataBackend.Close() } v.DataBackend = newBackend + v.hasRemoteFile.Store(hasRemoteFile) } func (v *Volume) doClose() { diff --git a/weed/storage/volume_tier.go b/weed/storage/volume_tier.go index 2704b9e60..21c442ee9 100644 --- a/weed/storage/volume_tier.go +++ b/weed/storage/volume_tier.go @@ -68,13 +68,12 @@ func (v *Volume) LoadRemoteFile() error { } // Swap under dataFileAccessLock (via SwapDataBackend) so the heartbeat's - // concurrent DataBackend read never races this reassignment. Then mark the - // volume tiered 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)) - v.hasRemoteFile.Store(true) + // 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) return nil }