This is an automated email from the ASF dual-hosted git repository. mblow pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/asterixdb.git
commit 2edb822b1094fb76282fc5da270dee32794f4f5d Author: Wail Alkowaileet <[email protected]> AuthorDate: Tue Jul 16 15:23:28 2024 -0700 [ASTERIXDB-3461][STO] Guard CachePage pinCount - user model changes: no - storage format changes: no - interface changes: no Details: Guard against decrementing pinCount to a negative number (cherry picked from commit d9fdcabd32) Ext-ref: MB-67050 Change-Id: I85196ec1d99259b43c94d6dc9916a2ae042a9a10 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19884 Tested-by: Jenkins <[email protected]> Reviewed-by: Ali Alsuliman <[email protected]> --- .../storage/common/buffercache/BufferCache.java | 30 +++++++++++----------- .../storage/common/buffercache/CachedPage.java | 14 +++++++++- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java index ea108cbfbb..d7be126860 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java @@ -240,7 +240,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I if (DEBUG) { assert !cPage.confiscated.get(); } - cPage.pinCount.incrementAndGet(); + cPage.incrementAndGetPinCount(); return cPage; } cPage = cPage.next; @@ -297,7 +297,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I // now that we have the pin, ensure the victim's dpid still is < 0, if it's not, decrement // pin count and try again if (victim.dpid >= 0) { - victim.pinCount.decrementAndGet(); + victim.decrementAndGetPinCount(); return null; } if (DEBUG) { @@ -339,7 +339,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I // now that we have the pin, ensure the victim's bucket hasn't changed, if it has, decrement // pin count and try again if (victimHash != hash(victim.dpid)) { - victim.pinCount.decrementAndGet(); + victim.decrementAndGetPinCount(); return null; } if (DEBUG) { @@ -383,7 +383,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I // now that we have the pin, ensure the victim's bucket hasn't changed, if it has, decrement // pin count and try again if (victimHash != hash(victim.dpid)) { - victim.pinCount.decrementAndGet(); + victim.decrementAndGetPinCount(); return null; } if (DEBUG && confiscatedPages.contains(victim)) { @@ -422,8 +422,8 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I private CachedPage findTargetInBucket(long dpid, CachedPage cPage, CachedPage victim) { while (cPage != null) { if (cPage.dpid == dpid) { - cPage.pinCount.incrementAndGet(); - victim.pinCount.decrementAndGet(); + cPage.incrementAndGetPinCount(); + victim.decrementAndGetPinCount(); if (DEBUG) { assert !cPage.confiscated.get(); } @@ -577,7 +577,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I if (closed) { throw new HyracksDataException("unpin called on a closed cache"); } - int pinCount = ((CachedPage) page).pinCount.decrementAndGet(); + int pinCount = ((CachedPage) page).decrementAndGetPinCount(); if (DEBUG && pinCount == 0) { pinnedPageOwner.remove(page); } @@ -664,7 +664,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I } if (cleaned) { cPage.dirty.set(false); - cPage.pinCount.decrementAndGet(); + cPage.decrementAndGetPinCount(); // this increment of a volatile is OK as there is only one writer cleanedCount++; synchronized (cleanNotification) { @@ -889,11 +889,11 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I write(cPage); } cPage.dirty.set(false); - pinCount = cPage.pinCount.decrementAndGet(); + pinCount = cPage.decrementAndGetPinCount(); } else { pinCount = cPage.pinCount.get(); } - if (pinCount > 0) { + if (pinCount != 0) { throw new IllegalStateException("Page " + BufferedFileHandle.getFileId(cPage.dpid) + ":" + BufferedFileHandle.getPageId(cPage.dpid) + " is pinned and file is being closed. Pincount is: " + pinCount + " Page is confiscated: " @@ -1045,7 +1045,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I // now that we have the pin, ensure the victim's dpid still is < 0, if it's not, decrement // pin count and try again if (victim.dpid >= 0) { - victim.pinCount.decrementAndGet(); + victim.decrementAndGetPinCount(); return false; } } else { @@ -1060,7 +1060,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I // now that we have the pin, ensure the victim's bucket hasn't changed, if it has, decrement // pin count and try again if (pageHash != hash(victim.dpid)) { - victim.pinCount.decrementAndGet(); + victim.decrementAndGetPinCount(); return false; } // readjust the next pointers to remove this page from @@ -1162,7 +1162,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I // now that we have the pin, ensure the victim's dpid still is < 0, if it's not, decrement // pin count and try again if (victim.dpid >= 0) { - victim.pinCount.decrementAndGet(); + victim.decrementAndGetPinCount(); return null; } returnPage = victim; @@ -1342,7 +1342,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I cPage.valid = true; cPage.next = bucket.cachedPage; bucket.cachedPage = cPage; - cPage.pinCount.decrementAndGet(); + cPage.decrementAndGetPinCount(); if (DEBUG) { assert cPage.pinCount.get() == 0; assert cPage.latch.getReadLockCount() == 0; @@ -1358,7 +1358,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent, I } } else { cPage.invalidate(); - cPage.pinCount.decrementAndGet(); + cPage.decrementAndGetPinCount(); if (DEBUG) { assert cPage.pinCount.get() == 0; assert cPage.latch.getReadLockCount() == 0; diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java index 1d4c789ad5..c6b10a62c8 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java @@ -60,7 +60,19 @@ public class CachedPage implements ICachedPageInternal { } public int incrementAndGetPinCount() { - return pinCount.incrementAndGet(); + int count = pinCount.incrementAndGet(); + if (count <= 0) { + throw new IllegalStateException("incrementAndGet: Invalid pinCount: " + count + " in page: " + this); + } + return count; + } + + public int decrementAndGetPinCount() { + int count = pinCount.decrementAndGet(); + if (count < 0) { + throw new IllegalStateException("decrementAndGet: Invalid pinCount: " + count + " in page: " + this); + } + return count; } public CachedPage(int cpid, ByteBuffer buffer, IPageReplacementStrategy pageReplacementStrategy) {
