Thanks for the thorough review Todd!
computeDatanodeWork calls lockManager.computeReplicationWork and blockManager.computeInvalidateWork...
Agree. Added a comment in computeDatanodeWork with the rationale.
In heartbeatCheck, I think we can simply put another "if (isInSafeMode()) return" in right after it takes the writeLock if it finds a dead node...
Agree, added the additional check, and a comment above the first lock aquisition.
isLockedReadOrWrite should be checking this.fsLock.getReadHoldCount()
rather than getReadLockCount()
FSDirectory#bLock says it protects the block map, but it also protects the directory, right? we should update the comment and perhaps the name.
Correct. I renamed it dirLock to be consistent with FSNameSystem and to reflect that it protects the entire directory, not just the block map. Updated the comment too.
various functions don't take the read lock because they call functions in FSDirectory that take FSDirectory.bLock. This seems incorrect..
I think the FSN methods do need to get the readlock before calling into FSD methods that get the read lock. In the latest patch I added this to the places where it was missing (getFileInfo, getContentSummary, getPreferredBlockSize, etc), hopefully the new asserts make it clear that we have the FSN lock we calling into FSD (where necessary). For HADOOP-7362 we should assert the lock hierarchy is obeyed.
handleHeartbeat calls getDatanode() while only holding locks on heartbeats and datanodeMap, but registerDatanode mutates datanodeMap without locking either.
I modified handleHeartbeat to take the read lock so it's synchronized with register datanode. I also added a missing synchronization of datanodeMap to wipeDatanode. I think the locking in FSN needs to be revisited (eg the way heartbeats and datanodeMap are locked) need to be reconsidered in light of this locking. I'll file a jira for that, that's probably out of scope for this jira.
getDataNodeInfo seems like an unused function with no locking - can we remove it?
several other places access datanodeMap with synchronization on that object itself. unprotectedAddDatanode should assert it holds that monitor lock
Made unprotectedAddDatanode datanodeMap synchronize on datanodeMap, per above I think we need to revisit how this datastructure is accessed.
when loading the edit log, why doesn't loadFSEdits take a write lock on the namesystem before it starts? then we could add all of the asserts and not worry about it.
Done. It now takes the namesystem and dir locks, and I modifed all the unprotected methods to assert the lock is held.
it looks like saving the image no longer works, since saveFilesUnderConstruction now takes the readLock, but it's being called by a different thread than took the write lock in saveNamespace.
Yup, TestEditLogRace quickly found this deadlock. I removed the new locking from saveFilesUnderConstruction.
When create() is called with the overwrite flag true, that calls delete() which will logSync() while holding the lock. We can hold off on fixing it since it's a performance problem, not correctness, and the operation is fairly rare.
getAdditionalBlock doesn't logSync() - I think there's another issue pending about that since it will affect HA. Let's address later.
Since getAdditionalBlock doesn't do anything that writes to the log the sync is not currently needed. I updated
HDFS-978 to indicate it will be needed when block allocations are logged.
checkFileProgress doesn't really need the write lock
Yup, modified it to use a read-lock.
seems like saveNamespace could safely just take the read lock to allow other readers to keep working
Yup, modified to take the read lock.
Fixed the nits.
The tests should be passing, I'm going to do some testing with jcarder enabled. Have also run TestNNThroughputBenchmark, the numbers are noisy, mostly better than w/o the patch (not sure I believe that).