I have purposely kept the try-catch block un-indented so that it is easier for you to review. Once the review is complete, I will upload a final patch with proper indentation.
I removed the import of the ReadWriteLock. However, I kept the fsLock initializing inside the initialize method rather than moving it to the declaration. Most variables seem to be getting initialized inside the initialize() method, isn't it?
getBlockLocations() - I now acquire the readLock and attempt to proceed ahead. If the access-time has to be set, then I release the readLock, acquire the writeLock and start all over again.
Check for safemode should be inside the FSnamesystem lock, in fact more of it is coming in
setTimes() move isAccessTimeSupported() call out side the lock : done.
CommitBlockSynchronization(): the key is to call the logSync() outside the FSnamesystem lock. And there is a LOG statement that prints out "src" after the call to logSync(). That is the reason why i had to declare "String src" right at the beginning of the method.
I agree with you that there are code portions in processReport, createSymLinkInternal, startFileInternal() that can move outside the FSNamesystem lock. However, I would like to avoid doing this code reorganizatin as part of this JIRA, especially because it makes the code difficult to review. Also, this is not a regression because the original code has all these code inside the synchronized section anyway. Please let me know if you agree on this one.
getDatanodeListForReport - move boolean and HashMap out of read lock: done.
getSafeModeTip - readLock instead of writeLock : done
chooseUnderReplicatedBlocks - should we grab readLock instead of writeLock? I wold prefer to keep the writeLock because this method updates member variables of the class, e.g. missingBlocksInPrevIter
removeStoredBlock - assert to be replaced by grab writeLock: I have the impression that all calls to removeStoredBlock already has the writeLock, that is the reason for the assert. Do you know of a code path via which this is not the case?
DecommissionManager.java - readLock instead of writeLock; The DecommissionManager.check() calls FSNamesystem.checkDecommissionStateInterna() which, in turn, invokes node.setDecommissioned(). This updates the state of the node, hence I was confortable keeping the writeLock in this code path.
BackupStorage.loadCheckPoint() is now fixed.
Do you have a suggestion on how I can fix the code in FSPermissionChecker.checkPermission()?