HDDS-16289. Reduce OM FSO apply-path write-lock hold and path-walk cost - #11328
Open
yandrey321 wants to merge 2 commits into
Open
yandrey321 wants to merge 2 commits into
yandrey321 wants to merge 2 commits into
Conversation
Contributor
Author
|
@ss77892 @jojochuang could you please take a look? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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/lookupKeyare frozen on a hot bucket.It costs more than it needs to. Per intermediate segment the walk fully parses
DirectoryInfoandrebuilds an
OmDirectoryInfo— ACLs, metadata map, owner, timestamps — when all it consumes isgetObjectID(). The same walk runs under the bucket read lock inKeyManagerImpl, on the200-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 eachsplit into a lock-free
prepare*(walk, checks, missing-parent build, prepared key/dir info) and anapply*under the narrowed lock (O(1) re-check → quota RMW → everyaddCacheEntry→ response). Allmutations 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(newfromKeyParentparam); delete also moveshasChildren, a full dirTable+fileTable cache scan plus twoRocksDB seeks and the largest of the five holds; mkdir previously took the lock even before
validateBucketAndVolume, and itsDIRECTORY_ALREADY_EXISTSpath — like rename'ssrc == dst— nowtakes 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,
FullTableCacheevicts only tombstones,TypedTable.getreturns adefensive copy). Each site documents that invariant and keeps an O(1) tripwire failing safe with the
error the lock-free phase would have raised;
OMAllocateBlockRequestis the existing precedent.The walk itself is then projected to the objectId. New
Table.getProjected(key, fromCachedValue, fromPersistedValue)keepsget()'s cache semantics but ona cache miss decodes only the requested field from the pooled direct
CodecBuffer. Itsdefaultiscorrect-but-unoptimized (
get()then project) soRDBTable,DatanodeTableandInMemoryTestTableneed no edit;
TypedTableoverrides it, reusing the existing pooled-buffer read and itscapacity-retry loop, with
SnapshotDiffValueParser.parseDirectoryInfoObjectId— the partialCodedInputStreamscan that file already does for snapshot diff — as the decoder.getParentIDprojects every segment,
getOMKeyInfoIfExistsall but the last,verifyDirectoryKeysInPatheverysegment plus one full
get()for ACL inheritance (1 get traded for D−1 avoided decodes; win fromdepth 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, asare four follow-ups the same sweep turned up (
OMKeySetTimesRequestWithFSO,OmKeysDeleteRequestWithFSO, theOzoneAcl.fromProtobufordinal-array fix, andBucketManagerImpl.getBucketInforead-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:
TestTypedTablegetProjectedover 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}RequestWithFSOfor 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 sunder-load, 1.1–1.2 M samples/op/window.
deg99= under-load p99 ÷ control p99:getBucketInfogetFileStatuslookupKeyWrite 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, nolock contention, arms interleaved round-by-round in one JVM, 120 000 walks per arm per cell,
cacheResidentSegments=0so both arms genuinely read the store. Thefullarm isdirTable.get(key).getObjectID()— literally the pre-change cost.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
DirectoryInfoparse, theOmDirectoryInforebuild and theOzoneAcl.fromProtobufchurn. Async-profiler attribution under the §2 workload (cpu@1ms), peroperation, isolating the projection on top of the narrowed lock:
OmDirectoryInfodecodeOzoneAcl*)Reader throughput +13.0 %; control-phase
getFileStatusp50 −21 to −24 %,lookupKeyp50−10 to −12 %; under-load
getFileStatusp99 −17 to −23 %.SegmentedRaftLogWorkeris 99.6 % busy infcntl(F_FULLFSYNC)on this box, so the −34 % apply CPU is headroom, not measured throughput.are different quantities;
lookupKeygains ~half ofgetFileStatusbecause its leaf is a file andstill pays a full
OmKeyInfodecode.