backlog#1823 step 1, the diagnosis half. Temporarily removing set_disk/mod.rs's #![allow(unused_variables)] surfaced nine bindings. The issue asks that values computed and then dropped on write/quorum paths be diagnosed before being underscored, and that turned out to matter: only four were plain leftovers.
Two errors were bound and then left out of the log they were bound for. complete_multipart_upload's checksum failures read `if let Err(err) = ...` and then log part_id, bucket and object with no `err` anywhere in the message, so a checksum failure in production told you which part failed but not why. Both messages now carry the error.
One is a lock guard. heal's write_lock_guard holds a namespace write lock for the rest of the scope; renaming it to a bare `_` would drop it immediately and release the lock. It is now `_write_lock_guard`, with a comment saying why it must not be `_`.
One was kept alive by a corpse. `errors` in read_multiple_files is read by nothing except two commented-out debug! lines directly below it; the binding and the commented lines go together.
One is a cfg split. heal's disk_index is read only inside the #[cfg(test)] fault-injection branch, so underscoring it would break the test build; a `#[cfg(not(test))] let _ = disk_index;` covers the non-test lane instead.
The remaining four are genuine leftovers: an unused enumerate index in list_object_parts, a discarded error in a heal reader loop, an inner binding shadowing its own iterator variable, and delete_object's write_quorum.
That last one is worth a separate look: delete_object asks get_object_info_and_quorum for a write quorum and never uses it, because delete_object_version below recomputes its own as disks.len() / 2 + 1. The two are not the same number — one comes from the object's erasure configuration, the other is a plain majority of the disk array. Pre-existing behaviour, untouched here.
The blankets stay for now. Removing #![allow(unused_imports)] exposes 76 unused imports in set_disk/mod.rs, and they cannot be removed per-lane: cargo fix, working from the lib lane, produced 54 compile errors in the test lane. That needs its own pass with both lanes checked per import.
Verification: cargo check -p rustfs-ecstore --tests and --features test-util --tests both warning-free; clippy --lib --tests -D warnings clean; cargo nextest run -p rustfs-ecstore 4101 passed; make pre-commit exit 0.
Ref rustfs/backlog#1823 (step 1).
Add a diagnostic metadata-only read_version delay hook for GET data-read fanout so bounded/default behavior can be compared under controlled slow-tail metadata responses.
Co-authored-by: heihutu <heihutu@gmail.com>
#6107 routed the transitioned read through the object's own ReadPlan so a tiered SSE object stops serving ciphertext. Compression rides that same plan and got fixed with it, but nothing pins it: revert the routing and a compressed object that ILM moved to a warm tier returns its stored (compressed) bytes under the compressed size, with every existing test still green.
The gap is easy to reopen because transition genuinely uploads the stored representation — the upload side is correct and the read side is the only place that can decode it. These tests state that contract at the boundary where it broke.
Four cases, all through SetDisks::get_object_reader against a mock warm tier:
- a full GET of a transitioned compressed object returns the plaintext and publishes the plaintext size (the test also asserts the remote copy holds the compressed bytes, so it fails loudly if the upload side ever changes instead);
- a ranged GET returns that plaintext slice, with the range deliberately starting past the compressed size so a range still measured in stored coordinates cannot produce it;
- a restore read still receives the stored bytes under the stored size — restore_request_active holds it on the Plain branch, and decompressing there would write plaintext under compressed metadata;
- a plain transitioned object still reads back byte-identical, full and ranged.
Verified as guards, not decoration: forcing the tiered read back onto the Plain branch turns the two compressed tests red and leaves the plain and restore tests green.
What these do not pin, so the gap stays recorded rather than implied covered: the fixture carries no compression index, so part.index stays None and the plan's storage offset is always 0 — the compressed-offset translation itself is still untested, as are multipart compressed objects, partNumber reads, and the encrypted tiered read that #6107 targeted.
* chore(ecstore): drop the set_disk dead_code blanket
Removing the blanket exposes 39 items; exactly one is deleted. The low share is a finding, not caution: unlike the disk root, where platform gating made local adjudication impossible, here the items were checked and nearly all of them are live.
Deleted: HealEntryResult, the only item with no reference anywhere.
What the checks turned up, in the order the warnings suggest deleting them:
SetDisks::rename_data looked like the head of a dead chain feeding into_legacy_tuple and RenameDataLegacyTuple. It is not: production goes through rename_data_owned, and rename_data itself has test callers at mod.rs:5809 and 5880. The chain below it is therefore live through the tests, and inferring "this is dead, so its callee is dead" would have removed three working items.
create_bitrot_readers_until_quorum, read_multiple_files and map_cleanup_join_result all have callers inside their files' test modules, so they only look dead in the lib target.
TransitionCommitBarrier and TransitionUploadedSaveProbe, with their install/wait_until_paused/release surfaces, are installed by tests behind #[cfg(all(test, feature = "test-util"))].
ctx.rs's SetDisksCtx accessors are the split seam left by the SetDisks god-object break-up (backlog#815).
heal_object_dir's two apparent references are comments, and they document an index-alignment contract that live code maintains for it, so they stay as they are.
Worth a maintainer decision: the metadata early-stop switch has a complete percentage-rollout facet — ENV_RUSTFS_GET_METADATA_EARLY_STOP_ROLLOUT_PCT, get_metadata_early_stop_rollout_pct and should_use_metadata_early_stop — with no caller, no test and no documentation, while its sibling enable flag is live. It is kept with an allow that says so rather than removed, since a rollout knob is a product call.
One placement note for anyone adding allows near heal code: check_logging_guardrails.sh requires #[instrument(level = "trace")] to sit immediately before async fn heal_object_dir, so the allow goes above the instrument attribute. Putting it between the two drops the guard's match count and fails the check.
Verification, four lanes warning-free: default, --tests, --features rio-v2 --tests, --features test-util --tests. cargo nextest run -p rustfs-ecstore 4096 passed; clippy --lib --tests -D warnings clean; make pre-commit exit 0.
Ref rustfs/backlog#1823 (step 2).
* chore(ecstore): fix duplicated and inaccurate dead_code reasons in set_disk
format_lock_error carried the same #[allow] twice. Five items in the
locking/heal roots were labelled 'asserted by this file's tests' while
having no reference at all - heal_object_dir's only two references are
comments, as this branch's own notes point out. Say what each item
actually is instead, so the next reader does not assume test coverage
that is not there.
Ref rustfs/backlog#1823.
* chore(ecstore): correct the bounded_spare_disk_index dead_code reason
The mod.rs copy is an unused test fixture, not something this module's
tests assert; the namesake that is exercised lives in the io_primitives
test module.
Ref rustfs/backlog#1823.
* fix(tier): decrypt transitioned objects instead of serving their ciphertext
A GET on a managed-SSE object that lifecycle had transitioned to a remote tier returned the ciphertext with the plaintext's Content-Length and no error: silent corruption on read-through, and worse than a failed request because nothing signals it. Restore of the same object failed server-side with IncompleteBody while POST ?restore still answered 200, so the object simply never came back and HEAD never showed an x-amz-restore marker.
Both symptoms are one cause. The transitioned read path built its fetch through new_getobjectreader, which decides nothing about encryption: it derived the range from the parts table — whose sizes are PLAINTEXT sizes — then used that range to fetch the object's STORED bytes from the tier, and handed the stream to the caller without any decrypt transform. The GET therefore served the first plaintext-length bytes of ciphertext; the restore copy-back, which validates against the stored size, came up short by exactly the encryption overhead.
The path now builds the same ReadPlan the local read path uses, so a single place decides how stored bytes map to requested bytes. ReadPlan gains a two-phase API — build_for_request to learn the storage coordinates before issuing the tier fetch, into_object_reader to wrap the returned stream — because the tier fetch has to be positioned before a stream exists. The encryption resolver reaches the path from InstanceContext, the same source the local read uses.
A restore read additionally stops synthesizing a range from the part number. A restore serves the stored representation (restore_request_active already forces the Plain branch), so a plaintext-coordinate range would be reinterpreted as a storage range and truncate the payload by its encoding overhead. An explicit caller range is already in storage coordinates on that path and is still honored, which two existing tests pin.
crates/e2e_test/src/kms/kms_ilm_sse_kms_test.rs drops its #[ignore]: the transition test now runs and asserts the plaintext round-trips byte-identically through transition, read-through and restore. The same file had its enforcement switch stuck at false from a control experiment; it is back to true, so the test again exercises what its name and module docs claim.
Fixes#6025. Refs rustfs/backlog#1582, rustfs/backlog#1637.
* test(tier): pass resolver to transitioned reader tests
Add a default-off inline-only data-read metadata early-stop gate that verifies inline plaintext before cancelling pending metadata tasks.
Keep non-inline, prepared, and request-shape-sensitive reads on full fanout, and record scheduled/completed/cancelled ReadVersion lifecycle metrics for normal fanout completion.
Co-authored-by: heihutu <heihutu@gmail.com>
* perf(metrics): attribute PUT stage costs
Co-Authored-By: heihutu <heihutu@gmail.com>
* perf(ecstore): move PUT metadata during shuffle
Co-Authored-By: heihutu <heihutu@gmail.com>
* perf(s3): reuse PUT object lock state
Co-Authored-By: heihutu <heihutu@gmail.com>
* perf(ecstore): trim PUT metadata fanout clones
Build per-disk PUT metadata only for committed writer slots, move the response metadata out of the fanout vector, and preserve fresh FileInfo shuffle semantics.
Co-Authored-By: heihutu <heihutu@gmail.com>
* perf(metrics): make PUT stage attribution opt-in
Co-Authored-By: heihutu <heihutu@gmail.com>
* perf(ecstore): commit inline PUT shards directly
Co-Authored-By: heihutu <heihutu@gmail.com>
* perf(ecstore): streamline rename staging cleanup
Use the directory-specific removal operation for rename_data staging parents. This avoids a guaranteed failed file-removal probe on Unix-like hosts and lets Windows remove the empty directory directly while preserving best-effort non-empty handling.
Co-Authored-By: heihutu <heihutu@gmail.com>
* test(ecstore): cover inline PUT rename failures
Cache the detailed stage metrics gate once per PUT and exercise exact-quorum and quorum-minus-one failures after inline shard encoding.
Co-Authored-By: heihutu <heihutu@gmail.com>
---------
Co-authored-by: heihutu <heihutu@gmail.com>
Keep fresh metadata fanout results owned while sharing cache-backed metadata through Arc, avoiding deep clones on eligible local cache hits without enabling unsafe distributed caching.
Co-authored-by: heihutu <heihutu@gmail.com>
Keep common shard-indexed decode scratch vectors inline while preserving heap fallback for larger supported erasure layouts. Consume scratch iterators directly at the stripe-state boundary to avoid reallocating.
Co-authored-by: heihutu <heihutu@gmail.com>
put_object_part's commit phase held an exclusive write lock on the whole
upload_id_path namespace, so concurrent UploadPart commits for different
part numbers of one upload serialized behind a single lock and returned
503 once the 5s lock-acquire timeout elapsed.
Adopt MinIO's PutObjectPart lock scope: a shared read lock on the
uploadId namespace plus an exclusive write lock on
{upload_id_path}/part.{N}. Different part numbers now commit
concurrently; same-part retries still serialize (backlog#853);
complete/abort keep the uploadId write lock and still exclude every
in-flight part commit. The lock-loss fence covers both guards.
Fixes#5961
Every GET fans out a `read_version` across all disks to resolve xl.meta. Each
fanout allocated an `Arc<ReadOptions>` (3 bools) plus four `Arc<String>`
(`Arc::new(x.to_string())` = two allocations each) and cloned them into every
spawned task. This trims the per-fanout allocation footprint.
- `ReadOptions` is three bools, so it is now `Copy`. The fanout drops the
`Arc<ReadOptions>` and hands each spawned task a copy; the two pre-existing
`ReadOptions::clone()` sites (set_disk/read.rs, set_disk/ops/heal.rs) stop
cloning a `Copy` type.
- The four request strings use `Arc::<str>::from(&str)` (one allocation each)
instead of `Arc::new(..to_string())` (string buffer + Arc = two each) — four
fewer allocations per fanout, transparent to the `read_version(&str)` call.
Behavior is unchanged: the fanout still spawns one task per disk (the spawn is
deliberate — `read_version_call_counter_observes_spawned_fanout` verifies the
process-global counter observes every per-disk increment across workers), quorum
/ early-stop / full-wait semantics are untouched, and no result ordering or
error handling changed.
Two larger items from the audit are intentionally NOT in this PR:
- `tokio::spawn` -> `FuturesUnordered`: the spawn is a tested, deliberate
design (cross-worker counter observation for #1309/#1314), and converting
would also change panic isolation. Left as-is.
- `vec![FileInfo::default(); N]`: `FileInfo`'s empty containers (String /
HashMap / Vec) do not allocate, so this is one `Vec` allocation, not the
per-element allocation the audit implied — not a real hot spot.
`cargo fmt`, `cargo clippy -p rustfs-ecstore --lib` (0 warnings),
`cargo check --lib --tests`, and the 26 fanout / call-counter unit tests pass
on macOS (the change is fully cross-platform).
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: zhi22915 <qiuzgang@gmail.com>
A small object whose data shards are inlined in xl.meta can be reassembled
straight from the already-resolved metadata, skipping the Erasure reconstruct
pipeline. The fast path existed but was opt-in
(`RUSTFS_GET_SMALL_OBJECT_DIRECT_MEMORY`, default off), so every deployment
paid the full shard-read fan-out for eligible small GETs by default.
- Flip `DEFAULT_RUSTFS_GET_SMALL_OBJECT_DIRECT_MEMORY` to `true`. The path is
correctness-neutral on a miss: `try_get_object_direct_data_shards_*` returns
None when the inline reassembly cannot satisfy the read, and the GET then
proceeds through the normal shard-read pipeline. The env stays as a kill
switch (`=false` restores the legacy path).
- Drop the bucket-level `versioned` / `version_suspended` exclusions. The
decision is made on `fi` — the already-resolved target version — so
reassembling its inlined data is correct on a versioned bucket too. An
explicit versionId GET still falls back (`opts.version_id`), and a
delete-marker latest is still rejected. The two now-unused fallback reasons
and their metric labels are removed.
- Tests updated: a versioned latest-version object is now eligible / `Use`
(covers the newly allowed path); the removed reasons' label assertions are
dropped.
cargo check --lib --tests and the direct_memory unit tests pass on macOS
(the change is fully cross-platform).
Co-authored-by: heihutu <heihutu@gmail.com>
Keep opt-in bounded GET data-read fanout from waiting on a single pending ReadVersion response when an unscheduled spare disk can satisfy quorum. Add a deterministic 2+2 regression that pauses the third scheduled metadata read and verifies the spare is started before returning.
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: zhi22915 <qiuzgang@gmail.com>
Assert the prepared GET metadata path keeps the default full data-read fanout after PR #5929 made bounded data-read fanout opt-in.
Co-authored-by: heihutu <heihutu@gmail.com>
Keep GET data-read metadata early-stop and bounded fanout behind explicit environment switches so the default path preserves full fanout read-failure tolerance.
Retain the focused opt-in A/B coverage and the invalid parity full-fanout guard for heterogeneous set layouts.
Co-authored-by: heihutu <heihutu@gmail.com>
Complete the encrypted-object replication series (backlog#1783, PR-C of
3, after #5872 and #5885): SSE-C objects replicate as ciphertext
passthrough — the source holds no customer key, so the stored bytes and
their encryption metadata travel verbatim and the replica decrypts only
with the original customer key, single-part and multipart.
- Sender: SSE-C objects read raw (raw_data_movement_read), transfer at
ciphertext size, and range multipart parts over stored part sizes.
- Receiver: authorized replication PUTs restore the stored SSE-C keys
from the transport headers (exact lowercase forms - the read-path
check is case-sensitive), set ObjectOptions.preserve_ciphertext, and
skip compression, bucket-default SSE, and sse_encryption behind one
restore-derived gate. Multipart uses an internal session marker to
store parts verbatim and strips it on complete.
- Convergence: the replication HEAD sends
x-rustfs-source-replication-check; the target authorizes it as
ReplicateObjectAction and skips SSE-C read validation for that
request only, so keyless convergence HEADs see etag/size/mtime
instead of 400 and SSE-C replicas stop re-driving forever.
- e2e: SSE-C contract flips to a key-gated readable replica (no-key and
wrong-key GETs fail - the direct silent-plaintext detector); new
multipart passthrough contract with ETag/marker/stability assertions.
Open the managed-SSE replication gate (backlog#1783, PR-B of 3, after
#5872): the replication reader already decrypts through the injected
object-encryption resolver, so the source sends plaintext plus an
encryption intent header (AES256 / aws:kms, never the source key id) and
the target re-encrypts on its normal PUT path with its own KMS. No DEK
crosses sites.
- replication_put_object_options: fail closed only on Unsupported;
insert the SSE intent after the strip loop.
- TargetClient::create_multipart_upload sends the full opts.header()
set, fixing multipart replicas losing content-type/user metadata
(plaintext included).
- Preserve source ETag and mtime on replicas (authorized replication
only): receiver wires x-rustfs-source-etag into preserve_etag for PUT
and CompleteMultipartUpload, resolve_complete_etag consumes it, and
complete options carry source_etag/source_mtime (absent mtime
degrades to epoch, not now_utc). Without this every replication HEAD
comparison re-drives re-encrypted objects forever.
- e2e: managed SSE contracts flip to success on an independent-KMS
dual-process pair (byte-identical plain GET proves target-owned
envelopes; ETag/mtime preserved; version stable across scanner
cycles; resync converges; multipart keeps structure and metadata);
new target-without-KMS fail-closed contract; SSE-C stays FAILED.
Co-authored-by: houseme <housemecn@gmail.com>
Zhengchao An