[ https://issues.apache.org/jira/browse/HDFS-3981?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Xiaobo Peng updated HDFS-3981: ------------------------------ Description: If now > inode.getAccessTime() + getAccessTimePrecision() when attempt == 0, we will call dir.setTimes(src, inode, -1, now, false) without writelock. So there will be races and an ealier access time might overwrite a later access time. Looks like we need to change the code to {noformat} if (doAccessTime && isAccessTimeSupported()) { 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 (attempt == 0) { continue; } dir.setTimes(src, inode, -1, now, false); } } {noformat} Also, seems we need to release readlock before trying to acquire writelock. Otherwise, we might end up with still holding readlock after the function call. The following code is from branch-2.0.1-alpha {code:title=FSNamesystem.java|borderStyle=solid} private LocatedBlocks getBlockLocationsUpdateTimes(String src, long offset, long length, boolean doAccessTime, boolean needBlockToken) throws FileNotFoundException, UnresolvedLinkException, IOException { for (int attempt = 0; attempt < 2; attempt++) { if (attempt == 0) { // first attempt is with readlock readLock(); } else { // second attempt is with write lock writeLock(); // writelock is needed to set accesstime } try { checkOperation(OperationCategory.READ); // if the namenode is in safemode, then do not update access time if (isInSafeMode()) { doAccessTime = false; } long now = now(); INodeFile inode = dir.getFileINode(src); if (inode == null) { throw new FileNotFoundException("File does not exist: " + src); } assert !inode.isLink(); if (doAccessTime && isAccessTimeSupported()) { 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 (attempt == 0) { continue; } } dir.setTimes(src, inode, -1, now, false); } return blockManager.createLocatedBlocks(inode.getBlocks(), inode.computeFileSize(false), inode.isUnderConstruction(), offset, length, needBlockToken); } finally { if (attempt == 0) { readUnlock(); } else { writeUnlock(); } } } return null; // can never reach here } {code} was: If now > inode.getAccessTime() + getAccessTimePrecision() when attempt == 0, we will call dir.setTimes(src, inode, -1, now, false) without writelock. So there will be races and an ealier access time might overwrite a later access time. Looks like we need to change the code to {noformat} if (doAccessTime && isAccessTimeSupported()) { 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 (attempt == 0) { continue; } dir.setTimes(src, inode, -1, now, false); } } {noformat} Also, seems we need to release readlock before trying to acquire writelock. Otherwise, we might end up with still holding readlock after the function call. The following code is from branch-2.0.1-alpha {noformat} private LocatedBlocks getBlockLocationsUpdateTimes(String src, long offset, long length, boolean doAccessTime, boolean needBlockToken) throws FileNotFoundException, UnresolvedLinkException, IOException { for (int attempt = 0; attempt < 2; attempt++) { if (attempt == 0) { // first attempt is with readlock readLock(); } else { // second attempt is with write lock writeLock(); // writelock is needed to set accesstime } try { checkOperation(OperationCategory.READ); // if the namenode is in safemode, then do not update access time if (isInSafeMode()) { doAccessTime = false; } long now = now(); INodeFile inode = dir.getFileINode(src); if (inode == null) { throw new FileNotFoundException("File does not exist: " + src); } assert !inode.isLink(); if (doAccessTime && isAccessTimeSupported()) { 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 (attempt == 0) { continue; } } dir.setTimes(src, inode, -1, now, false); } return blockManager.createLocatedBlocks(inode.getBlocks(), inode.computeFileSize(false), inode.isUnderConstruction(), offset, length, needBlockToken); } finally { if (attempt == 0) { readUnlock(); } else { writeUnlock(); } } } return null; // can never reach here } {noformat} > access time is set without holding writelock in FSNamesystem > ------------------------------------------------------------ > > Key: HDFS-3981 > URL: https://issues.apache.org/jira/browse/HDFS-3981 > Project: Hadoop HDFS > Issue Type: Bug > Components: name-node > Affects Versions: 2.0.1-alpha > Reporter: Xiaobo Peng > Priority: Minor > > If now > inode.getAccessTime() + getAccessTimePrecision() when attempt == 0, > we will call dir.setTimes(src, inode, -1, now, false) without writelock. So > there will be races and an ealier access time might overwrite a later access > time. Looks like we need to change the code to > {noformat} > if (doAccessTime && isAccessTimeSupported()) { > 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 (attempt == 0) { > continue; > } > dir.setTimes(src, inode, -1, now, false); > } > } > {noformat} > Also, seems we need to release readlock before trying to acquire writelock. > Otherwise, we might end up with still holding readlock after the function > call. > The following code is from branch-2.0.1-alpha > {code:title=FSNamesystem.java|borderStyle=solid} > private LocatedBlocks getBlockLocationsUpdateTimes(String src, > long offset, > long length, > boolean doAccessTime, > boolean needBlockToken) > throws FileNotFoundException, UnresolvedLinkException, IOException { > for (int attempt = 0; attempt < 2; attempt++) { > if (attempt == 0) { // first attempt is with readlock > readLock(); > } else { // second attempt is with write lock > writeLock(); // writelock is needed to set accesstime > } > try { > checkOperation(OperationCategory.READ); > // if the namenode is in safemode, then do not update access time > if (isInSafeMode()) { > doAccessTime = false; > } > long now = now(); > INodeFile inode = dir.getFileINode(src); > if (inode == null) { > throw new FileNotFoundException("File does not exist: " + src); > } > assert !inode.isLink(); > if (doAccessTime && isAccessTimeSupported()) { > 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 (attempt == 0) { > continue; > } > } > dir.setTimes(src, inode, -1, now, false); > } > return blockManager.createLocatedBlocks(inode.getBlocks(), > inode.computeFileSize(false), inode.isUnderConstruction(), > offset, length, needBlockToken); > } finally { > if (attempt == 0) { > readUnlock(); > } else { > writeUnlock(); > } > } > } > return null; // can never reach here > } > {code} -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira