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 0737824d4ee..9f5f29e3714 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 @@ public class BackupImage extends FSImage { } 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 5604a218d0b..c7285dc14b1 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 class EncryptionZoneManager { 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 @@ public class EncryptionZoneManager { 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 3d78172f923..9c112fcd2d3 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 @@ final class FSDirEncryptionZoneOp { 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 @@ final class FSDirEncryptionZoneOp { 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 d0ffb3b2294..9ee290e00fe 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.SortedSet; 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 @@ public class FSDirectory implements Closeable { // 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 class FSDirectory implements Closeable { 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(); } @VisibleForTesting @@ -269,7 +272,6 @@ public class FSDirectory implements Closeable { }; FSDirectory(FSNamesystem ns, Configuration conf) throws IOException { - this.dirLock = new ReentrantReadWriteLock(true); // fair this.inodeId = new INodeId(); rootDir = createRoot(ns); inodeMap = INodeMap.newInstance(rootDir); @@ -1490,12 +1492,7 @@ public class FSDirectory implements Closeable { * @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 e91776dbda4..9e799aab2e7 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 @@ -3661,6 +3661,7 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, @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 a8acccdd964..fa4de38eb75 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 class ReencryptionHandler implements Runnable { } final Long zoneId; - dir.readLock(); + dir.getFSNamesystem().readLock(); try { zoneId = getReencryptionStatus().getNextUnprocessedZone(); if (zoneId == null) { @@ -350,7 +350,7 @@ public class ReencryptionHandler implements Runnable { 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 b773b0cfce2..ad87363653c 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 @@ -503,7 +503,8 @@ public class TestBlocksWithNotEnoughRacks { 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 @@ -519,7 +520,8 @@ public class TestBlocksWithNotEnoughRacks { } } // 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 @@ -531,6 +533,16 @@ public class TestBlocksWithNotEnoughRacks { } } + 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 {