HDDS-16327. Read the block file length once per WriteChunk instead of three fstat calls - #11150
HDDS-16327. Read the block file length once per WriteChunk instead of three fstat calls#11150rich7420 wants to merge 3 commits into
Conversation
… three fstat calls FilePerBlockStrategy.writeChunk resolved FileChannel.size() three times per WriteChunk (twice on an overwrite). Read it once and reuse the length for the overwrite check, the offset validation and the space accounting. The FileChannel-based ChunkUtils.validateChunkForOverwrite / isOverWriteRequested / validateChunkSize now take the already-read length instead of re-stat-ing. Behavior unchanged. Claude-Session: https://claude.ai/code/session_01FUpCUnmy6JzHPGvwMhyGzq
|
Quick JMH of The saving is ~2 fstat (~0.6-0.9 us), roughly constant, so it is a meaningful fraction only for small cached writes (hsync / small keys / metadata) and is diluted by data movement at larger chunks. No fsync in this run, and macOS fstat is pricier than Linux, so the fraction is an upper bound. Standalone JMH, not part of the PR. |
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Nice improvement overall! @rich7420
the single read now throws
StorageContainerException(CHUNK_FILE_INCONSISTENCY), matching the other two size-read sites
Just want to confirm this is safe. Did you check how these two result codes are handled in ContainerStateMachine?
I also left a small suggestion to simplify the test.
| @@ -236,11 +236,11 @@ void validateChunkForOverwrite() throws IOException { | |||
| try (FileChannel fileChannel = | |||
| FileChannel.open(tempFile, StandardOpenOption.READ)) { | |||
There was a problem hiding this comment.
Could we simplify this test by passing the file length directly and removing the FileChannel setup? The helper still seems worth testing, but it no longer performs any file I/O.
What changes were proposed in this pull request?
FilePerBlockStrategy.writeChunkresolved the block file length viaFileChannel.size()three times per WriteChunk(twice on an overwrite), each an uncached
fstat: the overwrite check, the offset validation, and the spaceaccounting. All three run before the write, and WriteChunk apply for a given block file is serialized, so the length is
stable across them.
This reads
channel.size()once and reuses it for all three. TheFileChannel-basedChunkUtilshelpers(
validateChunkForOverwrite,isOverWriteRequested, andvalidateChunkSize) now take the already-read length insteadof re-stat-ing; the
File-based overloads used byFilePerChunkStrategyare unchanged.If the size read fails, the consolidated path preserves the previous effective result code. The old
UncheckedIOExceptionwas converted toCONTAINER_INTERNAL_ERRORbyKeyValueHandler; the new path throws aStorageContainerExceptionwith the same result code and retains the original cause.onFailure(volume)semantics areunchanged.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16327
How was this patch tested?
https://github.com/rich7420/ozone/actions/runs/33410588777