yandrey321 opened a new pull request, #11328: URL: https://github.com/apache/ozone/pull/11328
## What changes were proposed in this pull request? Every FSO metadata operation resolves its key by walking the path one segment at a time, a RocksDB point lookup per segment via `verifyDirectoryKeysInPath` / `getOMKeyInfoIfExists` / `getKeyParentDir`. That single walk is both the longest thing the write path holds the bucket lock for and the most expensive thing the read path does, and this PR fixes it on both counts. **It is held under the wrong lock.** The OM applies every write on a single serial state-machine thread, and each FSO writer takes the **bucket write lock** before the walk, then holds it across work that is almost entirely *reads of committed state*. A writer queued for that lock blocks every reader arriving after it, so the hold duration *is* the window in which `getBucketInfo` / `getFileStatus` / `lookupKey` are frozen on a hot bucket. **It costs more than it needs to.** Per intermediate segment the walk fully parses `DirectoryInfo` and rebuilds an `OmDirectoryInfo` — ACLs, metadata map, owner, timestamps — when all it consumes is `getObjectID()`. The same walk runs under the bucket **read** lock in `KeyManagerImpl`, on the 200-thread handler pool, so that waste is paid by readers as well as by the apply thread. So the walk moves out of the write lock **and** gets cheaper: Five FSO writers — `OMFileCreateRequestWithFSO`, `OMKeyCommitRequestWithFSO`, `OMDirectoryCreateRequestWithFSO`, `OMKeyRenameRequestWithFSO`, `OMKeyDeleteRequestWithFSO` — are each split into a lock-free `prepare*` (walk, checks, missing-parent build, prepared key/dir info) and an `apply*` under the narrowed lock (O(1) re-check → quota RMW → every `addCacheEntry` → response). All mutations stay inside the lock, so readers still see each transaction atomically; what they no longer wait on is the walk. Rename moves **five** walks out, one hoisted from private `renameKey` (new `fromKeyParent` param); delete also moves `hasChildren`, a full dirTable+fileTable cache scan plus two RocksDB seeks and the largest of the five holds; mkdir previously took the lock even before `validateBucketAndVolume`, and its `DIRECTORY_ALREADY_EXISTS` path — like rename's `src == dst` — now takes no write lock at all. *Safety:* apply is strictly single-threaded, so a writer contends only with readers, and the concurrent flush/cleanup threads cannot change a key's **visible** value (eviction drops only already-persisted epochs, `FullTableCache` evicts only tombstones, `TypedTable.get` returns a defensive copy). Each site documents that invariant and keeps an O(1) tripwire failing safe with the error the lock-free phase would have raised; `OMAllocateBlockRequest` is the existing precedent. The walk itself is then projected to the objectId. New `Table.getProjected(key, fromCachedValue, fromPersistedValue)` keeps `get()`'s cache semantics but on a cache miss decodes only the requested field from the pooled direct `CodecBuffer`. Its `default` is correct-but-unoptimized (`get()` then project) so `RDBTable`, `DatanodeTable` and `InMemoryTestTable` need no edit; `TypedTable` overrides it, reusing the existing pooled-buffer read and its capacity-retry loop, with `SnapshotDiffValueParser.parseDirectoryInfoObjectId` — the partial `CodedInputStream` scan that file already does for snapshot diff — as the decoder. `getParentID` projects every segment, `getOMKeyInfoIfExists` all but the last, `verifyDirectoryKeysInPath` every segment plus **one** full `get()` for ACL inheritance (1 get traded for D−1 avoided decodes; win from depth 3 up). No proto change, no new dependency, no caller signature change. `OMKeyCreateRequestWithFSO` (S3 PutObject), the OBS writers and the purge path are out of scope, as are four follow-ups the same sweep turned up (`OMKeySetTimesRequestWithFSO`, `OmKeysDeleteRequestWithFSO`, the `OzoneAcl.fromProtobuf` ordinal-array fix, and `BucketManagerImpl.getBucketInfo` read-locking a FULL_CACHE get). ## What is the link to the Apache JIRA https://issues.apache.org/jira/browse/HDDS-16289 ## How was this patch tested? CI: Correctness rests on **existing tests passing unchanged**, since the narrowing must not alter the cache mutations or the response for any existing case. Full unit run of both touched modules: **350 classes / 3 962 tests / 0 failures**, BUILD SUCCESS, including the FSO create, commit, mkdir, rename, delete and multipart suites with no assertion changes — those also cover ACL inheritance, the one behaviour the projection must preserve. New: `TestTypedTable` `getProjected` over a cache hit, a cache tombstone (`NOT_EXIST`), a store hit, an absent key and an over-capacity value exercising the pooled-buffer retry loop (14/14); `TestOMKey{Delete,Rename}RequestWithFSO` for the restructured delete branches (`DIRECTORY_NOT_EMPTY`, hsync open-key), previously uncovered in either delete suite; and `TestOmFsoWriteLockConvoyBench` (`@Tag("benchmark")`, excluded from default CI), ### Performance Local `MiniOzoneCluster`, macOS/aarch64. **1. Write-lock hold time.** `OMLockDetails.getWriteLockNanos()` for a recursive FSO create, master → this PR: depth 8 **4.43 → 1.31 ms**, depth 20 **6.37 → 1.55 ms**, depth 40 **6.75 → 2.01 ms** (3.4–4.1×). The shape is the evidence — master grows with depth because the per-segment gets are inside the lock; this PR is flat. **2. Reader latency under concurrent FSO writes, production default `fair=false`.** `TestOmFsoWriteLockConvoyBench`: 1 hot bucket, 8 readers + 4 writers, 4-op write mix incl. non-recursive dir delete so all five sites are driven, `pathDepth=24`, 150 s control + 150 s under-load, 1.1–1.2 M samples/op/window. `deg99` = under-load p99 ÷ control p99: | read op | master | this PR | under-load p99 | convoy excess removed | |---|---|---|---|---| | `getBucketInfo` | 1.94× | **1.26×** | 0.533 → 0.342 ms | 72 % | | `getFileStatus` | 1.56× | **1.18×** | 0.744 → 0.550 ms | 68 % | | `lookupKey` | 1.74× | **1.16×** | 1.222 → 0.801 ms | 78 % | **Write throughput is flat — 60688/60793/60797/60964 completions across the four cells, 0.45 % spread** — so the reader gain is not bought by starving writers. n=2, reproduced within 0.04× per cell, and the fix arm's control was marginally *slower*, making the win conservative. **3. Walk cost.** `benchmarkFsoWalkProjection`, isolated before/after: single-threaded, no writers, no lock contention, arms interleaved round-by-round in one JVM, 120 000 walks per arm per cell, `cacheResidentSegments=0` so both arms genuinely read the store. The `full` arm is `dirTable.get(key).getObjectID()` — literally the pre-change cost. | cell | full | projected | Δ mean | Δ p99 | ns/segment | |---|---|---|---|---|---| | depth 4 (6 segments) | 8.22 µs | 5.95 µs | **−27.6 %** | −20.8 % | 1371 → 992 | | depth 16 (18 segments) | 25.92 µs | 17.94 µs | **−30.8 %** | −29.1 % | 1440 → 997 | Projected per-segment cost is **flat at ~995 ns** while the full read drifts up with depth, so the relative win grows with depth. That floor is the RocksDB get, untouched here; the ~400–440 ns removed per segment is the `DirectoryInfo` parse, the `OmDirectoryInfo` rebuild and the `OzoneAcl.fromProtobuf` churn. Async-profiler attribution under the §2 workload (`cpu@1ms`), per operation, isolating the projection on top of the narrowed lock: | metric | narrowed lock only | + projected walk | Δ | |---|---|---|---| | handler busy CPU ms / 1k reads | 174.5 | 113.2 | **−35 %** | | …of which the FSO walk | 127.7 | 64.5 | **−50 %** | | full `OmDirectoryInfo` decode | 27.3 | ~0 (17 of 62 960 samples) | **−100 %** | | ACL rebuild (`OzoneAcl*`) | 22.6 | 0.5 | **−98 %** | | apply-thread busy CPU ms / 1k txns | 371.9 | 246.7 | **−34 %** | | …of which the FSO walk | 250.9 | 141.7 | **−43 %** | Reader throughput **+13.0 %**; control-phase `getFileStatus` p50 **−21 to −24 %**, `lookupKey` p50 −10 to −12 %; under-load `getFileStatus` p99 −17 to −23 %. * **No write-throughput win.** Writers are flat everywhere: `SegmentedRaftLogWorker` is 99.6 % busy in `fcntl(F_FULLFSYNC)` on this box, so the −34 % apply CPU is headroom, not measured throughput. * §3's −30.8 % (wall, rocksdb get in the denominator) and the profile's −50 % (CPU, rocksdb separate) are different quantities; `lookupKey` gains ~half of `getFileStatus` because its leaf is a file and still pays a full `OmKeyInfo` decode. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
