HDFS-14731. [FGL] Remove redundant locking on NameNode. Contributed by Konstantin V Shvachko.

Author: shvachko

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();
     }

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();

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<FileEdekInfo> 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();
     }
   }
 }

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<String> 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<? extends INode> 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

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();
   }

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 {

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 {