[
https://issues.apache.org/jira/browse/HBASE-30213?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18102359#comment-18102359
]
mazhengxuan edited comment on HBASE-30213 at 8/6/26 8:27 AM:
-------------------------------------------------------------
I investigated the relevant code paths and would like to work on this issue.
{{ThreadSafeMemStoreSizing}} stores {{{}dataSize{}}}, {{{}heapSize{}}},
{{{}offHeapSize{}}}, and {{cellsCount}} in separate atomic variables. Each
individual counter is thread-safe, but {{getMemStoreSize()}} does not return an
atomic snapshot of the four correlated values.
For the path:
1. {{HRegion.decrMemStoreSize()}} eventually calls
{{ThreadSafeMemStoreSizing.incMemStoreSize()}} with negative deltas.
2. {{offHeapSize}} and {{heapSize}} are updated separately.
3. A concurrent {{HRegion.checkResources()}} call obtains a {{MemStoreSize}}
snapshot and compares {{heapSize + offHeapSize}} with
{{{}blockingMemStoreSize{}}}.
4. The resulting sum may therefore contain values from different stages of the
update.
One detail worth discussing is that, for a normal flush where both deltas are
non-negative, the mixed sum is bounded by the pre-flush and post-flush totals:
{code:bash}
post-flush total <= mixed total <= pre-flush total
{code}
Therefore, an atomic snapshot could still legitimately return the old pre-flush
value while the flush accounting update is in progress. The underlying problem
is better described as the lack of a defined atomic value/linearization point
for the region-level memory limit decision, rather than every transient
rejection necessarily being caused only by the mixed snapshot.
My preferred fix is to maintain the value actually required by
{{checkResources()}} as a separate atomic region-level counter:
{code:bash}
heapSize + offHeapSize
{code}
The counter would be updated in the centralized {{HRegion.incMemStoreSize()}}
and {{HRegion.decrMemStoreSize()}} methods, and {{checkResources()}} would read
this single atomic value instead of adding two independently read counters.
This approach has several advantages:
* The resource-limit decision becomes atomic.
* It does not restore synchronization around all {{ThreadSafeMemStoreSizing}}
updates.
* It avoids allocating a new {{MemStoreSize}} object on every update.
* It does not add extra contention to the per-cell {{Segment}} accounting path.
* The change can be limited mainly to {{HRegion}} and a focused regression
test.
I would avoid synchronizing the whole {{ThreadSafeMemStoreSizing}} update/read
path without performance measurements. HBASE-20411 intentionally removed this
synchronization because it was visible in the MemStore write path. An
{{AtomicReference<MemStoreSize>}} would provide a coherent snapshot, but it
would also introduce allocation and CAS retry overhead on a hot path.
I think the following points would be useful to agree on before implementing
the patch:
1. Do we only require an atomic {{heapSize + offHeapSize}} value for
resource-limit decisions, or should {{getMemStoreSize()}} provide a fully
linearizable four-field snapshot?
2. If only the aggregate needs to be atomic, should it live in {{HRegion}} to
avoid affecting {{{}Segment{}}}, or should it be exposed by
{{ThreadSafeMemStoreSizing}} for reuse?
3. Should the same atomic aggregate also be used by the region-level
flush-threshold checks that currently consume
{{{}memStoreSizing.getMemStoreSize(){}}}?
For testing, I plan to add a deterministic test covering increment, decrement,
and mixed-sign deltas, and verify that the resource-limit decision always uses
the atomic aggregate. I would prefer this over a scheduling-dependent stress
test that could become flaky.
I would like to take ownership of this issue and prepare the patch/PR. I am
also happy to adjust the implementation based on the discussion above.
was (Author: JIRAUSER298959):
I investigated the relevant code paths and would like to work on this issue.
`ThreadSafeMemStoreSizing` stores `dataSize`, `heapSize`, `offHeapSize`, and
`cellsCount` in separate atomic variables. Each individual counter is
thread-safe, but `getMemStoreSize()` does not return an atomic snapshot of the
four correlated values.
For the path:
1. `HRegion.decrMemStoreSize()` eventually calls
`ThreadSafeMemStoreSizing.incMemStoreSize()` with negative deltas.
2. `offHeapSize` and `heapSize` are updated separately.
3. A concurrent `HRegion.checkResources()` call obtains a `MemStoreSize`
snapshot and compares `heapSize + offHeapSize` with `blockingMemStoreSize`.
4. The resulting sum may therefore contain values from different stages of the
update.
One detail worth discussing is that, for a normal flush where both deltas are
non-negative, the mixed sum is bounded by the pre-flush and post-flush totals:
post-flush total <= mixed total <= pre-flush total
Therefore, an atomic snapshot could still legitimately return the old pre-flush
value while the flush accounting update is in progress. The underlying problem
is better described as the lack of a defined atomic value/linearization point
for the region-level memory limit decision, rather than every transient
rejection necessarily being caused only by the mixed snapshot.
My preferred fix is to maintain the value actually required by
`checkResources()` as a separate atomic region-level counter:
heapSize + offHeapSize
The counter would be updated in the centralized `HRegion.incMemStoreSize()` and
`HRegion.decrMemStoreSize()` methods, and `checkResources()` would read this
single atomic value instead of adding two independently read counters.
This approach has several advantages:
* The resource-limit decision becomes atomic.
* It does not restore synchronization around all `ThreadSafeMemStoreSizing`
updates.
* It avoids allocating a new `MemStoreSize` object on every update.
* It does not add extra contention to the per-cell `Segment` accounting path.
* The change can be limited mainly to `HRegion` and a focused regression test.
I would avoid synchronizing the whole `ThreadSafeMemStoreSizing` update/read
path without performance measurements. HBASE-20411 intentionally removed this
synchronization because it was visible in the MemStore write path. An
`AtomicReference<MemStoreSize>` would provide a coherent snapshot, but it would
also introduce allocation and CAS retry overhead on a hot path.
I think the following points would be useful to agree on before implementing
the patch:
1. Do we only require an atomic `heapSize + offHeapSize` value for
resource-limit decisions, or should `getMemStoreSize()` provide a fully
linearizable four-field snapshot?
2. If only the aggregate needs to be atomic, should it live in `HRegion` to
avoid affecting `Segment`, or should it be exposed by
`ThreadSafeMemStoreSizing` for reuse?
3. Should the same atomic aggregate also be used by the region-level
flush-threshold checks that currently consume
`memStoreSizing.getMemStoreSize()`?
For testing, I plan to add a deterministic test covering increment, decrement,
and mixed-sign deltas, and verify that the resource-limit decision always uses
the atomic aggregate. I would prefer this over a scheduling-dependent stress
test that could become flaky.
I would like to take ownership of this issue and prepare the patch/PR. I am
also happy to adjust the implementation based on the discussion above.
> Spurious RegionTooBusyException due to non-atomic update of correlated fields
> heapSize/offHeapSize in ThreadSafeMemStoreSizing
> ------------------------------------------------------------------------------------------------------------------------------
>
> Key: HBASE-30213
> URL: https://issues.apache.org/jira/browse/HBASE-30213
> Project: HBase
> Issue Type: Bug
> Components: regionserver
> Reporter: John Doe
> Priority: Major
>
> A multi-variable concurrency bug in ThreadSafeMemStoreSizing can cause write
> threads to observe a transiently inflated MemStore size and throw a spurious
> RegionTooBusyException immediately after a flush
> completes.ThreadSafeMemStoreSizing maintains two semantically correlated
> AtomicLong fields, heapSize and offHeapSize, whose sum is used by
> HRegion.checkResources() to determine whether incoming writes should be
> rejected. During decrMemStoreSize(), these two fields are decremented by two
> separate addAndGet() calls with no common lock: offHeapSize is decremented
> first (line 59), heapSize second (line 60).
> A concurrent write RPC thread calling getMemStoreSize() reads heapSize first
> and offHeapSize second (line 53). If the read falls between the two
> decrements, it observes the stale pre-flush heapSize combined with the
> already-decremented offHeapSize, producing a sum that overestimates the true
> MemStore size by the full heapSizeDelta of the flush.
> If this inflated sum exceeds blockingMemStoreSize, checkResources()
> incorrectly throws RegionTooBusyException (HRegion.java:5029), even though
> the MemStore is already safely below the threshold.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
