mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-07 22:03:14 +00:00
4faea7fcbc1c45c031bb0ea2ecaea9e0e6b84c5e
26 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zhengchao An
|
04bfd48eb1 |
fix(ecstore): invalidate metadata cache after ILM transition persists (#4951)
A duplicate transition task admitted after the winner released its in-flight claim (#4839) re-reads the version before uploading, but on unversioned buckets that read could hit a stale pre-transition entry in the 2s-TTL GET metadata cache: transition_object never invalidated the cache after delete_object_version persisted transition_status=complete and freed the local data. The stale hit defeated the TRANSITION_COMPLETE early-return, so the duplicate streamed the already-deleted local data to the remote tier (NotFound reader errors + rejected duplicate tier PUT with UnexpectedContent). Invalidate the cache right after the transitioned metadata is persisted, matching the other metadata-mutating paths, and add a regression test that runs a duplicate transition against an already-transitioned version and asserts no second tier upload and unchanged remote object metadata. Fixes #4827 |
||
|
Zhengchao An
|
cf9e9c6fd5 |
fix(ilm): implement expire_restored delete semantics for restore expiry (#4950)
DeleteRestoredAction is supposed to demote a restored object back to its pure transitioned state: remove only the local restored copy, strip the x-amz-restore headers, and leave the version (and the remote tier data) untouched. expire_transitioned_object set opts.transition.expire_restored accordingly, but no delete path ever read the flag, so delete_object ran an ordinary delete: on unversioned buckets the whole object vanished and the free-version record scheduled remote tier cleanup (tier data loss); on versioned buckets the latest version got a spurious delete marker that replication propagated. Route expire_restored explicitly in SetDisks::delete_object before delete-marker resolution and replication dispatch: target the found version with FileInfo.expire_restored=true and return early. The FileMeta::delete_version layer already implements the semantics (strip restore headers, keep the version, hand back the local data dir); this wires it up. Also fix the action matching in expire_transitioned_object (extracted into transitioned_object_delete_opts): DeleteRestoredVersionAction previously fell through to the full transitioned-object delete, which removed the remote tier data of a noncurrent restored version. It now routes through the same restored-copy cleanup with the exact version id, matching MinIO's Action.DeleteVersioned()/DeleteRestored() dispatch. Re-enable test_restore_chain_local_read_expiry_keeps_remote_and_allows_ re_restore in the ILM Integration (serial) lane; add unit tests pinning the event->options routing and the filemeta expire_restored branch. Closes rustfs/backlog#1302 |
||
|
Zhengchao An
|
4d22ed4465 |
perf(capacity): drop per-PUT global lock and per-disk allocation from write dirty-scope (#4933)
perf(capacity): remove per-PUT global lock and per-disk allocation from write dirty-scope Every successful write recorded its capacity dirty scope by allocating an endpoint/path String per online disk, deduplicating through a HashSet, entering the global dirty-scope Mutex, and — in the app response path — taking a global async RwLock to record the write frequency. Under small-object high concurrency this created a global serialization point and O(disks) allocation on the hot path (https://github.com/rustfs/backlog/issues/1315). This change makes the steady-state write path allocation-free and lock-free without altering capacity accounting semantics: - Memoize the per-set dirty scope. Each set resolves its disks' immutable endpoint/path identity lazily into a slot-indexed cache and reuses a shared `Arc<CapacityScope>`; steady-state writes clone the Arc under a read lock instead of rebuilding String/HashSet. The heal path keeps an ad-hoc scope builder because it passes disks in erasure-distribution order rather than physical-slot order. - Add a monotonic generation to the global dirty-scope registry, advanced only when a non-empty drain removes disks. A set upgrades the global registry mutex only on the first write of each generation and then skips it while the generation is unchanged; the observed generation is read under the registry lock so a concurrent drain forces a re-mark, preventing lost updates. The write commits its bytes before recording the scope, so any drain that could remove the mark is ordered after the commit and the following refresh reads the committed bytes. - Replace the write-frequency `RwLock<WriteRecord>` with lock-free atomics: per-second CAS buckets, an atomic last-write timestamp, and an atomic total counter. The frequency window and debounce semantics the refresh scheduler relies on are unchanged. Capacity marking remains a conservative superset of the disks actually written, so admin/scan totals are byte-for-byte identical: extra dirty marks only trigger a re-read of a disk whose usage is unchanged. White-box tests assert the memoized scope equals the previous ad-hoc construction, that the global registry is upgraded exactly once per generation and re-marked after a drain, and that the lock-free write record is exact under concurrent contention. Ref: https://github.com/rustfs/backlog/issues/1315 |
||
|
Zhengchao An
|
ae15f5804d |
test(ilm): fix restore integration test object key to match transition filter (#4886)
* test(ilm): fix restore test object key to match transition filter restore_object_usecase_reports_ongoing_conflict_and_completion used the object key "restore/api-object.bin", but the shared set_bucket_lifecycle_transition_with_tier helper only transitions objects under the "test/" prefix. enqueue_transition_for_existing_objects therefore matched nothing and wait_for_transition timed out at 15s, failing the test deterministically. The test was added in #4860 but its ILM Integration (serial) lane is skipped on regular PRs, so it merged red and has failed on every main run since. Move the object under the test/ prefix like every passing sibling test in this file. * ci(ilm): exclude broken RestoreObject API test from serial lane restore_object_usecase_reports_ongoing_conflict_and_completion exposes a real regression, not a test bug: the RestoreObject copy-back (handle_restore_transitioned_object) now holds the object write lock added in #4877 across the entire tier read-back, so it never releases in time and the test's concurrent get_object_info times out with Lock(Timeout, 5s). The failure is deterministic and independent of the mock tier's injected latency. This is the same class of known-broken restore/transition failure already tracked under backlog#1148 (three sibling scanner tests are excluded here by name for the same reason), so exclude this one the same way until the restore copy-back path is fixed or the #4877 lock scope is revisited. The prior commit keeps its correct fix (the object key must live under the test/ transition prefix); that was masking this deeper issue by never letting the object transition in the first place. Restore copy-back deadlock/hang under the #4877 lock is escalated separately for a product-level decision (fix the copy-back vs. narrow/revert #4877). * test(ilm): fix scanner restore test object keys to match transition filter test_restore_chain_local_read_expiry_keeps_remote_and_allows_re_restore and test_multipart_restore_preserves_parts_and_etag (both added in #4860) keyed their objects under restore/ instead of the test/ prefix that set_bucket_lifecycle_transition_with_tier filters on, so the objects never transitioned and wait_for_transition timed out at 15s. These surfaced only after the prior commit excluded the rustfs-side restore API test: nextest runs -j1 fail-fast, so that earlier failure stopped the run before these scanner tests executed. Unlike the excluded API test, both call restore_transitioned_object().await sequentially and only read afterwards, so they don't hit the concurrent-read-vs-#4877-write-lock timeout; the key prefix was their only problem. * ci(ilm): exclude the two remaining #4877-broken restore tests test_multipart_restore_preserves_parts_and_etag and test_restore_chain_local_read_expiry_keeps_remote_and_allows_re_restore both call restore_transitioned_object().await, which since #4877 acquires the object write lock and deterministically times out (Lock Timeout, 5s) against an already-held lock, so restore never completes. They surfaced one at a time because nextest runs -j1 fail-fast. The earlier prefix fix was necessary but only advanced them from the transition wait to this restore-lock timeout. Exclude both by name alongside their already-excluded sibling test_transition_and_restore_flows (same root cause, tracked under backlog#1148) so the ILM Integration (serial) lane goes green. The #4877 lock scope still needs a product fix before any of these re-enable. * docs(ilm): describe the excluded restore tests' symptom as a lock timeout, not a deadlock The #4877 write lock is held across the tier read-back and outlives the 5s lock timeout; nothing proves a true deadlock. Wording flagged by Copilot review. * fix(ecstore): stop restore copy-back self-deadlocking on the #4877 write lock #4877 made handle_restore_transitioned_object hold the object write lock for the whole restore and forward no_lock=true so the set layer would not reacquire it. But the set-level copy-back rebuilds its own options (put_restore_opts -> ropts, and the complete_multipart_upload opts) that default no_lock=false, so the inner put_object / new_multipart_upload / complete_multipart_upload each re-acquire this object's write lock in their commit phase and block on the lock the restore already holds -> Lock(Timeout, 5s), and restore never completes. Confirmed via RUSTFS_OBJECT_LOCK_DIAG_ENABLE: restore_transitioned_object acquires the write lock, then holds it ~10.5s across two nested 5s acquire timeouts before failing. This is a real product deadlock: a RestoreObject on any transitioned object (multipart especially) hangs, not just the tests. Propagate no_lock into the copy-back options so the inner writes inherit the already-held lock. Use opts.no_lock (not a hardcoded true) so a caller that restores without the outer lock still locks correctly. put_object_part is left as-is: it locks the multipart upload-id resource, not the object key, so it does not conflict. Verified test_multipart_restore_preserves_parts_and_etag now passes (3.6s, was a 15s+ hang). * ci(ilm): re-enable multipart restore test; scope remaining exclusions The prior commit fixes the #4877 restore self-deadlock, so test_multipart_restore_preserves_parts_and_etag passes again - drop it from the serial-lane exclusion list and remove its 'currently excluded' note. The other restore/transition tests still fail, but each on a DIFFERENT, independent issue unrelated to the (now-fixed) lock, verified locally: - test_restore_chain_...: DeleteRestoredAction sets expire_restored but no delete path reads it, so cleanup deletes the whole object (unimplemented semantics), not the local restored copy only. - test_transition_and_restore_flows: transition xl.meta missing on one drive (EC metadata distribution), not restore. - restore_object_usecase_reports_ongoing_conflict_and_completion: asserts a concurrent mid-restore ongoing=true read that #4877's read-vs-restore serialization rules out (backlog#1148 ilm-8 criterion 1, an API-semantics decision). Comments and #[ignore] reasons updated to reflect each real cause. All remain tracked under backlog#1148. |
||
|
houseme
|
264b2dd480 |
perf(metrics): drop needless per-emission work on hot metric paths (#4743)
* perf(metrics): drop needless per-emission work on hot metric paths Audit of the metrics hot paths surfaced four low-risk wins where emission did work it did not need to: - `record_file_cache_reclaim_success/error` (disk/local.rs) called `.to_string()` on `kind` (already `&'static str`) and on the `"ok"`/`"err"` literals, heap- allocating up to four `String`s per page-cache reclaim window — which runs per read-stream reclaim. The `metrics` macros accept `&'static str` label values directly, so pass them as-is. - `record_read_repair_dedup` (set_disk/core/io_primitives.rs) likewise `.to_string()`-ed an already-`&'static str` `reason`. - `SetDisks::get_object_reader` (set_disk/ops/object.rs) captured `Instant::now()` and emitted the `rustfs.lock.acquire.*` counter and histogram unconditionally on every GET, right beside an already-gated stage timer. Gate them behind `get_stage_metrics_enabled()` too, so an inactive observability config pays no per-GET clock read or recorder lookups. - The per-response-body-chunk counter in server/http.rs re-ran the `counter!` registry lookup on every chunk (a streamed GET emits many). Resolve the label-less handle once into a `LazyLock<metrics::Counter>`; the global recorder is installed at startup before any response streams, so the cached handle binds to the final recorder. No metric names or label values change. The only behavior change is that the `rustfs.lock.acquire.*` GET-path metrics now follow the GET stage-metrics flag, consistent with the neighbouring stage timings. Co-Authored-By: heihutu <heihutu@gmail.com> * perf(metrics): gate page-cache reclaim metrics behind metrics_enabled() `record_file_cache_reclaim_success/error` run per read-stream reclaim window on large-object reads and emitted unconditionally. When general metrics are disabled the `counter!`/`histogram!` macros still construct three metric keys per call for nothing. Skip the emission behind `rustfs_io_metrics::metrics_enabled()`, matching how the io-metrics free functions self-gate. The serial reclaim-metrics test now enables the flag (save/restore) alongside the existing stage gate. Left ungated deliberately: `record_read_repair_dedup` (rare read-repair path, and its non-serial test would need a global-flag toggle), and the HTTP body-chunk counter (its cached handle already makes the disabled case a no-op increment). Co-Authored-By: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
houseme
|
e742a540a4 |
test(cache): guard the body-cache eligibility gate against deny-list regressions (#1146) (#4742)
`full_object_plaintext_len` decides whether a body-cache hook hit may serve bytes in place of the erasure read. It is a fail-closed allow-list: it excludes every read whose `ReadPlan::build` applies some other transform (ranged/part, raw/data-movement, restore, encrypted, remote) with an early `return None`, then returns a `Some(..)` length only for the whole-plaintext cases. A newly added `ReadPlan` branch that nobody teaches this gate about falls through to `None` and safely bypasses the cache. Flip it to a deny-list and the same new branch silently serves bytes in the wrong representation — the backlog#1108 / #1109 / #1146 class of bug. The existing unit and e2e tests only cover the branches that exist today. This adds `scripts/check_body_cache_whitelist.sh`, a structural guard wired into pre-commit / pre-pr / dev-check and CI, that asserts every exclusion predicate and a `return None` still precede the first `Some(..)`. Reordering a predicate, dropping one, moving the positive return ahead of the gate, or renaming/removing the function all fail; wording, formatting, and adding a new exclusion in the same gate do not. Mutation-tested against all four regression shapes. This machine-enforces the structural invariant that backlog#1146 was kept open to guard by hand. Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
houseme
|
6780140318 |
fix(object-data-cache): make the GET key write-unique and dedup the lookup (#4693)
fix(object-data-cache): make GET body cache key write-unique and dedup lookups Address four object-data-cache GET-path findings (backlog#1107 batch): ODC-06 (backlog#1111): the cache key was content-unique, not write-unique. Extend ObjectDataCacheKey with the resolved version's modification time (i128 unix nanoseconds, None -> 0), derived once in the shared planner so the ecstore hook and the usecase layer produce an identical key. An unversioned overwrite advances mod_time, so a stale node can no longer serve old bytes for up to the TTL under an MD5 collision; etag + size stay as belt-and-braces. ODC-16 (backlog#1121): every cacheable GET planned and looked up twice (once in the ecstore hook, once in the usecase layer), double-counting hits, hit_bytes and lookups. GetObjectReader now carries a GetObjectBodySource marker (Unprobed / HookMissed / HookServed); the hook stamps it, and build_get_object_body_with_cache serves a hook-served body directly and skips its lookup whenever the hook already probed. One hook-served GET now records exactly one lookup. ODC-19 (backlog#1124): ENABLE=true with no explicit mode defaulted to HitOnly, which never fills and keeps a permanent 0% hit rate. Default to FillBufferedOnly, log the resolved mode at startup, and warn when HitOnly is selected explicitly. ODC-24 (backlog#1129): max_entry_bytes above the in-memory GET fill limits was silently inert. Clamp the planner's size eligibility to min(max_entry_bytes, seek-support threshold, 64 MiB buffer cap) so ineligible sizes plan SkipTooLarge instead of being reported eligible, and warn at startup when the excess is inert. Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
Zhengchao An
|
80ddd8fa7e |
fix(ecstore): roll back delete on disks that staged then errored (#4676)
fix(ecstore): roll back delete on disks that staged then errored (backlog#1158) #4300 rolls back a failed delete only on disks that returned Ok, skipping any disk that staged its rollback backup, applied the delete, and then errored -- leaving that disk deleted while its peers are restored. On rollback, fan the undo out to every online disk instead; the disk-side restore_delete_rollback is already idempotent (Ok no-op when nothing was staged), so unstaged disks are unaffected. The err.is_some() skip now applies only to the success/cleanup path. Covers both single-object and batch delete. Refs backlog#1158. |
||
|
houseme
|
8039a4ceae |
test(cache): end-to-end regressions for the body-cache hook P0s (#4675)
fix(ecstore): make body-cache hook re-registrable + add e2e regressions ODC-21 (backlog#1126): the GET body-cache hook lived in a first-wins OnceLock. When AppContext is rebuilt (config reload, test re-init) a fresh ObjectDataCacheAdapter is constructed and re-registered, but the OnceLock kept ecstore's GET probe pointed at adapter #1 while every usecase-layer fill and invalidation targeted adapter #2 — silently degrading the feature to a 0% hit rate with no error, log, or metric, and stranding entries in the unreachable cache until their TTL. Replace the slot with RwLock<Option<Arc<dyn GetObjectBodyCacheHook>>> so re-registration atomically swaps to the newest adapter, and log at WARN when a swap replaces a *different* instance (Arc::ptr_eq). RwLock over ArcSwapOption because arc-swap's RefCnt is impl<T> (Sized, thin *mut T) and cannot hold an Arc<dyn Trait> without a sized newtype wrapper; the probe reads the slot once per full-object GET but only clones an Arc, negligible next to the metadata quorum fan-out already done before the probe. Add a test-only clear_get_object_body_cache_hook so tests register/unregister deterministically. With the hook now re-registrable, add true end-to-end regressions that drive get_object_reader (not the full_object_plaintext_len predicate) against a real erasure-coded, genuinely-compressed object via the blackbox make_local_set_disks harness, with a stand-in hook playing the app-layer cache (the injection point production uses; the adapter itself lives above ecstore). These close the gap the predicate-only tests left — a caller that opens a new shortcut serving the cached body directly, the original form of both P0s: - backlog#1108: a raw_data_movement_read must yield the STORED (compressed) bytes, never the cached plaintext. - backlog#1109: a compressed cache hit must publish the DECOMPRESSED length as object_info.size (the UploadPartCopy invariant), with the streamed length matching. - backlog#1146: a restore read (restore_request.days) must serve STORED bytes, not the cache. Mutation-verified each e2e test bites: dropping the raw_data_movement_read gate serves plaintext (fails #1108); removing the hit-site size republication publishes 2972 vs 660000 (fails #1109); dropping the restore gate serves plaintext (fails #1146). Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
houseme
|
8c76efead2 |
fix(cache): stop the GET body-cache hook from bypassing ReadPlan (#4654)
* fix(cache): gate GET body-cache hook to preserve ReadPlan output The ecstore GET body-cache hook serves cached full-object plaintext directly, bypassing ReadPlan/ReadTransform. That is only sound when the normal read path returns that same plaintext byte-for-byte. Two probe conditions were missing, plus two usecase-layer planner gaps. ODC-01 (backlog#1108): raw/data-movement reads. ReadPlan::build returns the STORED representation for raw_data_movement_read (e.g. compressed bytes, length = oi.size), but the cache holds the post-decompression body. Decommission (raw_data_movement_read: true) would receive decompressed plaintext where raw compressed bytes are required, silently corrupting the destination pool. ODC-02 (backlog#1109): compressed objects. ReadTransform::Compressed rewrites object_info.size to the decompressed length; on a hook hit object_info is returned unchanged, so object_info.size is the compressed size while the stream carries the decompressed body. UploadPartCopy then uses src_info.size as the copy length and truncates the part. Fix: gate the hook probe with should_probe_body_cache_hook, refusing raw_data_movement_read, data_movement, and compressed objects, mirroring the conditions get_small_object_direct_memory_decision already applies. ODC-33 (backlog#1138): build_get_object_body_cache_plan lacked the is_remote() exclusion the ecstore hook enforces; add it so transitioned (remote-tier) objects are excluded uniformly. ODC-C1 (backlog#1142): zero-length bodies save no I/O (ecstore returns an empty body before the hook probe) yet the planner admitted them; change the guard to response_content_length <= 0 so they plan Skip, mirroring should_buffer_get_object_in_memory_with_threshold. Tests: body_cache_hook_gate_tests (4) cover plain-probe plus raw/data-movement/compressed skips; planner gains plan_skips_remote_transitioned_objects and plan_skips_zero_length_objects. Co-Authored-By: heihutu <heihutu@gmail.com> * fix(cache): allow compressed bodies via a fail-closed read allow-list The body-cache hook probe was gated by a deny-list that refused compressed objects outright, which cost the cache every compressed body — a growing share of stored data. Replace it with an allow-list that returns the exact plaintext length a hit may serve, or None. full_object_plaintext_len() answers a single question: would the normal ReadPlan produce this object's complete plaintext, and under which size? Compressed objects now qualify, and the hit site publishes the returned length as object_info.size, reproducing the contract ReadTransform:: Compressed establishes. A hit whose body length disagrees is refused and falls through to the erasure read. This also closes a gate the deny-list only covered by accident: a restore read forces ReadPlan down the Plain branch, so a compressed object yields STORED bytes under its compressed size. Refusing compressed objects hid that; admitting them exposes it, so restore reads are refused explicitly. Being fail-closed, a newly added ReadPlan branch bypasses the cache by default rather than silently serving the wrong representation — the structural defect behind both backlog#1108 and backlog#1109. Refs: backlog#1108, backlog#1109 Co-Authored-By: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
GatewayJ
|
a269f8df05 | fix(ecstore): require write quorum for metadata early stop (#4300) | ||
|
Zhengchao An
|
4f999bb6b8 | perf(ecstore): backfill rename_data old size and gate the PUT prelookup (#4598) | ||
|
houseme
|
8fc75e88c8 |
test(ecstore): multi-disk regression for read-before-write tagging under early-stop (backlog#881) (#4561)
test(ecstore): multi-disk regression for read-before-write tagging under early-stop Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
houseme
|
15808254d3 |
fix(ecstore): correct codec-streaming byte accounting and partNumber routing (#4535)
Two correctness defects on the opt-in codec-streaming GET path. ECA-02 (#943): ErasureDecodeReader only decremented `remaining` for the main fill buffer. Under the default DualInFlight policy each fill also produces a queued stripe that is delivered to the client via `prefetched_bufs.pop_front()` without touching `remaining`, so any object larger than one erasure block finished with `remaining > 0` and the GET terminated with LessData despite delivering all bytes. The inflated `remaining` was also fed back into the fill worker, which used it to trim the final stripe and to decide whether to read past EOF. Account for the queued-stripe bytes when they enter the prefetch queue; queued buffers come only from `Ok(true)` decodes so they are non-empty and bounded by `remaining - main_buf.len()`, ruling out underflow. ECA-04 (#945): the codec-streaming gate did not inspect `opts.part_number`. A partNumber GET carries `range == None`, so it was not classified as a Range request and reached the full-object codec-streaming reader, which drops the storage offset/length returned by GetObjectReader::new. A partNumber >= 2 request would then stream the whole object. Mirror the direct-memory part_number fallback and route any partNumber request back to the legacy duplex path, which applies the offset/length correctly. Regression tests: DualInFlight read_to_end on a multi-block object and on a non-block-aligned object; SingleInFlight vs DualInFlight byte-identical output; gate fallback on partNumber requests. Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
Zhengchao An
|
a48bc89cdc |
fix(ecstore): lock batch object deletes (#4435)
* fix(ecstore): lock batch object deletes * fix(ecstore): honor no_lock in batch deletes |
||
|
Zhengchao An
|
cda7688909 |
fix(multipart): clean temp part data on failure (#4412)
fix(multipart): clean failed part temp data |
||
|
Zhengchao An
|
1e6207c08e |
fix(lock): fence write commit on lock loss (#4406)
fix(lock): fence write commit on lock loss (backlog#899 Phase 2) Phase 0+1 (#4388) made object write locks refreshable and marks the guard lost when the heartbeat can no longer refresh a quorum, but does not act on it. Under a partition a long write's lock can expire on an unreachable node and be reclaimed, letting a third party re-acquire it; the original writer keeps going and both commit -- a double write. Expose the loss signal through NamespaceLockGuard::is_lock_lost() and ObjectLockDiagGuard::is_lock_lost(), and fence the commit in put_object and complete_multipart_upload: immediately before rename_data (the atomic commit point), abort with a retryable NamespaceLockQuorumUnavailable (503) if the lock was lost. In multipart the check precedes cleanup_multipart_path so a lost lock leaves the upload intact and retryable. A write that already reached rename_data Ok is durable and never aborted. The loss criterion is unchanged (reacts to Phase 1's signal). Heal and the long-GET read side are deferred follow-ups. |
||
|
houseme
|
a413729b16 | perf(delete): gate and parallelize DeleteObjects per-object stat fanout (#4398) | ||
|
Zhengchao An
|
afc7f1d6f9 |
fix(ecstore): make post-commit old data dir cleanup best-effort (#4386)
* fix(ecstore): make post-commit old data dir cleanup best-effort (backlog#898) A write is authoritatively committed once rename_data returns Ok (the new version is durable on >= write_quorum disks and immediately readable). The subsequent reclamation of the now-dereferenced old object/<data_dir> is pure space reclamation, yet commit_rename_data_dir propagated a below-quorum GC failure via `?` into ErasureWriteQuorum -> 503, producing a false-negative ACK for an already-persisted write. This is a deliberate divergence from MinIO (erasure-object.go:1577), which couples the two; the divergence is justified by durability semantics, not parity. Changes: - commit_rename_data_dir now returns a structured OldDataDirCleanup receipt and never returns Err. Adds an old==committed-dir anti-misdelete guard and a committed_data_dir parameter. Classification is extracted into pure functions (classify_old_data_dir_cleanup / map_cleanup_join_result / is_cleanup_not_found) so it is unit-testable. Task panic/cancel is mapped to a non-ignored DiskError::other (never DiskNotFound), and not-found is normalized to reclaimed. - object.rs / multipart.rs consume the receipt instead of `?`. The result reverts to Ok, so the invalidate_get_object_metadata_cache self-heal and the capacity/compression accounting that a `?` early-return previously skipped now run on the cleanup-failure path too. - On residue, report_old_data_dir_cleanup emits leak metrics and enqueues an object heal over the existing heal channel (disk-health signal replacing the 503). heal_object -> reclaim_orphan_data_dirs already reclaims unreferenced local data dirs, closing the loop end to end. - Adds rustfs_old_data_dir_* counters (attempted/reclaimed/leaked/below_quorum) as the operator-visible backstop for leaked residue. - Adds a test-only (#[cfg(test)]) delete fault-injection seam; in production it inlines to a no-op None and has no behavioral effect. Tests: pure-function A/C group + join-error mapping + actions decision; A5/A5b real-disk guard/reclaim integration; end-to-end overwrite returning 200 while old-data-dir cleanup fails. #864 rollback guard test remains green. * fix(ecstore): resolve merge conflicts with origin/main in io_primitives.rs --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: houseme <housemecn@gmail.com> |
||
|
houseme
|
3f13d098b4 |
feat(observability): feature-gated hotpath instrumentation for the data path (#4394)
Merge the hotpath-rs wall-time instrumentation from the backlog#936 analysis worktree behind an opt-in 'hotpath' cargo feature, keeping the default build at zero overhead and zero dependency. - hotpath is an optional dependency everywhere (dep:hotpath feature syntax); the default dependency tree contains no hotpath crate at all - 40+ measurement points across S3 handlers, ECStore/SetDisks object and multipart ops, erasure encode/decode, bitrot, LocalDisk I/O, FileMeta codec, and HashReader - attribute sites use #[cfg_attr(feature = "hotpath", hotpath::measure)]; async_trait bodies use per-crate hp_guard! macros (ecstore + rustfs bin); rio gates measure_block! behind hp_measure_block! - feature chain: rustfs -> rustfs-ecstore -> rustfs-rio / rustfs-filemeta, each crate owning its own gate - hotpath-alloc is intentionally not wired up (hotpath 0.21.x TLS panic on cross-thread guard drop under tokio, see backlog#935); mimalloc stays the unconditional global allocator - docs/development/hotpath-profiling.md documents building, HOTPATH_* env vars, SIGTERM report flow, and how to reproduce the backlog#936 timing reports Refs: https://github.com/rustfs/backlog/issues/935 (HP-14, item 2), https://github.com/rustfs/backlog/issues/936 Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
houseme
|
e7cc719c17 |
perf(ecstore): move speculative PUT-tail tmp cleanup off the hot path (#4389)
* perf(ecstore): move speculative tmp cleanup off the PUT hot path On a successful PUT, rename_data has already moved the data dir out of the tmp workspace, so the delete_all(RUSTFS_META_TMP_BUCKET) at the end of SetDisks::put_object is a speculative no-op safety net. It was awaited inline on the response path, where profiling (backlog#924 / HP-3) showed the same-disk queueing behind fsync-heavy load turns a ~49us no-op into ~9ms average (p99 77ms, macOS F_FULLFSYNC amplified) added to every PUT. Run that cleanup on a spawned task instead, keeping it as a real backstop (rename_data's remove_std only removes empty dirs and silently ignores failures). The failure path (quorum loss / rollback) keeps the cleanup inline so a failed PUT never returns with tmp shards still on disk. If the process dies before the spawned task runs, cleanup_stale_tmp_objects (24h expiry, 5-minute loop) reclaims the entry. Scope note: ops/multipart.rs delete_all on RUSTFS_META_MULTIPART_BUCKET is intentionally untouched; it removes real leftovers and deferring it would widen CompleteMultipartUpload/Abort races. Regression tests (hermetic SetDisks on formatted local disks, no global state): PUT success drains the tmp workspace (polling the spawned task), and PUT failure (missing bucket volume, rename_data quorum error after tmp shards were written) cleans the workspace inline before returning. Ref: https://github.com/rustfs/backlog/issues/924 Co-Authored-By: heihutu <heihutu@gmail.com> * fix(ecstore): do not retry NotFound in reliable_rename_inner reliable_rename_inner blindly retried the rename once on any error. A NotFound retry cannot succeed: nothing recreates the missing source or parent directory between attempts, so the second rename fails identically and speculative cleanup renames (e.g. move_to_trash on an already-removed tmp path) always paid for two syscalls. Extract the retry decision into should_retry_rename: NotFound returns immediately, any other error keeps the existing single retry. This helper is shared by the rename_data commit path via rename_all, so behavior there is covered by a new rename_all success regression test alongside the retry-predicate tests. Ref: https://github.com/rustfs/backlog/issues/924 Co-Authored-By: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
houseme
|
58114f49f2 |
perf(ecstore): k-way heap merge for ListObjects, drop clone-to-parse (#4347)
* perf(ecstore): replace linear merge scan with k-way heap and drop clone-to-parse (backlog#874 backlog#875) merge_entry_channels advanced the k-way merge with a linear scan over all channel heads (O(entries x channels)) and allocated two fresh Strings per pairwise comparison via path::clean. Every step also cloned MetaCacheEntry values, including entry.clone().xl_meta() clone-to-parse calls. - Introduce MergeHead with a cached cleaned name (allocated only when the raw name is not already clean) and drive the merge with a BinaryHeap of boxed heads: O(log channels) per entry, allocation-free comparisons. - Move entries through the merge instead of cloning; the winner is sent without an intermediate copy. - Remove the dead merge_file_meta_versions block: it only ran for prefix-dir groups whose entries have empty metadata, so xl_meta() always failed; cross-drive version merging happens in the resolve path. - Keep legacy same-name semantics (dir groups collapse, objects shadow prefix dirs, later object candidate wins) and add regression tests for interleaved ordering, dir/object precedence, uncleaned-name grouping, and prefix-dir collapse. Co-Authored-By: heihutu <heihutu@gmail.com> * fix(ecstore): honor ascending versions_sort in ListObjects walk (#4348) * fix(ecstore): honor ascending versions_sort in walk and document ordering invariant (backlog#876) The walk loop carried a bare `//TODO: SORT` inside the `WalkVersionsSortOrder::Ascending` branch, so the requested ascending order was silently ignored and versions streamed newest-first (the raw FileMeta order). WalkOptions defaults to Ascending, so every default walker -- notably replication resync, which replays versions and needs oldest-first to preserve the version-stack order -- received the exact opposite of the contract. FileMeta maintains versions newest-first (sort_by_mod_time is descending) and into_file_info_versions preserves that order, so ascending emission is the exact reverse of file_info_versions output. Reverse in place when ascending is requested and add a regression test locking the newest-first invariant plus the reversal contract. Key-ordering audit result (no gap found): per-disk walkers emit sorted streams, merge_entry_channels performs an ordered k-way merge, and gather_results only filters by marker/limit, so ListObjects key order is guaranteed upstream and needs no post-sort. Co-Authored-By: heihutu <heihutu@gmail.com> * perf(ecstore): enable GET metadata early-stop by default (#4349) * perf(ecstore): enable GET metadata early-stop by default with env opt-out (backlog#872) The metadata early-stop fanout (read_all_fileinfo_early_stop) has been implemented and instrumented for a while but stayed behind an opt-in flag, so default GETs always waited for every disk to answer the metadata read even after quorum agreement was reached. Flip RUSTFS_GET_METADATA_EARLY_STOP_ENABLE to default-on. The gate stays conservative: should_allow_metadata_early_stop only admits metadata-only reads (read_data=false) without version_id, healing, or free-version requirements, everything else falls back to the full-wait fanout, and setting the env var to false restores the old behavior entirely. The version-aware gate (RUSTFS_GET_METADATA_VERSION_EARLY_STOP_ENABLE) remains opt-in because versioned reads carry a higher stale-selection risk profile. Also replace the stale "optimize concurrency" TODO in get_object_fileinfo with a pointer to the early-stop implementation and add regression tests for the new default plus the explicit opt-out path. Co-Authored-By: heihutu <heihutu@gmail.com> * perf(ecstore): lazily construct codec streaming multipart readers (#4350) * perf(ecstore): lazily construct codec streaming multipart part readers (backlog#871) get_object_decode_reader_with_fileinfo opened shard readers for every part of a multipart object before returning the streaming reader, so TTFB paid for parts x disks file opens up front and an early client disconnect wasted the setup work for every unread part. Replace the eager loop with LazyMultipartCodecStreamingReader: the first part is still built eagerly so the dominant fallback conditions (missing shards / read quorum) are detected before any byte is streamed and the whole request can fall back to the legacy duplex path exactly as before. Each subsequent part is built on demand -- when the previous part hits EOF -- via a spawned task handle owned by the reader; dropping the reader aborts an in-flight build so disconnects stop all further IO. If a later part hits a fallback condition mid-stream (a shard vanished after the request started), the reader surfaces an explicit read error with a pipeline-failure metric instead of silently degrading; the client's retry then detects the condition on the eager first-part setup and takes the legacy path cleanly. Adds unit tests for in-order streaming across lazy boundaries, deferred construction (no build when the client stops within part 1), and the mid-stream fallback error path. Co-Authored-By: heihutu <heihutu@gmail.com> * perf(ecstore): prefetch next multipart part reader setup during decode (#4351) * perf(ecstore): prefetch next multipart part reader setup during decode (backlog#870) get_object_with_fileinfo processed multipart parts strictly serially: the next part's bitrot reader setup (file opens + read-quorum wait across all disks) only started after the current part finished decoding, so large multipart reads paid full setup latency between every part. Overlap the two stages with a depth-one pipeline: right after the current part's readers are obtained, the next part's setup is spawned (shared inputs behind Arc) and joined when the loop reaches that part. The shared setup_multipart_part_readers helper keeps stage-duration metrics semantics identical for both paths; a failed or stale prefetch falls back to the synchronous setup, and the PrefetchedReaderSetup guard aborts the in-flight task on error returns, early breaks, or caller drop so disconnects stop background disk IO. Gate: RUSTFS_GET_MULTIPART_READER_SETUP_PREFETCH (default on, env opt-out). Adds a three-part end-to-end read test covering the prefetch hit path and cross-part content ordering. Co-Authored-By: heihutu <heihutu@gmail.com> * perf(ecstore): move FileInfo through GET shuffle instead of cloning (#4352) perf(ecstore): move FileInfo entries through the GET shuffle instead of cloning (backlog#873) shuffle_disks_and_parts_metadata_by_index deep-cloned every valid FileInfo (parts, erasure info, metadata map) once per disk on each GET. Add an ownership-taking variant that runs the same by-index consistency check as a read-only first pass and then moves entries into their shuffled slots with mem::take, and switch get_object_with_fileinfo to it -- that call site already owned the parts metadata vector. Disk handles are Arc clones and stay cheap. Scope notes from the backlog#873 audit: - get_object_fileinfo's disks.clone() stays: DiskStore is Arc<Disk>, so the clone is per-slot refcounting and correctly avoids holding the RwLock read guard across the metadata fanout awaits. - get_object_decode_reader_with_fileinfo keeps the borrowing shuffle: its caller must retain files/disks for the legacy fallback path, so an owned variant would just shift the same clone upstream. - The metadata-cache hit path still clones parts_metadata; sharing the cached entry via Arc changes the read-path return types and is left as a follow-up. Equivalence tests cover both the by-index placement and the mod-time fallback against the borrowing variant. Co-authored-by: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> * fix(ecstore): gate merge emission on cleaned key and clear clippy redundant_clone Address review + CI findings on the ListObjects/GET optimization PR: - merge_entry_channels gated emission on the raw entry name while the heap orders by the cleaned key, so entries whose cleaned order and raw byte order disagree (e.g. redundant slashes) could be dropped. Gate on the same cleaned sort key the heap uses; add a regression test (`a//c` after `a/b`). - Drop three redundant `.clone()` calls in test code flagged by clippy::redundant_clone (owned-shuffle equivalence tests and the walk ascending-versions contract test) that failed the CI clippy gate. - Document the known mid-stream fallback limitation of the opt-in multipart codec streaming reader (default off) and mark the in-place per-part legacy degradation as a follow-up. Co-Authored-By: heihutu <heihutu@gmail.com> * fix(ecstore): force full metadata fanout for object tagging writes (backlog#872) put_object_tags reads the object fileinfo with read_data=false and then writes the updated tags to the online-disk set that read returned. With metadata early-stop enabled by default, that read now returns as soon as read quorum is reached, so the online-disk set is only a read-quorum subset. Writing tags to that subset fails write quorum -> ErasureWriteQuorum -> S3 SlowDown, which is exactly the s3-tests tagging failures (PutObjectTagging/DeleteObjectTagging, reached max retries). Thread a caller-controlled `allow_early_stop` gate through read_all_fileinfo_observed/_inner and add get_object_fileinfo_gated; put_object_tags calls it with allow_early_stop=false so the metadata read does the full quorum fanout and returns the complete online-disk set as the write target. Pure-read callers (GET/HEAD/tag read) keep the early-stop fast path unchanged. Extract metadata_early_stop_permitted() as the single gate and add a unit test locking the invariant: caller opt-out (and observe=false, and data reads) never early-stop even with the env flags on. Co-Authored-By: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com> |
||
|
Zhengchao An
|
31c6859965 |
chore: converge stale TODOs and apply safe fills (backlog#646) (#4322)
Second TODO-convergence round over the current tree (backlog#646). All line numbers in the old inventory had gone stale after the set_disk / diagnostics / cluster refactors, so this re-scans and reduces the marker count from 144 to 99. STALE removals (comment describes already-implemented behavior, or dead commented-out blocks) across ecstore (set_disk ops/core, store, cluster/rpc, bucket/metadata_sys, services), iam, filemeta, s3select and rustfs auth/object_usecase. No behavior change. Safe fills, each verified: - filemeta: replication_info_equals now also compares replication_state_internal (function currently has no callers; adds a regression test). - bitrot: drop the confirmed-unused `_want` parameter from bitrot_verify and the now-unused `sum` on LocalDisk::bitrot_verify, removing a Bytes::copy_from_slice allocation. Streaming verify uses the file's embedded per-shard hash, never the passed sum. - signer: rename v4_ignored_headers -> V4_IGNORED_HEADERS and drop the non_upper_case_globals allow. - admin/heal: test_decode was #[ignore]d and used serde_urlencoded on a JSON body (would panic); rewire to serde_json::from_slice to match the production decode path, add assertions, un-ignore. Verified: cargo fmt; cargo check on touched crates; tests pass (filemeta, signer, bitrot, heal::test_decode); arch guardrail scripts pass. |
||
|
Zhengchao An
|
1cc1fc0f83 |
fix(ecstore): exclude failed-shard disks from commit so write quorum isn't inflated (backlog#852) (#4309)
`MultiWriter` recorded per-writer failures only in a private `errs` vector and nulled the failed writer locally, but `put_object` never used that to prune the disk set: `rename_data` ran over the full `shuffle_disks`, so a disk that took a short/failed write still had its truncated shard renamed into place and counted as an online disk. The object then claimed N good shards while one was short/corrupt, so a single later disk failure could drop it below reconstructable quorum — silent data loss. - `write_shard`: null the writer on a generic write error too (not only on `ShortWrite`), so a failed writer is uniformly represented as `None` — matching `shutdown_writer`, which already does this. - `put_object`: after encode, drop every disk whose writer failed (`drop_failed_writer_disks`) from `shuffle_disks` before `rename_data`, and re-check write quorum over the survivors. `rename_data` already re-checks quorum and rolls back if too few disks remain, so excluded disks are neither committed nor counted (MinIO sets failed writers to nil before `renameData`). Adds unit tests for the exclusion/quorum accounting. Refs backlog#799 (B3). |
||
|
Zhengchao An
|
3e4c15da5d | fix(object-lock): prevent locked version deletes (#4297) | ||
|
Zhengchao An
|
f737b39cfc |
refactor(ecstore): move ObjectIO/ObjectOperations into set_disk::ops::object (backlog#821) (#4290)
P6 of the SetDisks God-Object split (tracking backlog#815, issue backlog#821; depends on P5 #4288). Relocate the core object read/write hot-path contract impls out of the set_disk/mod.rs God-Object into their own module home: - impl ObjectIO for SetDisks (~1,010 lines) -> set_disk/ops/object.rs - impl ObjectOperations for SetDisks (~1,137 lines) -> set_disk/ops/object.rs - Registered pub(crate) mod object; under set_disk/ops. Pure move — zero logic change. Both contracts stay implemented for SetDisks, so their EcstoreObjectIO / EcstoreObjectOperations associated-type bounds are unchanged and the contract-compat tests still guard them. Method bodies are moved verbatim: a whitespace-insensitive token-stream diff of the two impl blocks (with their #[async_trait::async_trait] attributes) against the pre-move mod.rs source is byte-identical (5,754 tokens each) — NO visibility widening was required, because the impls reach SetDisks helpers and the P5 io_primitives through inherent self./Self:: calls that resolve across modules unchanged. The two inherent impl SetDisks blocks that sat between the trait impls (lock batch helpers) remain in mod.rs, verified present exactly once and uncorrupted. The issue's borrow/Arc-clone-avoidance optimization is intentionally deferred: it is a perf-sensitive change requiring the #738 benchmark and would risk the 'byte-level behavior unchanged' acceptance; this PR delivers the relocation. Verification: - cargo check / clippy -D warnings -p rustfs-ecstore --all-targets: clean - cargo test -p rustfs-ecstore --lib: 1841 passed, 0 failed - all five arch guard scripts: pass - token-stream diff of moved impls vs original: identical (mod.rs diff is a pure relocation; ObjectIO/ObjectOperations each defined exactly once post-move) |