Index: src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java (working copy) @@ -21,7 +21,6 @@ import java.io.IOException; import java.nio.ByteBuffer; -import java.util.concurrent.atomic.AtomicLong; import org.apache.hadoop.fs.FSDataInputStream; import org.apache.hadoop.fs.Path; @@ -90,10 +89,6 @@ /** Block cache configuration. */ protected final CacheConfig cacheConf; - protected AtomicLong cacheHits = new AtomicLong(); - protected AtomicLong blockLoads = new AtomicLong(); - protected AtomicLong metaLoads = new AtomicLong(); - /** Path of file */ protected final Path path; Index: src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java (working copy) @@ -48,9 +48,12 @@ * Fetch block from cache. * @param cacheKey Block to fetch. * @param caching Whether this request has caching enabled (used for stats) + * @param repeat Whether this is a repeat lookup for the same block + * (used to avoid double counting cache misses when doing double-check locking) + * {@see HFileReaderV2#readBlock(long, long, boolean, boolean, boolean, BlockType)} * @return Block or null if block is not in 2 cache. */ - public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching); + public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat); /** * Evict block from cache. Index: src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java (working copy) @@ -90,14 +90,14 @@ } @Override - public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching) { + public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat) { Cacheable cachedBlock; - if ((cachedBlock = onHeapCache.getBlock(cacheKey, caching)) != null) { + if ((cachedBlock = onHeapCache.getBlock(cacheKey, caching, repeat)) != null) { stats.hit(caching); return cachedBlock; - } else if ((cachedBlock = offHeapCache.getBlock(cacheKey, caching)) != null) { + } else if ((cachedBlock = offHeapCache.getBlock(cacheKey, caching, repeat)) != null) { if (caching) { onHeapCache.cacheBlock(cacheKey, cachedBlock); } @@ -105,7 +105,7 @@ return cachedBlock; } - stats.miss(caching); + if (!repeat) stats.miss(caching); return null; } Index: src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java (working copy) @@ -225,15 +225,12 @@ // Per meta key from any given file, synchronize reads for said block synchronized (metaBlockIndexReader.getRootBlockKey(block)) { - metaLoads.incrementAndGet(); - // Check cache for block. If found return. if (cacheConf.isBlockCacheEnabled()) { HFileBlock cachedBlock = (HFileBlock) cacheConf.getBlockCache().getBlock(cacheKey, - cacheConf.shouldCacheBlockOnRead(effectiveCategory)); + cacheConf.shouldCacheBlockOnRead(effectiveCategory), false); if (cachedBlock != null) { - cacheHits.incrementAndGet(); getSchemaMetrics().updateOnCacheHit(effectiveCategory, SchemaMetrics.NO_COMPACTION); return cachedBlock.getBufferWithoutHeader(); @@ -290,15 +287,12 @@ // the other choice is to duplicate work (which the cache would prevent you // from doing). synchronized (dataBlockIndexReader.getRootBlockKey(block)) { - blockLoads.incrementAndGet(); - // Check cache for block. If found return. if (cacheConf.isBlockCacheEnabled()) { HFileBlock cachedBlock = (HFileBlock) cacheConf.getBlockCache().getBlock(cacheKey, - cacheConf.shouldCacheDataOnRead()); + cacheConf.shouldCacheDataOnRead(), false); if (cachedBlock != null) { - cacheHits.incrementAndGet(); getSchemaMetrics().updateOnCacheHit( cachedBlock.getBlockType().getCategory(), isCompaction); return cachedBlock.getBufferWithoutHeader(); Index: src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java (working copy) @@ -210,8 +210,6 @@ // is OK to do for meta blocks because the meta block index is always // single-level. synchronized (metaBlockIndexReader.getRootBlockKey(block)) { - metaLoads.incrementAndGet(); - // Check cache for block. If found return. long metaBlockOffset = metaBlockIndexReader.getRootBlockOffset(block); BlockCacheKey cacheKey = new BlockCacheKey(name, metaBlockOffset, @@ -220,11 +218,10 @@ cacheBlock &= cacheConf.shouldCacheDataOnRead(); if (cacheConf.isBlockCacheEnabled()) { HFileBlock cachedBlock = - (HFileBlock) cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock); + (HFileBlock) cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock, false); if (cachedBlock != null) { // Return a distinct 'shallow copy' of the block, // so pos does not get messed by the scanner - cacheHits.incrementAndGet(); getSchemaMetrics().updateOnCacheHit(BlockCategory.META, false); return cachedBlock.getBufferWithoutHeader(); } @@ -288,69 +285,85 @@ new BlockCacheKey(name, dataBlockOffset, dataBlockEncoder.getEffectiveEncodingInCache(isCompaction), expectedBlockType); - IdLock.Entry lockEntry = offsetLock.getLockEntry(dataBlockOffset); + + boolean useLock = false; + IdLock.Entry lockEntry = null; + try { - blockLoads.incrementAndGet(); + while (true) { - // Check cache for block. If found return. - if (cacheConf.isBlockCacheEnabled()) { - HFileBlock cachedBlock = (HFileBlock) - cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock); - if (cachedBlock != null) { - BlockCategory blockCategory = - cachedBlock.getBlockType().getCategory(); - cacheHits.incrementAndGet(); + if (useLock) { + lockEntry = offsetLock.getLockEntry(dataBlockOffset); + } - getSchemaMetrics().updateOnCacheHit(blockCategory, isCompaction); + // Check cache for block. If found return. + if (cacheConf.isBlockCacheEnabled()) { + // Try and get the block from the block cache. If the useLock variable is true then this + // is the second time through the loop and it should not be counted as a block cache miss. + HFileBlock cachedBlock = (HFileBlock) + cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock, useLock); + if (cachedBlock != null) { + BlockCategory blockCategory = + cachedBlock.getBlockType().getCategory(); - if (cachedBlock.getBlockType() == BlockType.DATA) { - HFile.dataBlockReadCnt.incrementAndGet(); - } + getSchemaMetrics().updateOnCacheHit(blockCategory, isCompaction); - validateBlockType(cachedBlock, expectedBlockType); + if (cachedBlock.getBlockType() == BlockType.DATA) { + HFile.dataBlockReadCnt.incrementAndGet(); + } - // Validate encoding type for encoded blocks. We include encoding - // type in the cache key, and we expect it to match on a cache hit. - if (cachedBlock.getBlockType() == BlockType.ENCODED_DATA && - cachedBlock.getDataBlockEncoding() != - dataBlockEncoder.getEncodingInCache()) { - throw new IOException("Cached block under key " + cacheKey + " " + - "has wrong encoding: " + cachedBlock.getDataBlockEncoding() + - " (expected: " + dataBlockEncoder.getEncodingInCache() + ")"); + validateBlockType(cachedBlock, expectedBlockType); + + // Validate encoding type for encoded blocks. We include encoding + // type in the cache key, and we expect it to match on a cache hit. + if (cachedBlock.getBlockType() == BlockType.ENCODED_DATA && + cachedBlock.getDataBlockEncoding() != + dataBlockEncoder.getEncodingInCache()) { + throw new IOException("Cached block under key " + cacheKey + " " + + "has wrong encoding: " + cachedBlock.getDataBlockEncoding() + + " (expected: " + dataBlockEncoder.getEncodingInCache() + ")"); + } + return cachedBlock; } - return cachedBlock; + // Carry on, please load. } - // Carry on, please load. - } + if (!useLock) { + // check cache again with lock + useLock = true; + continue; + } - // Load block from filesystem. - long startTimeNs = System.nanoTime(); - HFileBlock hfileBlock = fsBlockReader.readBlockData(dataBlockOffset, - onDiskBlockSize, -1, pread); - hfileBlock = dataBlockEncoder.diskToCacheFormat(hfileBlock, - isCompaction); - validateBlockType(hfileBlock, expectedBlockType); - passSchemaMetricsTo(hfileBlock); - BlockCategory blockCategory = hfileBlock.getBlockType().getCategory(); + // Load block from filesystem. + long startTimeNs = System.nanoTime(); + HFileBlock hfileBlock = fsBlockReader.readBlockData(dataBlockOffset, + onDiskBlockSize, -1, pread); + hfileBlock = dataBlockEncoder.diskToCacheFormat(hfileBlock, + isCompaction); + validateBlockType(hfileBlock, expectedBlockType); + passSchemaMetricsTo(hfileBlock); + BlockCategory blockCategory = hfileBlock.getBlockType().getCategory(); - final long delta = System.nanoTime() - startTimeNs; - HFile.offerReadLatency(delta, pread); - getSchemaMetrics().updateOnCacheMiss(blockCategory, isCompaction, delta); + final long delta = System.nanoTime() - startTimeNs; + HFile.offerReadLatency(delta, pread); + getSchemaMetrics().updateOnCacheMiss(blockCategory, isCompaction, delta); - // Cache the block if necessary - if (cacheBlock && cacheConf.shouldCacheBlockOnRead( - hfileBlock.getBlockType().getCategory())) { - cacheConf.getBlockCache().cacheBlock(cacheKey, hfileBlock, - cacheConf.isInMemory()); - } + // Cache the block if necessary + if (cacheBlock && cacheConf.shouldCacheBlockOnRead( + hfileBlock.getBlockType().getCategory())) { + cacheConf.getBlockCache().cacheBlock(cacheKey, hfileBlock, + cacheConf.isInMemory()); + } - if (hfileBlock.getBlockType() == BlockType.DATA) { - HFile.dataBlockReadCnt.incrementAndGet(); + if (hfileBlock.getBlockType() == BlockType.DATA) { + HFile.dataBlockReadCnt.incrementAndGet(); + } + + return hfileBlock; } - - return hfileBlock; } finally { - offsetLock.releaseLockEntry(lockEntry); + if (lockEntry != null) { + offsetLock.releaseLockEntry(lockEntry); + } } } Index: src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java (working copy) @@ -327,13 +327,16 @@ * Get the buffer of the block with the specified name. * @param cacheKey block's cache key * @param caching true if the caller caches blocks on cache misses + * @param repeat Whether this is a repeat lookup for the same block + * (used to avoid double counting cache misses when doing double-check locking) + * {@see HFileReaderV2#readBlock(long, long, boolean, boolean, boolean, BlockType)} * @return buffer of specified cache key, or null if not in cache */ @Override - public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching) { + public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat) { CachedBlock cb = map.get(cacheKey); if(cb == null) { - stats.miss(caching); + if (!repeat) stats.miss(caching); return null; } stats.hit(caching); Index: src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java (working copy) @@ -68,7 +68,7 @@ return cache.size(); } - public synchronized Cacheable getBlock(BlockCacheKey cacheKey, boolean caching) { + public synchronized Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat) { processQueue(); // clear out some crap. Ref ref = cache.get(cacheKey); if (ref == null) Index: src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java (working copy) @@ -152,10 +152,10 @@ } @Override - public Cacheable getBlock(BlockCacheKey key, boolean caching) { + public Cacheable getBlock(BlockCacheKey key, boolean caching, boolean repeat) { CacheablePair contentBlock = backingMap.get(key); if (contentBlock == null) { - stats.miss(caching); + if (!repeat) stats.miss(caching); return null; } Index: src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java (revision 1408615) +++ src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java (working copy) @@ -228,24 +228,25 @@ /** * Get the buffer of the block with the specified name. + * @param caching + * @param key + * @param repeat * - * @param key - * @param caching * @return buffer of specified block name, or null if not in cache */ - public Cacheable getBlock(BlockCacheKey key, boolean caching) { + public Cacheable getBlock(BlockCacheKey key, boolean caching, boolean repeat) { SingleSizeCache cachedBlock = backingStore.get(key); if (cachedBlock == null) { - stats.miss(caching); + if (!repeat) stats.miss(caching); return null; } - Cacheable contentBlock = cachedBlock.getBlock(key, caching); + Cacheable contentBlock = cachedBlock.getBlock(key, caching, false); if (contentBlock != null) { stats.hit(caching); } else { - stats.miss(caching); + if (!repeat) stats.miss(caching); } return contentBlock; } Index: src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java =================================================================== --- src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java (revision 1408615) +++ src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java (working copy) @@ -92,12 +92,12 @@ } toBeTested.cacheBlock(ourBlock.blockName, ourBlock.block); Cacheable retrievedBlock = toBeTested.getBlock(ourBlock.blockName, - false); + false, false); if (retrievedBlock != null) { assertEquals(ourBlock.block, retrievedBlock); toBeTested.evictBlock(ourBlock.blockName); hits.incrementAndGet(); - assertNull(toBeTested.getBlock(ourBlock.blockName, false)); + assertNull(toBeTested.getBlock(ourBlock.blockName, false, false)); } else { miss.incrementAndGet(); } @@ -125,7 +125,7 @@ HFileBlockPair[] blocks = generateHFileBlocks(numBlocks, blockSize); // Confirm empty for (HFileBlockPair block : blocks) { - assertNull(toBeTested.getBlock(block.blockName, true)); + assertNull(toBeTested.getBlock(block.blockName, true, false)); } // Add blocks @@ -138,7 +138,7 @@ // MapMaker makes no guarantees when it will evict, so neither can we. for (HFileBlockPair block : blocks) { - HFileBlock buf = (HFileBlock) toBeTested.getBlock(block.blockName, true); + HFileBlock buf = (HFileBlock) toBeTested.getBlock(block.blockName, true, false); if (buf != null) { assertEquals(block.block, buf); } @@ -149,7 +149,7 @@ for (HFileBlockPair block : blocks) { try { - if (toBeTested.getBlock(block.blockName, true) != null) { + if (toBeTested.getBlock(block.blockName, true, false) != null) { toBeTested.cacheBlock(block.blockName, block.block); fail("Cache should not allow re-caching a block"); } @@ -179,7 +179,7 @@ @Override public void doAnAction() throws Exception { ByteArrayCacheable returned = (ByteArrayCacheable) toBeTested - .getBlock(key, false); + .getBlock(key, false, false); assertArrayEquals(buf, returned.buf); totalQueries.incrementAndGet(); } @@ -218,7 +218,7 @@ final ByteArrayCacheable bac = new ByteArrayCacheable(buf); ByteArrayCacheable gotBack = (ByteArrayCacheable) toBeTested - .getBlock(key, true); + .getBlock(key, true, false); if (gotBack != null) { assertArrayEquals(gotBack.buf, bac.buf); } else { Index: src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java =================================================================== --- src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java (revision 1408615) +++ src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java (working copy) @@ -240,7 +240,7 @@ false, null); BlockCacheKey blockCacheKey = new BlockCacheKey(reader.getName(), offset, encodingInCache, block.getBlockType()); - boolean isCached = blockCache.getBlock(blockCacheKey, true) != null; + boolean isCached = blockCache.getBlock(blockCacheKey, true, false) != null; boolean shouldBeCached = cowType.shouldBeCached(block.getBlockType()); if (shouldBeCached != isCached) { throw new AssertionError( Index: src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java =================================================================== --- src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java (revision 1408615) +++ src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java (working copy) @@ -98,7 +98,7 @@ BlockCacheKey cacheKey = new BlockCacheKey("test", 0); blockCache.cacheBlock(cacheKey, cacheBlock); - HeapSize heapSize = blockCache.getBlock(cacheKey, false); + HeapSize heapSize = blockCache.getBlock(cacheKey, false, false); assertTrue(heapSize instanceof HFileBlock); HFileBlock returnedBlock = (HFileBlock) heapSize;; Index: src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java =================================================================== --- src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java (revision 1408615) +++ src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java (working copy) @@ -116,7 +116,7 @@ // Confirm empty for (CachedItem block : blocks) { - assertTrue(cache.getBlock(block.cacheKey, true) == null); + assertTrue(cache.getBlock(block.cacheKey, true, false) == null); } // Add blocks @@ -130,7 +130,7 @@ // Check if all blocks are properly cached and retrieved for (CachedItem block : blocks) { - HeapSize buf = cache.getBlock(block.cacheKey, true); + HeapSize buf = cache.getBlock(block.cacheKey, true, false); assertTrue(buf != null); assertEquals(buf.heapSize(), block.heapSize()); } @@ -150,7 +150,7 @@ // Check if all blocks are properly cached and retrieved for (CachedItem block : blocks) { - HeapSize buf = cache.getBlock(block.cacheKey, true); + HeapSize buf = cache.getBlock(block.cacheKey, true, false); assertTrue(buf != null); assertEquals(buf.heapSize(), block.heapSize()); } @@ -195,10 +195,10 @@ (maxSize * LruBlockCache.DEFAULT_ACCEPTABLE_FACTOR)); // All blocks except block 0 and 1 should be in the cache - assertTrue(cache.getBlock(blocks[0].cacheKey, true) == null); - assertTrue(cache.getBlock(blocks[1].cacheKey, true) == null); + assertTrue(cache.getBlock(blocks[0].cacheKey, true, false) == null); + assertTrue(cache.getBlock(blocks[1].cacheKey, true, false) == null); for(int i=2;i