diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java index 5fceea2a5f9be..7f703d135cb4f 100644 --- a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java +++ b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java @@ -28,7 +28,7 @@ import java.nio.file.StandardOpenOption; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantLock; import java.util.concurrent.locks.StampedLock; import java.util.function.Consumer; import java.util.function.LongSupplier; @@ -249,12 +249,24 @@ public void startupState( boolean clean ) private volatile boolean changesSinceLastCheckpoint; /** - * Check-pointing flushes updates to stable storage. - * There's a critical section in check-pointing where, in order to guarantee a consistent check-pointed state - * on stable storage, no writes are allowed to happen. - * For this reason both writer and check-pointing acquires this lock. + * There are a few different scenarios that involve writing or flushing that can not be happen concurrently: + * + * This lock is acquired as part of all of those tasks. + */ + private final Lock writerLock = new ReentrantLock(); + + /** + * If cleaning of crash pointers is needed the tree can not be allowed to perform a checkpoint until that job + * has finished. Therefore this lock is acquired by checkpoint and when cleaning is needed. + *

+ * The cleaner job is responsible for releasing this lock after it has finished. This job will be executed by + * some other thread, that's why this lock is a {@link StampedLock}. */ - private final ReadWriteLock writerCheckpointMutex = new StampedLock().asReadWriteLock(); + private final StampedLock cleanerLock = new StampedLock(); /** * Page size, i.e. tree node size, of the tree nodes in this tree. The page size is determined on @@ -759,8 +771,8 @@ private void checkpoint( IOLimiter ioLimiter, Header.Writer headerWriter ) throw // Block writers, or if there's a current writer then wait for it to complete and then block // From this point and till the lock is released we know that the tree won't change. - Lock writeLock = writerCheckpointMutex.writeLock(); - writeLock.lock(); + long stamp = cleanerLock.writeLock(); + writerLock.lock(); try { // Flush dirty pages since that last flush above. This should be a very small set of pages @@ -787,7 +799,8 @@ private void checkpoint( IOLimiter ioLimiter, Header.Writer headerWriter ) throw { // Unblock writers, any writes after this point and up until the next checkpoint will have // the new unstable generation. - writeLock.unlock(); + writerLock.unlock(); + cleanerLock.unlockWrite( stamp ); } } @@ -809,8 +822,7 @@ private void assertRecoveryCleanSuccessful() throws IOException @Override public void close() throws IOException { - Lock writeLock = writerCheckpointMutex.writeLock(); - writeLock.lock(); + writerLock.lock(); try { if ( closed ) @@ -835,7 +847,7 @@ public void close() throws IOException } finally { - writeLock.unlock(); + writerLock.unlock(); } } @@ -911,8 +923,7 @@ private CleanupJob createCleanupJob( boolean needsCleaning ) throws IOException } else { - Lock readLock = writerCheckpointMutex.readLock(); - readLock.lock(); + long stamp = cleanerLock.writeLock(); long generation = this.generation; long stableGeneration = stableGeneration( generation ); @@ -922,7 +933,7 @@ private CleanupJob createCleanupJob( boolean needsCleaning ) throws IOException CrashGenerationCleaner crashGenerationCleaner = new CrashGenerationCleaner( pagedFile, bTreeNode, IdSpace.MIN_TREE_NODE_ID, highTreeNodeId, stableGeneration, unstableGeneration, monitor ); - return new GBPTreeCleanupJob( crashGenerationCleaner, readLock ); + return new GBPTreeCleanupJob( crashGenerationCleaner, cleanerLock, stamp ); } } @@ -987,7 +998,7 @@ private class SingleWriter implements Writer private final InternalTreeLogic treeLogic; private final StructurePropagation structurePropagation; private PageCursor cursor; - private Lock readLock; + private Lock sessionLock; // Writer can't live past a checkpoint because of the mutex with checkpoint, // therefore safe to locally cache these generation fields from the volatile generation in the tree @@ -1006,13 +1017,13 @@ private class SingleWriter implements Writer * Either fully initialized: *

* Of fully closed: * * @@ -1030,8 +1041,7 @@ void initialize() throws IOException boolean success = false; try { - readLock = writerCheckpointMutex.readLock(); - readLock.lock(); + writerLock.lock(); cursor = openRootCursor( PagedFile.PF_SHARED_WRITE_LOCK ); stableGeneration = stableGeneration( generation ); unstableGeneration = unstableGeneration( generation ); @@ -1135,16 +1145,7 @@ public void close() throws IOException ", but writer is already closed." ); } closeCursor(); - releaseLock(); - } - - private void releaseLock() - { - if ( readLock != null ) - { - readLock.unlock(); - readLock = null; - } + writerLock.unlock(); } private void closeCursor() diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java index ec2509c392f0c..3840bb67e387a 100644 --- a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java +++ b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java @@ -21,11 +21,13 @@ import java.io.IOException; import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.StampedLock; class GBPTreeCleanupJob implements CleanupJob { private final CrashGenerationCleaner crashGenerationCleaner; - private final Lock readLock; + private final StampedLock stampedLock; + private final long stamp; private volatile boolean needed; private volatile Exception failure; @@ -33,10 +35,11 @@ class GBPTreeCleanupJob implements CleanupJob * @param crashGenerationCleaner {@link CrashGenerationCleaner} to use for cleaning. * @param lock {@link Lock} to be released when job has either successfully finished or failed. */ - GBPTreeCleanupJob( CrashGenerationCleaner crashGenerationCleaner, Lock lock ) + GBPTreeCleanupJob( CrashGenerationCleaner crashGenerationCleaner, StampedLock lock, long stamp ) { this.crashGenerationCleaner = crashGenerationCleaner; - this.readLock = lock; + this.stampedLock = lock; + this.stamp = stamp; this.needed = true; } @@ -73,7 +76,7 @@ public void run() } finally { - readLock.unlock(); + stampedLock.unlockWrite( stamp ); } } } diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeTest.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeTest.java index 76341bf414776..01be0921bc5d9 100644 --- a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeTest.java +++ b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeTest.java @@ -769,7 +769,7 @@ public void cleanJobShouldLockOutCheckpoint() throws Exception } @Test( timeout = 5_000L ) - public void cleanJobShouldLockOutClose() throws Exception + public void cleanJobShouldNotLockOutClose() throws Exception { // GIVEN try ( GBPTree index = index().build() ) @@ -789,11 +789,10 @@ public void cleanJobShouldLockOutClose() throws Exception // THEN Future close = executor.submit( throwing( index::close ) ); - shouldWait( close ); + close.get(); monitor.barrier.release(); cleanup.get(); - close.get(); } @Test( timeout = 5_000L )