Return-Path: X-Original-To: apmail-hadoop-common-commits-archive@www.apache.org Delivered-To: apmail-hadoop-common-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id BF0371822C for ; Wed, 29 Apr 2015 18:12:55 +0000 (UTC) Received: (qmail 65158 invoked by uid 500); 29 Apr 2015 18:12:55 -0000 Delivered-To: apmail-hadoop-common-commits-archive@hadoop.apache.org Received: (qmail 65087 invoked by uid 500); 29 Apr 2015 18:12:55 -0000 Mailing-List: contact common-commits-help@hadoop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: common-dev@hadoop.apache.org Delivered-To: mailing list common-commits@hadoop.apache.org Received: (qmail 65077 invoked by uid 99); 29 Apr 2015 18:12:55 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 29 Apr 2015 18:12:55 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 61CB2E0984; Wed, 29 Apr 2015 18:12:55 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: wheat9@apache.org To: common-commits@hadoop.apache.org Message-Id: <5770d4ebb7b942419c07f4c6d0b993b6@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: hadoop git commit: HDFS-8269. getBlockLocations() does not resolve the .reserved path and generates incorrect edit logs when updating the atime. Contributed by Haohui Mai. Date: Wed, 29 Apr 2015 18:12:55 +0000 (UTC) Repository: hadoop Updated Branches: refs/heads/branch-2 ecdebb736 -> 460127e6f HDFS-8269. getBlockLocations() does not resolve the .reserved path and generates incorrect edit logs when updating the atime. Contributed by Haohui Mai. Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/460127e6 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/460127e6 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/460127e6 Branch: refs/heads/branch-2 Commit: 460127e6f2dc172240fbcf1271ddc1691f1910f0 Parents: ecdebb7 Author: Haohui Mai Authored: Wed Apr 29 11:12:45 2015 -0700 Committer: Haohui Mai Committed: Wed Apr 29 11:12:54 2015 -0700 ---------------------------------------------------------------------- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 3 + .../hdfs/server/namenode/FSNamesystem.java | 63 ++++++--- .../hdfs/server/namenode/NamenodeFsck.java | 4 +- .../hadoop/hdfs/server/namenode/TestFsck.java | 8 +- .../server/namenode/TestGetBlockLocations.java | 133 +++++++++++++++++++ 5 files changed, 188 insertions(+), 23 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/460127e6/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 8a4fd79..f9a6949 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -311,6 +311,9 @@ Release 2.7.1 - UNRELEASED HDFS-8273. FSNamesystem#Delete() should not call logSync() when holding the lock. (wheat9) + HDFS-8269. getBlockLocations() does not resolve the .reserved path and + generates incorrect edit logs when updating the atime. (wheat9) + Release 2.7.0 - 2015-04-20 INCOMPATIBLE CHANGES http://git-wip-us.apache.org/repos/asf/hadoop/blob/460127e6/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java ---------------------------------------------------------------------- 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 d0d5f70..a92eba6 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 @@ -1687,13 +1687,14 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, } static class GetBlockLocationsResult { - final INodesInPath iip; + final boolean updateAccessTime; final LocatedBlocks blocks; boolean updateAccessTime() { - return iip != null; + return updateAccessTime; } - private GetBlockLocationsResult(INodesInPath iip, LocatedBlocks blocks) { - this.iip = iip; + private GetBlockLocationsResult( + boolean updateAccessTime, LocatedBlocks blocks) { + this.updateAccessTime = updateAccessTime; this.blocks = blocks; } } @@ -1702,34 +1703,58 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, * Get block locations within the specified range. * @see ClientProtocol#getBlockLocations(String, long, long) */ - LocatedBlocks getBlockLocations(String clientMachine, String src, + LocatedBlocks getBlockLocations(String clientMachine, String srcArg, long offset, long length) throws IOException { checkOperation(OperationCategory.READ); GetBlockLocationsResult res = null; + FSPermissionChecker pc = getPermissionChecker(); readLock(); try { checkOperation(OperationCategory.READ); - res = getBlockLocations(src, offset, length, true, true); + res = getBlockLocations(pc, srcArg, offset, length, true, true); } catch (AccessControlException e) { - logAuditEvent(false, "open", src); + logAuditEvent(false, "open", srcArg); throw e; } finally { readUnlock(); } - logAuditEvent(true, "open", src); + logAuditEvent(true, "open", srcArg); if (res.updateAccessTime()) { + byte[][] pathComponents = FSDirectory.getPathComponentsForReservedPath( + srcArg); + String src = srcArg; writeLock(); final long now = now(); try { checkOperation(OperationCategory.WRITE); - INode inode = res.iip.getLastINode(); - boolean updateAccessTime = now > inode.getAccessTime() + - getAccessTimePrecision(); + /** + * Resolve the path again and update the atime only when the file + * exists. + * + * XXX: Races can still occur even after resolving the path again. + * For example: + * + *
    + *
  • Get the block location for "/a/b"
    • + *
  • Rename "/a/b" to "/c/b"
    • + *
  • The second resolution still points to "/a/b", which is + * wrong.
    • + *
+ * + * The behavior is incorrect but consistent with the one before + * HDFS-7463. A better fix is to change the edit log of SetTime to + * use inode id instead of a path. + */ + src = dir.resolvePath(pc, srcArg, pathComponents); + final INodesInPath iip = dir.getINodesInPath(src, true); + INode inode = iip.getLastINode(); + boolean updateAccessTime = inode != null && + now > inode.getAccessTime() + getAccessTimePrecision(); if (!isInSafeMode() && updateAccessTime) { boolean changed = FSDirAttrOp.setTimes(dir, - inode, -1, now, false, res.iip.getLatestSnapshotId()); + inode, -1, now, false, iip.getLatestSnapshotId()); if (changed) { getEditLog().logTimes(src, -1, now); } @@ -1763,8 +1788,8 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, * @throws IOException */ GetBlockLocationsResult getBlockLocations( - String src, long offset, long length, boolean needBlockToken, - boolean checkSafeMode) throws IOException { + FSPermissionChecker pc, 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); @@ -1774,7 +1799,7 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, "Negative length is not supported. File: " + src); } final GetBlockLocationsResult ret = getBlockLocationsInt( - src, offset, length, needBlockToken); + pc, src, offset, length, needBlockToken); if (checkSafeMode && isInSafeMode()) { for (LocatedBlock b : ret.blocks.getLocatedBlocks()) { @@ -1795,12 +1820,12 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, } private GetBlockLocationsResult getBlockLocationsInt( - final String srcArg, long offset, long length, boolean needBlockToken) + FSPermissionChecker pc, final String srcArg, long offset, long length, + boolean needBlockToken) throws IOException { String src = srcArg; - FSPermissionChecker pc = getPermissionChecker(); byte[][] pathComponents = FSDirectory.getPathComponentsForReservedPath(src); - src = dir.resolvePath(pc, src, pathComponents); + src = dir.resolvePath(pc, srcArg, pathComponents); final INodesInPath iip = dir.getINodesInPath(src, true); final INodeFile inode = INodeFile.valueOf(iip.getLastINode(), src); if (isPermissionEnabled) { @@ -1836,7 +1861,7 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, boolean updateAccessTime = isAccessTimeSupported() && !isInSafeMode() && !iip.isSnapshot() && now > inode.getAccessTime() + getAccessTimePrecision(); - return new GetBlockLocationsResult(updateAccessTime ? iip : null, blocks); + return new GetBlockLocationsResult(updateAccessTime, blocks); } /** http://git-wip-us.apache.org/repos/asf/hadoop/blob/460127e6/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java ---------------------------------------------------------------------- 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 3bd3405..5ea8b8b 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 @@ -482,7 +482,9 @@ public class NamenodeFsck implements DataEncryptionKeyFactory { FSNamesystem fsn = namenode.getNamesystem(); fsn.readLock(); try { - blocks = fsn.getBlockLocations(path, 0, fileLen, false, false).blocks; + blocks = fsn.getBlockLocations( + fsn.getPermissionChecker(), path, 0, fileLen, false, false) + .blocks; } catch (FileNotFoundException fnfe) { blocks = null; } finally { http://git-wip-us.apache.org/repos/asf/hadoop/blob/460127e6/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java ---------------------------------------------------------------------- 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 aa1f07e..458b0f6 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 @@ -24,6 +24,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyString; @@ -1154,10 +1155,11 @@ public class TestFsck { FSNamesystem fsName = mock(FSNamesystem.class); BlockManager blockManager = mock(BlockManager.class); DatanodeManager dnManager = mock(DatanodeManager.class); - + when(namenode.getNamesystem()).thenReturn(fsName); - when(fsName.getBlockLocations( - anyString(), anyLong(), anyLong(), anyBoolean(), anyBoolean())) + when(fsName.getBlockLocations(any(FSPermissionChecker.class), anyString(), + anyLong(), anyLong(), + anyBoolean(), anyBoolean())) .thenThrow(new FileNotFoundException()); when(fsName.getBlockManager()).thenReturn(blockManager); when(blockManager.getDatanodeManager()).thenReturn(dnManager); http://git-wip-us.apache.org/repos/asf/hadoop/blob/460127e6/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGetBlockLocations.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGetBlockLocations.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGetBlockLocations.java new file mode 100644 index 0000000..a19eb1d --- /dev/null +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGetBlockLocations.java @@ -0,0 +1,133 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode; + +import org.apache.commons.io.Charsets; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.fs.permission.PermissionStatus; +import org.apache.hadoop.hdfs.server.blockmanagement.BlockInfoContiguous; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import java.io.IOException; +import java.util.ArrayList; + +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_BLOCK_SIZE_DEFAULT; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACCESSTIME_PRECISION_KEY; +import static org.apache.hadoop.util.Time.now; +import static org.mockito.Mockito.anyLong; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class TestGetBlockLocations { + private static final String FILE_NAME = "foo"; + private static final String FILE_PATH = "/" + FILE_NAME; + private static final long MOCK_INODE_ID = 16386; + private static final String RESERVED_PATH = + "/.reserved/.inodes/" + MOCK_INODE_ID; + + @Test(timeout = 30000) + public void testResolveReservedPath() throws IOException { + FSNamesystem fsn = setupFileSystem(); + FSEditLog editlog = fsn.getEditLog(); + fsn.getBlockLocations("dummy", RESERVED_PATH, 0, 1024); + verify(editlog).logTimes(eq(FILE_PATH), anyLong(), anyLong()); + fsn.close(); + } + + @Test(timeout = 30000) + public void testGetBlockLocationsRacingWithDelete() throws IOException { + FSNamesystem fsn = spy(setupFileSystem()); + final FSDirectory fsd = fsn.getFSDirectory(); + FSEditLog editlog = fsn.getEditLog(); + + doAnswer(new Answer() { + + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + INodesInPath iip = fsd.getINodesInPath(FILE_PATH, true); + FSDirDeleteOp.delete(fsd, iip, new INode.BlocksMapUpdateInfo(), + new ArrayList(), now()); + invocation.callRealMethod(); + return null; + } + }).when(fsn).writeLock(); + fsn.getBlockLocations("dummy", RESERVED_PATH, 0, 1024); + + verify(editlog, never()).logTimes(anyString(), anyLong(), anyLong()); + fsn.close(); + } + + @Test(timeout = 30000) + public void testGetBlockLocationsRacingWithRename() throws IOException { + FSNamesystem fsn = spy(setupFileSystem()); + final FSDirectory fsd = fsn.getFSDirectory(); + FSEditLog editlog = fsn.getEditLog(); + final String DST_PATH = "/bar"; + final boolean[] renamed = new boolean[1]; + + doAnswer(new Answer() { + + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + invocation.callRealMethod(); + if (!renamed[0]) { + FSDirRenameOp.renameTo(fsd, fsd.getPermissionChecker(), FILE_PATH, + DST_PATH, new INode.BlocksMapUpdateInfo(), + false); + renamed[0] = true; + } + return null; + } + }).when(fsn).writeLock(); + fsn.getBlockLocations("dummy", RESERVED_PATH, 0, 1024); + + verify(editlog).logTimes(eq(DST_PATH), anyLong(), anyLong()); + fsn.close(); + } + + private static FSNamesystem setupFileSystem() throws IOException { + Configuration conf = new Configuration(); + conf.setLong(DFS_NAMENODE_ACCESSTIME_PRECISION_KEY, 1L); + FSEditLog editlog = mock(FSEditLog.class); + FSImage image = mock(FSImage.class); + when(image.getEditLog()).thenReturn(editlog); + final FSNamesystem fsn = new FSNamesystem(conf, image, true); + + final FSDirectory fsd = fsn.getFSDirectory(); + INodesInPath iip = fsd.getINodesInPath("/", true); + PermissionStatus perm = new PermissionStatus( + "hdfs", "supergroup", + FsPermission.createImmutable((short) 0x1ff)); + final INodeFile file = new INodeFile( + MOCK_INODE_ID, FILE_NAME.getBytes(Charsets.UTF_8), + perm, 1, 1, new BlockInfoContiguous[] {}, (short) 1, + DFS_BLOCK_SIZE_DEFAULT); + fsn.getFSDirectory().addINode(iip, file); + return fsn; + } + +}