From 49d243b13c658c798430674ee452635d66bb7dc7 Mon Sep 17 00:00:00 2001 From: Alina Djamankulova Date: Tue, 16 Nov 2021 09:53:20 -0800 Subject: DFS block cache: harden against race over ref locks. With change https://git.eclipse.org/r/c/jgit/jgit/+/186455 a thread loading a bitmap index could hold two ref locks at the same time (one for bitmap and one for either index or reverse index). So it is possible that two threads loading bitmaps end up in a deadlock for ref locks e.g. threadA has refLock[1] (for bitmap) and wants refLock[2] (for index or revIndex) and threadB has refLock[2] (for bitmap) and wants refLock[1]. This change introduces separate pools of locks per pack extension instead of a shared pool. So threads loading bitmap can hold two locks but with different extensions and no overlap, e.g. threadA holds refLock[BITMAP_INDEX][1] and refLock[INDEX][2] and threadB holds refLock[BITMAP_INDEX][2] and refLock[INDEX][1]. More unit tests were added to cover various paralell loading scenarios. Signed-off-by: Alina Djamankulova Change-Id: I89704b4721c21548929608d3798ef60925280755 --- .../jgit/internal/storage/dfs/DfsBlockCache.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java index e87bfe24e6..54c527c03c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java @@ -104,9 +104,10 @@ public final class DfsBlockCache { private final ReentrantLock[] loadLocks; /** - * A separate pool of locks to prevent concurrent loads for same index or bitmap from PackFile. + * A separate pool of locks per pack extension to prevent concurrent loads + * for same index or bitmap from PackFile. */ - private final ReentrantLock[] refLocks; + private final ReentrantLock[][] refLocks; /** Maximum number of bytes the cache should hold. */ private final long maxBytes; @@ -173,13 +174,16 @@ public final class DfsBlockCache { } table = new AtomicReferenceArray<>(tableSize); - loadLocks = new ReentrantLock[cfg.getConcurrencyLevel()]; + int concurrencyLevel = cfg.getConcurrencyLevel(); + loadLocks = new ReentrantLock[concurrencyLevel]; for (int i = 0; i < loadLocks.length; i++) { loadLocks[i] = new ReentrantLock(true /* fair */); } - refLocks = new ReentrantLock[cfg.getConcurrencyLevel()]; - for (int i = 0; i < refLocks.length; i++) { - refLocks[i] = new ReentrantLock(true /* fair */); + refLocks = new ReentrantLock[PackExt.values().length][concurrencyLevel]; + for (int i = 0; i < PackExt.values().length; i++) { + for (int j = 0; j < concurrencyLevel; ++j) { + refLocks[i][j] = new ReentrantLock(true /* fair */); + } } maxBytes = cfg.getBlockLimit(); @@ -636,7 +640,8 @@ public final class DfsBlockCache { } private ReentrantLock lockForRef(DfsStreamKey key) { - return refLocks[(key.hash >>> 1) % refLocks.length]; + int slot = (key.hash >>> 1) % refLocks[key.packExtPos].length; + return refLocks[key.packExtPos][slot]; } private static AtomicLong[] newCounters() { -- cgit v1.2.3