You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-commits@hadoop.apache.org by sh...@apache.org on 2020/02/27 23:55:56 UTC
[hadoop] branch branch-3.2 updated: HDFS-14731. [FGL] Remove
redundant locking on NameNode. Contributed by Konstantin V Shvachko.
This is an automated email from the ASF dual-hosted git repository.
shv pushed a commit to branch branch-3.2
in repository https://gitbox.apache.org/repos/asf/hadoop.git
The following commit(s) were added to refs/heads/branch-3.2 by this push:
new c82e20f HDFS-14731. [FGL] Remove redundant locking on NameNode. Contributed by Konstantin V Shvachko.
c82e20f is described below
commit c82e20f9f8dd3b1223e32a880a1bce585db02683
Author: Konstantin V Shvachko <sh...@apache.org>
AuthorDate: Fri Feb 21 17:53:37 2020 -0800
HDFS-14731. [FGL] Remove redundant locking on NameNode. Contributed by Konstantin V Shvachko.
(cherry picked from commit ecbcb058b8bc0fbc3903acb56814c6d9608bc396)
---
.../hadoop/hdfs/server/namenode/BackupImage.java | 7 +++-
.../server/namenode/EncryptionZoneManager.java | 8 ++---
.../server/namenode/FSDirEncryptionZoneOp.java | 5 ++-
.../hadoop/hdfs/server/namenode/FSDirectory.java | 37 ++++++++++------------
.../hadoop/hdfs/server/namenode/FSNamesystem.java | 1 +
.../hdfs/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 0737824..9f5f29e 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 5604a21..c7285dc 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 3d78172..9c112fc 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<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 @@ 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 d0ffb3b..9ee290e 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<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 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 e91776d..9e799aa 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 a8acccd..fa4de38 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 b773b0c..ad87363 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 {
---------------------------------------------------------------------
To unsubscribe, e-mail: common-commits-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-commits-help@hadoop.apache.org