hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiaobo Peng (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (HDFS-3981) access time is set without holding writelock in FSNamesystem
Date Wed, 26 Sep 2012 20:33:07 GMT

     [ 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. Or we could simply remove
the condition "if (attempt == 0)" for readUnlock(), i.e. readUnlock() should be called even
if attempt is 1.

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. Or we could simply remove
the condition "if (attempt == 0)" for readUnlock(), i.e. readUnlock() should be called even
if attempt is 1.
> 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

Mime
View raw message