[
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
{code}
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);
}
}
{code}
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
{code}
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);
}
}
{code}
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}
> 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
> {code}
> 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);
> }
> }
> {code}
> 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
