From common-commits-return-97174-archive-asf-public=cust-asf.ponee.io@hadoop.apache.org Tue Oct 22 23:50:13 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 28B7418062C for ; Wed, 23 Oct 2019 01:50:13 +0200 (CEST) Received: (qmail 57917 invoked by uid 500); 22 Oct 2019 23:50:12 -0000 Mailing-List: contact common-commits-help@hadoop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list common-commits@hadoop.apache.org Received: (qmail 57908 invoked by uid 99); 22 Oct 2019 23:50:12 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 22 Oct 2019 23:50:12 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id BB4AD80B20; Tue, 22 Oct 2019 23:50:11 +0000 (UTC) Date: Tue, 22 Oct 2019 23:50:10 +0000 To: "common-commits@hadoop.apache.org" Subject: [hadoop] branch branch-2 updated: HDFS-13901. INode access time is ignored because of race between open and rename. Contributed by Jinglun. MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <157178820967.12524.8379720193033002663@gitbox.apache.org> From: weichiu@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: hadoop X-Git-Refname: refs/heads/branch-2 X-Git-Reftype: branch X-Git-Oldrev: 339ab2347d4fcf95e7cbeef1c49cfdf4006871fe X-Git-Newrev: a805d80ebe3b1683049119d3d19e8b085141e3cf X-Git-Rev: a805d80ebe3b1683049119d3d19e8b085141e3cf X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. weichiu pushed a commit to branch branch-2 in repository https://gitbox.apache.org/repos/asf/hadoop.git The following commit(s) were added to refs/heads/branch-2 by this push: new a805d80 HDFS-13901. INode access time is ignored because of race between open and rename. Contributed by Jinglun. a805d80 is described below commit a805d80ebe3b1683049119d3d19e8b085141e3cf Author: Wei-Chiu Chuang AuthorDate: Tue Oct 22 16:43:25 2019 -0700 HDFS-13901. INode access time is ignored because of race between open and rename. Contributed by Jinglun. --- .../server/namenode/FSDirStatAndListingOp.java | 9 ++- .../hadoop/hdfs/server/namenode/FSNamesystem.java | 36 +++------- .../apache/hadoop/hdfs/server/namenode/INode.java | 12 ++++ .../hdfs/server/namenode/TestDeleteRace.java | 76 ++++++++++++++++++++++ 4 files changed, 105 insertions(+), 28 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirStatAndListingOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirStatAndListingOp.java index 4ada2be..fe0e85f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirStatAndListingOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirStatAndListingOp.java @@ -176,7 +176,7 @@ class FSDirStatAndListingOp { boolean updateAccessTime = fsd.isAccessTimeSupported() && !iip.isSnapshot() && now > inode.getAccessTime() + fsd.getAccessTimePrecision(); - return new GetBlockLocationsResult(updateAccessTime, blocks); + return new GetBlockLocationsResult(updateAccessTime, blocks, iip); } finally { fsd.readUnlock(); } @@ -566,13 +566,18 @@ class FSDirStatAndListingOp { static class GetBlockLocationsResult { final boolean updateAccessTime; final LocatedBlocks blocks; + private final INodesInPath iip; boolean updateAccessTime() { return updateAccessTime; } + public INodesInPath getIIp() { + return iip; + } private GetBlockLocationsResult( - boolean updateAccessTime, LocatedBlocks blocks) { + boolean updateAccessTime, LocatedBlocks blocks, INodesInPath iip) { this.updateAccessTime = updateAccessTime; this.blocks = blocks; + this.iip = iip; } } } 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 fe84ebd..3536433 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 @@ -1836,11 +1836,13 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, checkOperation(OperationCategory.READ); GetBlockLocationsResult res = null; FSPermissionChecker pc = getPermissionChecker(); + final INode inode; readLock(); try { checkOperation(OperationCategory.READ); res = FSDirStatAndListingOp.getBlockLocations( dir, pc, srcArg, offset, length, true); + inode = res.getIIp().getLastINode(); if (isInSafeMode()) { for (LocatedBlock b : res.blocks.getLocatedBlocks()) { // if safemode & no block locations yet then throw safemodeException @@ -1881,34 +1883,16 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, final long now = now(); try { checkOperation(OperationCategory.WRITE); - /** - * 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. - */ - final INodesInPath iip = dir.resolvePath(pc, srcArg, DirOp.READ); - src = iip.getPath(); - - INode inode = iip.getLastINode(); - boolean updateAccessTime = inode != null && + boolean updateAccessTime = now > inode.getAccessTime() + dir.getAccessTimePrecision(); if (!isInSafeMode() && updateAccessTime) { - boolean changed = FSDirAttrOp.setTimes(dir, iip, -1, now, false); - if (changed) { - getEditLog().logTimes(src, -1, now); + if (!inode.isDeleted()) { + src = inode.getFullPathName(); + final INodesInPath iip = dir.resolvePath(pc, src, DirOp.READ); + boolean changed = FSDirAttrOp.setTimes(dir, iip, -1, now, false); + if (changed) { + getEditLog().logTimes(src, -1, now); + } } } } catch (Throwable e) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java index 27c4244..1d2654e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java @@ -600,6 +600,18 @@ public abstract class INode implements INodeAttributes, Diff.Element { return DFSUtil.bytes2String(path); } + public boolean isDeleted() { + INode pInode = this; + while (pInode != null && !pInode.isRoot()) { + pInode = pInode.getParent(); + } + if (pInode == null) { + return true; + } else { + return !pInode.isRoot(); + } + } + public byte[][] getPathComponents() { int n = 0; for (INode inode = this; inode != null; inode = inode.getParent()) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java index a13574f..b581e8a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hdfs.server.namenode; import java.io.FileNotFoundException; +import java.io.IOException; import java.util.AbstractMap; import java.util.ArrayList; import java.util.Comparator; @@ -27,10 +28,13 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.concurrent.Semaphore; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.Options; import org.apache.hadoop.hdfs.AddBlockFlag; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FileSystem; @@ -66,6 +70,7 @@ import org.mockito.internal.util.reflection.Whitebox; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_LEASE_RECHECK_INTERVAL_MS_DEFAULT; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_LEASE_RECHECK_INTERVAL_MS_KEY; +import static org.junit.Assert.assertNotEquals; /** * Test race between delete and other operations. For now only addBlock() @@ -442,4 +447,75 @@ public class TestDeleteRace { } } } + + @Test(timeout = 20000) + public void testOpenRenameRace() throws Exception { + Configuration config = new Configuration(); + config.setLong(DFSConfigKeys.DFS_NAMENODE_ACCESSTIME_PRECISION_KEY, 1); + MiniDFSCluster dfsCluster = null; + final String src = "/dir/src-file"; + final String dst = "/dir/dst-file"; + final DistributedFileSystem hdfs; + try { + dfsCluster = new MiniDFSCluster.Builder(config).build(); + dfsCluster.waitActive(); + final FSNamesystem fsn = dfsCluster.getNamesystem(); + hdfs = dfsCluster.getFileSystem(); + DFSTestUtil.createFile(hdfs, new Path(src), 5, (short) 1, 0xFEED); + FileStatus status = hdfs.getFileStatus(new Path(src)); + long accessTime = status.getAccessTime(); + + final Semaphore openSem = new Semaphore(0); + final Semaphore renameSem = new Semaphore(0); + // 1.hold writeLock. + // 2.start open thread. + // 3.openSem & yield makes sure open thread wait on readLock. + // 4.start rename thread. + // 5.renameSem & yield makes sure rename thread wait on writeLock. + // 6.release writeLock, it's fair lock so open thread gets read lock. + // 7.open thread unlocks, rename gets write lock and does rename. + // 8.rename thread unlocks, open thread gets write lock and update time. + Thread open = new Thread(new Runnable() { + @Override public void run() { + try { + openSem.release(); + fsn.getBlockLocations("foo", src, 0, 5); + } catch (IOException e) { + } + } + }); + Thread rename = new Thread(new Runnable() { + @Override public void run() { + try { + openSem.acquire(); + renameSem.release(); + fsn.renameTo(src, dst, false, Options.Rename.NONE); + } catch (IOException e) { + } catch (InterruptedException e) { + } + } + }); + fsn.writeLock(); + open.start(); + openSem.acquire(); + Thread.yield(); + openSem.release(); + rename.start(); + renameSem.acquire(); + Thread.yield(); + fsn.writeUnlock(); + + // wait open and rename threads finish. + open.join(); + rename.join(); + + status = hdfs.getFileStatus(new Path(dst)); + assertNotEquals(accessTime, status.getAccessTime()); + dfsCluster.restartNameNode(0); + } finally { + if (dfsCluster != null) { + dfsCluster.shutdown(); + } + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: common-commits-unsubscribe@hadoop.apache.org For additional commands, e-mail: common-commits-help@hadoop.apache.org