From 7c252e1f169908fbc779730edf8b161b83290182 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 18 May 2026 20:51:04 -0700 Subject: [PATCH] fix(volume): reopen .idx writable after MarkVolumeWritable (fixes #9515) (#9526) * fix(volume): reopen .idx writable after MarkVolumeWritable When .vif has ReadOnly=true, load() opens .idx as O_RDONLY and builds a SortedFileNeedleMap whose Put returns os.ErrInvalid. MarkVolumeWritable only flipped noWriteOrDelete back to false and rewrote .vif, so writes still failed at v.nm.Put. Reopen .idx in O_RDWR and rebuild v.nm in its writable form (in-memory or leveldb small/medium/large) before flipping the flag. Mirror the same fix in seaweed-volume: the Rust load path leaves CompactNeedleMap/RedbNeedleMap with no idx_file writer when the volume boots read-only, so post-MarkVolumeWritable puts silently succeeded in-memory only and were lost on the next restart. set_writable now reattaches an append-mode writer when one is missing. * fix(volume): keep old needle map until replacement is built; defer writable flag Go: build the writable needle map into a local before swapping. A construction failure now leaves v.nm pointing at the original SortedFileNeedleMap so MarkVolumeWritable can roll back, instead of stranding the volume with v.nm == nil. Rust: attach the .idx writer before flipping no_write_or_delete to false. A transient open/metadata failure used to leave the volume marked writable with no writer attached, and subsequent puts would silently skip the on-disk append. --- seaweed-volume/src/storage/needle_map.rs | 20 ++++ seaweed-volume/src/storage/volume.rs | 112 ++++++++++++++++++++++ weed/storage/store.go | 7 ++ weed/storage/volume_loading.go | 78 +++++++++++++++ weed/storage/volume_mark_writable_test.go | 87 +++++++++++++++++ 5 files changed, 304 insertions(+) create mode 100644 weed/storage/volume_mark_writable_test.go diff --git a/seaweed-volume/src/storage/needle_map.rs b/seaweed-volume/src/storage/needle_map.rs index 140ac5b5e..2d0dfb456 100644 --- a/seaweed-volume/src/storage/needle_map.rs +++ b/seaweed-volume/src/storage/needle_map.rs @@ -212,6 +212,13 @@ impl CompactNeedleMap { self.idx_file_offset = offset; } + /// True when an .idx file writer is attached. A read-only load leaves + /// this `false` — set_writable() must reattach a writer or subsequent + /// puts silently skip the disk append. + pub fn has_idx_writer(&self) -> bool { + self.idx_file.is_some() + } + // ---- Map operations ---- /// Insert or update an entry. Appends to .idx file if present. @@ -705,6 +712,11 @@ impl RedbNeedleMap { self.idx_file_offset = offset; } + /// True when an .idx file writer is attached. See CompactNeedleMap. + pub fn has_idx_writer(&self) -> bool { + self.idx_file.is_some() + } + // ---- Map operations ---- /// Insert or update an entry. Writes to idx file first, then redb. @@ -1000,6 +1012,14 @@ impl NeedleMap { } } + /// True when an .idx file writer is attached. + pub fn has_idx_writer(&self) -> bool { + match self { + NeedleMap::InMemory(nm) => nm.has_idx_writer(), + NeedleMap::Redb(nm) => nm.has_idx_writer(), + } + } + /// Content byte count. pub fn content_size(&self) -> u64 { match self { diff --git a/seaweed-volume/src/storage/volume.rs b/seaweed-volume/src/storage/volume.rs index d4dc682e9..b01bf6b0a 100644 --- a/seaweed-volume/src/storage/volume.rs +++ b/seaweed-volume/src/storage/volume.rs @@ -2122,7 +2122,34 @@ impl Volume { } /// Mark this volume as writable (allow writes and deletes). + /// + /// If the volume booted with .vif ReadOnly=true, `load_index` built the + /// needle map without an .idx writer attached, so subsequent puts would + /// silently skip the on-disk append and only mutate in-memory state — + /// surviving until the next restart, then vanishing. Re-attach a writer + /// here so writes persist again. pub fn set_writable(&mut self) -> Result<(), VolumeError> { + // Attach the writer (if missing) before flipping the flag — otherwise + // a transient open/metadata failure would leave the volume marked + // writable with no .idx writer, and subsequent puts would silently + // skip the on-disk append and vanish on the next restart. + let needs_idx_writer = self + .nm + .as_ref() + .map(|nm| !nm.has_idx_writer()) + .unwrap_or(false); + if needs_idx_writer { + let idx_path = self.file_name(".idx"); + let write_file = OpenOptions::new() + .write(true) + .append(true) + .create(true) + .open(&idx_path)?; + let idx_size = write_file.metadata()?.len(); + if let Some(ref mut nm) = self.nm { + nm.set_idx_file(Box::new(write_file), idx_size); + } + } self.no_write_or_delete = false; self.save_vif() } @@ -4191,6 +4218,91 @@ mod tests { assert!(v.no_write_can_delete); } + // A volume booted with .vif ReadOnly=true used to come back stuck — + // load_index_inmemory built the CompactNeedleMap without an .idx writer + // attached, and set_writable only flipped the flag and rewrote .vif. + // The next put silently skipped the .idx append, so the write landed in + // memory only and was lost on the next restart. + #[test] + fn test_set_writable_reattaches_idx_writer_after_persisted_readonly() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().to_str().unwrap(); + + { + let mut v = make_test_volume(dir); + let mut n = Needle { + id: NeedleId(1), + cookie: Cookie(1), + data: b"initial".to_vec(), + data_size: 7, + ..Needle::default() + }; + v.write_needle(&mut n, true).unwrap(); + v.set_read_only_persist(true).unwrap(); + v.sync_to_disk().unwrap(); + } + + let mut v = Volume::new( + dir, + dir, + "", + VolumeId(1), + NeedleMapKind::InMemory, + None, + None, + 0, + Version::current(), + ) + .unwrap(); + assert!( + v.no_write_or_delete, + "reloaded volume should be read-only from .vif" + ); + assert!( + !v.nm.as_ref().unwrap().has_idx_writer(), + "read-only load should not attach an .idx writer" + ); + + v.set_writable().unwrap(); + assert!(!v.is_read_only()); + assert!( + v.nm.as_ref().unwrap().has_idx_writer(), + "set_writable must reattach the .idx writer or post-restart writes vanish" + ); + + let mut n = Needle { + id: NeedleId(2), + cookie: Cookie(2), + data: b"after-mark-writable".to_vec(), + data_size: 19, + ..Needle::default() + }; + v.write_needle(&mut n, true).unwrap(); + v.sync_to_disk().unwrap(); + + // Reload one more time — the .idx must contain the post-mark-writable + // entry, not just have it in memory. + drop(v); + let v = Volume::new( + dir, + dir, + "", + VolumeId(1), + NeedleMapKind::InMemory, + None, + None, + 0, + Version::current(), + ) + .unwrap(); + let mut probe = Needle { + id: NeedleId(2), + ..Needle::default() + }; + v.read_needle(&mut probe).unwrap(); + assert_eq!(std::str::from_utf8(&probe.data).unwrap(), "after-mark-writable"); + } + #[test] fn test_load_vif_defaults_local_version_and_bytes_offset() { let tmp = TempDir::new().unwrap(); diff --git a/weed/storage/store.go b/weed/storage/store.go index c00f82072..9eeeb1493 100644 --- a/weed/storage/store.go +++ b/weed/storage/store.go @@ -697,6 +697,13 @@ func (s *Store) MarkVolumeWritable(i needle.VolumeId) error { if v == nil { return fmt.Errorf("volume %d not found", i) } + // If the volume booted with .vif ReadOnly=true, .idx is opened O_RDONLY + // and v.nm is a SortedFileNeedleMap that rejects Put. Swap to writable + // form before flipping the flag so the next write doesn't race past a + // stale read-only handle. + if err := v.reopenIdxForWrite(); err != nil { + return fmt.Errorf("volume %d reopen idx for write: %v", i, err) + } v.noWriteLock.Lock() v.noWriteOrDelete = false v.PersistReadOnly(false) diff --git a/weed/storage/volume_loading.go b/weed/storage/volume_loading.go index c77ec2f48..70ab51df8 100644 --- a/weed/storage/volume_loading.go +++ b/weed/storage/volume_loading.go @@ -55,6 +55,84 @@ func loadVolumeWithoutWorker(dirname string, dirIdx string, collection string, i return } +// reopenIdxForWrite swaps the read-only SortedFileNeedleMap (loaded when the +// volume booted with .vif ReadOnly=true) for the writable needle map matching +// v.needleMapKind. Without this, MarkVolumeWritable flips noWriteOrDelete back +// to false but leaves .idx opened O_RDONLY and v.nm as a SortedFileNeedleMap +// whose Put returns os.ErrInvalid, so subsequent writes still fail. +// +// No-op when v.nm is already a writable form. +func (v *Volume) reopenIdxForWrite() error { + v.dataFileAccessLock.Lock() + defer v.dataFileAccessLock.Unlock() + + oldNm, isSorted := v.nm.(*SortedFileNeedleMap) + if !isSorted { + return nil + } + + indexFile, err := os.OpenFile(v.FileName(".idx"), os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + return fmt.Errorf("reopen %s read-write: %v", v.FileName(".idx"), err) + } + + // Build the replacement first; only swap once we have a live writable + // map. A construction failure must leave v.nm pointing at the original + // SortedFileNeedleMap so the caller (MarkVolumeWritable) can roll back + // cleanly instead of stranding the volume with v.nm == nil. + var newNm NeedleMapper + switch v.needleMapKind { + case NeedleMapInMemory: + if newNm, err = LoadCompactNeedleMap(indexFile, v.Version()); err != nil { + indexFile.Close() + return fmt.Errorf("rebuild memory needle map for volume %d: %v", v.Id, err) + } + case NeedleMapLevelDb: + opts := &opt.Options{ + BlockCacheCapacity: 2 * 1024 * 1024, + WriteBuffer: 1 * 1024 * 1024, + CompactionTableSizeMultiplier: 10, + OpenFilesCacheCapacity: LevelDbOpenFilesCacheCapacity, + } + if newNm, err = NewLevelDbNeedleMap(v.FileName(".ldb"), indexFile, opts, v.ldbTimeout, v.Version()); err != nil { + indexFile.Close() + return fmt.Errorf("rebuild leveldb needle map for volume %d: %v", v.Id, err) + } + case NeedleMapLevelDbMedium: + opts := &opt.Options{ + BlockCacheCapacity: 4 * 1024 * 1024, + WriteBuffer: 2 * 1024 * 1024, + CompactionTableSizeMultiplier: 10, + OpenFilesCacheCapacity: LevelDbMediumOpenFilesCacheCapacity, + } + if newNm, err = NewLevelDbNeedleMap(v.FileName(".ldb"), indexFile, opts, v.ldbTimeout, v.Version()); err != nil { + indexFile.Close() + return fmt.Errorf("rebuild leveldb medium needle map for volume %d: %v", v.Id, err) + } + case NeedleMapLevelDbLarge: + opts := &opt.Options{ + BlockCacheCapacity: 8 * 1024 * 1024, + WriteBuffer: 4 * 1024 * 1024, + CompactionTableSizeMultiplier: 10, + OpenFilesCacheCapacity: LevelDbLargeOpenFilesCacheCapacity, + } + if newNm, err = NewLevelDbNeedleMap(v.FileName(".ldb"), indexFile, opts, v.ldbTimeout, v.Version()); err != nil { + indexFile.Close() + return fmt.Errorf("rebuild leveldb large needle map for volume %d: %v", v.Id, err) + } + default: + indexFile.Close() + return fmt.Errorf("unsupported needle map kind %v for volume %d", v.needleMapKind, v.Id) + } + + if err := oldNm.Sync(); err != nil { + glog.Warningf("volume %d: sync sorted needle map before reopen: %v", v.Id, err) + } + oldNm.Close() // closes the O_RDONLY .idx handle held inside + v.nm = newNm + return nil +} + func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind NeedleMapKind, preallocate int64, ver needle.Version) (err error) { alreadyHasSuperBlock := false diff --git a/weed/storage/volume_mark_writable_test.go b/weed/storage/volume_mark_writable_test.go new file mode 100644 index 000000000..3d6959de6 --- /dev/null +++ b/weed/storage/volume_mark_writable_test.go @@ -0,0 +1,87 @@ +package storage + +import ( + "errors" + "os" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/storage/needle" + "github.com/seaweedfs/seaweedfs/weed/storage/super_block" +) + +// A volume booted with .vif ReadOnly=true used to come back stuck: load() +// opens .idx as O_RDONLY and builds a SortedFileNeedleMap whose Put returns +// os.ErrInvalid, and MarkVolumeWritable only flipped the in-memory flag and +// rewrote .vif. Subsequent writes failed at v.nm.Put. +func TestMarkVolumeWritable_ReopensPersistedReadOnly(t *testing.T) { + dir := t.TempDir() + + v, err := NewVolume(dir, dir, "", 1, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + if err != nil { + t.Fatalf("create volume: %v", err) + } + + if _, _, _, err := v.writeNeedle2(newRandomNeedle(1), true, false); err != nil { + t.Fatalf("initial write: %v", err) + } + + // Persist read-only state into .vif, then simulate a server restart by + // closing and re-opening the volume from the same directory. + v.PersistReadOnly(true) + v.Close() + + v2, err := NewVolume(dir, dir, "", 1, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + if err != nil { + t.Fatalf("reload volume: %v", err) + } + defer v2.Close() + + if !v2.noWriteOrDelete { + t.Fatalf("reloaded volume should be in noWriteOrDelete=true after .vif ReadOnly=true") + } + if _, ok := v2.nm.(*SortedFileNeedleMap); !ok { + t.Fatalf("reloaded readonly volume should use SortedFileNeedleMap, got %T", v2.nm) + } + + // Pre-fix behaviour: SortedFileNeedleMap.Put returns os.ErrInvalid even + // once noWriteOrDelete is cleared. Confirm the failure mode the issue + // describes — flipping only the flag is not enough. + v2.noWriteOrDelete = false + _, _, _, writeErr := v2.writeNeedle2(newRandomNeedle(2), true, false) + if !errors.Is(writeErr, os.ErrInvalid) { + t.Fatalf("expected write through SortedFileNeedleMap to fail with os.ErrInvalid, got %v", writeErr) + } + v2.noWriteOrDelete = true // restore so reopenIdxForWrite reflects the real entry condition + + if err := v2.reopenIdxForWrite(); err != nil { + t.Fatalf("reopenIdxForWrite: %v", err) + } + if _, stillSorted := v2.nm.(*SortedFileNeedleMap); stillSorted { + t.Fatalf("reopenIdxForWrite left SortedFileNeedleMap in place") + } + + v2.noWriteOrDelete = false + if _, _, _, err := v2.writeNeedle2(newRandomNeedle(3), true, false); err != nil { + t.Fatalf("write after reopen: %v", err) + } +} + +// reopenIdxForWrite must be a no-op when the volume already has a writable +// needle map — otherwise repeated MarkVolumeWritable calls would churn the +// index file handle for no reason. +func TestReopenIdxForWrite_NoopWhenAlreadyWritable(t *testing.T) { + dir := t.TempDir() + v, err := NewVolume(dir, dir, "", 2, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + if err != nil { + t.Fatalf("create volume: %v", err) + } + defer v.Close() + + before := v.nm + if err := v.reopenIdxForWrite(); err != nil { + t.Fatalf("reopenIdxForWrite on writable volume: %v", err) + } + if v.nm != before { + t.Fatalf("reopenIdxForWrite replaced nm on a writable volume (before=%p after=%p)", before, v.nm) + } +}