diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index ba0ddc7e49..88b956dfac 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -463,9 +463,7 @@ void CacheAllocator::addChainedItem(WriteHandle& parent, // Parent will decrement the refcount upon release. Since this is an // internal refcount, we dont include it in active handle tracking. child->incRef(); - auto ref = child->getRefCount(); - // ref == 3 if child is moving - XDCHECK(ref == 2u || ref == 3); + XDCHECK_EQ(2u, child->getRefCount()); invalidateNvm(*parent); if (auto eventTracker = getEventTracker()) { @@ -555,10 +553,7 @@ void CacheAllocator::transferChainLocked(WriteHandle& parent, ChainedItem* curr = &headHandle->asChainedItem(); const auto newParentPtr = compressor_.compress(newParent.get()); while (curr) { - if (!curr->isMoving()) - XDCHECK_EQ(curr == headHandle.get() ? 2u : 1u, curr->getRefCount()); - else - XDCHECK_EQ(curr == headHandle.get() ? 3u : 2u, curr->getRefCount()); + XDCHECK_EQ(curr == headHandle.get() ? 2u : 1u, curr->getRefCount()); XDCHECK(curr->isInMMContainer()); curr->changeKey(newParentPtr); curr = curr->getNext(compressor_); @@ -654,7 +649,7 @@ CacheAllocator::replaceChainedItemLocked(Item& oldItem, WriteHandle newItemHdl, const Item& parent) { XDCHECK(newItemHdl != nullptr); - XDCHECK_GE(2u, oldItem.getRefCount()); + XDCHECK_GE(1u, oldItem.getRefCount()); // grab the handle to the old item so that we can return this. Also, we need // to drop the refcount the parent holds on oldItem by manually calling @@ -1157,7 +1152,7 @@ bool CacheAllocator::moveRegularItem(Item& oldItem, // no one can add or remove chained items at this point if (oldItem.hasChainedItem()) { - auto oldItemHdl = WriteHandle{&oldItem, *this}; + auto oldItemHdl = acquire(&oldItem); XDCHECK_EQ(1u, oldItemHdl->getRefCount()) << oldItemHdl->toString(); XDCHECK(!newItemHdl->hasChainedItem()) << newItemHdl->toString(); try { @@ -1171,10 +1166,6 @@ bool CacheAllocator::moveRegularItem(Item& oldItem, XDCHECK(!oldItem.hasChainedItem()); XDCHECK(newItemHdl->hasChainedItem()); - - // drop the handle, no need to decRef since we relied on - // item being moved - oldItemHdl.release(); } newItemHdl.unmarkNascent(); return true; diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index 5ab3a3c4f9..79854029a8 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1906,7 +1906,7 @@ class CacheAllocator : public CacheBase { void saveRamCache(); static bool itemSlabMovePredicate(const Item& item) { - return item.isMoving() && item.getRefCount() == 1; + return item.isMoving() && item.getRefCount() == 0; } static bool itemExpiryPredicate(const Item& item) { diff --git a/cachelib/allocator/Refcount.h b/cachelib/allocator/Refcount.h index 8ad05d1d52..22eb82428a 100644 --- a/cachelib/allocator/Refcount.h +++ b/cachelib/allocator/Refcount.h @@ -200,8 +200,18 @@ class FOLLY_PACK_ATTR RefcountWithFlags { } } - // Return refcount excluding control bits and flags - Value getAccessRef() const noexcept { return getRaw() & kAccessRefMask; } + // Return refcount excluding control bits and flags. + Value getAccessRef() const noexcept { + auto raw = getRaw(); + auto accessRef = raw & kAccessRefMask; + + if ((raw & getAdminRef()) && accessRef >= 1) { + // if item is moving, ignore the extra ref + return accessRef - static_cast(1); + } else { + return accessRef; + } + } // Return access ref and the admin ref bits Value getRefWithAccessAndAdmin() const noexcept { @@ -331,6 +341,9 @@ class FOLLY_PACK_ATTR RefcountWithFlags { }; auto newValue = [](const Value curValue) { + // Set exclusive flag and make the ref count non-zero (to distinguish + // from exclusive case). This extra ref will not be reported to the + // user/ return (curValue + static_cast(1)) | getAdminRef(); };