>From Michael Blow <[email protected]>: Michael Blow has submitted this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19884 )
Change subject: [ASTERIXDB-3461][STO] Guard CachePage pinCount ...................................................................... [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]> --- M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java 2 files changed, 51 insertions(+), 16 deletions(-) Approvals: Ali Alsuliman: Looks good to me, approved Jenkins: Verified 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 ea108cb..d7be126 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 @@ if (DEBUG) { assert !cPage.confiscated.get(); } - cPage.pinCount.incrementAndGet(); + cPage.incrementAndGetPinCount(); return cPage; } cPage = cPage.next; @@ -297,7 +297,7 @@ // 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 @@ // 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 @@ // 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 @@ 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 @@ 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 @@ } 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 @@ 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 @@ // 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 @@ // 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 @@ // 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 @@ 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 @@ } } 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 1d4c789..c6b10a6 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 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) { -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19884 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: neo Gerrit-Change-Id: I85196ec1d99259b43c94d6dc9916a2ae042a9a10 Gerrit-Change-Number: 19884 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Blow <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-CC: Wail Alkowaileet <[email protected]> Gerrit-MessageType: merged
