Thanks Uma, agree with that it's potential risk that can cause deadlock.
It's dangerous to have lock in toString() implementation, this function is called sometimes but you may not notice.

Thanks for filing this Uma Maheswara Rao G. I suspect it applies to branch-2 as well, could you please confirm?

Yes Jitendra Nath Pandey, this applies to branch-2 as well.

Attached a patch for fixing this issue.

-1 overall

This message was automatically generated.

Thanks Uma for the patch. I agree we don't need synchronization of DataNode for Datanode#triggerBlockReport, so the current fix looks good overall. My comment is:

+      IOException exception = null;
       try {
         LOG.info("Reconfiguring " + property + " to " + newVal);
         this.refreshVolumes(newVal);
       } catch (IOException e) {
-        throw new ReconfigurationException(property, newVal,
-            getConf().get(property), e);
+        exception = e;
+      } finally {
+        // Send a full block report to let NN acknowledge the volume changes.
+        try {
+          triggerBlockReport(
+              new BlockReportOptions.Factory().setIncremental(false).build());
+        } catch (IOException e) {
+          LOG.warn("Exception while sending the block report after refresh"
+              + " volumes " + property + " to " + newVal, e);
+        } finally {
+          if (exception != null) {
+            throw new ReconfigurationException(property, newVal,
+                getConf().get(property), exception);
+          }
+        }

I think for the IOException of refreshVolumes, we just need to wrap it to ReconfigurationException then throw as original. For the exception of triggerBlockReport, we also need to wrap it and throw. I see you just log warn for the IOException of triggerBlockReport in the patch and ignore it, any reason to do this? If we want to do this, we don't need to save the IOException of refreshVolumns and throw it in the finally clause of triggerBlockReport, we just need to throw it directly, since the {{finally }} cause is executed before the throwing happens.

I agree with Yi Liu, exception could be thrown directly. I think Uma trying not to hide the original exception, in case triggerBlockReport throws any exception.

And I see, triggering block reports may not be required in case of unchanged configurations. Currently w/ or w/o patch, blockreports are getting triggered when there is nothing changed.
This also could be considered, to reduce the load on NameNode on unnecessary block report processing?

In an another thought.. How about updating the BPOS#toString() itself, without holding any lock. The return value of tostring() could be some string field which could be updated when registrationSucceeded() call comes.

In an another thought.. How about updating the BPOS#toString() itself, without holding any lock. The return value of tostring() could be some string field which could be updated when registrationSucceeded() call comes.

Good idea.

Thanks for the comments, Yi , Vinay and Colin. Here actually me and Yi discussed initially to cleanup toString and later I found triggerBlockReport itself unnecessary inside lock and me and Yi concluded we work on toString separately. So removing whole BR under lock can fix this deadlock. I just filed HDFS-9200 for tracking toString issue.

Coming to comments, Yes I was trying to avoid suppressing root exceptions but it turning to have more if check. Let me throw directly then. Also above patch missed to copy the triggerBR exception, which will miss to throw exception out if there is any exception only in BR.

Attached the patch with small fix in my previous patch. Generally finally clauses are having the risk of suppressing root exceptions. So, here I was trying retain root exception as is. Hope this patch make it more clear. I had offline chat with Yi to explain my thought on this and he agreed to that now.
-Thanks

I think it's OK to do the fix using this approach and update the BPOS#toString() in a follow-on.
The new patch looks good to me, +1 pending Jeninks, thanks Uma, Vinay, Colin.
How do you think Vinayakumar B, Colin P. McCabe?

512	          LOG.warn("Exception while sending the block report after refresh"
513	              + " volumes " + property + " to " + newVal, e);

Should be "refreshing"

+1 pending that fix and jenkins

Thanks, Colin and Yi for for the reviews. I have attached a patch here to address log message.

-1 overall

This message was automatically generated.

+1, thanks Uma, Colin and Vinay.

Committed to trunk and branch-2.

FAILURE: Integrated in Hadoop-trunk-Commit #8592 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8592/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #497 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/497/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

SUCCESS: Integrated in Hadoop-Yarn-trunk-Java8 #506 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/506/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

SUCCESS: Integrated in Hadoop-Yarn-trunk #1234 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1234/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2441 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2441/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk #2408 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2408/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #470 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/470/)
HDFS-9137. DeadLock between DataNode#refreshVolumes and (yliu: rev 35affec38e17e3f9c21d36be111171476072c03f)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java