From ecbcb058b8bc0fbc3903acb56814c6d9608bc396 Mon Sep 17 00:00:00 2001 From: Konstantin V Shvachko Date: Fri, 21 Feb 2020 17:53:37 -0800 Subject: [PATCH] HDFS-14731. [FGL] Remove redundant locking on NameNode. Contributed by Konstantin V Shvachko. --- .../hdfs/server/namenode/BackupImage.java | 7 +++- .../namenode/EncryptionZoneManager.java | 8 ++-- .../namenode/FSDirEncryptionZoneOp.java | 5 +-- .../hdfs/server/namenode/FSDirectory.java | 37 +++++++++---------- .../hdfs/server/namenode/FSNamesystem.java | 1 + .../server/namenode/ReencryptionHandler.java | 4 +- .../TestBlocksWithNotEnoughRacks.java | 16 +++++++- 7 files changed, 46 insertions(+), 32 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java index 0737824d4e..9f5f29e371 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java @@ -218,7 +218,12 @@ private synchronized void applyEdits(long firstTxId, int numTxns, byte[] data) } lastAppliedTxId = logLoader.getLastAppliedTxId(); - getNamesystem().dir.updateCountForQuota(); + getNamesystem().writeLock(); + try { + getNamesystem().dir.updateCountForQuota(); + } finally { + getNamesystem().writeUnlock(); + } } finally { backupInputStream.clear(); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EncryptionZoneManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EncryptionZoneManager.java index e98d1126ff..48e405ba35 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EncryptionZoneManager.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EncryptionZoneManager.java @@ -187,11 +187,11 @@ public void pauseForTestingAfterNthCheckpoint(final String zone, final int count) throws IOException { INodesInPath iip; final FSPermissionChecker pc = dir.getPermissionChecker(); - dir.readLock(); + dir.getFSNamesystem().readLock(); try { iip = dir.resolvePath(pc, zone, DirOp.READ); } finally { - dir.readUnlock(); + dir.getFSNamesystem().readUnlock(); } reencryptionHandler .pauseForTestingAfterNthCheckpoint(iip.getLastINode().getId(), count); @@ -280,11 +280,11 @@ void stopReencryptThread() { if (getProvider() == null || reencryptionHandler == null) { return; } - dir.writeLock(); + dir.getFSNamesystem().writeLock(); try { reencryptionHandler.stopThreads(); } finally { - dir.writeUnlock(); + dir.getFSNamesystem().writeUnlock(); } if (reencryptHandlerExecutor != null) { reencryptHandlerExecutor.shutdownNow(); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirEncryptionZoneOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirEncryptionZoneOp.java index 02e09faff4..6c7b1fae50 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirEncryptionZoneOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirEncryptionZoneOp.java @@ -382,7 +382,6 @@ private static ZoneEncryptionInfoProto getZoneEncryptionInfoProto( static void saveFileXAttrsForBatch(FSDirectory fsd, List batch) { assert fsd.getFSNamesystem().hasWriteLock(); - assert !fsd.hasWriteLock(); if (batch != null && !batch.isEmpty()) { for (FileEdekInfo entry : batch) { final INode inode = fsd.getInode(entry.getInodeId()); @@ -727,13 +726,13 @@ static String getKeyNameForZone(final FSDirectory dir, final FSPermissionChecker pc, final String zone) throws IOException { assert dir.getProvider() != null; final INodesInPath iip; - dir.readLock(); + dir.getFSNamesystem().readLock(); try { iip = dir.resolvePath(pc, zone, DirOp.READ); dir.ezManager.checkEncryptionZoneRoot(iip.getLastINode(), zone); return dir.ezManager.getKeyName(iip); } finally { - dir.readUnlock(); + dir.getFSNamesystem().readUnlock(); } } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java index a926afe13f..77d8518de9 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java @@ -82,7 +82,6 @@ import java.util.TreeSet; import java.util.concurrent.ForkJoinPool; import java.util.concurrent.RecursiveAction; -import java.util.concurrent.locks.ReentrantReadWriteLock; import static org.apache.hadoop.fs.CommonConfigurationKeys.FS_PROTECTED_DIRECTORIES; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACCESSTIME_PRECISION_DEFAULT; @@ -172,9 +171,6 @@ private static INodeDirectory createRoot(FSNamesystem namesystem) { // Each entry in this set must be a normalized path. private volatile SortedSet protectedDirectories; - // lock to protect the directory and BlockMap - private final ReentrantReadWriteLock dirLock; - private final boolean isPermissionEnabled; private final boolean isPermissionContentSummarySubAccess; /** @@ -215,37 +211,44 @@ public void setINodeAttributeProvider(INodeAttributeProvider provider) { attributeProvider = provider; } - // utility methods to acquire and release read lock and write lock + /** + * The directory lock dirLock provided redundant locking. + * It has been used whenever namesystem.fsLock was used. + * dirLock is now removed and utility methods to acquire and release dirLock + * remain as placeholders only + */ void readLock() { - this.dirLock.readLock().lock(); + assert namesystem.hasReadLock() : "Should hold namesystem read lock"; } void readUnlock() { - this.dirLock.readLock().unlock(); + assert namesystem.hasReadLock() : "Should hold namesystem read lock"; } void writeLock() { - this.dirLock.writeLock().lock(); + assert namesystem.hasWriteLock() : "Should hold namesystem write lock"; } void writeUnlock() { - this.dirLock.writeLock().unlock(); + assert namesystem.hasWriteLock() : "Should hold namesystem write lock"; } boolean hasWriteLock() { - return this.dirLock.isWriteLockedByCurrentThread(); + return namesystem.hasWriteLock(); } boolean hasReadLock() { - return this.dirLock.getReadHoldCount() > 0 || hasWriteLock(); + return namesystem.hasReadLock(); } + @Deprecated // dirLock is obsolete, use namesystem.fsLock instead public int getReadHoldCount() { - return this.dirLock.getReadHoldCount(); + return namesystem.getReadHoldCount(); } + @Deprecated // dirLock is obsolete, use namesystem.fsLock instead public int getWriteHoldCount() { - return this.dirLock.getWriteHoldCount(); + return namesystem.getWriteHoldCount(); } public int getListLimit() { @@ -273,7 +276,6 @@ public enum DirOp { }; FSDirectory(FSNamesystem ns, Configuration conf) throws IOException { - this.dirLock = new ReentrantReadWriteLock(true); // fair this.inodeId = new INodeId(); rootDir = createRoot(ns); inodeMap = INodeMap.newInstance(rootDir); @@ -1492,12 +1494,7 @@ public final void removeFromInodeMap(List inodes) { * @return The inode associated with the given id */ public INode getInode(long id) { - readLock(); - try { - return inodeMap.get(id); - } finally { - readUnlock(); - } + return inodeMap.get(id); } @VisibleForTesting diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index ab12c8880f..004da05fe7 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -3723,6 +3723,7 @@ INodeFile getBlockCollection(BlockInfo b) { @Override public INodeFile getBlockCollection(long id) { + assert hasReadLock() : "Accessing INode id = " + id + " without read lock"; INode inode = getFSDirectory().getInode(id); return inode == null ? null : inode.asFile(); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ReencryptionHandler.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ReencryptionHandler.java index 2e13df5fd3..fd9cbd7527 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ReencryptionHandler.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ReencryptionHandler.java @@ -338,7 +338,7 @@ public void run() { } final Long zoneId; - dir.readLock(); + dir.getFSNamesystem().readLock(); try { zoneId = getReencryptionStatus().getNextUnprocessedZone(); if (zoneId == null) { @@ -350,7 +350,7 @@ public void run() { getReencryptionStatus().markZoneStarted(zoneId); resetSubmissionTracker(zoneId); } finally { - dir.readUnlock(); + dir.getFSNamesystem().readUnlock(); } try { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java index c0cf7ea396..7dfb9514f9 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java @@ -570,7 +570,8 @@ public void testMultipleReplicasScheduledForUpgradeDomain() throws Exception { BlockInfo storedBlock = bm.getStoredBlock(b.getLocalBlock()); // The block should be replicated OK - so Reconstruction Work will be null - BlockReconstructionWork work = bm.scheduleReconstruction(storedBlock, 2); + BlockReconstructionWork work = scheduleReconstruction( + cluster.getNamesystem(), storedBlock, 2); assertNull(work); // Set the upgradeDomain to "3" for the 3 nodes hosting the block. // Then alternately set the remaining 3 nodes to have an upgradeDomain @@ -586,7 +587,8 @@ public void testMultipleReplicasScheduledForUpgradeDomain() throws Exception { } } // Now reconWork is non-null and 2 extra targets are needed - work = bm.scheduleReconstruction(storedBlock, 2); + work = scheduleReconstruction( + cluster.getNamesystem(), storedBlock, 2); assertEquals(2, work.getAdditionalReplRequired()); // Add the block to the replication queue and ensure it is replicated @@ -598,6 +600,16 @@ public void testMultipleReplicasScheduledForUpgradeDomain() throws Exception { } } + static BlockReconstructionWork scheduleReconstruction( + FSNamesystem fsn, BlockInfo block, int priority) { + fsn.writeLock(); + try { + return fsn.getBlockManager().scheduleReconstruction(block, priority); + } finally { + fsn.writeUnlock(); + } + } + @Test public void testUnderReplicatedRespectsRacksAndUpgradeDomain() throws Exception {