-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-14183. Attempted to decrement available space to a negative value #9655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1037,6 +1037,11 @@ ContainerCommandResponseProto handleWriteChunk( | |
| } | ||
|
|
||
| ContainerProtos.BlockData blockDataProto = null; | ||
| HddsVolume volume = kvContainer.getContainerData().getVolume(); | ||
| long bytesToWrite = 0; | ||
| boolean spaceReserved = false; | ||
| boolean writeChunkSucceeded = false; | ||
|
|
||
| try { | ||
| checkContainerOpen(kvContainer); | ||
|
|
||
|
|
@@ -1057,9 +1062,17 @@ ContainerCommandResponseProto handleWriteChunk( | |
| ChunkBuffer.wrap(writeChunk.getData().asReadOnlyByteBufferList()); | ||
| // TODO: Can improve checksum validation here. Make this one-shot after protocol change. | ||
| validateChunkChecksumData(data, chunkInfo); | ||
| bytesToWrite = chunkInfo.getLen(); | ||
|
|
||
| // Reserve space before writing | ||
| if (volume != null && bytesToWrite > 0) { | ||
| volume.reserveSpaceForWrite(bytesToWrite); | ||
| spaceReserved = true; | ||
| } | ||
| } | ||
| chunkManager | ||
| .writeChunk(kvContainer, blockID, chunkInfo, data, dispatcherContext); | ||
| writeChunkSucceeded = true; | ||
|
|
||
| final boolean isCommit = dispatcherContext.getStage().isCommit(); | ||
| if (isCommit && writeChunk.hasBlock()) { | ||
|
|
@@ -1086,14 +1099,17 @@ ContainerCommandResponseProto handleWriteChunk( | |
| metrics.incContainerOpsLatencies(Type.PutBlock, Time.monotonicNowNanos() - startTime); | ||
| } | ||
|
|
||
| commitSpaceReservedForWrite(volume, spaceReserved, bytesToWrite); | ||
| // We should increment stats after writeChunk | ||
| if (isWrite) { | ||
| metrics.incContainerBytesStats(Type.WriteChunk, writeChunk | ||
| .getChunkData().getLen()); | ||
| } | ||
| } catch (StorageContainerException ex) { | ||
| releaseSpaceReservedForWrite(volume, spaceReserved, writeChunkSucceeded, bytesToWrite, kvContainer); | ||
| return ContainerUtils.logAndReturnError(LOG, ex, request); | ||
| } catch (IOException ex) { | ||
| releaseSpaceReservedForWrite(volume, spaceReserved, writeChunkSucceeded, bytesToWrite, kvContainer); | ||
| return ContainerUtils.logAndReturnError(LOG, | ||
| new StorageContainerException("Write Chunk failed", ex, IO_EXCEPTION), | ||
| request); | ||
|
|
@@ -1102,6 +1118,31 @@ ContainerCommandResponseProto handleWriteChunk( | |
| return getWriteChunkResponseSuccess(request, blockDataProto); | ||
| } | ||
|
|
||
| /** | ||
| * Commit space reserved for write to usedSpace when write operation succeeds. | ||
| */ | ||
| private void commitSpaceReservedForWrite(HddsVolume volume, boolean spaceReserved, long bytes) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can commit happen when space is not reserved? |
||
| if (spaceReserved) { | ||
| volume.releaseReservedSpaceForWrite(bytes); | ||
| volume.incrementUsedSpace(bytes); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since release and increment are not inside critical section, its better to incrementUsedSpace first and then release reserved space, if its done as it currently implemented if other thread check free space between these 2 calls it could try to use free space that already used by this write. |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Release space reserved for write when write operation fails. | ||
| * Also restores committedBytes if it was decremented during write. | ||
| */ | ||
| private void releaseSpaceReservedForWrite(HddsVolume volume, boolean spaceReserved, | ||
| boolean writeChunkSucceeded, long bytes, KeyValueContainer kvContainer) { | ||
| if (spaceReserved) { | ||
| volume.releaseReservedSpaceForWrite(bytes); | ||
| // Only restore committedBytes if write chunk succeeded | ||
| if (writeChunkSucceeded) { | ||
| kvContainer.getContainerData().restoreCommittedBytesOnWriteFailure(bytes); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handle Write Chunk operation for closed container. Calls ChunkManager to process the request. | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add test coverage to verify the restore of both usedspace and committedBytes