HDFS-16855. Remove the redundant write lock in addBlockPool. #5170

dingshun3016 · 2022-11-28T13:32:22Z

When patching the datanode's fine-grained lock, we found that the datanode couldn't start，maybe happened deadlock，when addBlockPool, so we can remove it.

org.apache.hadoop.hdfs.server.datanode.fsdataset.impl.FsDatasetImpl#addBlockPool get writeLock

org.apache.hadoop.hdfs.server.datanode.fsdataset.impl.FsDatasetImpl#deepCopyReplica need readLock

because it is not the same thread, so the write lock cannot be downgraded to a read lock

…ted by DingShun.

hadoop-yetus · 2022-11-28T22:06:44Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 50s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 2s		codespell was not available.
+0 🆗	detsecrets	0m 2s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	44m 10s		trunk passed
+1 💚	compile	1m 40s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	compile	1m 34s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 28s		trunk passed
+1 💚	mvnsite	2m 2s		trunk passed
+1 💚	javadoc	1m 34s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 48s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	4m 16s		trunk passed
+1 💚	shadedclient	28m 43s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 32s		the patch passed
+1 💚	compile	1m 34s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javac	1m 34s		the patch passed
+1 💚	compile	1m 19s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 19s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 0s		the patch passed
+1 💚	mvnsite	1m 24s		the patch passed
+1 💚	javadoc	0m 58s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 30s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 34s		the patch passed
+1 💚	shadedclient	26m 18s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
-1 ❌	unit	388m 9s	/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt	hadoop-hdfs in the patch passed.
+1 💚	asflicense	0m 54s		The patch does not generate ASF License warnings.
		513m 3s

Reason	Tests
Failed junit tests	hadoop.hdfs.TestLeaseRecovery2

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5170/1/artifact/out/Dockerfile
GITHUB PR	#5170
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 0f9749a8f192 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `54a0786`
Default Java	Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5170/1/testReport/
Max. process+thread count	2123 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5170/1/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

MingXiangLi · 2022-12-01T05:14:36Z

@dingshun3016 This seems to only happen when invoke addBlockPool() and CachingGetSpaceUsed#used < 0, so why not handle it for example like forbid refresh() when ReplicaCachingGetSpaceUsed#init() at first time ?

dingshun3016 · 2022-12-01T06:19:58Z

@dingshun3016 This seems to only happen when invoke addBlockPool() and CachingGetSpaceUsed#used < 0, so why not handle it for example like forbid refresh() when ReplicaCachingGetSpaceUsed#init() at first time ?

@MingXiangLi thanks reply

forbid refresh() when ReplicaCachingGetSpaceUsed #init() at first time，it will cause the value of dfsUsage to be 0 until the next time refresh().

if remove the BLOCK_POOl level write lock in the org.apache.hadoop.hdfs.server.datanode.fsdataset.impl.FsDatasetImpl#addBlockPool(String bpid, Configuration conf) method, what will be the impact ?

do you have any other suggestions?

MingXiangLi · 2022-12-01T08:09:52Z

The BLOCK_POOl level lock is to protect replica consistency for FsDataSetImpl when read or write operating happend at same time.

forbid refresh() when ReplicaCachingGetSpaceUsed #init() at first time，it will cause the value of dfsUsage to be 0 until the next time refresh().

For example we can use df command instead at first time or other way.

On my side It's less risky to change ReplicaCachingGetSpaceUsed logic than remove the write lock.
Or we can further discussion to make sure no case will lead to consistency problem if we remove write lock.

dingshun3016 · 2022-12-01T12:42:39Z

according to the situation discussed so far, it seems that there are several ways to solve this problem

remove the BLOCK_POOl level write lock in #addBlockPool

but worry about having replica consistency problem
forbid refresh() when ReplicaCachingGetSpaceUsed#init() at first time

it will cause the value of dfsUsage to be 0 until the next time refresh()
use du or df command instead at first time

du is very expensive and slow
df is inaccurate when the disk sharing by other servers
reference HDFS-14313

Now that, this case only happen when invoke addBlockPool() and CachingGetSpaceUsed#used < 0, I have an idea, is it possible to add a switch, not add lock when ReplicaCachingGetSpaceUsed#init() at first time , and add it at other times

do you think it's possible？@MingXiangLi

