Mailing-List: contact hdfs-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: hdfs-issues@hadoop.apache.org
Date: Thu, 27 Sep 2012 06:59:08 +1100 (NCT)
From: "Xiaobo Peng (JIRA)" <jira@apache.org>
To: hdfs-issues@hadoop.apache.org
Message-ID: <320336326.130294.1348689549003.JavaMail.jiratomcat@arcas>
In-Reply-To: <2049707075.130151.1348687748382.JavaMail.jiratomcat@arcas>
Subject: [jira] [Updated] (HDFS-3981) access time is set without holding
 writelock in FSNamesystem
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


     [ 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 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}

  was:
If now > inode.getAccessTime() + getAccessTimePrecision() when attempt == 0, we will call dir.setTimes(src, inode, -1, now, false) without writelock. So 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 code in branch-2.0.1-alpha
  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
  }


> 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 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}

--
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