mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-08-21 22:56:55 +00:00
* 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.
This commit is contained in:
Chris Lu
@@ -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 {
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user