From cb6108a93a6102fec80593a2ff9548b0b312e498 Mon Sep 17 00:00:00 2001 From: prasad-acit Date: Fri, 3 Sep 2021 03:45:54 +0530 Subject: [PATCH 1/2] HDFS-16208. Implement Delete API with FGL --- .../hdfs/server/namenode/FSDirDeleteOp.java | 4 ++ .../hdfs/server/namenode/FSNamesystem.java | 47 ++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java index 2dfb90ee67282..e3d7a57c07b3a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java @@ -243,6 +243,10 @@ private static boolean unprotectedDelete(FSDirectory fsd, INodesInPath iip, final int latestSnapshot = iip.getLatestSnapshotId(); targetNode.recordModification(latestSnapshot); + // switch the locks + fsd.getINodeMap().latchWriteLock(iip.getParentINodesInPath(), + new INode[] { iip.getLastINode() }); + // Remove the node from the namespace long removed = fsd.removeLastINode(iip); if (removed == -1) { 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 96324e3f50a54..4d958895a65f2 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 @@ -2785,7 +2785,7 @@ private HdfsFileStatus startFileInt(String src, if (!skipSync) { getEditLog().logSync(); if (toRemoveBlocks != null) { - removeBlocks(toRemoveBlocks); + removeBlocksWithFGL(toRemoveBlocks, iip.getPath(), pc); toRemoveBlocks.clear(); } } @@ -3349,11 +3349,54 @@ boolean delete(String src, boolean recursive, boolean logRetryCache) getEditLog().logSync(); logAuditEvent(ret, operationName, src); if (toRemovedBlocks != null) { - removeBlocks(toRemovedBlocks); // Incremental deletion of blocks + removeBlocksWithFGL(toRemovedBlocks, src, pc); // Incremental deletion of blocks } return ret; } + /** Duplicate of removeBlocks(blocks), difference is it acquire partition + * lock instead of global write lock. + * @param blocks blocks to be removed. + * @param src Source file on which partition lock held. + * @param pc Permission checker for resolving the path. + * @throws IOException + * @see this.removeBlocks(BlocksMapUpdateInfo ), duplicate method. + * Remove the above one once all APIs implemented FGL. + */ + private void removeBlocksWithFGL(BlocksMapUpdateInfo blocks, String src, + FSPermissionChecker pc) throws IOException { + List toDeleteList = blocks.getToDeleteList(); + Iterator iter = toDeleteList.iterator(); + while (iter.hasNext()) { + writeLock(); + try { + switchToPartitionLock(src, pc); + for (int i = 0; i < blockDeletionIncrement && iter.hasNext(); i++) { + blockManager.removeBlock(iter.next()); + } + } finally { + writeUnlock("removeBlocks"); + } + } + } + + /** + * Switch global write lock with the partition lock. + * + * @param src source file on which lock to be acquired + * @param pc PermissionChecker to resolve the source + * @throws IOException when fails to access the path. + */ + private void switchToPartitionLock(String src, FSPermissionChecker pc) + throws IOException { + INodesInPath iip = dir.resolvePath(pc, src, DirOp.WRITE_LINK); + iip = iip.getExistingINodes(); + if (null != iip.getParentINodesInPath()) + // switch the locks + // Only content gets deleted, acquire only file / directory level lock. + dir.getINodeMap().latchWriteLock(iip, new INode[0]); + } + FSPermissionChecker getPermissionChecker() throws AccessControlException { return dir.getPermissionChecker(); From ff2a395fd73ddfdb00b494eaf56c46577333ad7b Mon Sep 17 00:00:00 2001 From: prasad-acit Date: Thu, 7 Jul 2022 20:01:11 +0530 Subject: [PATCH 2/2] HDFS-16208 Acquire lock on delete children --- .../apache/hadoop/util/PartitionedGSet.java | 17 +++++++++++++++++ .../hdfs/server/namenode/FSDirDeleteOp.java | 3 +-- .../hdfs/server/namenode/FSDirectory.java | 12 ++++++++++++ .../hdfs/server/namenode/FSNamesystem.java | 3 ++- .../hadoop/hdfs/server/namenode/INodeMap.java | 18 ++++++++++++++++++ 5 files changed, 50 insertions(+), 3 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/PartitionedGSet.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/PartitionedGSet.java index f493402959031..0d1af53dcd7a9 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/PartitionedGSet.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/PartitionedGSet.java @@ -345,4 +345,21 @@ public void latchWriteLock(K[] keys) { assert pLock != null : "pLock is null"; pLock.writeTopUnlock(); } + + /** + * Verify whether the current thread holds the lock on given key / partition. + * @param key INode for which lock to be verified. + * @return true if the current thread holds the partition lock. + */ + public boolean hasWriteLock(final K key) { + Entry partEntry = partitions.floorEntry(key); + if (partEntry == null) { + return false; + } + PartitionEntry part = partEntry.getValue(); + if (part == null) { + throw new IllegalStateException("Null partition for key: " + key); + } + return part.partLock.hasWriteChildLock(); + } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java index e3d7a57c07b3a..ac8850893a030 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java @@ -244,8 +244,7 @@ private static boolean unprotectedDelete(FSDirectory fsd, INodesInPath iip, targetNode.recordModification(latestSnapshot); // switch the locks - fsd.getINodeMap().latchWriteLock(iip.getParentINodesInPath(), - new INode[] { iip.getLastINode() }); + fsd.getINodeMap().latchWriteLock(iip.getParentINodesInPath(), new INode[] {iip.getLastINode()}); // Remove the node from the namespace long removed = fsd.removeLastINode(iip); 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 8ddf0ffa9e857..040352a546516 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 @@ -1607,6 +1607,18 @@ void moveInodes() throws IOException { */ public final void removeFromInodeMap(List inodes) { if (inodes != null) { + for (INode inode : inodes) { + if (inodeMap.hasWriteLock(inode)) { + // Consider /hive/blr to be removed, and it contains children as below + // /hive/blr/A1, /hive/blr/A2, /hive/blr/A3 all are in same + // partition P1 + // /hive/blr/A3/a1, /hive/blr/A3/a2 and in Partition P2 + // Here we iterate through all inodes (5), and acquire lock on each + // Partition only once + continue; + } + inodeMap.latchWriteLock(inode); + } for (INode inode : inodes) { if (inode != null && inode instanceof INodeWithAdditionalFields) { inodeMap.remove(inode); 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 4d958895a65f2..532f680469f95 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 @@ -3391,10 +3391,11 @@ private void switchToPartitionLock(String src, FSPermissionChecker pc) throws IOException { INodesInPath iip = dir.resolvePath(pc, src, DirOp.WRITE_LINK); iip = iip.getExistingINodes(); - if (null != iip.getParentINodesInPath()) + if (null != iip.getParentINodesInPath()) { // switch the locks // Only content gets deleted, acquire only file / directory level lock. dir.getINodeMap().latchWriteLock(iip, new INode[0]); + } } FSPermissionChecker getPermissionChecker() diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeMap.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeMap.java index 0fdda96448f1f..ede6dd27a9b6f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeMap.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeMap.java @@ -329,4 +329,22 @@ public void latchWriteLock(INodesInPath iip, INode[] missing) { ((PartitionedGSet) map).latchWriteLock(allINodes); } + + /** + * Take the partition lock on the given INode key. + * @param iNode INode to acquire the partition lock. + */ + public void latchWriteLock(INode iNode) { + if (!(map instanceof PartitionedGSet)) { + return; + } + + ((PartitionedGSet) map).latchWriteLock( + new INode[] {iNode }); + } + + public boolean hasWriteLock(final INode key) { + return ((PartitionedGSet) map). + hasWriteLock(key); + } }