[
https://issues.apache.org/jira/browse/HDDS-16052?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105793#comment-18105793
]
Ivan Andika commented on HDDS-16052:
------------------------------------
*Finding:* {{ContainerData}} is not independently thread-safe. It relies on the
owning {{KeyValueContainer}} lock.
* State changes are made through {{KeyValueContainer}} under its write lock.
For example, close/quasi-close paths lock before calling
{{{}ContainerData.setState(){}}}. [KeyValueHandler.java (line
1629)](/Users/ivan.andika/.codex/worktrees/22ec/ozone-community/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:1629)
* {{commitSpace()}} is called during container loading, after insertion into
{{{}ContainerSet{}}}; it is not a normal request-serving path.
[ContainerReader.java (line
235)](/Users/ivan.andika/.codex/worktrees/22ec/ozone-community/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:235)
* {{{}setVolume{}}}, {{{}setChunksPath{}}}, and replica-index updates are
creation/reload setup operations.
* The class comments explicitly require callers to hold the container lock for
metadata mutations. [ContainerData.java (line
270)](/Users/ivan.andika/.codex/worktrees/22ec/ozone-community/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java:270)
Therefore, RacerD’s {{ContainerData}} warnings are mostly expected:
{{@ThreadSafe}} tells Infer every public method must be safe without external
locking, which is stronger than this class’s actual contract.
The potentially real risk is the interaction among {{{}commitSpace(){}}},
{{{}releaseCommitSpace(){}}}, and write accounting if any production write path
bypasses {{{}KeyValueContainer{}}}’s lock. I have not found such a bypass yet,
so this is {*}not a confirmed race{*}.
Recommendation: do not add {{synchronized}} broadly to {{{}ContainerData{}}};
that would change the lock hierarchy and may hurt the write path. Instead,
remove {{@ThreadSafe}} from {{ContainerData}} or use a narrower analysis
strategy, then focus RacerD on classes that own their synchronization.
> ContainerData updateWriteStats is not atomic
> --------------------------------------------
>
> Key: HDDS-16052
> URL: https://issues.apache.org/jira/browse/HDDS-16052
> Project: Apache Ozone
> Issue Type: Sub-task
> Reporter: Ivan Andika
> Assignee: Ivan Andika
> Priority: Major
>
> ContainerData#updateWriteStats is not atomic. It is updated for each
> WriteChunk, but WriteChunk is only serialized for a single block (see
> ContainerStateMachine#getChunkExecutor). If there are two concurrent
> WriteChunk for the same container, but with different blocks, this can
> trigger race conditions which can cause updateWriteStats to be interleaved
> which can end up causing inconsistency in ContainerData. We also need to
> specify the thread safety guarantee of ContainerData.
> Additionally, ContainerData is also has memory visibility (no happens-before
> guarantee) issue since the shared variables like committedSpace is not
> volatile (or protected by synchronization), although ContainerData can be
> accessed by multiple threads.
> Example: container has 100 bytes unused, T1 and T2 each write 100 bytes.
> * T1 reads {{{}unused = 100{}}}, gets preempted.
> * T2 writes: {{bytesUsed}} goes to +100, releases 100 from
> {{{}committedBytes{}}}. Container is now full, committed space fully
> released. Correct so far.
> * T1 resumes with its stale {{{}unused = 100{}}}, writes (now {{bytesUsed}}
> = +200), and releases _another_ 100 from {{{}committedBytes{}}}.
> End state: {{committedBytes}} was decremented by 200 when only 100 was ever
> reserved for that container. The volume-level {{committedBytes}}
> under-reports by 100, so {{VolumeUsage}} thinks there's more free space for
> new containers than there really is.
> A few things worth keeping in mind:
> * The error compounds over many concurrent writes rather than being a
> one-off, but it's bounded by how much committed space was actually reserved —
> it can't go arbitrarily wrong from a single container.
> * It self-corrects partially: DU/{{{}DedicatedDiskSpaceUsage{}}}
> periodically recomputes real used space, and {{committedBytes}} is released
> wholesale on container close via {{releaseCommitSpace}} (which has its own
> races with the same stale-read pattern).
> * The cleanest fix is to compute the decrement from the return value of
> {{incrBytesUsed}} (the post-increment total) instead of the pre-read
> {{{}unused{}}}, or guard the whole release block with the container's lock —
> though that also needs {{{}commitSpace{}}}/{{{}releaseCommitSpace{}}} to
> participate in the same locking to close the check-then-act window on the
> {{committedSpace}} flag.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
