[
https://issues.apache.org/jira/browse/HDDS-16052?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100687#comment-18100687
]
Ivan Andika commented on HDDS-16052:
------------------------------------
Two concrete race windows:
# *Stale {{unused}} → over-release of committed space.* Thread A reads
{{unused = 100}} for a 100-byte write; before A reaches
{{{}incCommittedBytes{}}}, thread B writes 100 bytes (its {{updateWriteStats}}
bumps {{bytesUsed}} by 100). A still decrements {{committedBytes}} by 100. Now
{{bytesUsed}} has grown by 200 but {{committedBytes}} was only released by 100,
when the whole 100 should have been released exactly once (or B's write
shouldn't have been counted against it at all). Result: volume
{{committedBytes}} drifts negative / under-reported, which makes
{{VolumeUsage}} over-estimate available space for new container creation.
# *Non-atomic, non-volatile {{{}committedSpace{}}}.* It's a plain boolean with
no synchronization ([ContainerData.java (line
96)](hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java:96)),
read in {{incrWriteBytes}} while {{commitSpace}} / {{releaseCommitSpace}}
([ContainerData.java (line
425)](hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java:425))
mutate it from container lifecycle paths. There's no happens-before edge, so
visibility isn't guaranteed, and the check-then-act across {{commitSpace}} ↔
{{incrWriteBytes}} can double-release or double-acquire.
There's also an internal ordering wrinkle: {{incrBytesUsed}} runs in
{{updateWriteStats}} ([ContainerData.java (line
769)](hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java:769))
_before_ {{{}incrWriteBytes{}}}, and {{incrementUsedSpace}} runs _inside_
{{incrWriteBytes}} — so concurrent updates interleave the {{bytesUsed}}
snapshot against volume-level updates arbitrarily.
*Severity caveat:* this is bounded accounting drift in space bookkeeping
(committed/used bytes), not data corruption — the counters that matter for
correctness ({{{}bytesUsed{}}}, {{{}writeBytes{}}}) can't lose updates. Fixing
it properly would mean making the commit-space release atomic with the
{{bytesUsed}} increment, e.g. a synchronized block or moving the decrement
computation to use the return value of {{incrBytesUsed}} instead of the
pre-read snapshot.
> 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. 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]
