From d693a252bd0041c2493e7e07a3bf8bcf28e1923c Mon Sep 17 00:00:00 2001 From: Haohui Mai Date: Wed, 10 Dec 2014 23:01:17 -0800 Subject: [PATCH] HDFS-7463. Simplify FSNamesystem#getBlockLocationsUpdateTimes. Contributed by Haohui Mai. --- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 2 + .../hdfs/server/namenode/FSNamesystem.java | 224 +++++++++--------- .../hdfs/server/namenode/NamenodeFsck.java | 9 +- .../org/apache/hadoop/hdfs/TestGetBlocks.java | 4 +- .../hdfs/server/namenode/NameNodeAdapter.java | 4 +- .../hadoop/hdfs/server/namenode/TestFsck.java | 6 +- 6 files changed, 121 insertions(+), 128 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 7b4e0c5159..fd486051b8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -449,6 +449,8 @@ Release 2.7.0 - UNRELEASED HDFS-7498. Simplify the logic in INodesInPath. (jing9) + HDFS-7463. Simplify FSNamesystem#getBlockLocationsUpdateTimes. (wheat9) + OPTIMIZATIONS HDFS-7454. Reduce memory footprint for AclEntries in NameNode. 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 30ac941ccb..c17c4f51e1 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 @@ -1749,27 +1749,76 @@ private void setOwnerInt(final String srcArg, String username, String group) logAuditEvent(true, "setOwner", srcArg, null, resultingStat); } + static class GetBlockLocationsResult { + final INodesInPath iip; + final LocatedBlocks blocks; + boolean updateAccessTime() { + return iip != null; + } + private GetBlockLocationsResult(INodesInPath iip, LocatedBlocks blocks) { + this.iip = iip; + this.blocks = blocks; + } + } + /** * Get block locations within the specified range. * @see ClientProtocol#getBlockLocations(String, long, long) */ LocatedBlocks getBlockLocations(String clientMachine, String src, - long offset, long length) throws AccessControlException, - FileNotFoundException, UnresolvedLinkException, IOException { - LocatedBlocks blocks = getBlockLocations(src, offset, length, true, true, - true); + long offset, long length) throws IOException { + checkOperation(OperationCategory.READ); + GetBlockLocationsResult res = null; + readLock(); + try { + checkOperation(OperationCategory.READ); + res = getBlockLocations(src, offset, length, true, true); + } catch (AccessControlException e) { + logAuditEvent(false, "open", src); + throw e; + } finally { + readUnlock(); + } + + logAuditEvent(true, "open", src); + + if (res == null) { + return null; + } + + if (res.updateAccessTime()) { + writeLock(); + final long now = now(); + try { + checkOperation(OperationCategory.WRITE); + INode inode = res.iip.getLastINode(); + boolean updateAccessTime = now > inode.getAccessTime() + + getAccessTimePrecision(); + if (!isInSafeMode() && updateAccessTime) { + boolean changed = dir.setTimes( + inode, -1, now, false, res.iip.getLatestSnapshotId()); + if (changed) { + getEditLog().logTimes(src, -1, now); + } + } + } catch (Throwable e) { + LOG.warn("Failed to update the access time of " + src, e); + } finally { + writeUnlock(); + } + } + + LocatedBlocks blocks = res.blocks; if (blocks != null) { - blockManager.getDatanodeManager().sortLocatedBlocks(clientMachine, - blocks.getLocatedBlocks()); + blockManager.getDatanodeManager().sortLocatedBlocks( + clientMachine, blocks.getLocatedBlocks()); // lastBlock is not part of getLocatedBlocks(), might need to sort it too LocatedBlock lastBlock = blocks.getLastLocatedBlock(); if (lastBlock != null) { - ArrayList lastBlockList = - Lists.newArrayListWithCapacity(1); - lastBlockList.add(lastBlock); - blockManager.getDatanodeManager().sortLocatedBlocks(clientMachine, - lastBlockList); + ArrayList lastBlockList = Lists.newArrayList(lastBlock); + blockManager.getDatanodeManager().sortLocatedBlocks( + clientMachine, lastBlockList); } } return blocks; @@ -1778,24 +1827,11 @@ LocatedBlocks getBlockLocations(String clientMachine, String src, /** * Get block locations within the specified range. * @see ClientProtocol#getBlockLocations(String, long, long) - * @throws FileNotFoundException, UnresolvedLinkException, IOException + * @throws IOException */ - LocatedBlocks getBlockLocations(String src, long offset, long length, - boolean doAccessTime, boolean needBlockToken, boolean checkSafeMode) - throws FileNotFoundException, UnresolvedLinkException, IOException { - try { - return getBlockLocationsInt(src, offset, length, doAccessTime, - needBlockToken, checkSafeMode); - } catch (AccessControlException e) { - logAuditEvent(false, "open", src); - throw e; - } - } - - private LocatedBlocks getBlockLocationsInt(String src, long offset, - long length, boolean doAccessTime, boolean needBlockToken, - boolean checkSafeMode) - throws FileNotFoundException, UnresolvedLinkException, IOException { + GetBlockLocationsResult getBlockLocations( + String src, long offset, long length, boolean needBlockToken, + boolean checkSafeMode) throws IOException { if (offset < 0) { throw new HadoopIllegalArgumentException( "Negative offset is not supported. File: " + src); @@ -1804,16 +1840,16 @@ private LocatedBlocks getBlockLocationsInt(String src, long offset, throw new HadoopIllegalArgumentException( "Negative length is not supported. File: " + src); } - final LocatedBlocks ret = getBlockLocationsUpdateTimes(src, - offset, length, doAccessTime, needBlockToken); - logAuditEvent(true, "open", src); + final GetBlockLocationsResult ret = getBlockLocationsInt( + src, offset, length, needBlockToken); + if (checkSafeMode && isInSafeMode()) { - for (LocatedBlock b : ret.getLocatedBlocks()) { + for (LocatedBlock b : ret.blocks.getLocatedBlocks()) { // if safemode & no block locations yet then throw safemodeException if ((b.getLocations() == null) || (b.getLocations().length == 0)) { SafeModeException se = new SafeModeException( "Zero blocklocations for " + src, safeMode); - if (haEnabled && haContext != null && + if (haEnabled && haContext != null && haContext.getState().getServiceState() == HAServiceState.ACTIVE) { throw new RetriableException(se); } else { @@ -1825,95 +1861,49 @@ private LocatedBlocks getBlockLocationsInt(String src, long offset, return ret; } - /* - * Get block locations within the specified range, updating the - * access times if necessary. - */ - private LocatedBlocks getBlockLocationsUpdateTimes(final String srcArg, - long offset, long length, boolean doAccessTime, boolean needBlockToken) + private GetBlockLocationsResult getBlockLocationsInt( + final String srcArg, long offset, long length, boolean needBlockToken) throws IOException { String src = srcArg; FSPermissionChecker pc = getPermissionChecker(); byte[][] pathComponents = FSDirectory.getPathComponentsForReservedPath(src); - for (int attempt = 0; attempt < 2; attempt++) { - boolean isReadOp = (attempt == 0); - if (isReadOp) { // first attempt is with readlock - checkOperation(OperationCategory.READ); - readLock(); - } else { // second attempt is with write lock - checkOperation(OperationCategory.WRITE); - writeLock(); // writelock is needed to set accesstime - } - try { - if (isReadOp) { - checkOperation(OperationCategory.READ); - } else { - checkOperation(OperationCategory.WRITE); - } - src = dir.resolvePath(pc, src, pathComponents); - final INodesInPath iip = dir.getINodesInPath(src, true); - if (isPermissionEnabled) { - dir.checkPathAccess(pc, iip, FsAction.READ); - } - - // if the namenode is in safemode, then do not update access time - if (isInSafeMode()) { - doAccessTime = false; - } - - final INodeFile inode = INodeFile.valueOf(iip.getLastINode(), src); - if (isPermissionEnabled) { - checkUnreadableBySuperuser(pc, inode, iip.getPathSnapshotId()); - } - if (!iip.isSnapshot() //snapshots are readonly, so don't update atime. - && doAccessTime && isAccessTimeSupported()) { - final long now = now(); - if (now > inode.getAccessTime() + getAccessTimePrecision()) { - // if we have to set access time but we only have the readlock, then - // restart this entire operation with the writeLock. - if (isReadOp) { - continue; - } - boolean changed = dir.setTimes(inode, -1, now, false, - iip.getLatestSnapshotId()); - if (changed) { - getEditLog().logTimes(src, -1, now); - } - } - } - final long fileSize = iip.isSnapshot() ? - inode.computeFileSize(iip.getPathSnapshotId()) - : inode.computeFileSizeNotIncludingLastUcBlock(); - boolean isUc = inode.isUnderConstruction(); - if (iip.isSnapshot()) { - // if src indicates a snapshot file, we need to make sure the returned - // blocks do not exceed the size of the snapshot file. - length = Math.min(length, fileSize - offset); - isUc = false; - } - - final FileEncryptionInfo feInfo = - FSDirectory.isReservedRawName(srcArg) ? - null : dir.getFileEncryptionInfo(inode, iip.getPathSnapshotId(), - iip); - - final LocatedBlocks blocks = - blockManager.createLocatedBlocks(inode.getBlocks(), fileSize, - isUc, offset, length, needBlockToken, iip.isSnapshot(), feInfo); - // Set caching information for the located blocks. - for (LocatedBlock lb: blocks.getLocatedBlocks()) { - cacheManager.setCachedLocations(lb); - } - return blocks; - } finally { - if (isReadOp) { - readUnlock(); - } else { - writeUnlock(); - } - } + src = dir.resolvePath(pc, src, pathComponents); + final INodesInPath iip = dir.getINodesInPath(src, true); + final INodeFile inode = INodeFile.valueOf(iip.getLastINode(), src); + if (isPermissionEnabled) { + dir.checkPathAccess(pc, iip, FsAction.READ); + checkUnreadableBySuperuser(pc, inode, iip.getPathSnapshotId()); } - return null; // can never reach here + + final long fileSize = iip.isSnapshot() + ? inode.computeFileSize(iip.getPathSnapshotId()) + : inode.computeFileSizeNotIncludingLastUcBlock(); + boolean isUc = inode.isUnderConstruction(); + if (iip.isSnapshot()) { + // if src indicates a snapshot file, we need to make sure the returned + // blocks do not exceed the size of the snapshot file. + length = Math.min(length, fileSize - offset); + isUc = false; + } + + final FileEncryptionInfo feInfo = + FSDirectory.isReservedRawName(srcArg) ? null + : dir.getFileEncryptionInfo(inode, iip.getPathSnapshotId(), iip); + + final LocatedBlocks blocks = blockManager.createLocatedBlocks( + inode.getBlocks(), fileSize, isUc, offset, length, needBlockToken, + iip.isSnapshot(), feInfo); + + // Set caching information for the located blocks. + for (LocatedBlock lb : blocks.getLocatedBlocks()) { + cacheManager.setCachedLocations(lb); + } + + final long now = now(); + boolean updateAccessTime = isAccessTimeSupported() && !isInSafeMode() + && !iip.isSnapshot() + && now > inode.getAccessTime() + getAccessTimePrecision(); + return new GetBlockLocationsResult(updateAccessTime ? iip : null, blocks); } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java index f82f0ea06a..bab8f5e1b3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java @@ -443,12 +443,15 @@ void check(String parent, HdfsFileStatus file, Result res) throws IOException { long fileLen = file.getLen(); // Get block locations without updating the file access time // and without block access tokens - LocatedBlocks blocks; + LocatedBlocks blocks = null; + FSNamesystem fsn = namenode.getNamesystem(); + fsn.readLock(); try { - blocks = namenode.getNamesystem().getBlockLocations(path, 0, - fileLen, false, false, false); + blocks = fsn.getBlockLocations(path, 0, fileLen, false, false).blocks; } catch (FileNotFoundException fnfe) { blocks = null; + } finally { + fsn.readUnlock(); } if (blocks == null) { // the file is deleted return; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java index 2af86bd6bb..cc89852788 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java @@ -167,9 +167,7 @@ public void testReadSelectNonStaleDatanode() throws Exception { if (stm != null) { stm.close(); } - if (client != null) { - client.close(); - } + client.close(); cluster.shutdown(); } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java index 61e7f145fb..7aad37854e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java @@ -64,8 +64,8 @@ public static FSNamesystem getNamesystem(NameNode namenode) { */ public static LocatedBlocks getBlockLocations(NameNode namenode, String src, long offset, long length) throws IOException { - return namenode.getNamesystem().getBlockLocations( - src, offset, length, false, true, true); + return namenode.getNamesystem().getBlockLocations("foo", + src, offset, length); } public static HdfsFileStatus getFileInfo(NameNode namenode, String src, diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java index ef7de0d1c5..aecf55ef02 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java @@ -996,9 +996,9 @@ public void testFsckFileNotFound() throws Exception { DatanodeManager dnManager = mock(DatanodeManager.class); when(namenode.getNamesystem()).thenReturn(fsName); - when(fsName.getBlockLocations(anyString(), anyLong(), anyLong(), - anyBoolean(), anyBoolean(), anyBoolean())). - thenThrow(new FileNotFoundException()) ; + when(fsName.getBlockLocations( + anyString(), anyLong(), anyLong(), anyBoolean(), anyBoolean())) + .thenThrow(new FileNotFoundException()); when(fsName.getBlockManager()).thenReturn(blockManager); when(blockManager.getDatanodeManager()).thenReturn(dnManager);