hadoop-yetus · 2022-12-01T15:15:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	1m 11s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	42m 16s		trunk passed
+1 💚	compile	1m 36s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	compile	1m 30s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 17s		trunk passed
+1 💚	mvnsite	1m 39s		trunk passed
+1 💚	javadoc	1m 18s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 41s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 51s		trunk passed
+1 💚	shadedclient	26m 27s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 23s		the patch passed
+1 💚	compile	1m 32s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javac	1m 32s		the patch passed
+1 💚	compile	1m 21s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 21s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 59s		the patch passed
+1 💚	mvnsite	1m 25s		the patch passed
+1 💚	javadoc	0m 57s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 31s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 30s		the patch passed
+1 💚	shadedclient	26m 7s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
-1 ❌	unit	415m 49s	/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt	hadoop-hdfs in the patch passed.
+1 💚	asflicense	1m 5s		The patch does not generate ASF License warnings.
		535m 40s

Reason	Tests
Failed junit tests	hadoop.hdfs.qjournal.server.TestJournalNode
	hadoop.hdfs.TestLeaseRecovery2

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5170/2/artifact/out/Dockerfile
GITHUB PR	#5170
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux c8aeace311b7 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `54a0786`
Default Java	Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5170/2/testReport/
Max. process+thread count	1906 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5170/2/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

MingXiangLi · 2022-12-02T04:18:08Z

Now that, this case only happen when invoke addBlockPool() and CachingGetSpaceUsed#used < 0, I have an idea, is it possible to add a switch, not add lock when ReplicaCachingGetSpaceUsed#init() at first time , and add it at other times

This makes sense to me, get replicas usage message no need strong consistency.@Hexiaoqiao any suggestion?

Hexiaoqiao · 2022-12-04T13:59:36Z

Now that, this case only happen when invoke addBlockPool() and CachingGetSpaceUsed#used < 0, I have an idea, is it possible to add a switch, not add lock when ReplicaCachingGetSpaceUsed#init() at first time , and add it at other times

This makes sense to me, get replicas usage message no need strong consistency.@Hexiaoqiao any suggestion?

Thanks for the detailed discussions. +1. it seems good to me.
BTW, I try to dig PR to fix this bug but no found. It just at out internal branch which not refresh space used at init stage. And refresh-used is one complete async thread (at CachingGetSpaceUsed) , thus it could not dead lock when DataNode instance restart. Thanks.

dingshun3016 · 2022-12-05T07:25:13Z

Now that, this case only happen when invoke addBlockPool() and CachingGetSpaceUsed#used < 0, I have an idea, is it possible to add a switch, not add lock when ReplicaCachingGetSpaceUsed#init() at first time , and add it at other times

This makes sense to me, get replicas usage message no need strong consistency.@Hexiaoqiao any suggestion?

Thanks for the detailed discussions. +1. it seems good to me. BTW, I try to dig PR to fix this bug but no found. It just at out internal branch which not refresh space used at init stage. And refresh-used is one complete async thread (at CachingGetSpaceUsed) , thus it could not dead lock when DataNode instance restart. Thanks.

Thanks replay. looks like this PR HDFS-14986, forbid refresh() when ReplicaCachingGetSpaceUsed#init() at first time.
I prefer to not add lock when ReplicaCachingGetSpaceUsed#init() at first time

HDFS-16855. Remove the redundant write lock in addBlockPool. Contribu…

54a0786

…ted by DingShun.

dingshun3016 closed this Dec 1, 2022

dingshun3016 reopened this Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDFS-16855. Remove the redundant write lock in addBlockPool. #5170

HDFS-16855. Remove the redundant write lock in addBlockPool. #5170

dingshun3016 commented Nov 28, 2022

hadoop-yetus commented Nov 28, 2022

MingXiangLi commented Dec 1, 2022 •

edited

dingshun3016 commented Dec 1, 2022

MingXiangLi commented Dec 1, 2022 •

edited

dingshun3016 commented Dec 1, 2022

hadoop-yetus commented Dec 1, 2022

MingXiangLi commented Dec 2, 2022 •

edited

Hexiaoqiao commented Dec 4, 2022

dingshun3016 commented Dec 5, 2022

HDFS-16855. Remove the redundant write lock in addBlockPool. #5170

Are you sure you want to change the base?

HDFS-16855. Remove the redundant write lock in addBlockPool. #5170

Conversation

dingshun3016 commented Nov 28, 2022

hadoop-yetus commented Nov 28, 2022

MingXiangLi commented Dec 1, 2022 • edited

dingshun3016 commented Dec 1, 2022

MingXiangLi commented Dec 1, 2022 • edited

dingshun3016 commented Dec 1, 2022

hadoop-yetus commented Dec 1, 2022

MingXiangLi commented Dec 2, 2022 • edited

Hexiaoqiao commented Dec 4, 2022

dingshun3016 commented Dec 5, 2022

MingXiangLi commented Dec 1, 2022 •

edited

MingXiangLi commented Dec 1, 2022 •

edited

MingXiangLi commented Dec 2, 2022 •

edited