@@ -1294,8 +1294,21 @@ size_t CacheAllocator<CacheTrait>::wakeUpWaitersLocked(folly::StringPiece key,
1294
1294
}
1295
1295
1296
1296
template <typename CacheTrait>
1297
- void CacheAllocator<CacheTrait>::moveRegularItemWithSync(
1297
+ bool CacheAllocator<CacheTrait>::moveRegularItemWithSync(
1298
1298
Item& oldItem, WriteHandle& newItemHdl) {
1299
+ // on function exit - the new item handle is no longer moving
1300
+ // and other threads may access it - but in case where
1301
+ // we failed to replace in access container we can give the
1302
+ // new item back to the allocator
1303
+ auto guard = folly::makeGuard ([&]() {
1304
+ auto ref = newItemHdl->unmarkMoving ();
1305
+ if (UNLIKELY (ref == 0 )) {
1306
+ const auto res =
1307
+ releaseBackToAllocator (*newItemHdl, RemoveContext::kNormal , false );
1308
+ XDCHECK (res == ReleaseRes::kReleased );
1309
+ }
1310
+ });
1311
+
1299
1312
XDCHECK (oldItem.isMoving ());
1300
1313
XDCHECK (!oldItem.isExpired ());
1301
1314
// TODO: should we introduce new latency tracker. E.g. evictRegularLatency_
@@ -1326,6 +1339,22 @@ void CacheAllocator<CacheTrait>::moveRegularItemWithSync(
1326
1339
1327
1340
auto replaced = accessContainer_->replaceIf (oldItem, *newItemHdl,
1328
1341
predicate);
1342
+ // another thread may have called insertOrReplace which could have
1343
+ // marked this item as unaccessible causing the replaceIf
1344
+ // in the access container to fail - in this case we want
1345
+ // to abort the move since the item is no longer valid
1346
+ if (!replaced) {
1347
+ return false ;
1348
+ }
1349
+ // what if another thread calls insertOrReplace now when
1350
+ // the item is moving and already replaced in the hash table?
1351
+ // 1. it succeeds in updating the hash table - so there is
1352
+ // no guarentee that isAccessible() is true
1353
+ // 2. it will then try to remove from MM container
1354
+ // - this operation will wait for newItemHdl to
1355
+ // be unmarkedMoving via the waitContext
1356
+ // 3. replaced handle is returned and eventually drops
1357
+ // ref to 0 and the item is recycled back to allocator.
1329
1358
1330
1359
if (config_.moveCb ) {
1331
1360
// Execute the move callback. We cannot make any guarantees about the
@@ -1367,14 +1396,7 @@ void CacheAllocator<CacheTrait>::moveRegularItemWithSync(
1367
1396
XDCHECK (newItemHdl->hasChainedItem ());
1368
1397
}
1369
1398
newItemHdl.unmarkNascent ();
1370
- auto ref = newItemHdl->unmarkMoving ();
1371
- // remove because there is a chance the new item was not
1372
- // added to the access container
1373
- if (UNLIKELY (ref == 0 )) {
1374
- const auto res =
1375
- releaseBackToAllocator (*newItemHdl, RemoveContext::kNormal , false );
1376
- XDCHECK (res == ReleaseRes::kReleased );
1377
- }
1399
+ return true ;
1378
1400
}
1379
1401
1380
1402
template <typename CacheTrait>
@@ -1756,7 +1778,10 @@ CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
1756
1778
1757
1779
if (newItemHdl) {
1758
1780
XDCHECK_EQ (newItemHdl->getSize (), item.getSize ());
1759
- moveRegularItemWithSync (item, newItemHdl);
1781
+ if (!moveRegularItemWithSync (item, newItemHdl)) {
1782
+ return WriteHandle{};
1783
+ }
1784
+ XDCHECK_EQ (newItemHdl->getKey (),item.getKey ());
1760
1785
item.unmarkMoving ();
1761
1786
return newItemHdl;
1762
1787
} else {
@@ -1795,7 +1820,9 @@ CacheAllocator<CacheTrait>::tryPromoteToNextMemoryTier(
1795
1820
1796
1821
if (newItemHdl) {
1797
1822
XDCHECK_EQ (newItemHdl->getSize (), item.getSize ());
1798
- moveRegularItemWithSync (item, newItemHdl);
1823
+ if (!moveRegularItemWithSync (item, newItemHdl)) {
1824
+ return WriteHandle{};
1825
+ }
1799
1826
item.unmarkMoving ();
1800
1827
return newItemHdl;
1801
1828
} else {
@@ -3148,9 +3175,13 @@ bool CacheAllocator<CacheTrait>::tryMovingForSlabRelease(
3148
3175
// TODO: add support for chained items
3149
3176
return false ;
3150
3177
} else {
3151
- moveRegularItemWithSync (oldItem, newItemHdl);
3178
+ // move can fail if another thread calls insertOrReplace
3179
+ // in this case oldItem is no longer valid (not accessible,
3180
+ // it gets removed from MMContainer and evictForSlabRelease
3181
+ // will send it back to the allocator
3182
+ bool ret = moveRegularItemWithSync (oldItem, newItemHdl);
3152
3183
removeFromMMContainer (oldItem);
3153
- return true ;
3184
+ return ret ;
3154
3185
}
3155
3186
}
3156
3187
}
0 commit comments