alex-plekhanov commented on code in PR #13554:
URL: https://github.com/apache/ignite/pull/13554#discussion_r4044721322
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java:
##########
@@ -4182,6 +4199,23 @@ private int extrasSize() {
lock.lock();
}
+ /**
+ * Acquires the entry lock either blocking ({@code tryLock == false}) or
non-blockingly with an immediate
+ * {@code tryLock(0)} ({@code tryLock == true}). Used by {@link
#evictInternal} to let size-aware
+ * eviction skip contended entries instead of blocking, avoiding a
lock-ordering deadlock.
+ *
+ * @param tryLock {@code true} to acquire the lock non-blockingly.
+ * @return {@code true} if the lock was acquired (always {@code true} when
{@code tryLock == false}).
+ */
+ private boolean lockEntry(boolean tryLock) {
+ if (tryLock)
+ return !lock.isHeldByCurrentThread() && tryLockEntry(0);
Review Comment:
As far as I understand we forbid lock reentry for tryLock to avoid
self-eviction (not to avoid deadlocks). This behavior looks strange in general
lockEntry method, someone can reuse this method and get unexpected results,
when instead of reentry lock fails. Let's move `f (tryLock &&
lock.isHeldByCurrentThread()) return false; ` to the evictInternal method, with
comment, why we forbid reentry.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -591,6 +610,44 @@ private long allocateDataPage(int part) throws
IgniteCheckedException {
return pageMem.allocatePage(grpId, part, FLAG_DATA);
}
+ /**
+ * @return {@code true} when the region has effectively no headroom left
(allocated pages reached the configured
+ * max), so a fresh {@code allocateDataPage} could no longer grow it.
+ */
+ private boolean regionEffectivelyFull() {
+ return pageMem.loadedPages() >= dataRegion.config().getMaxSize() /
pageMem.systemPageSize();
+ }
+
+ /**
+ * Take a page, and if the free list cannot hand one out, re-run the
size-aware reserve and retry. The reserve only
+ * bounds the shared empty-pages counter and does not pin pages to this
thread, so a concurrent writer may consume
+ * them before this allocation; retrying closes that TOCTOU instead of
falling straight to a raw
+ * {@code allocateDataPage}. How hard to retry depends on whether the
region can still grow: on an effectively-full
+ * region re-reserving is bounded (each attempt itself fails with a clean
OOM when eviction cannot progress), while
+ * with headroom a single re-reserve suffices and the subsequent {@code
allocateDataPage} grows the region.
+ *
+ * @param size Free space required on the page.
+ * @param row Row to write.
+ * @param statHolder Statistics holder to track IO operations.
+ * @return Page identifier or 0 if no page could be obtained after
re-reserving.
+ * @throws IgniteCheckedException If failed.
+ */
+ private long takePageWithReserve(int size, T row, IoStatisticsHolder
statHolder) throws IgniteCheckedException {
+ long pageId = takePage(size, row, statHolder);
+
+ if (pageId == 0L && dbMgr != null) {
Review Comment:
`&& !dataRegion.config().isPersistenceEnabled()`
We can't evict pages for persistent region, so 1-4 cycles with
ensureFreeSpace and takePage are redundant.
The same for disabled eviction. takePage is not free, there is a cycle over
buckets inside. It Worth to check preconditions before next attempt to
takePage. For disabled eviction checking oom threshold. For disabled eviction
check oom threshold on each page is also redundant. We still will fail if
there is an error during allocation, so it's no matter if it happens sooner or
later.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -591,6 +610,44 @@ private long allocateDataPage(int part) throws
IgniteCheckedException {
return pageMem.allocatePage(grpId, part, FLAG_DATA);
}
+ /**
+ * @return {@code true} when the region has effectively no headroom left
(allocated pages reached the configured
+ * max), so a fresh {@code allocateDataPage} could no longer grow it.
+ */
+ private boolean regionEffectivelyFull() {
+ return pageMem.loadedPages() >= dataRegion.config().getMaxSize() /
pageMem.systemPageSize();
Review Comment:
In real case scenarios this condition almost never reachable. All page
memory divided to segments (up to 16), most probably there will be some
unusable space at the end of each segment, so total pages that can be allocated
inside each segment is less than maxSize/pageSize.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IgniteCacheDatabaseSharedManager.java:
##########
@@ -1213,27 +1267,159 @@ public void ensureFreeSpaceForInsert(DataRegion
region, int dataRowSize) throws
// Note that not the whole page can be used to storing links,
// see PagesListNodeIO and PagesListMetaIO#getCapacity(), so we
pessimistically multiply the result on 1.5,
// in any way, the number of required pages is less than 1 percent.
- boolean oomThreshold = (memorySize / pageMem.systemPageSize()) <
+ boolean oomThreshold = (regCfg.getMaxSize() /
pageMem.systemPageSize()) <
((double)dataRowSize / pageMem.pageSize() + nonEmptyPages * (8.0 *
1.5 / pageMem.pageSize() + 1) + 256 /*one page per bucket*/);
- if (oomThreshold) {
- IgniteOutOfMemoryException oom = new
IgniteOutOfMemoryException("Out of memory in data region [" +
- "name=" + regCfg.getName() +
- ", initSize=" + U.readableSize(regCfg.getInitialSize(), false)
+
- ", maxSize=" + U.readableSize(regCfg.getMaxSize(), false) +
- ", persistenceEnabled=" + regCfg.isPersistenceEnabled() + "]
Try the following:" + U.nl() +
- " ^-- Increase maximum off-heap memory size
(DataRegionConfiguration.maxSize)" + U.nl() +
- " ^-- Enable Ignite persistence
(DataRegionConfiguration.persistenceEnabled)" + U.nl() +
- " ^-- Enable eviction or expiration policies"
- );
+ if (oomThreshold)
+ throw outOfMemory(regCfg);
+ }
+
+ /**
+ * Size-aware reserve for an eviction-enabled non-persistent region. Runs
eviction until the free list holds
+ * enough real empty pages to accommodate the row, or throws {@link
IgniteOutOfMemoryException} if the goal is
+ * unreachable / no progress can be made. Progress is measured against the
number of empty pages in the free list
+ * (the only resource a subsequent fragmented write can reliably consume
once the region is effectively full); the
+ * region's spare capacity (headroom) is only trusted in the fast path
while the region is below the eviction
+ * threshold.
+ *
+ * @param region Data region.
+ * @param regCfg Data region configuration.
+ * @param dataRowSize Size of data row to be inserted.
+ * @throws IgniteOutOfMemoryException If the target cannot be reached (row
too large for the region or eviction
+ * makes no progress).
+ * @throws IgniteCheckedException If failed to evict data pages.
+ */
+ private void ensureFreeSpaceForEviction(
+ DataRegion region,
+ DataRegionConfiguration regCfg,
+ int dataRowSize
+ ) throws IgniteOutOfMemoryException, IgniteCheckedException {
+ PageMemory pageMem = region.pageMemory();
+
+ // Maximum payload bytes that a single data page can hold for a
fragmented row.
+ long pagePayload = pageMem.pageSize() -
AbstractDataPageIO.MIN_DATA_PAGE_OVERHEAD;
- if (cctx.kernalContext() != null)
- cctx.kernalContext().failure().process(new
FailureContext(FailureType.CRITICAL_ERROR, oom));
+ // A row that fits into the steady-state empty-pages pool is satisfied
by normal threshold eviction, so the
+ // fast path is a single comparison (no page computation, free-list
lookup or page-memory reads on the hot
+ // small-put path).
+ if (dataRowSize <= regCfg.getEmptyPagesPoolSize() * pagePayload)
Review Comment:
What if there are two concurrent puts with 60 pages each and only 100 pages
left?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -607,6 +664,9 @@ private long allocateDataPage(int part) throws
IgniteCheckedException {
catch (IgniteCheckedException | Error e) {
throw e;
}
+ catch (IgniteOutOfMemoryException e) {
Review Comment:
catch (IgniteCheckedException | Error | IgniteOutOfMemoryException e)
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IgniteCacheDatabaseSharedManager.java:
##########
@@ -1213,27 +1267,159 @@ public void ensureFreeSpaceForInsert(DataRegion
region, int dataRowSize) throws
// Note that not the whole page can be used to storing links,
// see PagesListNodeIO and PagesListMetaIO#getCapacity(), so we
pessimistically multiply the result on 1.5,
// in any way, the number of required pages is less than 1 percent.
- boolean oomThreshold = (memorySize / pageMem.systemPageSize()) <
+ boolean oomThreshold = (regCfg.getMaxSize() /
pageMem.systemPageSize()) <
((double)dataRowSize / pageMem.pageSize() + nonEmptyPages * (8.0 *
1.5 / pageMem.pageSize() + 1) + 256 /*one page per bucket*/);
- if (oomThreshold) {
- IgniteOutOfMemoryException oom = new
IgniteOutOfMemoryException("Out of memory in data region [" +
- "name=" + regCfg.getName() +
- ", initSize=" + U.readableSize(regCfg.getInitialSize(), false)
+
- ", maxSize=" + U.readableSize(regCfg.getMaxSize(), false) +
- ", persistenceEnabled=" + regCfg.isPersistenceEnabled() + "]
Try the following:" + U.nl() +
- " ^-- Increase maximum off-heap memory size
(DataRegionConfiguration.maxSize)" + U.nl() +
- " ^-- Enable Ignite persistence
(DataRegionConfiguration.persistenceEnabled)" + U.nl() +
- " ^-- Enable eviction or expiration policies"
- );
+ if (oomThreshold)
+ throw outOfMemory(regCfg);
+ }
+
+ /**
+ * Size-aware reserve for an eviction-enabled non-persistent region. Runs
eviction until the free list holds
+ * enough real empty pages to accommodate the row, or throws {@link
IgniteOutOfMemoryException} if the goal is
+ * unreachable / no progress can be made. Progress is measured against the
number of empty pages in the free list
+ * (the only resource a subsequent fragmented write can reliably consume
once the region is effectively full); the
+ * region's spare capacity (headroom) is only trusted in the fast path
while the region is below the eviction
+ * threshold.
+ *
+ * @param region Data region.
+ * @param regCfg Data region configuration.
+ * @param dataRowSize Size of data row to be inserted.
+ * @throws IgniteOutOfMemoryException If the target cannot be reached (row
too large for the region or eviction
+ * makes no progress).
+ * @throws IgniteCheckedException If failed to evict data pages.
+ */
+ private void ensureFreeSpaceForEviction(
+ DataRegion region,
+ DataRegionConfiguration regCfg,
+ int dataRowSize
+ ) throws IgniteOutOfMemoryException, IgniteCheckedException {
+ PageMemory pageMem = region.pageMemory();
+
+ // Maximum payload bytes that a single data page can hold for a
fragmented row.
+ long pagePayload = pageMem.pageSize() -
AbstractDataPageIO.MIN_DATA_PAGE_OVERHEAD;
- if (cctx.kernalContext() != null)
- cctx.kernalContext().failure().process(new
FailureContext(FailureType.CRITICAL_ERROR, oom));
+ // A row that fits into the steady-state empty-pages pool is satisfied
by normal threshold eviction, so the
+ // fast path is a single comparison (no page computation, free-list
lookup or page-memory reads on the hot
+ // small-put path).
+ if (dataRowSize <= regCfg.getEmptyPagesPoolSize() * pagePayload)
+ return;
- throw oom;
+ CacheFreeList freeList = freeListMap.get(regCfg.getName());
+
+ if (freeList == null)
+ return;
+
+ long totalPages = regCfg.getMaxSize() / pageMem.systemPageSize();
+
+ // Pages the row will actually occupy once written, and which the free
list must hand out on demand during
+ // the fragmented write.
+ long requiredPages = (dataRowSize + pagePayload - 1) / pagePayload;
+
+ // The row fundamentally cannot fit into the whole region.
+ if (requiredPages > totalPages)
+ throw outOfMemory(regCfg);
+
+ // The reserve must guarantee `requiredPages` REAL empty pages, not
just apparent headroom. Both are shared and
+ // non-exclusive (emptyDataPages() is a snapshot; any writer can
consume them), but once the region is full
+ // (loadedPages == totalPages) headroom can no longer grow it (fresh
allocateDataPage -> raw OOM), while empty
+ // pages in the reuse bucket stay reachable via takePage(). So empty
pages are the only resource the fragmented
+ // write can consume on a full region. The TOCTOU between this reserve
and the actual write is closed by the
+ // lazy re-reserve in AbstractFreeList#writeSinglePage.
+ long emptyPages = freeList.emptyDataPages();
+
+ // The gate reuses evictionThreshold as a regime boundary, not as
"when to start eviction" (evictionRequired()
+ // does that, stopping on emptyPages >= poolSize; no last 10% of page
memory is left unusable). Below the
Review Comment:
Hardcoded 10%
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -701,10 +713,29 @@ private int writeWholePages(T row, IoStatisticsHolder
statHolder) throws IgniteC
* @throws IgniteCheckedException If failed.
*/
private int writeSinglePage(T row, int written, IoStatisticsHolder
statHolder) throws IgniteCheckedException {
+ // TOCTOU closure: the size-aware reserve (ensureFreeSpaceForInsert,
invoked from RowStore.addRow/addRows
+ // before this write) accumulates enough real empty pages but does not
pin them to this thread - a concurrent
+ // writer can consume them between the reserve and this allocation.
When the free list cannot hand out a page,
+ // re-reserve on the remaining size and retry before allocating a
brand-new page; otherwise the race surfaces
+ // as a raw IgniteOutOfMemoryException (wrapped into
CorruptedFreeListException in the batch path).
+ //
+ // The re-reserve is an inline demand-eviction: reached from the
BPlusTree.invoke row-creation closure, it may
+ // re-entrantly remove other entries from the same data tree. That is
safe because the closure runs with no
Review Comment:
Also statement `removes entries with no data-tree page locks held` can be
read as `locks held by removing entries`, but here it means `locks held by
row-creation closure`, please rephrase.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionConcurrentWritesAbstractTest.java:
##########
@@ -0,0 +1,176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.cache.eviction.paged;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_PAGE_SIZE;
+
+/**
+ * Concurrent deadlock test for size-aware page eviction.
+ * <p>
+ * The region is first filled with a large number of small entries (so there
is plenty of evictable page space), then
+ * several threads concurrently insert large rows (larger than the empty-pages
pool). Each large insert goes through
+ * the size-aware reserve and, for the single-row path, eviction under the new
entry lock with the non-blocking
+ * {@code tryLockEntry}. The average data volume is kept within the region
capacity, so eviction frees already-stored
+ * small entries rather than overrunning the free list. The test asserts that
no deadlock occurs (all threads finish
+ * within a global deadline).
+ */
+public abstract class PageEvictionConcurrentWritesAbstractTest extends
GridCommonAbstractTest {
+ /** Off-heap region size. */
+ private static final int SIZE = 256 * 1024 * 1024;
+
+ /** Partition count (kept low so that index-tree structures do not exhaust
the region). */
+ private static final int PARTITIONS = 32;
+
+ /** Large record size (larger than the empty-pages pool so that each write
is size-aware). */
+ private static final int LARGE_RECORD_SIZE = 2 * 1024 * 1024;
+
+ /** Small record size used to pre-fill the region with evictable data. */
+ private static final int SMALL_RECORD_SIZE = 4096;
+
+ /** Empty pages pool size. */
+ private static final int POOL_SIZE = 100;
+
+ /** Number of small pre-fill entries, leaving a buffer that is exceeded by
the total of the large writes, so that
+ * the last of them can only be stored by freeing pages via size-aware
eviction. The large records are small
+ * enough that concurrent size-aware eviction reliably frees the required
pages (no spurious guard OOM). */
+ private static final int SMALL_ENTRIES = 48_000;
+
+ /** Number of writer threads. */
+ private static final int THREADS = 2;
Review Comment:
It's not too concurrent, maybe increase threads count to increase
probability of catching concurrent problems.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -693,6 +756,15 @@ private int writeWholePages(T row, IoStatisticsHolder
statHolder) throws IgniteC
/**
* Take a page and write row on it.
+ * <p>
+ * The page is acquired via {@link #takePageWithReserve}: the size-aware
reserve (RowStore.addRow/addRows) only
Review Comment:
Comment partially intersects with takePageWithReserve javadoc
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IgniteCacheDatabaseSharedManager.java:
##########
@@ -1213,27 +1267,159 @@ public void ensureFreeSpaceForInsert(DataRegion
region, int dataRowSize) throws
// Note that not the whole page can be used to storing links,
// see PagesListNodeIO and PagesListMetaIO#getCapacity(), so we
pessimistically multiply the result on 1.5,
// in any way, the number of required pages is less than 1 percent.
- boolean oomThreshold = (memorySize / pageMem.systemPageSize()) <
+ boolean oomThreshold = (regCfg.getMaxSize() /
pageMem.systemPageSize()) <
((double)dataRowSize / pageMem.pageSize() + nonEmptyPages * (8.0 *
1.5 / pageMem.pageSize() + 1) + 256 /*one page per bucket*/);
- if (oomThreshold) {
- IgniteOutOfMemoryException oom = new
IgniteOutOfMemoryException("Out of memory in data region [" +
- "name=" + regCfg.getName() +
- ", initSize=" + U.readableSize(regCfg.getInitialSize(), false)
+
- ", maxSize=" + U.readableSize(regCfg.getMaxSize(), false) +
- ", persistenceEnabled=" + regCfg.isPersistenceEnabled() + "]
Try the following:" + U.nl() +
- " ^-- Increase maximum off-heap memory size
(DataRegionConfiguration.maxSize)" + U.nl() +
- " ^-- Enable Ignite persistence
(DataRegionConfiguration.persistenceEnabled)" + U.nl() +
- " ^-- Enable eviction or expiration policies"
- );
+ if (oomThreshold)
+ throw outOfMemory(regCfg);
+ }
+
+ /**
+ * Size-aware reserve for an eviction-enabled non-persistent region. Runs
eviction until the free list holds
+ * enough real empty pages to accommodate the row, or throws {@link
IgniteOutOfMemoryException} if the goal is
+ * unreachable / no progress can be made. Progress is measured against the
number of empty pages in the free list
+ * (the only resource a subsequent fragmented write can reliably consume
once the region is effectively full); the
+ * region's spare capacity (headroom) is only trusted in the fast path
while the region is below the eviction
+ * threshold.
+ *
+ * @param region Data region.
+ * @param regCfg Data region configuration.
+ * @param dataRowSize Size of data row to be inserted.
+ * @throws IgniteOutOfMemoryException If the target cannot be reached (row
too large for the region or eviction
+ * makes no progress).
+ * @throws IgniteCheckedException If failed to evict data pages.
+ */
+ private void ensureFreeSpaceForEviction(
+ DataRegion region,
+ DataRegionConfiguration regCfg,
+ int dataRowSize
+ ) throws IgniteOutOfMemoryException, IgniteCheckedException {
+ PageMemory pageMem = region.pageMemory();
+
+ // Maximum payload bytes that a single data page can hold for a
fragmented row.
+ long pagePayload = pageMem.pageSize() -
AbstractDataPageIO.MIN_DATA_PAGE_OVERHEAD;
- if (cctx.kernalContext() != null)
- cctx.kernalContext().failure().process(new
FailureContext(FailureType.CRITICAL_ERROR, oom));
+ // A row that fits into the steady-state empty-pages pool is satisfied
by normal threshold eviction, so the
+ // fast path is a single comparison (no page computation, free-list
lookup or page-memory reads on the hot
+ // small-put path).
+ if (dataRowSize <= regCfg.getEmptyPagesPoolSize() * pagePayload)
+ return;
- throw oom;
+ CacheFreeList freeList = freeListMap.get(regCfg.getName());
+
+ if (freeList == null)
+ return;
+
+ long totalPages = regCfg.getMaxSize() / pageMem.systemPageSize();
+
+ // Pages the row will actually occupy once written, and which the free
list must hand out on demand during
+ // the fragmented write.
+ long requiredPages = (dataRowSize + pagePayload - 1) / pagePayload;
+
+ // The row fundamentally cannot fit into the whole region.
+ if (requiredPages > totalPages)
+ throw outOfMemory(regCfg);
+
+ // The reserve must guarantee `requiredPages` REAL empty pages, not
just apparent headroom. Both are shared and
+ // non-exclusive (emptyDataPages() is a snapshot; any writer can
consume them), but once the region is full
+ // (loadedPages == totalPages) headroom can no longer grow it (fresh
allocateDataPage -> raw OOM), while empty
+ // pages in the reuse bucket stay reachable via takePage(). So empty
pages are the only resource the fragmented
+ // write can consume on a full region. The TOCTOU between this reserve
and the actual write is closed by the
+ // lazy re-reserve in AbstractFreeList#writeSinglePage.
+ long emptyPages = freeList.emptyDataPages();
+
+ // The gate reuses evictionThreshold as a regime boundary, not as
"when to start eviction" (evictionRequired()
+ // does that, stopping on emptyPages >= poolSize; no last 10% of page
memory is left unusable). Below the
+ // threshold the region has real slack, so a row fitting into the
combined spare space is satisfied without
+ // eviction (live, e.g. short-TTL, entries are not evicted just to
accumulate empty pages). At/above it headroom
+ // is no longer trustworthy (concurrent writers could commit the same
headroom - TOCTOU), so only real empty
+ // pages are counted and eviction is driven below.
+ boolean evictionRegime = pageMem.loadedPages() >= (long)(totalPages *
regCfg.getEvictionThreshold());
Review Comment:
Still don't understand how it works.
Suppose we have 10Gb total page memory, 1Gb not allocated and no empty
pages. We are trying to insert a 500Mb row. Here we have evictionRegime = true,
in this case we only check emptyPages >= requiredPages (false) before eviction
and stop to evict only when emptyPages >= requiredPages. So 500Mb will be
evicted and we came to the same state: 1Gb is not allocated and no empty pages.
How can we reuse top 10% of page memory in this case?
Perhaps we can use regCfg.getEvictionThreshold as fast path (never use
eviction if we are below this point), but not as "rely only on empty pages
after this point" flag.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionConcurrentWritesAbstractTest.java:
##########
@@ -0,0 +1,176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.cache.eviction.paged;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_PAGE_SIZE;
+
+/**
+ * Concurrent deadlock test for size-aware page eviction.
+ * <p>
+ * The region is first filled with a large number of small entries (so there
is plenty of evictable page space), then
+ * several threads concurrently insert large rows (larger than the empty-pages
pool). Each large insert goes through
+ * the size-aware reserve and, for the single-row path, eviction under the new
entry lock with the non-blocking
+ * {@code tryLockEntry}. The average data volume is kept within the region
capacity, so eviction frees already-stored
+ * small entries rather than overrunning the free list. The test asserts that
no deadlock occurs (all threads finish
+ * within a global deadline).
+ */
+public abstract class PageEvictionConcurrentWritesAbstractTest extends
GridCommonAbstractTest {
+ /** Off-heap region size. */
+ private static final int SIZE = 256 * 1024 * 1024;
+
+ /** Partition count (kept low so that index-tree structures do not exhaust
the region). */
+ private static final int PARTITIONS = 32;
+
+ /** Large record size (larger than the empty-pages pool so that each write
is size-aware). */
+ private static final int LARGE_RECORD_SIZE = 2 * 1024 * 1024;
+
+ /** Small record size used to pre-fill the region with evictable data. */
+ private static final int SMALL_RECORD_SIZE = 4096;
+
+ /** Empty pages pool size. */
+ private static final int POOL_SIZE = 100;
+
+ /** Number of small pre-fill entries, leaving a buffer that is exceeded by
the total of the large writes, so that
+ * the last of them can only be stored by freeing pages via size-aware
eviction. The large records are small
+ * enough that concurrent size-aware eviction reliably frees the required
pages (no spurious guard OOM). */
+ private static final int SMALL_ENTRIES = 48_000;
+
+ /** Number of writer threads. */
+ private static final int THREADS = 2;
+
+ /** Large rows inserted per thread. Their total (threads x rows) exceeds
the buffer left by the pre-fill, so the
+ * last large writes overflow the region and require size-aware eviction
to free small entry pages. */
+ private static final int LARGE_ROWS_PER_THREAD = 20;
+
+ /** Global deadline for the whole test (protects against a
deadlock/busy-spin hang). */
+ private static final long DEADLINE = TimeUnit.MINUTES.toMillis(3);
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String gridName)
throws Exception {
+ return super.getConfiguration(gridName)
+ .setDataStorageConfiguration(new DataStorageConfiguration()
+ .setDefaultDataRegionConfiguration(new
DataRegionConfiguration()
+ .setInitialSize(SIZE)
+ .setMaxSize(SIZE)
+ .setEmptyPagesPoolSize(POOL_SIZE))
+ .setPageSize(DFLT_PAGE_SIZE));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+ }
+
+ /**
+ * @param ignite Ignite node.
+ * @return Cache with a small partition count (reduces structural page
overhead).
+ */
+ private IgniteCache<Integer, Object> createCache(IgniteEx ignite) {
+ return ignite.createCache(new CacheConfiguration<Integer,
Object>(DEFAULT_CACHE_NAME)
+ .setAffinity(new RendezvousAffinityFunction(false, PARTITIONS)));
+ }
+
+ /**
+ * Concurrent large inserts into a region pre-filled with small entries
must complete within the deadline without
+ * deadlock, and without corrupting the free list (eviction frees small
entries rather than overrunning the region).
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testConcurrentLargeWritesNoDeadlock() throws Exception {
+ IgniteEx ignite = startGrid(1);
+
+ IgniteCache<Integer, Object> cache = createCache(ignite);
+
+ // Pre-fill the region with many small entries so that eviction always
has evictable pages to free.
+ for (int i = 0; i < SMALL_ENTRIES; i++)
+ cache.put(i, new byte[SMALL_RECORD_SIZE]);
+
+ byte[] largeVal = new byte[LARGE_RECORD_SIZE];
+
+ AtomicLong errors = new AtomicLong();
+
+ AtomicReference<Throwable> firstErr = new AtomicReference<>();
+
+ CountDownLatch startLatch = new CountDownLatch(1);
+
+ long deadline = System.currentTimeMillis() + DEADLINE;
+
+ Thread[] threads = new Thread[THREADS];
+
+ for (int i = 0; i < THREADS; i++) {
+ final int threadIdx = i;
+
+ threads[i] = new Thread(() -> {
+ try {
+ startLatch.await();
+
+ for (int k = 0; k < LARGE_ROWS_PER_THREAD; k++)
+ cache.put(SMALL_ENTRIES + threadIdx *
LARGE_ROWS_PER_THREAD + k, largeVal);
+ }
+ catch (Throwable e) {
+ errors.incrementAndGet();
+
+ firstErr.compareAndSet(null, e);
+
+ log.error("Unexpected error in writer thread", e);
+ }
+ }, "paged-writer-" + i);
+
+ threads[i].start();
+ }
+
+ startLatch.countDown();
+
+ long start = System.currentTimeMillis();
+
+ for (Thread t : threads)
+ t.join(Math.max(1, deadline - System.currentTimeMillis()));
+
+ // The core assertion of this deadlock test: every writer must have
completed (no thread is stuck waiting on
+ // an entry lock held by size-aware eviction running under another
entry lock).
+ for (Thread t : threads) {
+ if (t.isAlive()) {
+ log.error("Writer thread " + t.getName() + " is still alive
after " +
+ (System.currentTimeMillis() - start) + "ms, state=" +
t.getState());
+
+ for (StackTraceElement frame : t.getStackTrace())
+ log.error(" at " + frame);
+ }
+ }
+
+ for (Thread t : threads)
+ assertFalse("Writer thread " + t.getName() + " did not finish
(possible deadlock)", t.isAlive());
+
+ assertEquals("Writer threads reported errors, reason: " +
firstErr.get(), 0, errors.get());
Review Comment:
Why all this can't be replaced with:
```
IgniteInternalFuture<?> fut = GridTestUtils.runMultiThreadedAsync(()
-> {
U.awaitQuiet(startLatch);
for (int k = 0; k < LARGE_ROWS_PER_THREAD; k++)
cache.put(..., largeVal);
},
THREADS, "paged-writer");
startLatch.countDown();
fut.get(DEADLINE);
```
?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -701,10 +713,29 @@ private int writeWholePages(T row, IoStatisticsHolder
statHolder) throws IgniteC
* @throws IgniteCheckedException If failed.
*/
private int writeSinglePage(T row, int written, IoStatisticsHolder
statHolder) throws IgniteCheckedException {
+ // TOCTOU closure: the size-aware reserve (ensureFreeSpaceForInsert,
invoked from RowStore.addRow/addRows
+ // before this write) accumulates enough real empty pages but does not
pin them to this thread - a concurrent
+ // writer can consume them between the reserve and this allocation.
When the free list cannot hand out a page,
+ // re-reserve on the remaining size and retry before allocating a
brand-new page; otherwise the race surfaces
+ // as a raw IgniteOutOfMemoryException (wrapped into
CorruptedFreeListException in the batch path).
+ //
+ // The re-reserve is an inline demand-eviction: reached from the
BPlusTree.invoke row-creation closure, it may
+ // re-entrantly remove other entries from the same data tree. That is
safe because the closure runs with no
Review Comment:
Also statement `removes entries with no data-tree page locks held` can be
read as `locks held by removing entries`, but here it means `locks held by
row-creation closure`, please rephrase.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/RowStore.java:
##########
@@ -132,8 +133,25 @@ public void addRow(CacheDataRow row, IoStatisticsHolder
statHolder) throws Ignit
* @param statHolder Statistics holder to track IO operations.
* @throws IgniteCheckedException If failed.
*/
- public void addRows(Collection<? extends CacheDataRow> rows,
- IoStatisticsHolder statHolder) throws IgniteCheckedException {
+ public void addRows(Collection<? extends CacheDataRow> rows,
IoStatisticsHolder statHolder) throws IgniteCheckedException {
Review Comment:
But why can't we prereserve the whole size and not rely to per-page
reservation? What the profit of max entry size reservation?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -701,10 +713,29 @@ private int writeWholePages(T row, IoStatisticsHolder
statHolder) throws IgniteC
* @throws IgniteCheckedException If failed.
*/
private int writeSinglePage(T row, int written, IoStatisticsHolder
statHolder) throws IgniteCheckedException {
+ // TOCTOU closure: the size-aware reserve (ensureFreeSpaceForInsert,
invoked from RowStore.addRow/addRows
+ // before this write) accumulates enough real empty pages but does not
pin them to this thread - a concurrent
+ // writer can consume them between the reserve and this allocation.
When the free list cannot hand out a page,
+ // re-reserve on the remaining size and retry before allocating a
brand-new page; otherwise the race surfaces
+ // as a raw IgniteOutOfMemoryException (wrapped into
CorruptedFreeListException in the batch path).
+ //
+ // The re-reserve is an inline demand-eviction: reached from the
BPlusTree.invoke row-creation closure, it may
+ // re-entrantly remove other entries from the same data tree. That is
safe because the closure runs with no
Review Comment:
Looks like you are right about the cache data tree lock. But if lock is not
held during invokeClosure, then pendingTree lock is also can't lead to
deadlocks, so new comment is not correct. New eviction mechanism removes
entries from cache data tree (with write lock-unlock), and after that removes
pending tree entry (under write lock-unlock), and removes data entry (under
write lock-unlock). If invokeClosure is not holds the lock, there are no nested
locks and TTL path is safe too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
