ZanderXu commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1897678303
########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/fgl/FineGrainedFSNamesystemLock.java: ########## @@ -0,0 +1,300 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode.fgl; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hdfs.server.namenode.FSNamesystemLock; +import org.apache.hadoop.metrics2.lib.MutableRatesWithAggregation; + +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; + +/** + * Splitting the global FSN lock into FSLock and BMLock. + * FSLock is used to protect directory tree-related operations. + * BMLock is used to protect block-related and dn-related operations. + * The lock order should be: FSLock,BMLock. + */ +public class FineGrainedFSNamesystemLock implements FSNLockManager { + private final FSNamesystemLock fsLock; + private final FSNamesystemLock bmLock; + + public FineGrainedFSNamesystemLock(Configuration conf, MutableRatesWithAggregation aggregation) { + this.fsLock = new FSNamesystemLock(conf, "FS", aggregation); + this.bmLock = new FSNamesystemLock(conf, "BM", aggregation); + } + + @Override + public void readLock(FSNamesystemLockMode lockMode) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLock(); + this.bmLock.readLock(); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLock(); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLock(); + } + } + + public void readLockInterruptibly(FSNamesystemLockMode lockMode) throws InterruptedException { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLockInterruptibly(); + try { + this.bmLock.readLockInterruptibly(); + } catch (InterruptedException e) { + // The held FSLock should be released if the current thread is interrupted + // while acquiring the BMLock. + this.fsLock.readUnlock("BMReadLockInterruptiblyFailed"); + throw e; + } + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLockInterruptibly(); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLockInterruptibly(); + } + } + + @Override + public void readUnlock(FSNamesystemLockMode lockMode, String opName) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName); + this.fsLock.readUnlock(opName); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName); + } + } + + public void readUnlock(FSNamesystemLockMode lockMode, String opName, + Supplier<String> lockReportInfoSupplier) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); + this.fsLock.readUnlock(opName, lockReportInfoSupplier); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName, lockReportInfoSupplier); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); + } + } + + @Override + public void writeLock(FSNamesystemLockMode lockMode) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.writeLock(); + this.bmLock.writeLock(); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeLock(); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeLock(); + } + } + + @Override + public void writeUnlock(FSNamesystemLockMode lockMode, String opName) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.writeUnlock(opName); + this.fsLock.writeUnlock(opName); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeUnlock(opName); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeUnlock(opName); + } + } + + @Override + public void writeUnlock(FSNamesystemLockMode lockMode, String opName, + boolean suppressWriteLockReport) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.writeUnlock(opName, suppressWriteLockReport); + this.fsLock.writeUnlock(opName, suppressWriteLockReport); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeUnlock(opName, suppressWriteLockReport); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeUnlock(opName, suppressWriteLockReport); + } + } + + public void writeUnlock(FSNamesystemLockMode lockMode, String opName, + Supplier<String> lockReportInfoSupplier) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.writeUnlock(opName, lockReportInfoSupplier); + this.fsLock.writeUnlock(opName, lockReportInfoSupplier); + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeUnlock(opName, lockReportInfoSupplier); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeUnlock(opName, lockReportInfoSupplier); + } + } + + @Override + public void writeLockInterruptibly(FSNamesystemLockMode lockMode) + throws InterruptedException { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.writeLockInterruptibly(); + try { + this.bmLock.writeLockInterruptibly(); + } catch (InterruptedException e) { + // The held FSLock should be released if the current thread is interrupted + // while acquiring the BMLock. + this.fsLock.writeUnlock("BMWriteLockInterruptiblyFailed"); + throw e; + } + } else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeLockInterruptibly(); + } else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeLockInterruptibly(); + } + } + + @Override + public boolean hasWriteLock(FSNamesystemLockMode lockMode) { + if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + if (this.fsLock.isWriteLockedByCurrentThread()) { + // The bm writeLock should be held by the current thread. + assert this.bmLock.isWriteLockedByCurrentThread(); Review Comment: Thanks, @zhengchenyu , for your review. Regarding point (2), are you suggesting that the `hasWriteLock(RwLockMode lockMode)` method should strictly return `true` or `false` and never throw an `AssertionError`? Indeed, `hasWriteLock(RwLockMode lockMode)` is intended to always return `true` or `false`. However, I included the check to identify potential issue with lock ordering, because the caller often expects to receive a `true` result in NameNode. If this additional check is deemed unnecessary, the code could simply be: ``` if (lockMode.equals(RwLockMode.GLOBAL)) { return this.fsLock.isWriteLockedByCurrentThread() && this.bmLock.isWriteLockedByCurrentThread(); } ``` Looking forward your ideas, thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
