mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-18 04:20:53 +02:00
9b12d13934
* volume server: release the store lock before scrubbing EC volumes
`ec.scrub` makes a Rust volume server stop serving for the duration of the
scrub, and then kills its own gRPC connection:
error: rpc error: code = Unavailable desc = keepalive ping failed to
receive ACK within timeout
Measured on a 4.46 cluster (17 Rust volume servers on one host, ~520 volumes
and 53 EC volumes, --index=redb, EC 10+4). It reproduces against a SINGLE
node in 30-70s, in checksum, index and local modes, at -maxParallelization 1.
## Cause
The CHECKSUM arm of scrub_ec_volume reads every byte of every local shard
while holding the caller's store.read() guard:
let store = self.state.store.read().unwrap();
let ecv = store.find_ec_volume(vid)...?;
let (blocks, broken, errs) = ecv.checksum_scrub(); // GBs of I/O, lock held
VolumeServerState::store is a std::sync::RwLock, which is write-preferring.
The periodic heartbeat's collect_heartbeat_with_snapshot takes store.write()
and blocks; once that writer is pending, every later store.read() queues
behind it. Every HTTP handler takes store.read(), so the node serves nothing,
stops heart-beating, and cannot answer the scrub RPC's own keepalive - the
scrub kills the connection it is running on.
The INDEX and LOCAL arms have the same shape, and the node-wide scrub_volume
loop is worse: it held ONE guard across every volume on the node.
## Evidence
offcputime, off-CPU stacks >1s in a 30s window during a scrub:
futex_wait
seaweed_volume::server::heartbeat::collect_heartbeat_with_snapshot
- tokio-rt-worker
27967020 <- 27.97s blocked, of a 30s window
A single HTTP /status request issued 12s into a scrub, with 180s of patience,
was accepted and queued for 120 seconds, then served once the scrub released.
Thread states throughout: 1 D + 48 S. One thread working, 48 idle - not
executor starvation and no thread pileup, which is what a single lock holder
looks like.
Memory was tested and ruled out as the cause: the same scrub was run at
MemoryMax 3G, 8G and unlimited. With no limit there is no reclaim at all,
page cache grows freely to 22 GB, and the node still goes unresponsive at
t+30s. anon stays flat at 48-86 MB in every run.
## Fix
checksum_scrub, scrub_index and scrub_local gain plan types -
EcChecksumScrubPlan, EcIndexScrubPlan and EcLocalScrubPlan - snapshotted from
the volume under a brief guard. The handler builds a plan, drops the guard,
and runs the scan in spawn_blocking, off the async workers, since it is
synchronous CPU + file I/O either way.
A plan captures DESCRIPTORS, not paths. Resolving a path again after the
guard is dropped would let a writer that legitimately unlinks the files - the
heartbeat's delete_expired_ec_volumes, which reaches EcVolume::destroy(), or
volume_ec_shards_delete - surface an intentional removal as "scrub read
error: No such file or directory" and put the volume in broken_volume_ids. A
descriptor outlives the name.
For the shards it duplicates the handle the mounted EcVolumeShard already
holds (try_clone_file), which is what Go does: ChecksumScrub reads through
shard.ReadAt (weed/storage/erasure_coding/ec_volume_scrub.go:71), never
through a path. That also inherits open_volume_file's O_NOATIME and drops a
dead branch - the old code built {base}.ec{id}.v{gen} for a non-zero
generation, a name nothing in this tree writes. dup shares the kernel offset,
so shard reads stay positional; the .ecx gets a fresh open instead, since
check_index_file seeks.
FULL/READS is unchanged here: it already released the guard across the index
walk, and still re-takes it per needle in store_ec::scrub_snapshot_under_lock
for that needle's local shard intervals - short holds, many of them.
scrub_volume now takes the read guard PER VOLUME instead of across the whole
loop, so the heartbeat can land between volumes. Its per-volume work still
runs under the guard; Volume needs an equivalent plan to fix that properly,
left as a follow-up and noted in the code.
## A failed scrub task must not take the whole RPC down
Moving the scans into spawn_blocking changed where a panic lands. It no
longer unwinds inside the handler's own future; it comes back as a JoinError
at the .await, and all four join points sat behind a `?`. So one bad volume
out of six hundred returned Err from the entire handler: the
broken_volume_ids, broken_shard_infos and details already gathered for the
other 599 were dropped, and emit_scrub_metrics - the only writer of
SCRUB_LAST_TIME_SECONDS, SCRUB_VOLUME_FAILURES and SCRUB_SHARD_FAILURES - was
never reached, so the staleness alert kept firing while real corruption went
unreported.
And there is a reachable panic behind it. EcLocalScrubPlan::run() sized its
reassembly buffer with
Vec::with_capacity(get_actual_size(size, version) as usize)
which for any negative size that is not the -1 tombstone skipped above is a
capacity-overflow abort. Mode 3 (LOCAL) is the default of `weed shell
ec.scrub`, and a scrub is what you point at an index you already suspect, so
an arbitrary i32 in a .ecx size field is in-scope input. The buffer is
Rust-only - Go appends to a nil slice and has no capacity hint here. Guard on
`want <= 0` and fall through with an empty buffer: locate_data returns no
intervals for a non-positive size, read stays 0, and the existing
`read != want` error reports the row exactly as Go does.
Each join point now records the failure against its own volume and continues.
A panic is evidence about the volume and counts as broken; a non-panic
JoinError is not - spawn_blocking only reports one when the runtime is going
down, the volume was never scanned, and counting it would put a false
corruption into SCRUB_VOLUME_FAILURES. total_volumes moves before the join in
modes 1, 3 and 4 (2|5 already counted there) so a failed join cannot silently
shrink it. Mode 2|5's verify_ec_shards join is the one that must not
`continue`: the needle walk above has already produced findings for that
volume.
The tombstone guard stays is_tombstone() on purpose. ScrubLocal in
ec_volume_scrub.go:228 skips only IsTombstone(), while the distributed walk
in store_ec.go:516 skips all IsDeleted() - the asymmetry is Go's, and both
Rust walks mirror their own counterpart.
## Both servers: a node-wide scrub skips a volume that vanished mid-run
Releasing the lock makes the volume set legitimately mutable during a scrub,
so a node-wide run can reach a volume that has since been unmounted. That is
not a scrub failure. A node-wide run now logs and skips it; an explicitly
requested volume id still returns NotFound. The Go server is changed the same
way, so both implementations answer the same shell command identically.
mark_broken_volumes_readonly tolerates the same teardown one step later,
instead of throwing away the whole scrub report.
## Test
test_scrub_plans_are_self_contained_and_match_direct_call drops the EcVolume
and runs both plans on another thread, asserting the results match the direct
calls. A plan that borrowed from EcVolume could do neither, so the test stops
compiling if the snapshot regresses to a borrow.
test_scrub_plans_survive_files_removed_after_snapshot unlinks every shard and
the .ecx after the plans are built, then asserts the results still equal the
direct call. Against a path-resolving version it fails with all 14 shards
reported as "No such file or directory".
test_local_scrub_plan_reports_negative_size_ecx_row rewrites a .ecx row's
size to -1000 and runs the local plan on another thread, so the join is the
assertion - that thread is the spawn_blocking whose panic used to fail the
RPC. Without the capacity guard it fails with "capacity overflow"; with it,
the row is reported.
The Go tests cover both halves of the vanished-volume rule for volumes and EC
volumes.
517 lib tests pass, plus 34 across the other targets (`cargo test`).
`go test ./weed/server -run Scrub` passes.
## Known remaining, not fixed here
`ec.scrub -volumeId=N` is still fanned out to every node, and a node that
holds no shard of N returns NotFound, so the shell command errors even when
the nodes that do hold shards scrub cleanly. That is a shell-side fan-out
question rather than a volume-server one, and both servers keep the existing
behaviour for an explicitly requested id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DvHoW85w6SNKNBPvrqLMmK
* scrub: discard checksum block count from total_files; capture .ecx fd for FULL walk
Two review fixes:
1. CHECKSUM arm: plan.run() returns blocks scanned, not a file count.
Go discards it (_, shardInfos, serrs = v.ChecksumScrub()) so TotalFiles
stays a needle/file count. The Rust arm was adding it to total_files,
inflating the count. Discard it to match Go.
2. FULL/READS (scrub_ec_volume_distributed): the needle walk reopened the
.ecx by PATH after the store guard was released, so a concurrent teardown
that unlinks or replaces the .ecx (heartbeat delete_expired_ec_volumes,
volume_ec_shards_delete) could surface an intentional removal as a scrub
error or mix index generations within one scrub. Capture a second .ecx
descriptor under the guard (the index plan handle is consumed by its own
structural walk, and both seek) and read through it instead -- the same
descriptor-outlives-name invariant the checksum plan shard handles use.
* scrub: bind FULL/READS walk to one encode generation
Address Devin review: after capturing the .ecx descriptor under the guard,
scrub_snapshot_under_lock still re-resolves the volume by id per needle, so
a teardown-and-remount of the same vid between two rows would apply the
captured .ecx offsets to a replacement volume's shards -- falsely reporting
corruption.
Capture the volume's encode_ts_ns (encode-run identity) in Phase A and pass
it to scrub_snapshot_under_lock. If the mounted volume's encode_ts_ns no
longer matches, abort the walk like a mid-scan unmount instead of mixing
generations within one scrub.
* scrub: run FULL/READS index scan in the blocking pool
Address CodeRabbit review (5147767192): index_plan.run() reads the whole
.ecx synchronously, so running it on the async executor worker could block
unrelated RPC work handled on the same executor. Move it into spawn_blocking,
matching the treatment the CHECKSUM/LOCAL arms already give their plans. A
join failure (panic/cancellation) is reported as a seed error so the
per-volume findings below are not silently dropped.
* scrub: move ecx walk to blocking pool, classify join errors, guard encode_ts_ns==0
Three CodeRabbit review fixes (5148034447):
1. Move the FULL/READS needle walk (walk_index_file over the captured ecx
descriptor) into spawn_blocking. It reads the full .ecx synchronously and
was still running on the async executor worker, the same blocker the
index_plan.run() fix in the previous commit addressed.
2. Preserve JoinError classification in both spawn_blocking join points in
scrub_ec_volume_distributed. A panic is evidence about the volume and
counts as broken; a cancellation only happens at runtime shutdown, the
volume was never scanned, and returning it as an error would put a false
corruption into broken_volume_ids (the FULL/READS arm marks the volume
broken on any non-empty errs). Panics return an error; cancellations
return clean.
3. Do not treat encode_ts_ns == 0 as a verified generation match. The .vif
assigns 0 when it carries no encode-run identity (legacy/pre-feature
volumes), so 0 == 0 would accept a teardown-and-remount and apply the old
.ecx offsets to the replacement volume's shards. Only enforce the
generation check when the captured identity is non-zero; when it is zero,
fall back to the pre-check behavior (no generation binding) rather than
aborting a scrub that was already running without the guard.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>