From 84c69d8bc2195e0a441c5b88bebe6d9a2535524a Mon Sep 17 00:00:00 2001 From: Andrey Yarovoy Date: Fri, 25 Sep 2026 10:27:28 -0400 Subject: [PATCH 1/2] HDDS-16289. Optimized code executed under write lock --- .../apache/hadoop/hdds/utils/db/Table.java | 22 + .../hadoop/hdds/utils/db/TypedTable.java | 39 +- .../hadoop/hdds/utils/db/TestTypedTable.java | 56 +- .../TestOmFsoWriteLockConvoyBench.java | 833 ++++++++++++++++++ .../file/OMDirectoryCreateRequestWithFSO.java | 249 ++++-- .../file/OMFileCreateRequestWithFSO.java | 325 ++++--- .../ozone/om/request/file/OMFileRequest.java | 79 +- .../key/OMKeyCommitRequestWithFSO.java | 604 ++++++++----- .../key/OMKeyDeleteRequestWithFSO.java | 323 ++++--- .../key/OMKeyRenameRequestWithFSO.java | 353 +++++--- .../diff/SnapshotDiffValueParser.java | 29 + .../key/TestOMKeyDeleteRequestWithFSO.java | 98 +++ .../key/TestOMKeyRenameRequestWithFSO.java | 112 +++ 13 files changed, 2422 insertions(+), 700 deletions(-) create mode 100644 hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestOmFsoWriteLockConvoyBench.java diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java index d4a03dbb0a3c..0173f8f9427c 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java @@ -24,12 +24,14 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.Function; import org.apache.commons.lang3.NotImplementedException; import org.apache.hadoop.hdds.annotation.InterfaceStability; import org.apache.hadoop.hdds.utils.MetadataKeyFilters.KeyPrefixFilter; import org.apache.hadoop.hdds.utils.TableCacheMetrics; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.ratis.util.function.CheckedFunction; /** * Interface for key-value store that stores ozone metadata. Ozone metadata is @@ -127,6 +129,26 @@ default VALUE getReadCopy(KEY key) throws RocksDatabaseException, CodecException throw new NotImplementedException("getReadCopy is not implemented"); } + /** + * Returns a projection of the value mapped to the given key, i.e. only the fields the caller needs, + * or null if the key is not found. Implementations may decode {@code fromPersistedValue} straight + * from the serialized value instead of materializing the whole VALUE object. + *

+ * {@code fromCachedValue} is applied to the cached object itself, without the defensive copy + * {@link #get} makes, so it must neither mutate nor retain it. + * + * @param key metadata key + * @param fromCachedValue extracts the projection from a value found in the cache + * @param fromPersistedValue extracts the projection from the serialized value read from the store + * @return the projection, or null if the key is not found. + */ + default R getProjected(KEY key, Function fromCachedValue, + CheckedFunction fromPersistedValue) + throws RocksDatabaseException, CodecException { + final VALUE value = get(key); + return value == null ? null : fromCachedValue.apply(value); + } + /** * Returns the value mapped to the given key in byte array or returns null * if the key is not found. diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java index b9c0c56febac..532a6d01dc42 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.Function; import org.apache.hadoop.hdds.utils.IOUtils; import org.apache.hadoop.hdds.utils.MetadataKeyFilters.KeyPrefixFilter; import org.apache.hadoop.hdds.utils.TableCacheMetrics; @@ -43,6 +44,7 @@ import org.apache.hadoop.hdds.utils.db.cache.TableNoCache; import org.apache.ratis.util.Preconditions; import org.apache.ratis.util.function.CheckedBiFunction; +import org.apache.ratis.util.function.CheckedFunction; import org.rocksdb.ByteBufferGetStatus; /** @@ -218,6 +220,34 @@ public VALUE get(KEY key) throws RocksDatabaseException, CodecException { } } + /** + * Same cache semantics as {@link #get(Object)}, except that a value read from the store is decoded + * by {@code fromPersistedValue} directly from the buffer it was read into, so the fields the caller + * does not need are never decoded and no VALUE object is built. + */ + @Override + public R getProjected(KEY key, Function fromCachedValue, + CheckedFunction fromPersistedValue) + throws RocksDatabaseException, CodecException { + final CacheResult cacheResult = cache.lookup(new CacheKey<>(key)); + + if (cacheResult.getCacheStatus() == EXISTS) { + return fromCachedValue.apply(cacheResult.getValue().getCacheValue()); + } else if (cacheResult.getCacheStatus() == NOT_EXIST) { + return null; + } else if (supportCodecBuffer) { + return getFromTable(key, this::getFromTable, fromPersistedValue); + } else { + final byte[] valueBytes = rawTable.get(encodeKey(key)); + if (valueBytes == null) { + return null; + } + try (CodecBuffer buffer = CodecBuffer.wrap(valueBytes)) { + return fromPersistedValue.apply(buffer); + } + } + } + /** * Skip checking cache and get the value mapped to the given key in byte * array or returns null if the key is not found. @@ -466,6 +496,13 @@ private Integer getFromTableIfExist(CodecBuffer key, CodecBuffer outValue) throw private VALUE getFromTable(KEY key, CheckedBiFunction get) throws RocksDatabaseException, CodecException { + return getFromTable(key, get, valueCodec::fromCodecBuffer); + } + + private R getFromTable(KEY key, + CheckedBiFunction get, + CheckedFunction decoder) + throws RocksDatabaseException, CodecException { try (CodecBuffer inKey = keyCodec.toDirectCodecBuffer(key)) { for (; ;) { final Integer required; @@ -482,7 +519,7 @@ private VALUE getFromTable(KEY key, for (; ;) { if (required == outValue.readableBytes()) { // buffer size is big enough - return valueCodec.fromCodecBuffer(outValue); + return decoder.apply(outValue); } // buffer size too small, try increasing the capacity. if (!outValue.setCapacity(required)) { diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestTypedTable.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestTypedTable.java index 250d221ff3ae..076ca5c75452 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestTypedTable.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestTypedTable.java @@ -59,7 +59,7 @@ */ public class TestTypedTable { private final List families = Arrays.asList(StringUtils.bytes2String(RocksDB.DEFAULT_COLUMN_FAMILY), - "First", "Second", "Third", "Fourth", "Fifth", "Sixth", "Seventh", "Eighth"); + "First", "Second", "Third", "Fourth", "Fifth", "Sixth", "Seventh", "Eighth", "Ninth"); private RDBStore rdb; private final List closeables = new ArrayList<>(); @@ -231,6 +231,60 @@ void runTestEmptyString(Codec codec) throws Exception { runTestSingleKeyValue(nonEmpty, empty, table); } + @Test + public void testGetProjectedCodecBuffer() throws Exception { + runTestGetProjected(StringCodec.get()); + } + + @Test + public void testGetProjectedByteArray() throws Exception { + final Codec codec = CodecTestUtil.newCodecWithoutCodecBuffer(StringCodec.get()); + assertFalse(codec.supportCodecBuffer()); + runTestGetProjected(codec); + } + + /** + * The two projections are tagged differently so that each assertion also pins down which path + * produced the value: the table cache, or a decode of the value read from the store. + */ + void runTestGetProjected(Codec valueCodec) throws Exception { + final TypedTable table = newTypedTable(9, StringCodec.get(), valueCodec); + + // A value larger than the initial buffer capacity, to exercise the capacity-retry loop. + final StringBuilder large = new StringBuilder(); + for (int i = 0; i < TypedTable.BUFFER_SIZE_DEFAULT; i++) { + large.append('x'); + } + + table.put("inDb", "dbValue"); + table.put("large", large.toString()); + table.put("tombstoned", "gone"); + table.addCacheEntry("inCache", "cachedValue", 1L); + table.addCacheEntry("tombstoned", 2L); + + assertNull(getProjected(table, "absent")); + assertEquals("db:dbValue", getProjected(table, "inDb")); + assertEquals("db:" + large, getProjected(table, "large")); + assertEquals("cache:cachedValue", getProjected(table, "inCache")); + // Deleted in the cache, still in the store: the projection must not fall through to the store. + assertNull(getProjected(table, "tombstoned")); + } + + @Test + public void testGetProjectedInMemoryTable() throws Exception { + final Table table = new InMemoryTestTable<>(); + table.put("key", "value"); + + // The Table default has no serialized value to project from, so it always uses fromCachedValue. + assertEquals("cache:value", getProjected(table, "key")); + assertNull(getProjected(table, "absent")); + } + + static String getProjected(Table table, String key) throws Exception { + return table.getProjected(key, value -> "cache:" + value, + buffer -> "db:" + StringCodec.get().fromCodecBuffer(buffer)); + } + @Test public void testContainerIDvsLong() throws Exception { final Map keys = newMap(1000, ContainerID::valueOf); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestOmFsoWriteLockConvoyBench.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestOmFsoWriteLockConvoyBench.java new file mode 100644 index 000000000000..8495d7d09c8c --- /dev/null +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestOmFsoWriteLockConvoyBench.java @@ -0,0 +1,833 @@ +/* + * 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.ozone.om.service; + +import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_SERVICE_INTERVAL; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_MANAGER_FAIR_LOCK; + +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Method; +import java.net.URL; +import java.net.URLClassLoader; +import java.security.AccessController; +import java.security.PrivilegedActionException; +import java.security.PrivilegedExceptionAction; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Locale; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.utils.db.CodecBuffer; +import org.apache.hadoop.hdds.utils.db.Table; +import org.apache.hadoop.hdds.utils.db.cache.CacheKey; +import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.hadoop.ozone.MiniOzoneCluster; +import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.client.OzoneBucket; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneVolume; +import org.apache.hadoop.ozone.om.OMConfigKeys; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo; +import org.apache.hadoop.ozone.om.snapshot.diff.SnapshotDiffValueParser; +import org.apache.hadoop.util.concurrent.HadoopExecutors; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Benchmark isolating the FSO create/commit write-lock reader convoy on one hot bucket (HDDS-16289). + * + *

Every OM write is applied on a single serial state-machine apply thread. On the FSO create and commit path, + * {@code OMFileCreateRequestWithFSO.validateAndUpdateCache} / {@code OMKeyCommitRequestWithFSO.validateAndUpdateCache} + * acquire the bucket write lock and then hold it across mostly read-of-committed-state work — chiefly + * {@code verifyDirectoryKeysInPath}, one RocksDB point lookup per path segment. Because the OM bucket lock is a + * non-fair {@link java.util.concurrent.locks.ReentrantReadWriteLock} (ozone.om.lock.fair=false), once the lone apply + * thread queues for the write lock every arriving reader on that bucket blocks behind it, even though in-flight + * readers drain in microseconds. On a hot bucket this freezes the read RPCs that take the bucket read lock + * (getBucketInfo, getFileStatus, lookupKey) for the whole write-lock hold. + * + *

Unlike {@link TestOmMixedWorkloadUnderDeletionBench}, whose under-load driver is a deletion backlog and whose + * client threads cycle a 1:1 read/write mix, this benchmark targets the convoy directly: + *

+ * It measures read-RPC p50/p99 in two conditions — control (readers alone) and under-load (readers while + * writers drive create/commit on the same bucket) — and reports the under-load degradation per read op. The + * client-visible read p99 degradation is the latency face of the same convoy the jstack analysis counts as readers + * blocked in {@code OzoneManagerLock.acquireLock}; an async-profiler {@code lock} recording of the under-load window + * (see below) is the thread-level face. The fix (narrowing the write lock to the cache-mutation tail) should shrink + * the read degradation and the lock-contention time without changing writer throughput. + * + *

The {@code benchmark} tag is excluded from {@code mvn test} and CI by default; run on demand (rebuild the reactor + * first to avoid stale-class errors): + * 

+ *   mvn -pl :ozone-integration-test test -DskipShade -DskipRecon \
+ *     -Dtest=TestOmFsoWriteLockConvoyBench -Dgroups=benchmark -Dexcluded-test-groups= \
+ *     -Dsurefire.failIfNoSpecifiedTests=false -Djunit.jupiter.execution.timeout.default=20m
+ *
+ * + *

Tunables: {@code bench.readerThreads} (default 8), {@code bench.writerThreads} (default 4) — both sized to + * avoid oversubscribing the host the mini-cluster shares, + * {@code bench.pathDepth} (default 16 — directory levels the create walk resolves under the write lock), + * {@code bench.windowSec} (default 150 — duration of each of the control and under-load passes, so an arm spends + * 5 minutes measuring), + * {@code bench.warmupFilesPerDir} (default 200 — files pre-staged in the read directory so read ops hit live paths). + * + *

Adding {@code -Dbench.profile.event=lock} profiles only the under-load window with async-profiler, loaded + * reflectively from a local install supplied via {@code -Dbench.profiler.jar} and {@code -Dbench.profiler.lib}; the + * JFR is written under {@code -Dbench.profile.out} (default {@code /tmp}). The sampling interval is + * {@code -Dbench.profile.interval} (default {@code 1ms}; raise it for {@code wall}, which samples every live thread) + * and {@code -Dbench.profile.opts} appends further async-profiler options. {@code lock} shows who waits for the lock, + * {@code wall} on the {@code OMStateMachineApplyTransactionThread} shows what consumes the hold time; + * {@code cpu}/{@code wall}/{@code alloc} also work. For accurate leaf frames also pass + * {@code -DargLine="-XX:+UnlockDiagnosticVMOptions -XX:+DebugNonSafepoints"}. + */ +@Tag("benchmark") +public class TestOmFsoWriteLockConvoyBench { + + private static final Logger LOG = LoggerFactory.getLogger(TestOmFsoWriteLockConvoyBench.class); + + private static final String OP_GETBUCKETINFO = "getbucketinfo"; + private static final String OP_GETFILESTATUS = "getfilestatus"; + private static final String OP_LOOKUPKEY = "lookupkey"; + // The read ops each take the bucket READ lock; these are the callers that dominated the observed convoy. + private static final String[] READ_OPS = {OP_GETBUCKETINFO, OP_GETFILESTATUS, OP_LOOKUPKEY}; + + // The write ops the writers cycle, one per narrowed FSO apply path: create covers both the file create and the key + // commit transaction, mkdir the directory create, rename the key rename. + private static final String[] WRITE_OPS = {"create", "mkdir", "rename", "delete"}; + + // One hot bucket. The deep read/write subtrees live under it so create walks and read resolutions hit the same + // bucket stripe the convoy forms on. + private static final String READ_ROOT = "convoy/read"; + private static final String WRITE_ROOT = "convoy/write"; + + /** + * Removes test-harness-only overhead that would distort the measured lock/apply cost: the mini-cluster enables + * {@link CodecBuffer} leak detection (a per-allocation finalizer) and runs the CodecBuffer/managed-RocksDB loggers + * at DEBUG/TRACE (a stack trace per allocation), neither of which a production OM at INFO does. JVM-global, so the + * returned action restores them once the benchmark is done. + */ + private static Runnable stripTestOnlyOverhead() { + org.apache.log4j.Logger codecBufferLogger = + org.apache.log4j.Logger.getLogger("org.apache.hadoop.hdds.utils.db.CodecBuffer"); + org.apache.log4j.Logger managedRocksLogger = + org.apache.log4j.Logger.getLogger("org.apache.hadoop.hdds.utils.db.managed"); + org.apache.log4j.Level codecBufferLevel = codecBufferLogger.getLevel(); + org.apache.log4j.Level managedRocksLevel = managedRocksLogger.getLevel(); + + CodecBuffer.disableLeakDetection(); + codecBufferLogger.setLevel(org.apache.log4j.Level.INFO); + managedRocksLogger.setLevel(org.apache.log4j.Level.INFO); + + return () -> { + CodecBuffer.enableLeakDetection(); + codecBufferLogger.setLevel(codecBufferLevel); + managedRocksLogger.setLevel(managedRocksLevel); + }; + } + + @Test + // Two windows plus cluster start and deep pre-staging run well past the 5m + // junit.jupiter.execution.timeout.default that pom.xml pins in surefire's , which the + // JUnit platform resolves ahead of any -D system property. Only a method-level @Timeout overrides it. + @Timeout(value = 30, unit = TimeUnit.MINUTES) + public void benchmarkFsoWriteLockConvoy() throws Exception { + final String profileEvent = System.getProperty("bench.profile.event", ""); + // Defaults sized for a laptop-class host: 8 readers + 4 writers leaves cores for the in-JVM mini-cluster (OM + // handlers, Ratis, datanodes) instead of oversubscribing it, which adds scheduler latency to every sample and + // shows up as noise in both arms. Raise bench.readerThreads on a machine with cores to spare. + final int readerThreads = Integer.getInteger("bench.readerThreads", 8); + final int writerThreads = Integer.getInteger("bench.writerThreads", 4); + final int pathDepth = Integer.getInteger("bench.pathDepth", 16); + // Per window; an arm runs the control and under-load windows back to back, so 150s = 5 minutes measured per arm. + final int windowSec = Integer.getInteger("bench.windowSec", 150); + final int warmupFilesPerDir = Integer.getInteger("bench.warmupFilesPerDir", 200); + // Convoy-amplification levers (config/workload route). A mini-cluster cannot reach the production write-lock hold + // duration (its DB is tiny and cache-warm), so instead of inflating the hold we amplify how much a reader suffers + // per collision and shrink the pool headroom that absorbs one: + // - fairLock=true makes the non-fair->fair switch: once the apply thread is QUEUED for the bucket write lock, + // every arriving reader blocks behind it, so a convoy forms at far lower hold/occupancy. Run both modes. + // - a small OM read pool / handler count reproduces the blast radius (HDDS-16596): with little headroom a modest + // convoy exhausts the pool and read p99 explodes at lower absolute contention. + final boolean fairLock = Boolean.getBoolean("bench.fairLock"); + final int omReadThreads = Integer.getInteger("bench.omReadThreads", 0); + final int omHandlers = Integer.getInteger("bench.omHandlers", 0); + + OzoneConfiguration conf = new OzoneConfiguration(); + // Push both deletion services far past the window: the only bucket write-lock holder under measurement must be + // FSO create/commit, never a purge transaction. + conf.setTimeDuration(OMConfigKeys.OZONE_DIR_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS); + conf.setTimeDuration(OZONE_BLOCK_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS); + conf.setBoolean(OZONE_MANAGER_FAIR_LOCK, fairLock); + if (omReadThreads > 0) { + conf.setInt(OMConfigKeys.OZONE_OM_READ_THREADPOOL_KEY, omReadThreads); + } + if (omHandlers > 0) { + conf.setInt(OMConfigKeys.OZONE_OM_HANDLER_COUNT_KEY, omHandlers); + } + + MiniOzoneCluster cluster = MiniOzoneCluster.newBuilder(conf) + .setNumDatanodes(3) + .build(); + Runnable restoreTestOnlyOverhead = stripTestOnlyOverhead(); + try { + cluster.waitForClusterToBeReady(); + try (OzoneClient client = cluster.newClient()) { + OzoneBucket bucket = org.apache.hadoop.ozone.DataTestUtil.createVolumeAndBucket(client, + BucketLayout.FILE_SYSTEM_OPTIMIZED); + OzoneVolume volume = client.getObjectStore().getVolume(bucket.getVolumeName()); + final String bucketName = bucket.getName(); + FileSystem fs = rootedFs(conf, volume.getName(), bucketName); + try { + // Pre-stage a deep read directory the readers resolve against, so getFileStatus/lookupKey hit live paths + // at full depth (their own path walk under the read lock), and the create walk below hits the same depth. + Path readDir = deepDir(READ_ROOT, pathDepth); + fs.mkdirs(readDir); + for (int i = 0; i < warmupFilesPerDir; i++) { + fs.create(new Path(readDir, "f" + i), true).close(); + } + String readKeyName = READ_ROOT + depthSuffix(pathDepth) + "/f0"; + fs.mkdirs(deepDir(WRITE_ROOT, pathDepth)); + + // Control: readers alone, no writer holding the bucket write lock. + Percentiles[] control; + try (RunningReaders readers = startReaders(fs, volume, bucketName, readKeyName, readerThreads)) { + Thread.sleep(TimeUnit.SECONDS.toMillis(windowSec)); + readers.stop(); + control = toPercentiles(readers.await(), "control"); + } + + Profiler profiler = profileEvent.isEmpty() ? null : Profiler.load(); + String profileOut = null; + if (profiler != null) { + profileOut = System.getProperty("bench.profile.out", "/tmp") + "/prof-convoy-" + profileEvent + ".jfr"; + profiler.start(profileEvent, profileOut); + } + + // Under load: readers while writers drive create/commit at depth on the same bucket, so the apply thread + // repeatedly takes the bucket write lock across the path walk and the readers queue behind it. + Percentiles[] underLoad; + long[] writerOps; + try (RunningReaders readers = startReaders(fs, volume, bucketName, readKeyName, readerThreads); + RunningWriters writers = startWriters(conf, volume.getName(), bucketName, pathDepth, writerThreads)) { + Thread.sleep(TimeUnit.SECONDS.toMillis(windowSec)); + writers.stop(); + readers.stop(); + if (profiler != null) { + profiler.stop(); + } + writerOps = writers.await(); + underLoad = toPercentiles(readers.await(), "under-load"); + } + + long writerTotal = 0; + for (long ops : writerOps) { + writerTotal += ops; + } + String header = String.format(Locale.ROOT, + "BENCH convoy fairLock=%b omReadThreads=%d omHandlers=%d readerThreads=%d writerThreads=%d " + + "pathDepth=%d windowSec=%d writerOps=%d writerCreate=%d writerMkdir=%d writerRename=%d " + + "writerDelete=%d", + fairLock, omReadThreads, omHandlers, readerThreads, writerThreads, pathDepth, windowSec, + writerTotal, writerOps[0], writerOps[1], writerOps[2], writerOps[3]); + printBenchLine(control, underLoad, header); + if (profileOut != null) { + System.out.printf(Locale.ROOT, "BENCH profile event=%s out=%s%n", profileEvent, profileOut); + } + } finally { + org.apache.hadoop.io.IOUtils.closeStream(fs); + } + } + } finally { + try { + cluster.shutdown(); + } finally { + restoreTestOnlyOverhead.run(); + } + } + } + + /** + * Micro-benchmark of the FSO path walk itself (HDDS-16289), with no writers and no lock contention in play. It + * measures what it costs to resolve the parent objectIDs of a depth-{@code bench.pathDepth} path on a live OM -- + * the work {@code OMFileRequest.getOMKeyInfoIfExists} / {@code verifyDirectoryKeysInPath} / {@code getParentID} do + * once per path segment, on the apply thread for every FSO write and on a handler thread for every + * {@code getFileStatus} and {@code lookupKey}. + * + *

Both arms run in the same JVM against the same OM RocksDB and the same table cache, interleaved round by round + * so neither JIT state nor thermal drift can favour one of them: + *

+ * {@link #resolveParentId} holds the one walk body both arms share, so they differ only in that single call. + * + *

Single-threaded on purpose: this isolates the per-operation cost of the change. Its effect on read latency and + * throughput under concurrency is what {@link #benchmarkFsoWriteLockConvoy} measures. + * + *

The projection only pays on a table-cache miss -- on a hit both arms return the cached {@code OmDirectoryInfo} + * and do identical work -- so the staged tree is first left to flush out of the OM double buffer + * ({@code bench.walkSettleSec}), which is also the steady state a production OM serves reads from. + * + *

Tunables: {@code bench.pathDepth} (default 16), {@code bench.walkRounds} (default 6), + * {@code bench.walkIterations} (default 20000 walks per arm per round), {@code bench.walkWarmup} (default 5000), + * {@code bench.walkSettleSec} (default 10). + */ + @Test + @Timeout(value = 20, unit = TimeUnit.MINUTES) + public void benchmarkFsoWalkProjection() throws Exception { + final int pathDepth = Integer.getInteger("bench.pathDepth", 16); + final int rounds = Integer.getInteger("bench.walkRounds", 6); + final int iterations = Integer.getInteger("bench.walkIterations", 20000); + final int warmupIterations = Integer.getInteger("bench.walkWarmup", 5000); + final int settleSec = Integer.getInteger("bench.walkSettleSec", 10); + + OzoneConfiguration conf = new OzoneConfiguration(); + // Keep the deletion services out of the measurement window, as the convoy benchmark does. + conf.setTimeDuration(OMConfigKeys.OZONE_DIR_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS); + conf.setTimeDuration(OZONE_BLOCK_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS); + + MiniOzoneCluster cluster = MiniOzoneCluster.newBuilder(conf) + .setNumDatanodes(3) + .build(); + Runnable restoreTestOnlyOverhead = stripTestOnlyOverhead(); + try { + cluster.waitForClusterToBeReady(); + try (OzoneClient client = cluster.newClient()) { + OzoneBucket bucket = org.apache.hadoop.ozone.DataTestUtil.createVolumeAndBucket(client, + BucketLayout.FILE_SYSTEM_OPTIMIZED); + final String volumeName = bucket.getVolumeName(); + final String bucketName = bucket.getName(); + FileSystem fs = rootedFs(conf, volumeName, bucketName); + try { + fs.mkdirs(deepDir(READ_ROOT, pathDepth)); + } finally { + org.apache.hadoop.io.IOUtils.closeStream(fs); + } + + OMMetadataManager metadataManager = cluster.getOzoneManager().getMetadataManager(); + final long volumeId = metadataManager.getVolumeId(volumeName); + final long bucketId = metadataManager.getBucketId(volumeName, bucketName); + final String[] pathElements = pathElements(READ_ROOT, pathDepth); + + Thread.sleep(TimeUnit.SECONDS.toMillis(settleSec)); + int cacheResidentSegments = + countCacheResidentSegments(metadataManager, volumeId, bucketId, pathElements); + + List full = new ArrayList<>(rounds * iterations); + List projected = new ArrayList<>(rounds * iterations); + runWalks(metadataManager, volumeId, bucketId, pathElements, false, warmupIterations, null); + runWalks(metadataManager, volumeId, bucketId, pathElements, true, warmupIterations, null); + for (int round = 0; round < rounds; round++) { + // Alternate which arm goes first so neither always gets the warmer or the cooler slot of a round. + boolean projectedFirst = (round & 1) == 1; + runWalks(metadataManager, volumeId, bucketId, pathElements, projectedFirst, iterations, + projectedFirst ? projected : full); + runWalks(metadataManager, volumeId, bucketId, pathElements, !projectedFirst, iterations, + projectedFirst ? full : projected); + } + printWalkBenchLine(pathDepth, rounds, iterations, settleSec, cacheResidentSegments, + full, projected); + } + } finally { + try { + cluster.shutdown(); + } finally { + restoreTestOnlyOverhead.run(); + } + } + } + + /** Runs {@code iterations} full path walks, recording one nanosecond sample per walk when {@code sink} is given. */ + private static void runWalks(OMMetadataManager metadataManager, long volumeId, long bucketId, String[] pathElements, + boolean projected, int iterations, List sink) throws IOException { + for (int i = 0; i < iterations; i++) { + long start = System.nanoTime(); + long leafObjectId = resolveParentId(metadataManager, volumeId, bucketId, pathElements, projected); + long elapsedNs = System.nanoTime() - start; + if (leafObjectId == 0) { + throw new IllegalStateException("path walk did not resolve; the staged directory tree is missing"); + } + if (sink != null) { + sink.add(elapsedNs); + } + } + } + + /** + * Resolves the objectID of the last of {@code pathElements} the way the FSO walks do: one dirTable lookup per + * segment, each keyed by the objectID the previous segment resolved to. The {@code projected} arm is the read the + * production walks do after HDDS-16289; the other is the full read it replaced. Returns 0 if the path does not + * resolve. + */ + private static long resolveParentId(OMMetadataManager metadataManager, long volumeId, long bucketId, + String[] pathElements, boolean projected) throws IOException { + Table dirTable = metadataManager.getDirectoryTable(); + long lastKnownParentId = bucketId; + for (String pathElement : pathElements) { + String dbNodeName = metadataManager.getOzonePathKey(volumeId, bucketId, lastKnownParentId, pathElement); + final Long objectId; + if (projected) { + objectId = dirTable.getProjected(dbNodeName, OmDirectoryInfo::getObjectID, + SnapshotDiffValueParser::parseDirectoryInfoObjectId); + } else { + OmDirectoryInfo omDirInfo = dirTable.get(dbNodeName); + objectId = omDirInfo == null ? null : omDirInfo.getObjectID(); + } + if (objectId == null) { + return 0; + } + lastKnownParentId = objectId; + } + return lastKnownParentId; + } + + /** + * Counts how many of the walked segments are still resident in the dirTable cache. The projection only pays on a + * cache miss, so a non-zero count means that many segments are being compared on the identical cache-hit path -- + * reported so that an equal-arms result is read as "the tree had not flushed yet", not as "the change does nothing". + */ + private static int countCacheResidentSegments(OMMetadataManager metadataManager, long volumeId, long bucketId, + String[] pathElements) throws IOException { + Table dirTable = metadataManager.getDirectoryTable(); + long lastKnownParentId = bucketId; + int resident = 0; + for (String pathElement : pathElements) { + String dbNodeName = metadataManager.getOzonePathKey(volumeId, bucketId, lastKnownParentId, pathElement); + CacheValue cached = dirTable.getCacheValue(new CacheKey<>(dbNodeName)); + if (cached != null && cached.getCacheValue() != null) { + resident++; + } + OmDirectoryInfo omDirInfo = dirTable.get(dbNodeName); + if (omDirInfo == null) { + break; + } + lastKnownParentId = omDirInfo.getObjectID(); + } + return resident; + } + + /** The path elements of {@code root} followed by {@code depth} nested directories, in walk order. */ + private static String[] pathElements(String root, int depth) { + List elements = new ArrayList<>(Arrays.asList(root.split("/"))); + for (int d = 0; d < depth; d++) { + elements.add("d" + d); + } + return elements.toArray(new String[0]); + } + + /** Prints the {@code BENCH walk} lines: per-walk and per-segment cost of both arms, plus the projected/full ratio. */ + private static void printWalkBenchLine(int pathDepth, int rounds, int iterations, int settleSec, + int cacheResidentSegments, List full, List projected) { + final int segments = pathDepth + READ_ROOT.split("/").length; + Percentiles fullPercentiles = Percentiles.of(full); + Percentiles projectedPercentiles = Percentiles.of(projected); + double fullMeanNs = meanNs(full); + double projectedMeanNs = meanNs(projected); + System.out.printf(Locale.ROOT, + "BENCH walk pathDepth=%d segments=%d rounds=%d iterationsPerRound=%d settleSec=%d cacheResidentSegments=%d%n" + + "BENCH walk full n=%d meanUs=%.2f p50Us=%.2f p90Us=%.2f p99Us=%.2f perSegmentNs=%.0f%n" + + "BENCH walk projected n=%d meanUs=%.2f p50Us=%.2f p90Us=%.2f p99Us=%.2f perSegmentNs=%.0f%n" + + "BENCH walk delta meanPct=%+.1f%% p50Pct=%+.1f%% p99Pct=%+.1f%% speedup=%.2fx%n", + pathDepth, segments, rounds, iterations, settleSec, cacheResidentSegments, + fullPercentiles.count, fullMeanNs / 1000.0, fullPercentiles.p50 * 1000.0, fullPercentiles.p90 * 1000.0, + fullPercentiles.p99 * 1000.0, fullMeanNs / segments, + projectedPercentiles.count, projectedMeanNs / 1000.0, projectedPercentiles.p50 * 1000.0, + projectedPercentiles.p90 * 1000.0, projectedPercentiles.p99 * 1000.0, projectedMeanNs / segments, + 100.0 * (projectedMeanNs - fullMeanNs) / fullMeanNs, + 100.0 * (projectedPercentiles.p50 - fullPercentiles.p50) / fullPercentiles.p50, + 100.0 * (projectedPercentiles.p99 - fullPercentiles.p99) / fullPercentiles.p99, + safeRatio(fullMeanNs, projectedMeanNs)); + } + + private static double meanNs(List samplesNs) { + if (samplesNs.isEmpty()) { + return Double.NaN; + } + long total = 0; + for (long sample : samplesNs) { + total += sample; + } + return (double) total / samplesNs.size(); + } + + /** One {@code o3fs} FileSystem rooted at the given bucket. */ + private static FileSystem rootedFs(OzoneConfiguration conf, String volumeName, String bucketName) + throws IOException { + OzoneConfiguration bucketConf = new OzoneConfiguration(conf); + bucketConf.set(FS_DEFAULT_NAME_KEY, + String.format("%s://%s.%s/", OzoneConsts.OZONE_URI_SCHEME, bucketName, volumeName)); + return FileSystem.get(bucketConf); + } + + private static Path deepDir(String root, int depth) { + return new Path("/" + root + depthSuffix(depth)); + } + + private static String depthSuffix(int depth) { + StringBuilder sb = new StringBuilder(); + for (int d = 0; d < depth; d++) { + sb.append("/d").append(d); + } + return sb.toString(); + } + + /** + * Starts {@code threads} reader threads that loop the {@link #READ_OPS} mix on the hot bucket until stopped, + * recording per-op nanosecond samples. getBucketInfo and lookupKey go through the client (bucket read lock); + * getFileStatus resolves the pre-staged deep path. + */ + private RunningReaders startReaders(FileSystem fs, OzoneVolume volume, String bucketName, String readKeyName, + int threads) { + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch startLatch = new CountDownLatch(1); + AtomicBoolean running = new AtomicBoolean(true); + AtomicBoolean failed = new AtomicBoolean(false); + List>> futures = new ArrayList<>(threads); + Path readFile = new Path("/" + readKeyName); + for (int t = 0; t < threads; t++) { + futures.add(pool.submit((Callable>) () -> { + List samples = new ArrayList<>(1 << 16); + startLatch.await(); + for (int i = 0; running.get(); i++) { + int opIdx = i % READ_OPS.length; + long t0 = System.nanoTime(); + try { + switch (opIdx) { + case 0: + volume.getBucket(bucketName); + break; + case 1: + fs.getFileStatus(readFile); + break; + default: + volume.getBucket(bucketName).getKey(readKeyName); + break; + } + } catch (IOException | RuntimeException e) { + failed.set(true); + throw e; + } + samples.add(new long[] {opIdx, System.nanoTime() - t0}); + } + return samples; + })); + } + startLatch.countDown(); + return new RunningReaders(pool, futures, running, failed); + } + + /** + * Starts {@code threads} writer threads that cycle the {@link #WRITE_OPS} mix at {@code pathDepth} into a per-thread + * deep subtree until stopped, forcing the apply thread to hold the bucket write lock across a + * depth-{@code pathDepth} path walk on every op. All five narrowed FSO apply paths are driven: create covers the + * file create and the key commit, mkdir the directory create, rename the key rename, delete the key delete. + * Returns the per-op counts + * (index-aligned with {@link #WRITE_OPS}). Each writer uses its own FileSystem so the client side does not + * serialize. + */ + private RunningWriters startWriters(OzoneConfiguration conf, String volumeName, String bucketName, int pathDepth, + int threads) throws IOException { + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch startLatch = new CountDownLatch(1); + AtomicBoolean running = new AtomicBoolean(true); + AtomicLong[] opCounts = new AtomicLong[WRITE_OPS.length]; + for (int op = 0; op < opCounts.length; op++) { + opCounts[op] = new AtomicLong(); + } + List writerFs = new ArrayList<>(threads); + List> futures = new ArrayList<>(threads); + for (int t = 0; t < threads; t++) { + final int writerId = t; + FileSystem fs = rootedFs(conf, volumeName, bucketName); + writerFs.add(fs); + Path writerDir = new Path(deepDir(WRITE_ROOT, pathDepth), "w" + writerId); + fs.mkdirs(writerDir); + futures.add(pool.submit(() -> { + long[] ops = new long[WRITE_OPS.length]; + Path lastCreated = null; + Path lastDir = null; + startLatch.await(); + for (int i = 0; running.get(); i++) { + int opIdx = i % WRITE_OPS.length; + if (opIdx == 1) { + lastDir = new Path(writerDir, "d" + i); + fs.mkdirs(lastDir); + ops[1]++; + } else if (opIdx == 2 && lastCreated != null) { + // Rename the file this thread created on its previous create op. + fs.rename(lastCreated, new Path(writerDir, "f" + i + "-r")); + lastCreated = null; + ops[2]++; + } else if (opIdx == 3 && lastDir != null) { + // Delete the (empty) directory this thread made on its previous mkdir op, non-recursively. On an FSO + // bucket BasicRootedOzoneFileSystem sends that as a single DeleteKey with recursive=false, so the apply + // path runs OMFileRequest.hasChildren -- a full scan of the dirTable and fileTable caches plus two + // RocksDB seeks, the largest of the narrowed write-lock holds. Deleting a file instead would skip + // hasChildren and add a createFakeParentDirectory round trip, so the directory case is both the + // expensive one and the cleaner one to measure. + fs.delete(lastDir, false); + lastDir = null; + ops[3]++; + } else { + // Create (and commit) a file; also the fallback when no file is pending, so the cycle never renames a + // missing path. + lastCreated = new Path(writerDir, "f" + i); + fs.create(lastCreated, true).close(); + ops[0]++; + } + } + for (int op = 0; op < ops.length; op++) { + opCounts[op].addAndGet(ops[op]); + } + return null; + })); + } + startLatch.countDown(); + return new RunningWriters(pool, futures, running, opCounts, writerFs); + } + + /** Handle to the reader pool: stop, then join and collect per-op samples. */ + private static final class RunningReaders implements AutoCloseable { + private final ExecutorService pool; + private final List>> futures; + private final AtomicBoolean running; + private final AtomicBoolean failed; + + RunningReaders(ExecutorService pool, List>> futures, AtomicBoolean running, + AtomicBoolean failed) { + this.pool = pool; + this.futures = futures; + this.running = running; + this.failed = failed; + } + + void stop() { + running.set(false); + } + + @Override + public void close() { + stop(); + HadoopExecutors.shutdown(pool, LOG, 60, TimeUnit.SECONDS); + } + + List> await() throws Exception { + List> byOp = new ArrayList<>(READ_OPS.length); + for (String ignored : READ_OPS) { + byOp.add(new ArrayList<>()); + } + for (Future> future : futures) { + for (long[] sample : future.get()) { + byOp.get((int) sample[0]).add(sample[1]); + } + } + if (failed.get()) { + throw new IllegalStateException("reader thread failed"); + } + return byOp; + } + } + + /** Handle to the writer pool: stop, then join and return the per-op counts. Closes the writer FileSystems. */ + private static final class RunningWriters implements AutoCloseable { + private final ExecutorService pool; + private final List> futures; + private final AtomicBoolean running; + private final AtomicLong[] opCounts; + private final List writerFs; + + RunningWriters(ExecutorService pool, List> futures, AtomicBoolean running, AtomicLong[] opCounts, + List writerFs) { + this.pool = pool; + this.futures = futures; + this.running = running; + this.opCounts = opCounts; + this.writerFs = writerFs; + } + + void stop() { + running.set(false); + } + + long[] await() throws Exception { + for (Future future : futures) { + future.get(); + } + long[] counts = new long[opCounts.length]; + for (int op = 0; op < counts.length; op++) { + counts[op] = opCounts[op].get(); + } + return counts; + } + + @Override + public void close() { + stop(); + HadoopExecutors.shutdown(pool, LOG, 120, TimeUnit.SECONDS); + for (FileSystem fs : writerFs) { + org.apache.hadoop.io.IOUtils.closeStream(fs); + } + } + } + + private static double safeRatio(double num, double den) { + return den <= 0 ? Double.NaN : num / den; + } + + /** Reduces per-op nanosecond samples to {@link Percentiles} (index-aligned with {@link #READ_OPS}). */ + private static Percentiles[] toPercentiles(List> byOp, String label) { + Percentiles[] result = new Percentiles[READ_OPS.length]; + for (int op = 0; op < READ_OPS.length; op++) { + result[op] = Percentiles.of(byOp.get(op)); + LOG.info("{} {}: ops={} p50Ms={} p99Ms={}", label, READ_OPS[op], byOp.get(op).size(), + result[op].p50, result[op].p99); + } + return result; + } + + /** Appends per-op control/under-load percentiles and degradation ratios and prints the {@code BENCH convoy} line. */ + private static void printBenchLine(Percentiles[] control, Percentiles[] underLoad, String header) { + StringBuilder line = new StringBuilder(header); + for (int op = 0; op < READ_OPS.length; op++) { + line.append(String.format(Locale.ROOT, " %s[controlN=%d underLoadN=%d " + + "control_p50=%.3f control_p90=%.3f control_p95=%.3f control_p99=%.3f " + + "underLoad_p50=%.3f underLoad_p90=%.3f underLoad_p95=%.3f underLoad_p99=%.3f " + + "deg50=%.2fx deg90=%.2fx deg95=%.2fx deg99=%.2fx]", + READ_OPS[op], control[op].count, underLoad[op].count, + control[op].p50, control[op].p90, control[op].p95, control[op].p99, + underLoad[op].p50, underLoad[op].p90, underLoad[op].p95, underLoad[op].p99, + safeRatio(underLoad[op].p50, control[op].p50), safeRatio(underLoad[op].p90, control[op].p90), + safeRatio(underLoad[op].p95, control[op].p95), safeRatio(underLoad[op].p99, control[op].p99))); + } + System.out.println(line); + } + + /** + * Thin reflective wrapper over async-profiler's {@code one.profiler.AsyncProfiler}, loaded at runtime from the + * configured jar so the benchmark carries no compile-time dependency on async-profiler. + */ + private static final class Profiler { + private final Object delegate; + private final Method execute; + + private Profiler(Object delegate, Method execute) { + this.delegate = delegate; + this.execute = execute; + } + + static Profiler load() throws Exception { + String jar = requireProfilerPath("bench.profiler.jar"); + String lib = requireProfilerPath("bench.profiler.lib"); + try { + return AccessController.doPrivileged((PrivilegedExceptionAction) () -> { + URLClassLoader loader = new URLClassLoader(new URL[] {new File(jar).toURI().toURL()}, + Profiler.class.getClassLoader()); + Class clazz = Class.forName("one.profiler.AsyncProfiler", true, loader); + Object instance = clazz.getMethod("getInstance", String.class).invoke(null, lib); + return new Profiler(instance, clazz.getMethod("execute", String.class)); + }); + } catch (PrivilegedActionException e) { + throw (Exception) e.getCause(); + } + } + + private static String requireProfilerPath(String property) { + String value = System.getProperty(property); + if (value == null || value.isEmpty()) { + throw new IllegalStateException("bench.profile.event is set but " + property + " is not; point it at your " + + "local async-profiler install (jar and native library) to enable profiling"); + } + return value; + } + + void start(String event, String jfrFile) throws Exception { + String interval = System.getProperty("bench.profile.interval", "1ms"); + String extra = System.getProperty("bench.profile.opts", ""); + execute.invoke(delegate, String.format(Locale.ROOT, "start,event=%s,interval=%s%s,file=%s", + event, interval, extra.isEmpty() ? "" : "," + extra, jfrFile)); + } + + void stop() throws Exception { + execute.invoke(delegate, "stop"); + } + } + + /** + * p50/p90/p95/p99 in milliseconds over a set of nanosecond latency samples, plus the sample count. The write lock + * is held for only a small fraction of the window, so a reader stall shows up in the upper percentiles while p50 + * stays flat; p90 and p95 are reported so that shape is visible rather than inferred from p99 alone. + */ + private static final class Percentiles { + private final int count; + private final double p50; + private final double p90; + private final double p95; + private final double p99; + + private Percentiles(int count, double p50, double p90, double p95, double p99) { + this.count = count; + this.p50 = p50; + this.p90 = p90; + this.p95 = p95; + this.p99 = p99; + } + + static Percentiles of(List samplesNs) { + if (samplesNs.isEmpty()) { + return new Percentiles(0, Double.NaN, Double.NaN, Double.NaN, Double.NaN); + } + long[] sorted = samplesNs.stream().mapToLong(Long::longValue).sorted().toArray(); + return new Percentiles(sorted.length, quantile(sorted, 0.50), quantile(sorted, 0.90), + quantile(sorted, 0.95), quantile(sorted, 0.99)); + } + + private static double quantile(long[] sorted, double q) { + return sorted[Math.min(sorted.length - 1, (int) (sorted.length * q))] / 1_000_000.0; + } + } +} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java index 77974fbf3541..d02b1ad868df 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java @@ -30,7 +30,6 @@ import java.nio.file.Paths; import java.util.List; import java.util.Map; -import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.OMAction; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OMMetrics; @@ -43,7 +42,6 @@ import org.apache.hadoop.ozone.om.request.util.OmResponseUtil; import org.apache.hadoop.ozone.om.response.OMClientResponse; import org.apache.hadoop.ozone.om.response.file.OMDirectoryCreateResponseWithFSO; -import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CreateDirectoryRequest; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CreateDirectoryResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyArgs; @@ -86,16 +84,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut OMMetrics omMetrics = ozoneManager.getMetrics(); omMetrics.incNumCreateDirectory(); - AuditLogger auditLogger = ozoneManager.getAuditLogger(); - OzoneManagerProtocolProtos.UserInfo userInfo = getOmRequest().getUserInfo(); - Map auditMap = buildKeyArgsAuditMap(keyArgs); OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); boolean acquiredLock = false; Exception exception = null; OMClientResponse omClientResponse = null; Result result = Result.FAILURE; - List missingParentInfos; try { // Check if this is the root of the filesystem. @@ -104,73 +98,30 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut "directory at root of the filesystem", OMException.ResultCodes.CANNOT_CREATE_DIRECTORY_AT_ROOT); } - // acquire lock - mergeOmLockDetails( - omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, - bucketName)); - acquiredLock = getOmLockDetails().isLockAcquired(); - - validateBucketAndVolume(omMetadataManager, volumeName, bucketName); - - Path keyPath = Paths.get(keyName); - - // Need to check if any files exist in the given path, if they exist we - // cannot create a directory with the given key. - // Verify the path against directory table - OMFileRequest.OMPathInfoWithFSO omPathInfo = - OMFileRequest.verifyDirectoryKeysInPath(omMetadataManager, volumeName, - bucketName, keyName, keyPath); - OMFileRequest.OMDirectoryResult omDirectoryResult = - omPathInfo.getDirectoryResult(); - - if (omDirectoryResult == FILE_EXISTS || - omDirectoryResult == FILE_EXISTS_IN_GIVENPATH) { - throw new OMException("Unable to create directory: " + keyName - + " in volume/bucket: " + volumeName + "/" + bucketName + " as " + - "file:" + omPathInfo.getFileExistsInPath() + " already exists", - FILE_ALREADY_EXISTS); - } else if (omDirectoryResult == DIRECTORY_EXISTS_IN_GIVENPATH || - omDirectoryResult == NONE) { - - OmBucketInfo omBucketInfo = - getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); - // prepare all missing parents - missingParentInfos = getAllMissingParentDirInfo( - ozoneManager, keyArgs, omBucketInfo, omPathInfo, trxnLogIndex); - - final long volumeId = omMetadataManager.getVolumeId(volumeName); - final long bucketId = omMetadataManager - .getBucketId(volumeName, bucketName); - - // total number of keys created. - numKeysCreated = missingParentInfos.size() + 1; - checkBucketQuotaInNamespace(omBucketInfo, numKeysCreated); - omBucketInfo.incrUsedNamespace(numKeysCreated); - - // prepare leafNode dir - OmDirectoryInfo dirInfo = createDirectoryInfoWithACL( - omPathInfo.getLeafNodeName(), - keyArgs, omPathInfo.getLeafNodeObjectId(), - omPathInfo.getLastKnownParentId(), trxnLogIndex, - omBucketInfo, omPathInfo, ozoneManager.getConfig()); - OMFileRequest.addDirectoryTableCacheEntries(omMetadataManager, - volumeId, bucketId, trxnLogIndex, - missingParentInfos, dirInfo); - - // Publish only here: createDirectoryInfoWithACL above can still fail with UNAUTHORIZED. - omMetadataManager.getBucketTable().addCacheEntry( - omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); - result = OMDirectoryCreateRequest.Result.SUCCESS; - omClientResponse = - new OMDirectoryCreateResponseWithFSO(omResponse.build(), - volumeId, bucketId, dirInfo, missingParentInfos, result, - getBucketLayout(), omBucketInfo.copyObject()); - } else { + PreparedDirCreate prepared = + prepareDirectoryCreate(ozoneManager, keyArgs, trxnLogIndex); + if (prepared == null) { + // The directory already exists: nothing to mutate, so no bucket lock is taken at all. result = Result.DIRECTORY_ALREADY_EXISTS; omResponse.setStatus(Status.DIRECTORY_ALREADY_EXISTS); omClientResponse = new OMDirectoryCreateResponseWithFSO(omResponse.build(), result); + } else { + numKeysCreated = prepared.numKeysCreated; + + // Phase 2 (bucket write lock): re-check the leaf, then apply the cache mutations and publish + // the quota copy. The lock is held only for this mutation tail, so bucket read-lock holders + // still observe each transaction atomically (all mutations or none), but are no longer + // blocked for the Phase 1 path walk. + mergeOmLockDetails( + omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, + bucketName)); + acquiredLock = getOmLockDetails().isLockAcquired(); + + omClientResponse = applyDirectoryCreate(omMetadataManager, keyArgs, + prepared, trxnLogIndex, omResponse); + result = OMDirectoryCreateRequest.Result.SUCCESS; } } catch (IOException | InvalidPathException ex) { exception = ex; @@ -186,24 +137,168 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut } } - markForAudit(auditLogger, buildAuditMessage(OMAction.CREATE_DIRECTORY, - auditMap, exception, userInfo)); - - logResult(createDirectoryRequest, keyArgs, omMetrics, numKeysCreated, - result, exception); + auditAndLogResult(ozoneManager, createDirectoryRequest, auditMap, exception, result, numKeysCreated); return omClientResponse; } - private void logResult(CreateDirectoryRequest createDirectoryRequest, - KeyArgs keyArgs, OMMetrics omMetrics, int numKeys, - Result result, - Exception exception) { + /** + * Phase 1 (no bucket lock): resolves the path and prepares the leaf directory, the missing parent + * directories and the namespace delta. All of this reads committed state only. The OM apply path is + * single-threaded (OzoneManagerStateMachine uses a single-thread executor), so no other transaction + * can change what these reads observe before Phase 2 mutates; the double-buffer flush/cleanup + * threads only materialize already-committed epochs and never alter a key's visible value. + *

+ * Keeping this walk out of the bucket write lock is the point of HDDS-16289: it stops the lone apply + * thread from gating readers of a hot bucket (getBucketInfo/getFileStatus/lookupKey) during the + * per-segment path resolution. If OM ever applies transactions in parallel per bucket/key, these + * reads must be re-validated under the lock in {@link #applyDirectoryCreate}. + * + * @return the prepared directory, or {@code null} when the directory already exists, in which case + * the caller reports DIRECTORY_ALREADY_EXISTS without taking the lock + */ + private PreparedDirCreate prepareDirectoryCreate(OzoneManager ozoneManager, + KeyArgs keyArgs, long trxnLogIndex) throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); + + Path keyPath = Paths.get(keyName); + + // Need to check if any files exist in the given path, if they exist we + // cannot create a directory with the given key. + // Verify the path against directory table + OMFileRequest.OMPathInfoWithFSO omPathInfo = + OMFileRequest.verifyDirectoryKeysInPath(omMetadataManager, volumeName, + bucketName, keyName, keyPath); + OMFileRequest.OMDirectoryResult omDirectoryResult = + omPathInfo.getDirectoryResult(); + + if (omDirectoryResult == FILE_EXISTS || + omDirectoryResult == FILE_EXISTS_IN_GIVENPATH) { + throw new OMException("Unable to create directory: " + keyName + + " in volume/bucket: " + volumeName + "/" + bucketName + " as " + + "file:" + omPathInfo.getFileExistsInPath() + " already exists", + FILE_ALREADY_EXISTS); + } + if (omDirectoryResult != DIRECTORY_EXISTS_IN_GIVENPATH && + omDirectoryResult != NONE) { + return null; + } + + final long volumeId = omMetadataManager.getVolumeId(volumeName); + final long bucketId = omMetadataManager + .getBucketId(volumeName, bucketName); + + // Read-only copy, used here only to inherit ACLs. The quota read-modify-publish in Phase 2 takes + // its own getBucketInfoForUpdate copy under the lock. + OmBucketInfo bucketInfo = omMetadataManager.getBucketTable() + .get(omMetadataManager.getBucketKey(volumeName, bucketName)); + // prepare all missing parents + List missingParentInfos = getAllMissingParentDirInfo( + ozoneManager, keyArgs, bucketInfo, omPathInfo, trxnLogIndex); + + // prepare leafNode dir. Keep after getAllMissingParentDirInfo, which sets the leaf node object id + // and the last known parent id on omPathInfo. This can still fail with UNAUTHORIZED, so it must + // precede the bucket publish; running it in Phase 1 assures that. + OmDirectoryInfo dirInfo = createDirectoryInfoWithACL( + omPathInfo.getLeafNodeName(), + keyArgs, omPathInfo.getLeafNodeObjectId(), + omPathInfo.getLastKnownParentId(), trxnLogIndex, + bucketInfo, omPathInfo, ozoneManager.getConfig()); + // total number of keys created. + return new PreparedDirCreate(volumeId, bucketId, omPathInfo, missingParentInfos, dirInfo, + missingParentInfos.size() + 1); + } + + /** + * Phase 2 (under the bucket write lock): re-checks the leaf resolved in Phase 1, then applies the + * directory-table cache entries and the bucket namespace increment and publishes the bucket copy. + */ + private OMClientResponse applyDirectoryCreate(OMMetadataManager omMetadataManager, + KeyArgs keyArgs, PreparedDirCreate prepared, long trxnLogIndex, + OMResponse.Builder omResponse) throws IOException { String volumeName = keyArgs.getVolumeName(); String bucketName = keyArgs.getBucketName(); String keyName = keyArgs.getKeyName(); + // Cheap O(1) re-check at the leaf of what the Phase 1 walk resolved. Under serial apply this + // always holds; it is a tripwire that fails rather than creating a duplicate if that invariant is + // ever broken by a concurrent writer. + final String dbLeafKey = omMetadataManager.getOzonePathKey(prepared.volumeId, + prepared.bucketId, prepared.omPathInfo.getLastKnownParentId(), + prepared.omPathInfo.getLeafNodeName()); + if (omMetadataManager.getDirectoryTable().get(dbLeafKey) != null) { + throw new OMException("Unable to create directory: " + keyName + + " in volume/bucket: " + volumeName + "/" + bucketName + " as it already exists", + OMException.ResultCodes.DIRECTORY_ALREADY_EXISTS); + } + if (omMetadataManager.getKeyTable(getBucketLayout()).get(dbLeafKey) != null) { + throw new OMException("Unable to create directory: " + keyName + + " in volume/bucket: " + volumeName + "/" + bucketName + + " as a file already exists at that path", FILE_ALREADY_EXISTS); + } + + OmBucketInfo omBucketInfo = + getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); + checkBucketQuotaInNamespace(omBucketInfo, prepared.numKeysCreated); + omBucketInfo.incrUsedNamespace(prepared.numKeysCreated); + + OMFileRequest.addDirectoryTableCacheEntries(omMetadataManager, + prepared.volumeId, prepared.bucketId, trxnLogIndex, + prepared.missingParentInfos, prepared.dirInfo); + + omMetadataManager.getBucketTable().addCacheEntry( + omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); + + return new OMDirectoryCreateResponseWithFSO(omResponse.build(), + prepared.volumeId, prepared.bucketId, prepared.dirInfo, + prepared.missingParentInfos, Result.SUCCESS, + getBucketLayout(), omBucketInfo.copyObject()); + } + + /** + * Phase 1 output of {@link #prepareDirectoryCreate}, consumed by {@link #applyDirectoryCreate} + * under the bucket write lock: the resolved path, the leaf and missing parent directories to cache + * and the namespace delta to charge. + */ + private static final class PreparedDirCreate { + private final long volumeId; + private final long bucketId; + private final OMFileRequest.OMPathInfoWithFSO omPathInfo; + private final List missingParentInfos; + private final OmDirectoryInfo dirInfo; + private final int numKeysCreated; + + PreparedDirCreate(long volumeId, long bucketId, OMFileRequest.OMPathInfoWithFSO omPathInfo, + List missingParentInfos, OmDirectoryInfo dirInfo, int numKeysCreated) { + this.volumeId = volumeId; + this.bucketId = bucketId; + this.omPathInfo = omPathInfo; + this.missingParentInfos = missingParentInfos; + this.dirInfo = dirInfo; + this.numKeysCreated = numKeysCreated; + } + } + + /** + * Emits the audit log and the result log outside the bucket lock. + */ + private void auditAndLogResult(OzoneManager ozoneManager, CreateDirectoryRequest createDirectoryRequest, + Map auditMap, Exception exception, Result result, int numKeys) { + markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(OMAction.CREATE_DIRECTORY, + auditMap, exception, getOmRequest().getUserInfo())); + + KeyArgs keyArgs = createDirectoryRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + OMMetrics omMetrics = ozoneManager.getMetrics(); + switch (result) { case SUCCESS: omMetrics.incNumKeys(numKeys); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java index 18f506cf728e..bd0f4a1c57a3 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java @@ -65,7 +65,6 @@ public OMFileCreateRequestWithFSO(OMRequest omRequest, } @Override - @SuppressWarnings("methodlength") public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { final long trxnLogIndex = context.getIndex(); @@ -77,27 +76,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut String bucketName = keyArgs.getBucketName(); String keyName = keyArgs.getKeyName(); - // if isRecursive is true, file would be created even if parent - // directories does not exist. - boolean isRecursive = createFileRequest.getIsRecursive(); if (LOG.isDebugEnabled()) { LOG.debug("File create for : " + volumeName + "/" + bucketName + "/" - + keyName + ":" + isRecursive); + + keyName + ":" + createFileRequest.getIsRecursive()); } - // if isOverWrite is true, file would be over written. - boolean isOverWrite = createFileRequest.getIsOverwrite(); - OMMetrics omMetrics = ozoneManager.getMetrics(); omMetrics.incNumCreateFile(); OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); boolean acquiredLock = false; - - OmBucketInfo omBucketInfo = null; - final List locations = new ArrayList<>(); - List missingParentInfos; int numKeysCreated = 0; OMClientResponse omClientResponse = null; @@ -112,114 +101,20 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut OMException.ResultCodes.NOT_A_FILE); } - // acquire lock + PreparedFileCreate prepared = + prepareFileCreate(ozoneManager, createFileRequest, trxnLogIndex); + numKeysCreated = prepared.missingParentInfos.size(); + + // Phase 2 (bucket write lock): re-check the overwrite invariant, then apply the cache + // mutations and publish the quota copy. The lock is held only for this mutation tail, so + // bucket read-lock holders still observe each transaction atomically (all mutations or none), + // but are no longer blocked for the Phase 1 path walk. mergeOmLockDetails(omMetadataManager.getLock() .acquireWriteLock(BUCKET_LOCK, volumeName, bucketName)); acquiredLock = getOmLockDetails().isLockAcquired(); - validateBucketAndVolume(omMetadataManager, volumeName, bucketName); - - final long volumeId = omMetadataManager.getVolumeId(volumeName); - final long bucketId = omMetadataManager - .getBucketId(volumeName, bucketName); - - OmKeyInfo dbFileInfo = null; - - OMFileRequest.OMPathInfoWithFSO pathInfoFSO = - OMFileRequest.verifyDirectoryKeysInPath(omMetadataManager, - volumeName, bucketName, keyName, Paths.get(keyName)); - - if (pathInfoFSO.getDirectoryResult() - == OMFileRequest.OMDirectoryResult.FILE_EXISTS) { - String dbFileKey = omMetadataManager.getOzonePathKey(volumeId, bucketId, - pathInfoFSO.getLastKnownParentId(), - pathInfoFSO.getLeafNodeName()); - dbFileInfo = OMFileRequest.getOmKeyInfoFromFileTable(false, - omMetadataManager, dbFileKey, keyName); - } - - // check if the file or directory already existed in OM - checkDirectoryResult(keyName, isOverWrite, - pathInfoFSO.getDirectoryResult()); - - if (!isRecursive) { - checkAllParentsExist(keyArgs, pathInfoFSO); - } - - // do open key - OmBucketInfo bucketInfo = omMetadataManager.getBucketTable().get( - omMetadataManager.getBucketKey(volumeName, bucketName)); - // add all missing parents to dir table - - missingParentInfos = getAllMissingParentDirInfo( - ozoneManager, keyArgs, bucketInfo, pathInfoFSO, trxnLogIndex); - - // total number of keys created. - numKeysCreated = missingParentInfos.size(); - - final ReplicationConfig repConfig = OzoneConfigUtil - .resolveReplicationConfigPreference(keyArgs.getType(), - keyArgs.getFactor(), keyArgs.getEcReplicationConfig(), - bucketInfo.getDefaultReplicationConfig(), - ozoneManager); - - OmKeyInfo omFileInfo = prepareFileInfo(omMetadataManager, keyArgs, - dbFileInfo, keyArgs.getDataSize(), locations, - getFileEncryptionInfo(keyArgs), ozoneManager.getPrefixManager(), - bucketInfo, pathInfoFSO, trxnLogIndex, - pathInfoFSO.getLeafNodeObjectId(), - repConfig, ozoneManager.getConfig()); - validateEncryptionKeyInfo(bucketInfo, keyArgs); - - long openVersion = omFileInfo.getLatestVersionLocations().getVersion(); - long clientID = createFileRequest.getClientID(); - String dbOpenFileName = omMetadataManager - .getOpenFileName(volumeId, bucketId, - pathInfoFSO.getLastKnownParentId(), - pathInfoFSO.getLeafNodeName(), clientID); - - // Append new blocks - List newLocationList = keyArgs.getKeyLocationsList() - .stream().map(OmKeyLocationInfo::getFromProtobuf) - .collect(Collectors.toList()); - omFileInfo.appendNewBlocks(newLocationList, false); - - omBucketInfo = getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); - // check bucket and volume quota - long preAllocatedSpace = - newLocationList.size() * ozoneManager.getScmBlockSize() * repConfig - .getRequiredNodes(); - checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, - preAllocatedSpace); - checkBucketQuotaInNamespace(omBucketInfo, numKeysCreated + 1L); - omBucketInfo.incrUsedNamespace(numKeysCreated); - - // Add to cache entry can be done outside of lock for this openKey. - // Even if bucket gets deleted, when commitKey we shall identify if - // bucket gets deleted. - OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, - dbOpenFileName, omFileInfo, keyName, trxnLogIndex); - - // Add cache entries for the prefix directories. - // Skip adding for the file key itself, until Key Commit. - OMFileRequest.addDirectoryTableCacheEntries(omMetadataManager, volumeId, - bucketId, trxnLogIndex, missingParentInfos, null); - - omMetadataManager.getBucketTable().addCacheEntry( - omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); - - // Prepare response. Sets user given full key name in the 'keyName' - // attribute in response object. - int clientVersion = getOmRequest().getVersion(); - omResponse.setCreateFileResponse(CreateFileResponse.newBuilder() - .setKeyInfo(omFileInfo.getNetworkProtobuf(keyName, clientVersion, - keyArgs.getLatestVersionLocation())) - .setID(clientID) - .setOpenVersion(openVersion).build()) - .setCmdType(Type.CreateFile); - omClientResponse = new OMFileCreateResponseWithFSO(omResponse.build(), - omFileInfo, missingParentInfos, clientID, - omBucketInfo.copyObject(), volumeId); + omClientResponse = applyFileCreate(omMetadataManager, createFileRequest, + prepared, trxnLogIndex, omResponse); result = Result.SUCCESS; } catch (IOException | InvalidPathException ex) { @@ -239,14 +134,206 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut } } - // Audit Log outside the lock + auditAndLogResult(ozoneManager, createFileRequest, auditMap, exception, result, numKeysCreated); + + return omClientResponse; + } + + /** + * Phase 1 (no bucket lock): resolves the path and prepares the open-file entry, the missing parent + * directories and the quota delta. All of this reads committed state only. The OM apply path is + * single-threaded (OzoneManagerStateMachine uses a single-thread executor), so no other transaction + * can change what these reads observe before Phase 2 mutates; the double-buffer flush/cleanup + * threads only materialize already-committed epochs and never alter a key's visible value. + *

+ * Keeping this walk out of the bucket write lock is the point of HDDS-16289: it stops the lone apply + * thread from gating readers of a hot bucket (getBucketInfo/getFileStatus/lookupKey) during the + * per-segment path resolution. If OM ever applies transactions in parallel per bucket/key, these + * reads must be re-validated under the lock in {@link #applyFileCreate}. + */ + private PreparedFileCreate prepareFileCreate(OzoneManager ozoneManager, + CreateFileRequest createFileRequest, long trxnLogIndex) throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + KeyArgs keyArgs = createFileRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + // if isOverWrite is true, file would be over written. + boolean isOverWrite = createFileRequest.getIsOverwrite(); + + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); + + final long volumeId = omMetadataManager.getVolumeId(volumeName); + final long bucketId = omMetadataManager + .getBucketId(volumeName, bucketName); + + OmKeyInfo dbFileInfo = null; + + OMFileRequest.OMPathInfoWithFSO pathInfoFSO = + OMFileRequest.verifyDirectoryKeysInPath(omMetadataManager, + volumeName, bucketName, keyName, Paths.get(keyName)); + + final String dbFileKey = omMetadataManager.getOzonePathKey(volumeId, + bucketId, pathInfoFSO.getLastKnownParentId(), + pathInfoFSO.getLeafNodeName()); + if (pathInfoFSO.getDirectoryResult() + == OMFileRequest.OMDirectoryResult.FILE_EXISTS) { + dbFileInfo = OMFileRequest.getOmKeyInfoFromFileTable(false, + omMetadataManager, dbFileKey, keyName); + } + + // check if the file or directory already existed in OM + checkDirectoryResult(keyName, isOverWrite, + pathInfoFSO.getDirectoryResult()); + + // if isRecursive is true, file would be created even if parent + // directories does not exist. + if (!createFileRequest.getIsRecursive()) { + checkAllParentsExist(keyArgs, pathInfoFSO); + } + + // do open key + // Read-only copy, used here only to inherit ACLs, encryption and replication defaults. The quota + // read-modify-publish in Phase 2 takes its own getBucketInfoForUpdate copy under the lock. + OmBucketInfo bucketInfo = omMetadataManager.getBucketTable().get( + omMetadataManager.getBucketKey(volumeName, bucketName)); + // add all missing parents to dir table + + List missingParentInfos = getAllMissingParentDirInfo( + ozoneManager, keyArgs, bucketInfo, pathInfoFSO, trxnLogIndex); + + final ReplicationConfig repConfig = OzoneConfigUtil + .resolveReplicationConfigPreference(keyArgs.getType(), + keyArgs.getFactor(), keyArgs.getEcReplicationConfig(), + bucketInfo.getDefaultReplicationConfig(), + ozoneManager); + + OmKeyInfo omFileInfo = prepareFileInfo(omMetadataManager, keyArgs, + dbFileInfo, keyArgs.getDataSize(), new ArrayList<>(), + getFileEncryptionInfo(keyArgs), ozoneManager.getPrefixManager(), + bucketInfo, pathInfoFSO, trxnLogIndex, + pathInfoFSO.getLeafNodeObjectId(), + repConfig, ozoneManager.getConfig()); + validateEncryptionKeyInfo(bucketInfo, keyArgs); + + String dbOpenFileName = omMetadataManager + .getOpenFileName(volumeId, bucketId, + pathInfoFSO.getLastKnownParentId(), + pathInfoFSO.getLeafNodeName(), createFileRequest.getClientID()); + + // Append new blocks + List newLocationList = keyArgs.getKeyLocationsList() + .stream().map(OmKeyLocationInfo::getFromProtobuf) + .collect(Collectors.toList()); + omFileInfo.appendNewBlocks(newLocationList, false); + + final long preAllocatedSpace = + newLocationList.size() * ozoneManager.getScmBlockSize() * repConfig + .getRequiredNodes(); + + return new PreparedFileCreate(volumeId, bucketId, dbFileKey, omFileInfo, + missingParentInfos, dbOpenFileName, preAllocatedSpace); + } + + /** + * Phase 2 (under the bucket write lock): re-checks the overwrite guard resolved in Phase 1, then + * applies the open-file and directory cache entries and the bucket quota charge and publishes the + * bucket copy. + */ + private OMClientResponse applyFileCreate(OMMetadataManager omMetadataManager, + CreateFileRequest createFileRequest, PreparedFileCreate prepared, long trxnLogIndex, + OMResponse.Builder omResponse) throws IOException { + KeyArgs keyArgs = createFileRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + + // Cheap O(1) re-check of the overwrite guard resolved in Phase 1. Under serial apply this always + // holds; it is a tripwire that fails safe (as checkDirectoryResult would) rather than silently + // overwriting if that invariant is ever broken by a concurrent writer. + if (!createFileRequest.getIsOverwrite() && OMFileRequest.getOmKeyInfoFromFileTable(false, + omMetadataManager, prepared.dbFileKey, keyName) != null) { + throw new OMException("File " + keyName + " already exists", + OMException.ResultCodes.FILE_ALREADY_EXISTS); + } + + OmBucketInfo omBucketInfo = getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); + // check bucket and volume quota + checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, + prepared.preAllocatedSpace); + final int numKeysCreated = prepared.missingParentInfos.size(); + checkBucketQuotaInNamespace(omBucketInfo, numKeysCreated + 1L); + omBucketInfo.incrUsedNamespace(numKeysCreated); + + OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, + prepared.dbOpenFileName, prepared.omFileInfo, keyName, trxnLogIndex); + + // Add cache entries for the prefix directories. + // Skip adding for the file key itself, until Key Commit. + OMFileRequest.addDirectoryTableCacheEntries(omMetadataManager, prepared.volumeId, + prepared.bucketId, trxnLogIndex, prepared.missingParentInfos, null); + + omMetadataManager.getBucketTable().addCacheEntry( + omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); + + // Prepare response. Sets user given full key name in the 'keyName' + // attribute in response object. + int clientVersion = getOmRequest().getVersion(); + long openVersion = prepared.omFileInfo.getLatestVersionLocations().getVersion(); + omResponse.setCreateFileResponse(CreateFileResponse.newBuilder() + .setKeyInfo(prepared.omFileInfo.getNetworkProtobuf(keyName, clientVersion, + keyArgs.getLatestVersionLocation())) + .setID(createFileRequest.getClientID()) + .setOpenVersion(openVersion).build()) + .setCmdType(Type.CreateFile); + return new OMFileCreateResponseWithFSO(omResponse.build(), + prepared.omFileInfo, prepared.missingParentInfos, createFileRequest.getClientID(), + omBucketInfo.copyObject(), prepared.volumeId); + } + + /** + * Phase 1 output of {@link #prepareFileCreate}, consumed by {@link #applyFileCreate} under the + * bucket write lock: the resolved path keys, the open file and missing parent directories to cache, + * and the quota deltas to charge. + */ + private static final class PreparedFileCreate { + private final long volumeId; + private final long bucketId; + private final String dbFileKey; + private final OmKeyInfo omFileInfo; + private final List missingParentInfos; + private final String dbOpenFileName; + private final long preAllocatedSpace; + + PreparedFileCreate(long volumeId, long bucketId, String dbFileKey, OmKeyInfo omFileInfo, + List missingParentInfos, String dbOpenFileName, long preAllocatedSpace) { + this.volumeId = volumeId; + this.bucketId = bucketId; + this.dbFileKey = dbFileKey; + this.omFileInfo = omFileInfo; + this.missingParentInfos = missingParentInfos; + this.dbOpenFileName = dbOpenFileName; + this.preAllocatedSpace = preAllocatedSpace; + } + } + + /** + * Emits the audit log and the result log outside the bucket lock. + */ + private void auditAndLogResult(OzoneManager ozoneManager, CreateFileRequest createFileRequest, + Map auditMap, Exception exception, Result result, int numKeysCreated) { markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage( OMAction.CREATE_FILE, auditMap, exception, getOmRequest().getUserInfo())); + KeyArgs keyArgs = createFileRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + switch (result) { case SUCCESS: - omMetrics.incNumKeys(numKeysCreated); + ozoneManager.getMetrics().incNumKeys(numKeysCreated); LOG.debug("File created. Volume:{}, Bucket:{}, Key:{}", volumeName, bucketName, keyName); break; @@ -258,7 +345,5 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut LOG.error("Unrecognized Result for OMFileCreateRequest: {}", createFileRequest); } - - return omClientResponse; } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java index 372e59317322..512c4d6dce8b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java @@ -53,6 +53,7 @@ import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; import org.apache.hadoop.ozone.om.helpers.OzoneFSUtils; import org.apache.hadoop.ozone.om.helpers.OzoneFileStatus; +import org.apache.hadoop.ozone.om.snapshot.diff.SnapshotDiffValueParser; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -154,6 +155,21 @@ public static OMPathInfo verifyFilesInPath( return new OMPathInfo(missing, OMDirectoryResult.NONE, acls); } + /** + * Reads only the objectID of the directory stored under the given directory table key, without + * materializing an {@link OmDirectoryInfo}. The path walks below need each parent directory's + * objectID, but a full read also decodes and rebuilds the ACL list, the metadata map and the + * timestamps, which they discard. + * + * @return the directory's objectID, or null if there is no directory under that key. + */ + private static Long getDirObjectId(OMMetadataManager omMetadataManager, String dbNodeName) + throws IOException { + return omMetadataManager.getDirectoryTable().getProjected(dbNodeName, + OmDirectoryInfo::getObjectID, + SnapshotDiffValueParser::parseDirectoryInfoObjectId); + } + /** * Verify any dir/key exist in the given path in the specified * volume/bucket by iterating through directory table. @@ -190,6 +206,9 @@ public static OMPathInfoWithFSO verifyDirectoryKeysInPath( // Get parent all acls including ACCESS and DEFAULT acls // The logic of specific inherited acl should be when creating dir/file List acls = omBucketInfo.getAcls(); + // Directory table key of the deepest existing directory on the path, i.e. the one the ACLs above + // are replaced with below; null means the path has no such directory and the bucket ACLs apply. + String aclParentDbKey = null; long lastKnownParentId = omBucketInfo.getObjectID(); StringBuilder dbDirName = new StringBuilder(); // absolute path for trace logs @@ -216,14 +235,14 @@ public static OMPathInfoWithFSO verifyDirectoryKeysInPath( // 3. Add 'sub-dir' to missing parents list String dbNodeName = omMetadataManager.getOzonePathKey(volumeId, bucketId, lastKnownParentId, fileName); - OmDirectoryInfo omDirInfo = omMetadataManager.getDirectoryTable(). - get(dbNodeName); - if (omDirInfo != null) { - dbDirName.append(omDirInfo.getName()).append(OzoneConsts.OZONE_URI_DELIMITER); + final Long dirObjectId = getDirObjectId(omMetadataManager, dbNodeName); + if (dirObjectId != null) { + // dbNodeName ends with the directory's name, so fileName is what the stored name field holds. + dbDirName.append(fileName).append(OzoneConsts.OZONE_URI_DELIMITER); if (elements.hasNext()) { result = OMDirectoryResult.DIRECTORY_EXISTS_IN_GIVENPATH; - lastKnownParentId = omDirInfo.getObjectID(); - acls = omDirInfo.getAcls(); + lastKnownParentId = dirObjectId; + aclParentDbKey = dbNodeName; continue; } else { // Checked all the sub-dirs till the leaf node. @@ -257,6 +276,15 @@ public static OMPathInfoWithFSO verifyDirectoryKeysInPath( LOG.trace("verifyFiles/Directories in Path : /{}/{}/{} : {}", volumeName, bucketName, keyName, result); + // Inherited ACLs come from the deepest existing directory on the path, so they are read once here + // instead of on every segment of the walk above. + if (aclParentDbKey != null) { + OmDirectoryInfo aclParent = omMetadataManager.getDirectoryTable().get(aclParentDbKey); + if (aclParent != null) { + acls = aclParent.getAcls(); + } + } + if (result == OMDirectoryResult.FILE_EXISTS_IN_GIVENPATH || result == OMDirectoryResult.FILE_EXISTS) { return new OMPathInfoWithFSO(leafNodeName, lastKnownParentId, missing, @@ -676,24 +704,33 @@ public static OzoneFileStatus getOMKeyInfoIfExists( String dbNodeName = omMetadataMgr.getOzonePathKey( volumeId, omBucketInfo.getObjectID(), lastKnownParentId, fileName); - omDirInfo = omMetadataMgr.getDirectoryTable().get(dbNodeName); - if (omDirInfo != null) { - lastKnownParentId = omDirInfo.getObjectID(); - } else if (!elements.hasNext() && - (!keyName.endsWith(PATH_SEPARATOR_STR))) { - // If the requested keyName contains "/" at the end then we need to - // just check the directory table. + if (elements.hasNext()) { + // Intermediate path component: only its objectID is needed to resolve the next one. + final Long dirObjectId = getDirObjectId(omMetadataMgr, dbNodeName); + if (dirObjectId == null) { + // Missing intermediate directory and just return null; + // key not found in DB + return null; + } + lastKnownParentId = dirObjectId; + continue; + } + + // Last path component: the whole directory is returned to the caller, so read it in full. + omDirInfo = omMetadataMgr.getDirectoryTable().get(dbNodeName); + if (omDirInfo == null) { + if (keyName.endsWith(PATH_SEPARATOR_STR)) { + // If the requested keyName contains "/" at the end then we need to + // just check the directory table. + return null; + } // reached last path component. Check file exists for the given path. OmKeyInfo omKeyInfo = OMFileRequest.getOmKeyInfoFromFileTable(false, omMetadataMgr, dbNodeName, keyName); if (omKeyInfo != null) { return new OzoneFileStatus(omKeyInfo, scmBlockSize, false); } - } else { - // Missing intermediate directory and just return null; - // key not found in DB - return null; } } @@ -1016,7 +1053,6 @@ public static long getParentID(long volumeId, long bucketId, String keyName, if (StringUtils.isBlank(errMsg)) { errMsg = "Failed to find parent directory of " + keyName; } - OmDirectoryInfo omDirectoryInfo; while (pathComponents.hasNext()) { String nodeName = pathComponents.next().toString(); boolean reachedLastPathComponent = !pathComponents.hasNext(); @@ -1025,15 +1061,14 @@ public static long getParentID(long volumeId, long bucketId, String keyName, lastKnownParentId, nodeName); - omDirectoryInfo = omMetadataManager. - getDirectoryTable().get(dbNodeName); - if (omDirectoryInfo != null) { + final Long dirObjectId = getDirObjectId(omMetadataManager, dbNodeName); + if (dirObjectId != null) { if (reachedLastPathComponent) { throw new OMException("Can not create file: " + keyName + " as there is already directory in the given path", NOT_A_FILE); } - lastKnownParentId = omDirectoryInfo.getObjectID(); + lastKnownParentId = dirObjectId; } else { // One of the sub-dir doesn't exists in DB. Immediate parent should // exists for committing the key, otherwise will fail the operation. diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java index 8e21cc8a4156..09d9a2fa5fde 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java @@ -30,7 +30,6 @@ import java.util.Map; import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.ozone.OzoneConsts; -import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.OMAction; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OMMetrics; @@ -75,7 +74,6 @@ public OMKeyCommitRequestWithFSO(OMRequest omRequest, } @Override - @SuppressWarnings("methodlength") public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { final long trxnLogIndex = context.getIndex(); @@ -89,16 +87,13 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut OMMetrics omMetrics = ozoneManager.getMetrics(); - AuditLogger auditLogger = ozoneManager.getAuditLogger(); - Map auditMap = buildKeyArgsAuditMap(commitKeyArgs); OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder( getOmRequest()); Exception exception = null; - OmKeyInfo omKeyInfo = null; - OmBucketInfo omBucketInfo; + PreparedCommit prepared = new PreparedCommit(); OMClientResponse omClientResponse = null; boolean bucketLockAcquired = false; Result result; @@ -120,275 +115,416 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); try { - String dbOpenFileKey = null; - - List - locationInfoList = getOmKeyLocationInfos(ozoneManager, commitKeyArgs); + prepareCommit(ozoneManager, commitKeyRequest, prepared, + getOmKeyLocationInfos(ozoneManager, commitKeyArgs), auditMap, trxnLogIndex); + // Phase 2 (bucket write lock): re-check the commit target, then apply the quota + // read-modify-publish and all cache mutations. The lock covers only this mutation tail, so + // bucket read-lock holders still observe each transaction atomically (all mutations or none) + // but are not blocked for the Phase 1 path walk. mergeOmLockDetails(omMetadataManager.getLock() .acquireWriteLock(BUCKET_LOCK, volumeName, bucketName)); bucketLockAcquired = getOmLockDetails().isLockAcquired(); - validateBucketAndVolume(omMetadataManager, volumeName, bucketName); - omBucketInfo = getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); - - String errMsg = "Cannot create file : " + keyName - + " as parent directory doesn't exist"; - OmFSOFile fsoFile = new OmFSOFile.Builder() - .setVolumeName(volumeName) - .setBucketName(bucketName) - .setKeyName(keyName) - .setOmMetadataManager(omMetadataManager) - .setErrMsg(errMsg) - .build(); + omClientResponse = applyKeyCommit(ozoneManager, commitKeyRequest, prepared, + trxnLogIndex, omResponse); - String fileName = fsoFile.getFileName(); - long volumeId = fsoFile.getVolumeId(); - String dbFileKey = fsoFile.getOzonePathKey(); - OmKeyInfo keyToDelete = - omMetadataManager.getKeyTable(getBucketLayout()).get(dbFileKey); - long writerClientId = commitKeyRequest.getClientID(); - boolean isSameHsyncKey = false; - boolean isOverwrittenHsyncKey = false; - final String clientIdString = String.valueOf(writerClientId); - if (null != keyToDelete) { - isSameHsyncKey = java.util.Optional.of(keyToDelete) - .map(WithMetadata::getMetadata) - .map(meta -> meta.get(OzoneConsts.HSYNC_CLIENT_ID)) - .filter(id -> id.equals(clientIdString)) - .isPresent(); - if (!isSameHsyncKey) { - isOverwrittenHsyncKey = java.util.Optional.of(keyToDelete) - .map(WithMetadata::getMetadata) - .map(meta -> meta.get(OzoneConsts.HSYNC_CLIENT_ID)) - .filter(id -> !id.equals(clientIdString)) - .isPresent() && !isRecovery; - } - } - - if (isRecovery && keyToDelete != null) { - String clientId = keyToDelete.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); - if (clientId == null) { - throw new OMException("Failed to recovery key, as " + - dbFileKey + " is already closed", KEY_ALREADY_CLOSED); - } - writerClientId = Long.parseLong(clientId); + result = Result.SUCCESS; + } catch (IOException | InvalidPathException ex) { + result = Result.FAILURE; + exception = ex; + omClientResponse = new OMKeyCommitResponseWithFSO(createErrorOMResponse( + omResponse, exception), getBucketLayout()); + } finally { + if (bucketLockAcquired) { + mergeOmLockDetails(omMetadataManager.getLock() + .releaseWriteLock(BUCKET_LOCK, volumeName, bucketName)); } - dbOpenFileKey = fsoFile.getOpenFileName(writerClientId); - omKeyInfo = OMFileRequest.getOmKeyInfoFromFileTable(true, - omMetadataManager, dbOpenFileKey, keyName); - if (omKeyInfo == null) { - String action = isRecovery ? "recovery" : isHSync ? "hsync" : "commit"; - throw new OMException("Failed to " + action + " key, as " + - dbOpenFileKey + " entry is not found in the OpenKey table", KEY_NOT_FOUND); - } else if (omKeyInfo.getMetadata().containsKey(OzoneConsts.DELETED_HSYNC_KEY) || - omKeyInfo.getMetadata().containsKey(OzoneConsts.OVERWRITTEN_HSYNC_KEY)) { - throw new OMException("Open Key " + keyName + " is already deleted/overwritten", - KEY_NOT_FOUND); + if (omClientResponse != null) { + omClientResponse.setOmLockDetails(getOmLockDetails()); } + } - if (omKeyInfo.getMetadata().containsKey(OzoneConsts.LEASE_RECOVERY) && - omKeyInfo.getMetadata().containsKey(OzoneConsts.HSYNC_CLIENT_ID)) { - if (!isRecovery) { - throw new OMException("Cannot commit key " + dbOpenFileKey + " with " + OzoneConsts.LEASE_RECOVERY + - " metadata while recovery flag is not set in request", KEY_UNDER_LEASE_RECOVERY); - } - } + auditAndLogResult(ozoneManager, commitKeyRequest, prepared, auditMap, exception, result); + + return omClientResponse; + } - OmKeyInfo openKeyToDelete = null; - String dbOpenKeyToDeleteKey = null; - if (isOverwrittenHsyncKey) { - // find the overwritten openKey and add OVERWRITTEN_HSYNC_KEY to it. - dbOpenKeyToDeleteKey = fsoFile.getOpenFileName( - Long.parseLong(keyToDelete.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID))); - openKeyToDelete = OMFileRequest.getOmKeyInfoFromFileTable(true, - omMetadataManager, dbOpenKeyToDeleteKey, keyName); - openKeyToDelete = openKeyToDelete.toBuilder() - .addMetadata(OzoneConsts.OVERWRITTEN_HSYNC_KEY, "true") - .setUpdateID(trxnLogIndex) - .build(); - openKeyToDelete.setModificationTime(Time.now()); - OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, - dbOpenKeyToDeleteKey, openKeyToDelete, keyName, trxnLogIndex); + /** + * Phase 1 (no bucket lock): resolves the parent path, reads the open and committed keys and runs the + * commit guards, filling {@code prepared} as it goes. All reads observe committed state only. The OM + * apply path is single-threaded (OzoneManagerStateMachine uses a single-thread executor), so no other + * transaction can change what these reads observe before Phase 2 mutates; the double-buffer + * flush/cleanup threads only materialize already-committed epochs and never alter a key's visible + * value. + *

+ * Keeping the parent walk (OmFSOFile.build -> getParentID) out of the bucket write lock is the point + * of HDDS-16289: it stops the lone apply thread from gating readers of a hot bucket during path + * resolution. If OM ever applies transactions in parallel per bucket/key, these reads must be + * re-validated under the lock in {@link #applyKeyCommit}. + */ + private void prepareCommit(OzoneManager ozoneManager, CommitKeyRequest commitKeyRequest, + PreparedCommit prepared, List locationInfoList, + Map auditMap, long trxnLogIndex) throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + KeyArgs commitKeyArgs = commitKeyRequest.getKeyArgs(); + String volumeName = commitKeyArgs.getVolumeName(); + String bucketName = commitKeyArgs.getBucketName(); + String keyName = commitKeyArgs.getKeyName(); + final boolean isHSync = commitKeyRequest.hasHsync() && commitKeyRequest.getHsync(); + final boolean isRecovery = commitKeyRequest.hasRecovery() && commitKeyRequest.getRecovery(); + + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); + + String errMsg = "Cannot create file : " + keyName + + " as parent directory doesn't exist"; + OmFSOFile fsoFile = new OmFSOFile.Builder() + .setVolumeName(volumeName) + .setBucketName(bucketName) + .setKeyName(keyName) + .setOmMetadataManager(omMetadataManager) + .setErrMsg(errMsg) + .build(); + + prepared.fileName = fsoFile.getFileName(); + prepared.volumeId = fsoFile.getVolumeId(); + String dbFileKey = fsoFile.getOzonePathKey(); + prepared.dbFileKey = dbFileKey; + OmKeyInfo keyToDelete = + omMetadataManager.getKeyTable(getBucketLayout()).get(dbFileKey); + prepared.keyToDelete = keyToDelete; + long writerClientId = commitKeyRequest.getClientID(); + final String clientIdString = String.valueOf(writerClientId); + if (null != keyToDelete) { + prepared.isSameHsyncKey = java.util.Optional.of(keyToDelete) + .map(WithMetadata::getMetadata) + .map(meta -> meta.get(OzoneConsts.HSYNC_CLIENT_ID)) + .filter(id -> id.equals(clientIdString)) + .isPresent(); + if (!prepared.isSameHsyncKey) { + prepared.isOverwrittenHsyncKey = java.util.Optional.of(keyToDelete) + .map(WithMetadata::getMetadata) + .map(meta -> meta.get(OzoneConsts.HSYNC_CLIENT_ID)) + .filter(id -> !id.equals(clientIdString)) + .isPresent() && !isRecovery; } + } - omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime()); - // non-null indicates it is necessary to update the open key - OmKeyInfo newOpenKeyInfo = null; + if (isRecovery && keyToDelete != null) { + String clientId = keyToDelete.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); + if (clientId == null) { + throw new OMException("Failed to recovery key, as " + + dbFileKey + " is already closed", KEY_ALREADY_CLOSED); + } + writerClientId = Long.parseLong(clientId); + } + final String dbOpenFileKey = fsoFile.getOpenFileName(writerClientId); + prepared.dbOpenFileKey = dbOpenFileKey; + prepared.omKeyInfo = OMFileRequest.getOmKeyInfoFromFileTable(true, + omMetadataManager, dbOpenFileKey, keyName); + if (prepared.omKeyInfo == null) { + String action = isRecovery ? "recovery" : isHSync ? "hsync" : "commit"; + throw new OMException("Failed to " + action + " key, as " + + dbOpenFileKey + " entry is not found in the OpenKey table", KEY_NOT_FOUND); + } else if (prepared.omKeyInfo.getMetadata().containsKey(OzoneConsts.DELETED_HSYNC_KEY) || + prepared.omKeyInfo.getMetadata().containsKey(OzoneConsts.OVERWRITTEN_HSYNC_KEY)) { + throw new OMException("Open Key " + keyName + " is already deleted/overwritten", + KEY_NOT_FOUND); + } - if (isHSync) { - if (!OmKeyHSyncUtil.isHSyncedPreviously(omKeyInfo, clientIdString, dbOpenFileKey)) { - // Update open key as well if it is the first hsync of this key - omKeyInfo = omKeyInfo.withMetadataMutations( - metadata -> metadata.put(OzoneConsts.HSYNC_CLIENT_ID, clientIdString)); - newOpenKeyInfo = omKeyInfo.copyObject(); - } + if (prepared.omKeyInfo.getMetadata().containsKey(OzoneConsts.LEASE_RECOVERY) && + prepared.omKeyInfo.getMetadata().containsKey(OzoneConsts.HSYNC_CLIENT_ID)) { + if (!isRecovery) { + throw new OMException("Cannot commit key " + dbOpenFileKey + " with " + OzoneConsts.LEASE_RECOVERY + + " metadata while recovery flag is not set in request", KEY_UNDER_LEASE_RECOVERY); } + } - // Set the new metadata from the request and UpdateID to current - // transactionLogIndex - omKeyInfo = omKeyInfo.toBuilder() - .addAllMetadata(KeyValueUtil.getFromProtobuf( - commitKeyArgs.getMetadataList())) - .setDataSize(commitKeyArgs.getDataSize()) + if (prepared.isOverwrittenHsyncKey) { + // find the overwritten openKey and add OVERWRITTEN_HSYNC_KEY to it. + prepared.dbOpenKeyToDeleteKey = fsoFile.getOpenFileName( + Long.parseLong(keyToDelete.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID))); + OmKeyInfo openKeyToDelete = OMFileRequest.getOmKeyInfoFromFileTable(true, + omMetadataManager, prepared.dbOpenKeyToDeleteKey, keyName); + openKeyToDelete = openKeyToDelete.toBuilder() + .addMetadata(OzoneConsts.OVERWRITTEN_HSYNC_KEY, "true") .setUpdateID(trxnLogIndex) .build(); + openKeyToDelete.setModificationTime(Time.now()); + prepared.openKeyToDelete = openKeyToDelete; + // The cache write for this overwritten open key is deferred to the Phase 2 mutation tail, so it + // happens under the narrowed bucket write lock with the other mutations. + } - List uncommitted = - omKeyInfo.updateLocationInfoList(locationInfoList, false); - - // If bucket versioning is turned on during the update, between key - // creation and key commit, old versions will be just overwritten and - // not kept. Bucket versioning will be effective from the first key - // creation after the knob turned on. - Map oldKeyVersionsToDeleteMap = null; + buildCommittedKey(commitKeyRequest, prepared, locationInfoList, auditMap, trxnLogIndex); + } - validateAtomicRewrite(keyToDelete, omKeyInfo, auditMap); - // Optimistic locking validation has passed. Now set the rewrite fields to null so they are - // not persisted in the key table. - omKeyInfo = omKeyInfo.toBuilder() - .setExpectedDataGeneration(null) - .build(); + /** + * Still Phase 1 (no bucket lock): turns the open key read above into the key to commit - request + * metadata, data size, update ID and block locations - and validates the atomic-rewrite expectation. + * Kept separate from {@link #prepareCommit} so the resolution and the guards stay readable next to + * the walk they belong to. + */ + private void buildCommittedKey(CommitKeyRequest commitKeyRequest, PreparedCommit prepared, + List locationInfoList, Map auditMap, long trxnLogIndex) + throws IOException { + KeyArgs commitKeyArgs = commitKeyRequest.getKeyArgs(); + final boolean isHSync = commitKeyRequest.hasHsync() && commitKeyRequest.getHsync(); + final String clientIdString = String.valueOf(commitKeyRequest.getClientID()); - long correctedSpace = omKeyInfo.getReplicatedSize(); - // Same-client hsync re-commit does not consume namespace. - if (keyToDelete != null && isSameHsyncKey) { - correctedSpace -= keyToDelete.getReplicatedSize(); - checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, - correctedSpace); - } else if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) { - RepeatedOmKeyInfo oldVerKeyInfo = getOldVersionsToCleanUp( - keyToDelete, omBucketInfo.getObjectID(), trxnLogIndex); - String delKeyName = omMetadataManager - .getOzoneKey(volumeName, bucketName, fileName); - // using pseudoObjId as objectId can be same in case of overwrite key - long pseudoObjId = ozoneManager.getObjectIdFromTxId(trxnLogIndex); - delKeyName = omMetadataManager.getOzoneDeletePathKey( - pseudoObjId, delKeyName); - if (null == oldKeyVersionsToDeleteMap) { - oldKeyVersionsToDeleteMap = new HashMap<>(); - } + prepared.omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime()); - // Remove any block from oldVerKeyInfo that share the same container ID - // and local ID with omKeyInfo blocks'. - // Otherwise, it causes data loss once those shared blocks are added - // to deletedTable and processed by KeyDeletingService for deletion. - Pair>, Integer> filteredUsedBlockCnt = - filterOutBlocksStillInUse(omKeyInfo, oldVerKeyInfo); - Map> blocks = filteredUsedBlockCnt.getLeft(); - correctedSpace -= blocks.entrySet().stream().mapToLong(filteredKeyBlocks -> - filteredKeyBlocks.getValue().stream().mapToLong(block -> QuotaUtil.getReplicatedSize( - block.getLength(), filteredKeyBlocks.getKey().getReplicationConfig())).sum()).sum(); - long totalSize = 0; - long totalNamespace = 0; - if (!oldVerKeyInfo.getOmKeyInfoList().isEmpty()) { - oldKeyVersionsToDeleteMap.put(delKeyName, oldVerKeyInfo); - List oldKeys = oldVerKeyInfo.getOmKeyInfoList(); - for (int i = 0; i < oldKeys.size(); i++) { - OmKeyInfo updatedOlderKeyVersions = - oldKeys.get(i).withCommittedKeyDeletedFlag(true); - oldKeys.set(i, updatedOlderKeyVersions); - totalSize += sumBlockLengths(updatedOlderKeyVersions); - totalNamespace += 1; - } - } - // Subtract the size of blocks to be overwritten. - checkBucketQuotaInNamespace(omBucketInfo, 1L); - checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, - correctedSpace); - // Subtract the size of blocks to be overwritten. - omBucketInfo.decrUsedNamespace(totalNamespace, true); - omBucketInfo.decrUsedNamespace(filteredUsedBlockCnt.getRight(), false); - omBucketInfo.decrUsedBytes(totalSize, true); - omBucketInfo.incrUsedNamespace(1L); - } else { - checkBucketQuotaInNamespace(omBucketInfo, 1L); - checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, - correctedSpace); - omBucketInfo.incrUsedNamespace(1L); + if (isHSync) { + if (!OmKeyHSyncUtil.isHSyncedPreviously(prepared.omKeyInfo, clientIdString, + prepared.dbOpenFileKey)) { + // Update open key as well if it is the first hsync of this key. A non-null newOpenKeyInfo + // indicates it is necessary to update the open key. + prepared.omKeyInfo = prepared.omKeyInfo.withMetadataMutations( + metadata -> metadata.put(OzoneConsts.HSYNC_CLIENT_ID, clientIdString)); + prepared.newOpenKeyInfo = prepared.omKeyInfo.copyObject(); } + } - // let the uncommitted blocks pretend as key's old version blocks - // which will be deleted as RepeatedOmKeyInfo - final OmKeyInfo pseudoKeyInfo = isHSync ? null - : wrapUncommittedBlocksAsPseudoKey(uncommitted, omKeyInfo); - if (pseudoKeyInfo != null) { - String delKeyName = omMetadataManager - .getOzoneKey(volumeName, bucketName, fileName); - long pseudoObjId = ozoneManager.getObjectIdFromTxId(trxnLogIndex); - delKeyName = omMetadataManager.getOzoneDeletePathKey( - pseudoObjId, delKeyName); - if (null == oldKeyVersionsToDeleteMap) { - oldKeyVersionsToDeleteMap = new HashMap<>(); - } - oldKeyVersionsToDeleteMap.computeIfAbsent(delKeyName, - key -> new RepeatedOmKeyInfo(omBucketInfo.getObjectID())).addOmKeyInfo(pseudoKeyInfo); + // Set the new metadata from the request and UpdateID to current + // transactionLogIndex + prepared.omKeyInfo = prepared.omKeyInfo.toBuilder() + .addAllMetadata(KeyValueUtil.getFromProtobuf( + commitKeyArgs.getMetadataList())) + .setDataSize(commitKeyArgs.getDataSize()) + .setUpdateID(trxnLogIndex) + .build(); + + prepared.uncommitted = + prepared.omKeyInfo.updateLocationInfoList(locationInfoList, false); + + validateAtomicRewrite(prepared.keyToDelete, prepared.omKeyInfo, auditMap); + // Optimistic locking validation has passed. Now set the rewrite fields to null so they are + // not persisted in the key table. + prepared.omKeyInfo = prepared.omKeyInfo.toBuilder() + .setExpectedDataGeneration(null) + .build(); + } + + /** + * Phase 2 (under the bucket write lock): re-checks the commit target resolved in Phase 1, then + * applies the quota read-modify-publish and every cache mutation of the commit, and builds the + * response. + */ + private OMClientResponse applyKeyCommit(OzoneManager ozoneManager, + CommitKeyRequest commitKeyRequest, PreparedCommit prepared, long trxnLogIndex, + OMResponse.Builder omResponse) throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + KeyArgs commitKeyArgs = commitKeyRequest.getKeyArgs(); + String volumeName = commitKeyArgs.getVolumeName(); + String bucketName = commitKeyArgs.getBucketName(); + String keyName = commitKeyArgs.getKeyName(); + final boolean isHSync = commitKeyRequest.hasHsync() && commitKeyRequest.getHsync(); + final boolean isRecovery = commitKeyRequest.hasRecovery() && commitKeyRequest.getRecovery(); + + // Cheap O(1) re-check of the commit target resolved in Phase 1. Under serial apply this always + // holds; it is a tripwire that fails safe (as the KEY_NOT_FOUND checks in Phase 1) rather than + // committing a stale open key if that invariant is ever broken by a concurrent writer. + OmKeyInfo recheckOpenKey = OMFileRequest.getOmKeyInfoFromFileTable(true, + omMetadataManager, prepared.dbOpenFileKey, keyName); + if (recheckOpenKey == null + || recheckOpenKey.getMetadata().containsKey(OzoneConsts.DELETED_HSYNC_KEY) + || recheckOpenKey.getMetadata().containsKey(OzoneConsts.OVERWRITTEN_HSYNC_KEY)) { + throw new OMException("Open Key " + keyName + " is already deleted/overwritten", + KEY_NOT_FOUND); + } + + OmBucketInfo omBucketInfo = getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); + + // Deferred from Phase 1: mark the overwritten hsync open key, under the lock with the rest. + if (prepared.isOverwrittenHsyncKey) { + OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, + prepared.dbOpenKeyToDeleteKey, prepared.openKeyToDelete, keyName, trxnLogIndex); + } + + CommitApplyState state = new CommitApplyState(); + applyCommitQuota(ozoneManager, commitKeyRequest, prepared, omBucketInfo, state, trxnLogIndex); + + // let the uncommitted blocks pretend as key's old version blocks + // which will be deleted as RepeatedOmKeyInfo + final OmKeyInfo pseudoKeyInfo = isHSync ? null + : wrapUncommittedBlocksAsPseudoKey(prepared.uncommitted, prepared.omKeyInfo); + if (pseudoKeyInfo != null) { + String delKeyName = omMetadataManager + .getOzoneKey(volumeName, bucketName, prepared.fileName); + long pseudoObjId = ozoneManager.getObjectIdFromTxId(trxnLogIndex); + delKeyName = omMetadataManager.getOzoneDeletePathKey( + pseudoObjId, delKeyName); + if (null == state.oldKeyVersionsToDeleteMap) { + state.oldKeyVersionsToDeleteMap = new HashMap<>(); } + state.oldKeyVersionsToDeleteMap.computeIfAbsent(delKeyName, + key -> new RepeatedOmKeyInfo(omBucketInfo.getObjectID())).addOmKeyInfo(pseudoKeyInfo); + } - // Add to cache of open key table and key table. - if (!isHSync) { - // If isHSync = false, put a tombstone in OpenKeyTable cache, - // indicating the key is removed from OpenKeyTable. - // So that this key can't be committed again. - OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, - dbOpenFileKey, null, keyName, trxnLogIndex); - - // Prevent hsync metadata from getting committed to the final key - omKeyInfo = omKeyInfo.withMetadataMutations( - metadata -> metadata.remove(OzoneConsts.HSYNC_CLIENT_ID)); - if (isRecovery) { - omKeyInfo = omKeyInfo.withMetadataMutations( - metadata -> metadata.remove(OzoneConsts.LEASE_RECOVERY)); - } - } else if (newOpenKeyInfo != null) { - // isHSync is true and newOpenKeyInfo is set, update OpenKeyTable - OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, - dbOpenFileKey, newOpenKeyInfo, keyName, trxnLogIndex); + // Add to cache of open key table and key table. + if (!isHSync) { + // If isHSync = false, put a tombstone in OpenKeyTable cache, + // indicating the key is removed from OpenKeyTable. + // So that this key can't be committed again. + OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, + prepared.dbOpenFileKey, null, keyName, trxnLogIndex); + + // Prevent hsync metadata from getting committed to the final key + prepared.omKeyInfo = prepared.omKeyInfo.withMetadataMutations( + metadata -> metadata.remove(OzoneConsts.HSYNC_CLIENT_ID)); + if (isRecovery) { + prepared.omKeyInfo = prepared.omKeyInfo.withMetadataMutations( + metadata -> metadata.remove(OzoneConsts.LEASE_RECOVERY)); } + } else if (prepared.newOpenKeyInfo != null) { + // isHSync is true and newOpenKeyInfo is set, update OpenKeyTable + OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, + prepared.dbOpenFileKey, prepared.newOpenKeyInfo, keyName, trxnLogIndex); + } - OMFileRequest.addFileTableCacheEntry(omMetadataManager, dbFileKey, - omKeyInfo, fileName, trxnLogIndex); + OMFileRequest.addFileTableCacheEntry(omMetadataManager, prepared.dbFileKey, + prepared.omKeyInfo, prepared.fileName, trxnLogIndex); - omBucketInfo.incrUsedBytes(correctedSpace); + omBucketInfo.incrUsedBytes(state.correctedSpace); - omMetadataManager.getBucketTable().addCacheEntry( - omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); + omMetadataManager.getBucketTable().addCacheEntry( + omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); - omResponse.setCommitKeyResponse(CommitKeyResponse.newBuilder() - .setModificationTime(commitKeyArgs.getModificationTime()) - .build()); + omResponse.setCommitKeyResponse(CommitKeyResponse.newBuilder() + .setModificationTime(commitKeyArgs.getModificationTime()) + .build()); - omClientResponse = new OMKeyCommitResponseWithFSO(omResponse.build(), - omKeyInfo, dbFileKey, dbOpenFileKey, omBucketInfo.copyObject(), - oldKeyVersionsToDeleteMap, volumeId, isHSync, newOpenKeyInfo, dbOpenKeyToDeleteKey, openKeyToDelete); + return new OMKeyCommitResponseWithFSO(omResponse.build(), + prepared.omKeyInfo, prepared.dbFileKey, prepared.dbOpenFileKey, omBucketInfo.copyObject(), + state.oldKeyVersionsToDeleteMap, prepared.volumeId, isHSync, prepared.newOpenKeyInfo, + prepared.dbOpenKeyToDeleteKey, prepared.openKeyToDelete); + } - result = Result.SUCCESS; - } catch (IOException | InvalidPathException ex) { - result = Result.FAILURE; - exception = ex; - omClientResponse = new OMKeyCommitResponseWithFSO(createErrorOMResponse( - omResponse, exception), getBucketLayout()); - } finally { - if (bucketLockAcquired) { - mergeOmLockDetails(omMetadataManager.getLock() - .releaseWriteLock(BUCKET_LOCK, volumeName, bucketName)); + /** + * Phase 2 (under the bucket write lock): charges the commit against the bucket quota and collects the + * old key versions to delete. Kept separate from {@link #applyKeyCommit} because the non-versioned + * overwrite branch is the bulk of the quota arithmetic. + */ + private void applyCommitQuota(OzoneManager ozoneManager, CommitKeyRequest commitKeyRequest, + PreparedCommit prepared, OmBucketInfo omBucketInfo, CommitApplyState state, long trxnLogIndex) + throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + KeyArgs commitKeyArgs = commitKeyRequest.getKeyArgs(); + String volumeName = commitKeyArgs.getVolumeName(); + String bucketName = commitKeyArgs.getBucketName(); + OmKeyInfo keyToDelete = prepared.keyToDelete; + + state.correctedSpace = prepared.omKeyInfo.getReplicatedSize(); + // Same-client hsync re-commit does not consume namespace. + if (keyToDelete != null && prepared.isSameHsyncKey) { + state.correctedSpace -= keyToDelete.getReplicatedSize(); + checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, + state.correctedSpace); + } else if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) { + // If bucket versioning is turned on during the update, between key + // creation and key commit, old versions will be just overwritten and + // not kept. Bucket versioning will be effective from the first key + // creation after the knob turned on. + RepeatedOmKeyInfo oldVerKeyInfo = getOldVersionsToCleanUp( + keyToDelete, omBucketInfo.getObjectID(), trxnLogIndex); + String delKeyName = omMetadataManager + .getOzoneKey(volumeName, bucketName, prepared.fileName); + // using pseudoObjId as objectId can be same in case of overwrite key + long pseudoObjId = ozoneManager.getObjectIdFromTxId(trxnLogIndex); + delKeyName = omMetadataManager.getOzoneDeletePathKey( + pseudoObjId, delKeyName); + if (null == state.oldKeyVersionsToDeleteMap) { + state.oldKeyVersionsToDeleteMap = new HashMap<>(); } - if (omClientResponse != null) { - omClientResponse.setOmLockDetails(getOmLockDetails()); + + // Remove any block from oldVerKeyInfo that share the same container ID + // and local ID with omKeyInfo blocks'. + // Otherwise, it causes data loss once those shared blocks are added + // to deletedTable and processed by KeyDeletingService for deletion. + Pair>, Integer> filteredUsedBlockCnt = + filterOutBlocksStillInUse(prepared.omKeyInfo, oldVerKeyInfo); + Map> blocks = filteredUsedBlockCnt.getLeft(); + state.correctedSpace -= blocks.entrySet().stream().mapToLong(filteredKeyBlocks -> + filteredKeyBlocks.getValue().stream().mapToLong(block -> QuotaUtil.getReplicatedSize( + block.getLength(), filteredKeyBlocks.getKey().getReplicationConfig())).sum()).sum(); + long totalSize = 0; + long totalNamespace = 0; + if (!oldVerKeyInfo.getOmKeyInfoList().isEmpty()) { + state.oldKeyVersionsToDeleteMap.put(delKeyName, oldVerKeyInfo); + List oldKeys = oldVerKeyInfo.getOmKeyInfoList(); + for (int i = 0; i < oldKeys.size(); i++) { + OmKeyInfo updatedOlderKeyVersions = + oldKeys.get(i).withCommittedKeyDeletedFlag(true); + oldKeys.set(i, updatedOlderKeyVersions); + totalSize += sumBlockLengths(updatedOlderKeyVersions); + totalNamespace += 1; + } } + // Subtract the size of blocks to be overwritten. + checkBucketQuotaInNamespace(omBucketInfo, 1L); + checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, + state.correctedSpace); + // Subtract the size of blocks to be overwritten. + omBucketInfo.decrUsedNamespace(totalNamespace, true); + omBucketInfo.decrUsedNamespace(filteredUsedBlockCnt.getRight(), false); + omBucketInfo.decrUsedBytes(totalSize, true); + omBucketInfo.incrUsedNamespace(1L); + } else { + checkBucketQuotaInNamespace(omBucketInfo, 1L); + checkBucketQuotaInBytes(omMetadataManager, omBucketInfo, + state.correctedSpace); + omBucketInfo.incrUsedNamespace(1L); } + } + + /** + * Phase 1 output of {@link #prepareCommit}, consumed by {@link #applyKeyCommit} under the bucket + * write lock: the resolved path keys, the key being overwritten and the hsync open keys. Mutable, + * because {@code omKeyInfo} is rebuilt as the commit is prepared and again under the lock, and the + * caller logs and audits the value reached. + */ + private static final class PreparedCommit { + private String fileName; + private long volumeId; + private String dbFileKey; + private String dbOpenFileKey; + private OmKeyInfo keyToDelete; + private boolean isSameHsyncKey; + private boolean isOverwrittenHsyncKey; + private String dbOpenKeyToDeleteKey; + private OmKeyInfo openKeyToDelete; + private OmKeyInfo newOpenKeyInfo; + private List uncommitted; + private OmKeyInfo omKeyInfo; + } + + /** + * Values produced under the bucket write lock and shared between {@link #applyCommitQuota} and the + * rest of {@link #applyKeyCommit}: the bucket byte delta to charge and the old key versions to move + * to the deleted table, which stays {@code null} while nothing has to be deleted. + */ + private static final class CommitApplyState { + private long correctedSpace; + private Map oldKeyVersionsToDeleteMap; + } + + /** + * Emits the debug, audit and result logs outside the bucket lock. + */ + private void auditAndLogResult(OzoneManager ozoneManager, CommitKeyRequest commitKeyRequest, + PreparedCommit prepared, Map auditMap, Exception exception, Result result) { + boolean isHSync = commitKeyRequest.hasHsync() && commitKeyRequest.getHsync(); // Debug logging for any key commit operation, successful or not LOG.debug("Key commit {} with isHSync = {}, omKeyInfo = {}", - result == Result.SUCCESS ? "succeeded" : "failed", isHSync, omKeyInfo); + result == Result.SUCCESS ? "succeeded" : "failed", isHSync, prepared.omKeyInfo); if (!isHSync) { - markForAudit(auditLogger, buildAuditMessage(OMAction.COMMIT_KEY, auditMap, + KeyArgs commitKeyArgs = commitKeyRequest.getKeyArgs(); + markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(OMAction.COMMIT_KEY, auditMap, exception, getOmRequest().getUserInfo())); - processResult(commitKeyRequest, volumeName, bucketName, keyName, - omMetrics, exception, omKeyInfo, result); + processResult(commitKeyRequest, commitKeyArgs.getVolumeName(), commitKeyArgs.getBucketName(), + commitKeyArgs.getKeyName(), ozoneManager.getMetrics(), exception, prepared.omKeyInfo, result); } - - return omClientResponse; } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequestWithFSO.java index c0c83f5c4f41..5c434283ba38 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequestWithFSO.java @@ -26,11 +26,9 @@ import java.io.IOException; import java.nio.file.InvalidPathException; import java.util.Map; -import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue; import org.apache.hadoop.ozone.OzoneConsts; -import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.OMAction; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OMMetrics; @@ -71,7 +69,6 @@ public OMKeyDeleteRequestWithFSO(OMRequest omRequest, } @Override - @SuppressWarnings("methodlength") public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { final long trxnLogIndex = context.getIndex(); DeleteKeyRequest deleteKeyRequest = getOmRequest().getDeleteKeyRequest(); @@ -82,16 +79,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut String volumeName = keyArgs.getVolumeName(); String bucketName = keyArgs.getBucketName(); - String keyName = keyArgs.getKeyName(); - boolean recursive = keyArgs.getRecursive(); OMMetrics omMetrics = ozoneManager.getMetrics(); omMetrics.incNumKeyDeletes(); OMPerformanceMetrics perfMetrics = ozoneManager.getPerfMetrics(); - AuditLogger auditLogger = ozoneManager.getAuditLogger(); - OzoneManagerProtocolProtos.UserInfo userInfo = getOmRequest().getUserInfo(); - OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder( getOmRequest()); OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); @@ -99,102 +91,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean acquiredLock = false; OMClientResponse omClientResponse = null; Result result = null; - OmBucketInfo omBucketInfo = null; long startNanos = Time.monotonicNowNanos(); try { + PreparedKeyDelete prepared = + prepareKeyDelete(ozoneManager, keyArgs, auditMap, trxnLogIndex); + + // Phase 2 (bucket write lock): re-check that the key still exists, then apply the cache + // mutations and publish the quota copy. The lock is held only for this mutation tail, so + // bucket read-lock holders still observe each transaction atomically (all mutations or none), + // but are no longer blocked for the Phase 1 path walk and emptiness scan. mergeOmLockDetails(omMetadataManager.getLock() .acquireWriteLock(BUCKET_LOCK, volumeName, bucketName)); acquiredLock = getOmLockDetails().isLockAcquired(); - // Validate bucket and volume exists or not. - validateBucketAndVolume(omMetadataManager, volumeName, bucketName); - - OzoneFileStatus keyStatus = OMFileRequest.getOMKeyInfoIfExists( - omMetadataManager, volumeName, bucketName, keyName, 0, - ozoneManager.getDefaultReplicationConfig()); - - if (keyStatus == null) { - throw new OMException("Key not found. Key:" + keyName, KEY_NOT_FOUND); - } - - OmKeyInfo omKeyInfo = keyStatus.getKeyInfo(); - validateIfMatchETag(keyArgs, omKeyInfo); - // New key format for the fileTable & dirTable. - // For example, the user given key path is '/a/b/c/d/e/file1', then in DB - // keyName field stores only the leaf node name, which is 'file1'. - String fileName = OzoneFSUtils.getFileName(keyName); - omKeyInfo.setKeyName(fileName); - - // Set the UpdateID to current transactionLogIndex - omKeyInfo = omKeyInfo.toBuilder() - .setUpdateID(trxnLogIndex) - .build(); - - final long volumeId = omMetadataManager.getVolumeId(volumeName); - final long bucketId = omMetadataManager.getBucketId(volumeName, - bucketName); - String ozonePathKey = omMetadataManager.getOzonePathKey(volumeId, - bucketId, omKeyInfo.getParentObjectID(), - omKeyInfo.getFileName()); - OmKeyInfo deletedOpenKeyInfo = null; - - if (keyStatus.isDirectory()) { - // Check if there are any sub path exists under the user requested path - if (!recursive && - OMFileRequest.hasChildren(omKeyInfo, omMetadataManager)) { - throw new OMException("Directory is not empty. Key:" + keyName, - DIRECTORY_NOT_EMPTY); - } - - // Update dir cache. - omMetadataManager.getDirectoryTable().addCacheEntry( - new CacheKey<>(ozonePathKey), - CacheValue.get(trxnLogIndex)); - } else { - // Update table cache. - omMetadataManager.getKeyTable(getBucketLayout()).addCacheEntry( - new CacheKey<>(ozonePathKey), - CacheValue.get(trxnLogIndex)); - } - - omBucketInfo = getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); - - long quotaReleased = sumBlockLengths(omKeyInfo); - // Empty entries won't be added to deleted table so this key shouldn't get added to snapshotUsed space. - boolean isKeyNonEmpty = !OmKeyInfo.isKeyEmpty(omKeyInfo); - omBucketInfo.decrUsedBytes(quotaReleased, isKeyNonEmpty); - omBucketInfo.decrUsedNamespace(1L, isKeyNonEmpty || keyStatus.isDirectory()); - - // If omKeyInfo has hsync metadata, delete its corresponding open key as well - String dbOpenKey = null; - String hsyncClientId = omKeyInfo.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); - if (hsyncClientId != null) { - Table openKeyTable = omMetadataManager.getOpenKeyTable(getBucketLayout()); - long parentId = omKeyInfo.getParentObjectID(); - dbOpenKey = omMetadataManager.getOpenFileName(volumeId, bucketId, parentId, fileName, hsyncClientId); - OmKeyInfo openKeyInfo = openKeyTable.get(dbOpenKey); - if (openKeyInfo != null) { - openKeyInfo = openKeyInfo.withMetadataMutations( - metadata -> metadata.put(DELETED_HSYNC_KEY, "true")); - openKeyTable.addCacheEntry(dbOpenKey, openKeyInfo, trxnLogIndex); - deletedOpenKeyInfo = openKeyInfo; - } else { - LOG.warn("Potentially inconsistent DB state: open key not found with dbOpenKey '{}'", dbOpenKey); - } - } - - if (keyStatus.isFile()) { - auditMap.put(OzoneConsts.DATA_SIZE, String.valueOf(omKeyInfo.getDataSize())); - auditMap.put(OzoneConsts.REPLICATION_CONFIG, omKeyInfo.getReplicationConfig().toString()); - } - - omMetadataManager.getBucketTable().addCacheEntry( - omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); - - omClientResponse = new OMKeyDeleteResponseWithFSO(omResponse - .setDeleteKeyResponse(DeleteKeyResponse.newBuilder()).build(), - keyName, omKeyInfo, - omBucketInfo.copyObject(), keyStatus.isDirectory(), volumeId, deletedOpenKeyInfo); + omClientResponse = applyKeyDelete(omMetadataManager, keyArgs, prepared, + trxnLogIndex, omResponse); result = Result.SUCCESS; long endNanosDeleteKeySuccessLatencyNs = Time.monotonicNowNanos(); @@ -216,10 +127,207 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut } } - // Performing audit logging outside of the lock. - markForAudit(auditLogger, buildAuditMessage(OMAction.DELETE_KEY, auditMap, - exception, userInfo)); + auditAndLogResult(ozoneManager, deleteKeyRequest, auditMap, exception, result); + return omClientResponse; + } + + /** + * Phase 1 (no bucket lock): resolves the key, checks directory emptiness and prepares the quota + * delta and the hsync open-key copy. All of this reads committed state only. The OM apply path is + * single-threaded (OzoneManagerStateMachine uses a single-thread executor), so no other transaction + * can change what these reads observe before Phase 2 mutates; the double-buffer flush/cleanup + * threads only materialize already-committed epochs and never alter a key's visible value. + *

+ * Keeping these reads out of the bucket write lock is the point of HDDS-16289: it stops the lone + * apply thread from gating readers of a hot bucket (getBucketInfo/getFileStatus/lookupKey). Delete + * has two costly reads here, not one: getOMKeyInfoIfExists walks the path segment by segment, and + * hasChildren scans the whole dirTable and fileTable cache before seeking RocksDB, so on a + * non-recursive directory delete it dominates the hold. If OM ever applies transactions in parallel + * per bucket/key, these reads must be re-validated under the lock in {@link #applyKeyDelete}. + *

+ * Also fills in the data-size and replication audit parameters for a file delete, which are read + * off the resolved key. + */ + private PreparedKeyDelete prepareKeyDelete(OzoneManager ozoneManager, + OzoneManagerProtocolProtos.KeyArgs keyArgs, Map auditMap, long trxnLogIndex) + throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + + // Validate bucket and volume exists or not. + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); + + OzoneFileStatus keyStatus = OMFileRequest.getOMKeyInfoIfExists( + omMetadataManager, volumeName, bucketName, keyName, 0, + ozoneManager.getDefaultReplicationConfig()); + + if (keyStatus == null) { + throw new OMException("Key not found. Key:" + keyName, KEY_NOT_FOUND); + } + + OmKeyInfo omKeyInfo = keyStatus.getKeyInfo(); + validateIfMatchETag(keyArgs, omKeyInfo); + // New key format for the fileTable & dirTable. + // For example, the user given key path is '/a/b/c/d/e/file1', then in DB + // keyName field stores only the leaf node name, which is 'file1'. + String fileName = OzoneFSUtils.getFileName(keyName); + omKeyInfo.setKeyName(fileName); + + // Set the UpdateID to current transactionLogIndex + omKeyInfo = omKeyInfo.toBuilder() + .setUpdateID(trxnLogIndex) + .build(); + + final long volumeId = omMetadataManager.getVolumeId(volumeName); + final long bucketId = omMetadataManager.getBucketId(volumeName, + bucketName); + String ozonePathKey = omMetadataManager.getOzonePathKey(volumeId, + bucketId, omKeyInfo.getParentObjectID(), + omKeyInfo.getFileName()); + + if (keyStatus.isDirectory() && !keyArgs.getRecursive() + && OMFileRequest.hasChildren(omKeyInfo, omMetadataManager)) { + // Check if there are any sub path exists under the user requested path + throw new OMException("Directory is not empty. Key:" + keyName, + DIRECTORY_NOT_EMPTY); + } + + // If omKeyInfo has hsync metadata, delete its corresponding open key as well. Only the cache + // entry is published under the lock in Phase 2; reading and rewriting the copy is done here. + OmKeyInfo deletedOpenKeyInfo = null; + String dbOpenKey = null; + String hsyncClientId = omKeyInfo.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); + if (hsyncClientId != null) { + long parentId = omKeyInfo.getParentObjectID(); + dbOpenKey = omMetadataManager.getOpenFileName(volumeId, bucketId, parentId, fileName, hsyncClientId); + OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey); + if (openKeyInfo != null) { + deletedOpenKeyInfo = openKeyInfo.withMetadataMutations( + metadata -> metadata.put(DELETED_HSYNC_KEY, "true")); + } else { + LOG.warn("Potentially inconsistent DB state: open key not found with dbOpenKey '{}'", dbOpenKey); + } + } + + if (keyStatus.isFile()) { + auditMap.put(OzoneConsts.DATA_SIZE, String.valueOf(omKeyInfo.getDataSize())); + auditMap.put(OzoneConsts.REPLICATION_CONFIG, omKeyInfo.getReplicationConfig().toString()); + } + + return new PreparedKeyDelete(keyStatus.isDirectory(), omKeyInfo, volumeId, ozonePathKey, + sumBlockLengths(omKeyInfo), dbOpenKey, deletedOpenKeyInfo); + } + + /** + * Phase 2 (under the bucket write lock): re-checks the key resolved in Phase 1, then tombstones it + * in the directory or file table, marks any hsync open key deleted, applies the bucket quota + * release and publishes the bucket copy. + */ + private OMClientResponse applyKeyDelete(OMMetadataManager omMetadataManager, + OzoneManagerProtocolProtos.KeyArgs keyArgs, PreparedKeyDelete prepared, long trxnLogIndex, + OMResponse.Builder omResponse) throws IOException { + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + + // Cheap O(1) re-check of the key resolved in Phase 1. Under serial apply this always holds; it + // is a tripwire that fails safe, the same way the Phase 1 existence check does, if that + // invariant is ever broken by a concurrent writer. + final boolean keyStillExists = prepared.isDirectory + ? omMetadataManager.getDirectoryTable().get(prepared.ozonePathKey) != null + : omMetadataManager.getKeyTable(getBucketLayout()).get(prepared.ozonePathKey) != null; + if (!keyStillExists) { + throw new OMException("Key not found. Key:" + keyName, KEY_NOT_FOUND); + } + + if (prepared.isDirectory) { + // Update dir cache. + omMetadataManager.getDirectoryTable().addCacheEntry( + new CacheKey<>(prepared.ozonePathKey), + CacheValue.get(trxnLogIndex)); + } else { + // Update table cache. + omMetadataManager.getKeyTable(getBucketLayout()).addCacheEntry( + new CacheKey<>(prepared.ozonePathKey), + CacheValue.get(trxnLogIndex)); + } + + OmBucketInfo omBucketInfo = + getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName); + + // Empty entries won't be added to deleted table so this key shouldn't get added to snapshotUsed space. + boolean isKeyNonEmpty = !OmKeyInfo.isKeyEmpty(prepared.omKeyInfo); + omBucketInfo.decrUsedBytes(prepared.quotaReleased, isKeyNonEmpty); + omBucketInfo.decrUsedNamespace(1L, isKeyNonEmpty || prepared.isDirectory); + + if (prepared.deletedOpenKeyInfo != null) { + omMetadataManager.getOpenKeyTable(getBucketLayout()).addCacheEntry( + prepared.dbOpenKey, prepared.deletedOpenKeyInfo, trxnLogIndex); + } + + omMetadataManager.getBucketTable().addCacheEntry( + omMetadataManager.getBucketKey(volumeName, bucketName), omBucketInfo, trxnLogIndex); + + return new OMKeyDeleteResponseWithFSO(omResponse + .setDeleteKeyResponse(DeleteKeyResponse.newBuilder()).build(), + keyName, prepared.omKeyInfo, + omBucketInfo.copyObject(), prepared.isDirectory, prepared.volumeId, + prepared.deletedOpenKeyInfo); + } + + /** + * Phase 1 output of {@link #prepareKeyDelete}, consumed by {@link #applyKeyDelete} under the bucket + * write lock: the resolved key and its path key, the bytes to release and the hsync open key to + * mark deleted. + */ + private static final class PreparedKeyDelete { + private final boolean isDirectory; + private final OmKeyInfo omKeyInfo; + private final long volumeId; + private final String ozonePathKey; + private final long quotaReleased; + private final String dbOpenKey; + private final OmKeyInfo deletedOpenKeyInfo; + + PreparedKeyDelete(boolean isDirectory, OmKeyInfo omKeyInfo, long volumeId, String ozonePathKey, + long quotaReleased, String dbOpenKey, OmKeyInfo deletedOpenKeyInfo) { + this.isDirectory = isDirectory; + this.omKeyInfo = omKeyInfo; + this.volumeId = volumeId; + this.ozonePathKey = ozonePathKey; + this.quotaReleased = quotaReleased; + this.dbOpenKey = dbOpenKey; + this.deletedOpenKeyInfo = deletedOpenKeyInfo; + } + } + + @Override + protected OzoneManagerProtocolProtos.KeyArgs resolveBucketAndCheckAcls( + OzoneManager ozoneManager, + OzoneManagerProtocolProtos.KeyArgs.Builder newKeyArgs) + throws IOException { + return captureLatencyNs( + ozoneManager.getPerfMetrics().getDeleteKeyResolveBucketAndAclCheckLatencyNs(), + () -> resolveBucketAndCheckKeyAclsWithFSO(newKeyArgs.build(), + ozoneManager, IAccessAuthorizer.ACLType.DELETE)); + } + + /** + * Emits the audit log and the result log outside the bucket lock. + */ + private void auditAndLogResult(OzoneManager ozoneManager, DeleteKeyRequest deleteKeyRequest, + Map auditMap, Exception exception, Result result) { + markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(OMAction.DELETE_KEY, auditMap, + exception, getOmRequest().getUserInfo())); + + OzoneManagerProtocolProtos.KeyArgs keyArgs = deleteKeyRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String keyName = keyArgs.getKeyName(); + OMMetrics omMetrics = ozoneManager.getMetrics(); switch (result) { case SUCCESS: @@ -236,18 +344,5 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut LOG.error("Unrecognized Result for OMKeyDeleteRequest: {}", deleteKeyRequest); } - - return omClientResponse; - } - - @Override - protected OzoneManagerProtocolProtos.KeyArgs resolveBucketAndCheckAcls( - OzoneManager ozoneManager, - OzoneManagerProtocolProtos.KeyArgs.Builder newKeyArgs) - throws IOException { - return captureLatencyNs( - ozoneManager.getPerfMetrics().getDeleteKeyResolveBucketAndAclCheckLatencyNs(), - () -> resolveBucketAndCheckKeyAclsWithFSO(newKeyArgs.build(), - ozoneManager, IAccessAuthorizer.ACLType.DELETE)); } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java index 028fbf605094..7faa5ab9af8e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java @@ -29,7 +29,6 @@ import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue; import org.apache.hadoop.ozone.OzoneConsts; -import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.OMAction; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OMMetrics; @@ -70,7 +69,6 @@ public OMKeyRenameRequestWithFSO(OMRequest omRequest, } @Override - @SuppressWarnings("methodlength") public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { final long trxnLogIndex = context.getIndex(); @@ -81,13 +79,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut String volumeName = keyArgs.getVolumeName(); String bucketName = keyArgs.getBucketName(); String fromKeyName = keyArgs.getKeyName(); - String toKeyName = renameKeyRequest.getToKeyName(); OMMetrics omMetrics = ozoneManager.getMetrics(); omMetrics.incNumKeyRenames(); - AuditLogger auditLogger = ozoneManager.getAuditLogger(); - OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder( getOmRequest()); @@ -95,7 +90,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean acquiredLock = false; OMClientResponse omClientResponse = null; Exception exception = null; - OmKeyInfo fromKeyValue; Result result; try { if (fromKeyName.isEmpty()) { @@ -103,109 +97,23 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut OMException.ResultCodes.INVALID_KEY_NAME); } - mergeOmLockDetails(omMetadataManager.getLock() - .acquireWriteLock(BUCKET_LOCK, volumeName, bucketName)); - acquiredLock = getOmLockDetails().isLockAcquired(); - - // Validate bucket and volume exists or not. - validateBucketAndVolume(omMetadataManager, volumeName, bucketName); - - // Check if fromKey exists - OzoneFileStatus fromKeyFileStatus = OMFileRequest.getOMKeyInfoIfExists( - omMetadataManager, volumeName, bucketName, fromKeyName, 0, - ozoneManager.getDefaultReplicationConfig()); - - // case-1) fromKeyName should exist, otw throws exception - if (fromKeyFileStatus == null) { - // TODO: Add support for renaming open key - throw new OMException("Key not found " + fromKeyName, KEY_NOT_FOUND); - } + PreparedRename prepared = prepareRename(ozoneManager, renameKeyRequest); - if (renameKeyRequest.hasUpdateID()) { - if (fromKeyFileStatus.getKeyInfo().getUpdateID() != renameKeyRequest.getUpdateID()) { - throw new OMException("UpdateID does not match. Key: " + fromKeyName + - ", Expected UpdateID: " + fromKeyFileStatus.getKeyInfo().getUpdateID() + - ", Given UpdateID: " + renameKeyRequest.getUpdateID(), OMException.ResultCodes.UPDATE_ID_NOT_MATCH); - } - } + if (prepared != null) { + // Phase 2 (bucket write lock): re-check the source, then apply the cache mutations. The lock + // is held only for this mutation tail, so bucket read-lock holders still observe each + // transaction atomically (all mutations or none), but are no longer blocked for the Phase 1 + // path walks. + mergeOmLockDetails(omMetadataManager.getLock() + .acquireWriteLock(BUCKET_LOCK, volumeName, bucketName)); + acquiredLock = getOmLockDetails().isLockAcquired(); - if (fromKeyFileStatus.getKeyInfo().isHsync()) { - throw new OMException("Open file cannot be renamed since it is " + - "hsync'ed: volumeName=" + volumeName + ", bucketName=" + - bucketName + ", key=" + fromKeyName, RENAME_OPEN_FILE); + omClientResponse = renameKey(prepared, fromKeyName, + keyArgs.getModificationTime(), ozoneManager, omResponse, + trxnLogIndex); } - // source existed - fromKeyValue = fromKeyFileStatus.getKeyInfo(); - boolean isRenameDirectory = fromKeyFileStatus.isDirectory(); - - // case-2) Cannot rename a directory to its own subdirectory - OMFileRequest.verifyToDirIsASubDirOfFromDirectory(fromKeyName, - toKeyName, fromKeyFileStatus.isDirectory()); - - OzoneFileStatus toKeyFileStatus = OMFileRequest.getOMKeyInfoIfExists( - omMetadataManager, volumeName, bucketName, toKeyName, 0, - ozoneManager.getDefaultReplicationConfig()); - - // Check if toKey exists. - if (toKeyFileStatus != null) { - // Destination exists and following are different cases: - OmKeyInfo toKeyValue = toKeyFileStatus.getKeyInfo(); - - if (fromKeyValue.getKeyName().equals(toKeyValue.getKeyName())) { - // case-3) If src == destin then check source and destin of same type - // (a) If dst is a file then return true. - // (b) Otherwise throws exception. - // TODO: Discuss do we need to throw exception for file as well. - if (toKeyFileStatus.isFile()) { - result = Result.SUCCESS; - } else { - throw new OMException("Key already exists " + toKeyName, - OMException.ResultCodes.KEY_ALREADY_EXISTS); - } - } else if (toKeyFileStatus.isDirectory()) { - // case-4) If dst is a directory then rename source as sub-path of it - // For example: rename /source to /dst will lead to /dst/source - String fromFileName = OzoneFSUtils.getFileName(fromKeyName); - String newToKeyName = OzoneFSUtils.appendFileNameToKeyPath(toKeyName, - fromFileName); - OzoneFileStatus newToOzoneFileStatus = - OMFileRequest.getOMKeyInfoIfExists(omMetadataManager, - volumeName, bucketName, newToKeyName, 0, - ozoneManager.getDefaultReplicationConfig()); - - if (newToOzoneFileStatus != null) { - // case-5) If new destin '/dst/source' exists then throws exception - throw new OMException(String.format( - "Failed to rename %s to %s, file already exists or not " + - "empty!", fromKeyName, newToKeyName), - OMException.ResultCodes.KEY_ALREADY_EXISTS); - } - - omClientResponse = renameKey(toKeyValue, newToKeyName, fromKeyValue, - fromKeyName, isRenameDirectory, keyArgs.getModificationTime(), - ozoneManager, omResponse, trxnLogIndex); - result = Result.SUCCESS; - } else { - // case-6) If destination is a file type and if exists then throws - // key already exists exception. - throw new OMException("Failed to rename, key already exists " - + toKeyName, OMException.ResultCodes.KEY_ALREADY_EXISTS); - } - } else { - // Destination doesn't exist and the cases are: - // case-7) Check whether dst parent dir exists or not. If parent - // doesn't exist then throw exception, otw the source can be renamed to - // destination path. - OmKeyInfo toKeyParent = OMFileRequest.getKeyParentDir(volumeName, - bucketName, toKeyName, ozoneManager, omMetadataManager); - - omClientResponse = renameKey(toKeyParent, toKeyName, fromKeyValue, - fromKeyName, isRenameDirectory, keyArgs.getModificationTime(), - ozoneManager, omResponse, trxnLogIndex); - - result = Result.SUCCESS; - } + result = Result.SUCCESS; } catch (IOException | InvalidPathException ex) { result = Result.FAILURE; exception = ex; @@ -221,26 +129,162 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut } } - markForAudit(auditLogger, buildAuditMessage(OMAction.RENAME_KEY, auditMap, - exception, getOmRequest().getUserInfo())); + auditAndLogResult(ozoneManager, renameKeyRequest, auditMap, exception, result); + return omClientResponse; + } - switch (result) { - case SUCCESS: - LOG.debug("Rename Key is successfully completed for volume:{} bucket:{}" + - " fromKey:{} toKey:{}. ", volumeName, bucketName, - fromKeyName, toKeyName); - break; - case FAILURE: - ozoneManager.getMetrics().incNumKeyRenameFails(); - LOG.error("Rename key failed for volume:{} bucket:{} fromKey:{} " + - "toKey:{}. Exception: {}.", volumeName, bucketName, - fromKeyName, toKeyName, exception.getMessage()); - break; - default: - LOG.error("Unrecognized Result for OMKeyRenameRequest: {}", - renameKeyRequest); + /** + * Phase 1 (no bucket lock): resolves the source, the destination and both parent directories. All + * of this reads committed state only. The OM apply path is single-threaded + * (OzoneManagerStateMachine uses a single-thread executor), so no other transaction can change what + * these reads observe before Phase 2 mutates; the double-buffer flush/cleanup threads only + * materialize already-committed epochs and never alter a key's visible value. + *

+ * Keeping these walks out of the bucket write lock is the point of HDDS-16289: it stops the lone + * apply thread from gating readers of a hot bucket (getBucketInfo/getFileStatus/lookupKey) while the + * path is resolved segment by segment. If OM ever applies transactions in parallel per bucket/key, + * these reads must be re-validated under the lock in {@link #renameKey}. + * + * @return the resolved rename, or {@code null} when there is nothing to apply (case-3, source and + * destination are the same file), in which case the caller takes no lock at all + */ + private PreparedRename prepareRename(OzoneManager ozoneManager, RenameKeyRequest renameKeyRequest) + throws IOException { + OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager(); + KeyArgs keyArgs = renameKeyRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String fromKeyName = keyArgs.getKeyName(); + String toKeyName = renameKeyRequest.getToKeyName(); + + // Validate bucket and volume exists or not. + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); + + // Check if fromKey exists + OzoneFileStatus fromKeyFileStatus = OMFileRequest.getOMKeyInfoIfExists( + omMetadataManager, volumeName, bucketName, fromKeyName, 0, + ozoneManager.getDefaultReplicationConfig()); + + // case-1) fromKeyName should exist, otw throws exception + if (fromKeyFileStatus == null) { + // TODO: Add support for renaming open key + throw new OMException("Key not found " + fromKeyName, KEY_NOT_FOUND); + } + + if (renameKeyRequest.hasUpdateID()) { + if (fromKeyFileStatus.getKeyInfo().getUpdateID() != renameKeyRequest.getUpdateID()) { + throw new OMException("UpdateID does not match. Key: " + fromKeyName + + ", Expected UpdateID: " + fromKeyFileStatus.getKeyInfo().getUpdateID() + + ", Given UpdateID: " + renameKeyRequest.getUpdateID(), OMException.ResultCodes.UPDATE_ID_NOT_MATCH); + } + } + + if (fromKeyFileStatus.getKeyInfo().isHsync()) { + throw new OMException("Open file cannot be renamed since it is " + + "hsync'ed: volumeName=" + volumeName + ", bucketName=" + + bucketName + ", key=" + fromKeyName, RENAME_OPEN_FILE); + } + + // source existed + OmKeyInfo fromKeyValue = fromKeyFileStatus.getKeyInfo(); + boolean isRenameDirectory = fromKeyFileStatus.isDirectory(); + + // case-2) Cannot rename a directory to its own subdirectory + OMFileRequest.verifyToDirIsASubDirOfFromDirectory(fromKeyName, + toKeyName, fromKeyFileStatus.isDirectory()); + + OzoneFileStatus toKeyFileStatus = OMFileRequest.getOMKeyInfoIfExists( + omMetadataManager, volumeName, bucketName, toKeyName, 0, + ozoneManager.getDefaultReplicationConfig()); + + // Resolved rename target: the parent directory the source moves under, and the destination name. + OmKeyInfo renameToParent; + String renameToKeyName; + + // Check if toKey exists. + if (toKeyFileStatus != null) { + // Destination exists and following are different cases: + OmKeyInfo toKeyValue = toKeyFileStatus.getKeyInfo(); + + if (fromKeyValue.getKeyName().equals(toKeyValue.getKeyName())) { + // case-3) If src == destin then check source and destin of same type + // (a) If dst is a file then return true. + // (b) Otherwise throws exception. + // TODO: Discuss do we need to throw exception for file as well. + if (!toKeyFileStatus.isFile()) { + throw new OMException("Key already exists " + toKeyName, + OMException.ResultCodes.KEY_ALREADY_EXISTS); + } + // Nothing to apply, so the caller skips the write lock entirely. + return null; + } else if (toKeyFileStatus.isDirectory()) { + // case-4) If dst is a directory then rename source as sub-path of it + // For example: rename /source to /dst will lead to /dst/source + String fromFileName = OzoneFSUtils.getFileName(fromKeyName); + String newToKeyName = OzoneFSUtils.appendFileNameToKeyPath(toKeyName, + fromFileName); + OzoneFileStatus newToOzoneFileStatus = + OMFileRequest.getOMKeyInfoIfExists(omMetadataManager, + volumeName, bucketName, newToKeyName, 0, + ozoneManager.getDefaultReplicationConfig()); + + if (newToOzoneFileStatus != null) { + // case-5) If new destin '/dst/source' exists then throws exception + throw new OMException(String.format( + "Failed to rename %s to %s, file already exists or not " + + "empty!", fromKeyName, newToKeyName), + OMException.ResultCodes.KEY_ALREADY_EXISTS); + } + + renameToParent = toKeyValue; + renameToKeyName = newToKeyName; + } else { + // case-6) If destination is a file type and if exists then throws + // key already exists exception. + throw new OMException("Failed to rename, key already exists " + + toKeyName, OMException.ResultCodes.KEY_ALREADY_EXISTS); + } + } else { + // Destination doesn't exist and the cases are: + // case-7) Check whether dst parent dir exists or not. If parent + // doesn't exist then throw exception, otw the source can be renamed to + // destination path. + renameToParent = OMFileRequest.getKeyParentDir(volumeName, + bucketName, toKeyName, ozoneManager, omMetadataManager); + renameToKeyName = toKeyName; + } + + // The source's parent directory also gets its modification time bumped, so resolve it here + // instead of inside renameKey: that keeps the last path walk out of the write lock, and it makes + // this walk's KEY_RENAME_ERROR precede every cache mutation rather than follow the destination + // parent's. + OmKeyInfo fromKeyParent = OMFileRequest.getKeyParentDir(volumeName, + bucketName, fromKeyName, ozoneManager, omMetadataManager); + + return new PreparedRename(fromKeyValue, isRenameDirectory, renameToParent, renameToKeyName, + fromKeyParent); + } + + /** + * Phase 1 output of {@link #prepareRename}, consumed by {@link #renameKey} under the bucket write + * lock: the resolved source, the destination parent and name, and the source's parent whose + * modification time is bumped with it. + */ + private static final class PreparedRename { + private final OmKeyInfo fromKeyValue; + private final boolean isRenameDirectory; + private final OmKeyInfo renameToParent; + private final String renameToKeyName; + private final OmKeyInfo fromKeyParent; + + PreparedRename(OmKeyInfo fromKeyValue, boolean isRenameDirectory, OmKeyInfo renameToParent, + String renameToKeyName, OmKeyInfo fromKeyParent) { + this.fromKeyValue = fromKeyValue; + this.isRenameDirectory = isRenameDirectory; + this.renameToParent = renameToParent; + this.renameToKeyName = renameToKeyName; + this.fromKeyParent = fromKeyParent; } - return omClientResponse; } @Override @@ -271,11 +315,19 @@ protected KeyArgs resolveBucketAndCheckAcls(KeyArgs keyArgs, return resolvedArgs; } - @SuppressWarnings("parameternumber") - private OMClientResponse renameKey(OmKeyInfo toKeyParent, String toKeyName, - OmKeyInfo fromKeyValue, String fromKeyName, boolean isRenameDirectory, - long modificationTime, OzoneManager ozoneManager, - OMResponse.Builder omResponse, long trxnLogIndex) throws IOException { + /** + * Phase 2 (under the bucket write lock): re-checks the source resolved by {@link #prepareRename}, + * then applies the dir-or-key table cache entries, the parent modification times and the bucket + * copy. + */ + private OMClientResponse renameKey(PreparedRename prepared, String fromKeyName, + long modificationTime, OzoneManager ozoneManager, OMResponse.Builder omResponse, + long trxnLogIndex) throws IOException { + final OmKeyInfo toKeyParent = prepared.renameToParent; + final String toKeyName = prepared.renameToKeyName; + final OmKeyInfo fromKeyParent = prepared.fromKeyParent; + final boolean isRenameDirectory = prepared.isRenameDirectory; + OmKeyInfo fromKeyValue = prepared.fromKeyValue; final OMMetadataManager ommm = ozoneManager.getMetadataManager(); final long volumeId = ommm.getVolumeId(fromKeyValue.getVolumeName()); final long bucketId = ommm.getBucketId(fromKeyValue.getVolumeName(), @@ -290,12 +342,21 @@ private OMClientResponse renameKey(OmKeyInfo toKeyParent, String toKeyName, } else { toKeyFileName = OzoneFSUtils.getFileName(toKeyName); } - OmKeyInfo fromKeyParent = null; OMMetadataManager metadataMgr = ozoneManager.getMetadataManager(); Table dirTable = metadataMgr.getDirectoryTable(); String bucketKey = metadataMgr.getBucketKey( fromKeyValue.getVolumeName(), fromKeyValue.getBucketName()); + // Cheap O(1) re-check of the source resolved in Phase 1 without the bucket lock. Under serial + // apply this always holds; it is a tripwire that fails safe, the same way the Phase 1 existence + // check does, if that invariant is ever broken by a concurrent writer. + final boolean fromKeyStillExists = isRenameDirectory + ? dirTable.get(dbFromKey) != null + : metadataMgr.getKeyTable(getBucketLayout()).get(dbFromKey) != null; + if (!fromKeyStillExists) { + throw new OMException("Key not found " + fromKeyName, KEY_NOT_FOUND); + } + OmKeyInfo.Builder fromKeyBuilder = fromKeyValue.toBuilder() .setUpdateID(trxnLogIndex); // Set toFileName @@ -312,8 +373,6 @@ private OMClientResponse renameKey(OmKeyInfo toKeyParent, String toKeyName, // Set modification time omBucketInfo = setModificationTime(ommm, omBucketInfo, toKeyParent, volumeId, bucketId, modificationTime, dirTable, trxnLogIndex); - fromKeyParent = OMFileRequest.getKeyParentDir(fromKeyValue.getVolumeName(), - fromKeyValue.getBucketName(), fromKeyName, ozoneManager, metadataMgr); if (fromKeyParent == null && omBucketInfo == null) { // Get omBucketInfo only when needed to reduce unnecessary DB IO omBucketInfo = metadataMgr.getBucketTable().get(bucketKey); @@ -426,4 +485,36 @@ protected String extractDstKey(RenameKeyRequest request) throws OMException { protected String extractSrcKey(KeyArgs keyArgs) throws OMException { return validateAndNormalizeKey(keyArgs.getKeyName()); } + + /** + * Emits the audit log and the result log outside the bucket lock. + */ + private void auditAndLogResult(OzoneManager ozoneManager, RenameKeyRequest renameKeyRequest, + Map auditMap, Exception exception, Result result) { + markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(OMAction.RENAME_KEY, auditMap, + exception, getOmRequest().getUserInfo())); + + KeyArgs keyArgs = renameKeyRequest.getKeyArgs(); + String volumeName = keyArgs.getVolumeName(); + String bucketName = keyArgs.getBucketName(); + String fromKeyName = keyArgs.getKeyName(); + String toKeyName = renameKeyRequest.getToKeyName(); + + switch (result) { + case SUCCESS: + LOG.debug("Rename Key is successfully completed for volume:{} bucket:{}" + + " fromKey:{} toKey:{}. ", volumeName, bucketName, + fromKeyName, toKeyName); + break; + case FAILURE: + ozoneManager.getMetrics().incNumKeyRenameFails(); + LOG.error("Rename key failed for volume:{} bucket:{} fromKey:{} " + + "toKey:{}. Exception: {}.", volumeName, bucketName, + fromKeyName, toKeyName, exception.getMessage()); + break; + default: + LOG.error("Unrecognized Result for OMKeyRenameRequest: {}", + renameKeyRequest); + } + } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapshotDiffValueParser.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapshotDiffValueParser.java index 6728eca2b07b..523f1db8697e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapshotDiffValueParser.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapshotDiffValueParser.java @@ -27,6 +27,8 @@ import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.KeyValue; +import org.apache.hadoop.hdds.utils.db.CodecBuffer; +import org.apache.hadoop.hdds.utils.db.CodecException; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.DirectoryInfo; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyInfo; @@ -172,6 +174,33 @@ public static ParsedRequiredInfo parseDirectoryInfoRequiredFields(byte[] value, return new ParsedRequiredInfo(updateId, hasUpdateId, objectId, parentId, name); } + /** + * Reads only the objectID of a serialized {@link DirectoryInfo}, skipping every other field. Used by + * the FSO path walk, which needs each parent directory's objectID but none of its other fields. + * + * @param value serialized {@link DirectoryInfo} + * @return the objectID, or 0 if the field is not set. + */ + public static long parseDirectoryInfoObjectId(CodecBuffer value) throws CodecException { + try { + CodedInputStream input = CodedInputStream.newInstance(value.asReadOnlyByteBuffer()); + long objectId = 0L; + + int tag; + while ((tag = input.readTag()) != 0) { + if (WireFormat.getTagFieldNumber(tag) == DirectoryInfo.OBJECTID_FIELD_NUMBER) { + objectId = input.readUInt64(); + } else { + input.skipField(tag); + } + } + + return objectId; + } catch (IOException e) { + throw new CodecException("Failed to read the objectID of a DirectoryInfo", e); + } + } + public static byte[] computeDirectoryInfoCompareSignature(byte[] value) throws IOException { CodedInputStream input = CodedInputStream.newInstance(value); MessageDigest digest = newDigest(); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java index cbaa63446991..6cecb67963f2 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java @@ -31,6 +31,7 @@ import java.util.NoSuchElementException; import java.util.UUID; import org.apache.hadoop.hdds.client.RatisReplicationConfig; +import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.om.OzonePrefixPathImpl; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.helpers.BucketLayout; @@ -402,4 +403,101 @@ public void testSnapshotUsedNamespaceAfterDirectoryDeleteAndPurge() throws Excep assertTrue(bucketInfoAfterPurge.getSnapshotUsedNamespace() >= 0, "SnapshotUsedNamespace went negative (" + bucketInfoAfterPurge.getSnapshotUsedNamespace() + ") due to bug."); } + + /** + * HDDS-16289 moved the emptiness check out of the bucket write lock, so assert that a non-recursive + * delete of a non-empty directory still fails with DIRECTORY_NOT_EMPTY and, because the check now + * precedes the lock, mutates nothing: the directory stays in the table and the bucket's used + * namespace is unchanged. + */ + @Test + public void testDeleteNonEmptyDirectoryNonRecursiveFails() throws Exception { + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName, omMetadataManager, getBucketLayout()); + + String parentDir = "parent"; + OMRequestTestUtils.addParentsToDirTable(volumeName, bucketName, parentDir, omMetadataManager); + OMRequestTestUtils.addParentsToDirTable(volumeName, bucketName, parentDir + "/childDir", omMetadataManager); + + String bucketKey = omMetadataManager.getBucketKey(volumeName, bucketName); + long usedNamespaceBefore = omMetadataManager.getBucketTable().get(bucketKey).getUsedNamespace(); + long volumeId = omMetadataManager.getVolumeId(volumeName); + long bucketId = omMetadataManager.getBucketId(volumeName, bucketName); + String dirKey = omMetadataManager.getOzonePathKey(volumeId, bucketId, bucketId, parentDir); + assertNotNull(omMetadataManager.getDirectoryTable().get(dirKey)); + + OMRequest deleteRequest = doPreExecute(createDeleteKeyRequest(parentDir, false)); + OMClientResponse response = getOmKeyDeleteRequest(deleteRequest) + .validateAndUpdateCache(ozoneManager, 100L); + + assertEquals(OzoneManagerProtocolProtos.Status.DIRECTORY_NOT_EMPTY, response.getOMResponse().getStatus()); + assertNotNull(omMetadataManager.getDirectoryTable().get(dirKey)); + assertEquals(usedNamespaceBefore, omMetadataManager.getBucketTable().get(bucketKey).getUsedNamespace()); + } + + /** + * The recursive counterpart of {@link #testDeleteNonEmptyDirectoryNonRecursiveFails()}: with + * recursive set, the emptiness check is skipped and the delete applies. + */ + @Test + public void testDeleteNonEmptyDirectoryRecursiveSucceeds() throws Exception { + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName, omMetadataManager, getBucketLayout()); + + String parentDir = "parent"; + OMRequestTestUtils.addParentsToDirTable(volumeName, bucketName, parentDir, omMetadataManager); + OMRequestTestUtils.addParentsToDirTable(volumeName, bucketName, parentDir + "/childDir", omMetadataManager); + + long volumeId = omMetadataManager.getVolumeId(volumeName); + long bucketId = omMetadataManager.getBucketId(volumeName, bucketName); + String dirKey = omMetadataManager.getOzonePathKey(volumeId, bucketId, bucketId, parentDir); + assertNotNull(omMetadataManager.getDirectoryTable().get(dirKey)); + + OMRequest deleteRequest = doPreExecute(createDeleteKeyRequest(parentDir, true)); + OMClientResponse response = getOmKeyDeleteRequest(deleteRequest) + .validateAndUpdateCache(ozoneManager, 100L); + + assertEquals(OzoneManagerProtocolProtos.Status.OK, response.getOMResponse().getStatus()); + assertNull(omMetadataManager.getDirectoryTable().get(dirKey)); + } + + /** + * HDDS-16289 split the hsync open-key handling across the two phases: the open key is read and + * rewritten before the bucket write lock, and only its cache entry is published under the lock. + * Assert the published entry still carries the deleted-hsync marker. + */ + @Test + public void testDeleteHsyncKeyMarksOpenKeyDeleted() throws Exception { + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName, omMetadataManager, getBucketLayout()); + + long parentId = OMRequestTestUtils.addParentsToDirTable(volumeName, bucketName, PARENT_DIR, omMetadataManager); + final long clientId = 100L; + + OmKeyInfo openKeyInfo = + OMRequestTestUtils.createOmKeyInfo(volumeName, bucketName, FILE_KEY, RatisReplicationConfig.getInstance(ONE)) + .setObjectID(parentId + 1L) + .setParentObjectID(parentId) + .setUpdateID(100L) + .build(); + openKeyInfo.setKeyName(FILE_NAME); + String dbOpenKey = OMRequestTestUtils.addFileToKeyTable(true, false, FILE_NAME, openKeyInfo, + clientId, 50L, omMetadataManager); + + OmKeyInfo committedKeyInfo = + OMRequestTestUtils.createOmKeyInfo(volumeName, bucketName, FILE_KEY, RatisReplicationConfig.getInstance(ONE)) + .setObjectID(parentId + 1L) + .setParentObjectID(parentId) + .setUpdateID(100L) + .addMetadata(OzoneConsts.HSYNC_CLIENT_ID, String.valueOf(clientId)) + .build(); + committedKeyInfo.setKeyName(FILE_NAME); + OMRequestTestUtils.addFileToKeyTable(false, false, FILE_NAME, committedKeyInfo, -1, 50L, omMetadataManager); + + OMRequest deleteRequest = doPreExecute(createDeleteKeyRequest(FILE_KEY, false)); + OMClientResponse response = getOmKeyDeleteRequest(deleteRequest) + .validateAndUpdateCache(ozoneManager, 101L); + + assertEquals(OzoneManagerProtocolProtos.Status.OK, response.getOMResponse().getStatus()); + OmKeyInfo cachedOpenKey = omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey); + assertNotNull(cachedOpenKey); + assertEquals("true", cachedOpenKey.getMetadata().get(OzoneConsts.DELETED_HSYNC_KEY)); + } } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java index e79f55a53dd7..f2da52ed6790 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java @@ -21,6 +21,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.IOException; @@ -122,6 +124,116 @@ public void testValidateAndUpdateCacheWithFromKeyInvalid() throws Exception { volumeName, bucketName, invalidFromKeyName, toKeyName))); } + @Test + public void testValidateAndUpdateCacheWithToKeyAsExistingDirectory() + throws Exception { + // case-4) toKey is an existing directory, so the source is renamed as a + // sub-path of it: /fromKey. + OmKeyInfo toKeyDirInfo = addToKeyAsDirectory(); + addKeyToTable(fromKeyInfo); + + OMRequest modifiedOmRequest = doPreExecute(createRenameKeyRequest( + volumeName, bucketName, fromKeyName, toKeyName)); + OMClientResponse response = getOMKeyRenameRequest(modifiedOmRequest) + .validateAndUpdateCache(ozoneManager, 100L); + + assertEquals(OzoneManagerProtocolProtos.Status.OK, + response.getOMResponse().getStatus()); + + // Source is gone and the key now hangs off the toKey directory. + assertNull(omMetadataManager.getKeyTable(getBucketLayout()) + .get(getDbPathKey(fromKeyParentInfo.getObjectID(), "fromKey"))); + assertNotNull(omMetadataManager.getKeyTable(getBucketLayout()) + .get(getDbPathKey(toKeyDirInfo.getObjectID(), "fromKey"))); + + // Both the source parent and the destination directory are bumped. Here the + // destination parent is the toKey directory itself, not toKeyParentInfo. + long modificationTime = modifiedOmRequest.getRenameKeyRequest() + .getKeyArgs().getModificationTime(); + assertEquals(modificationTime, omMetadataManager.getDirectoryTable() + .get(getDBKeyName(fromKeyParentInfo)).getModificationTime()); + assertEquals(modificationTime, omMetadataManager.getDirectoryTable() + .get(dbToKey).getModificationTime()); + } + + @Test + public void testValidateAndUpdateCacheWithNewToKeyAlreadyExists() + throws Exception { + // case-5) toKey is an existing directory but /fromKey is already + // taken, so the rename is rejected and nothing is mutated. + OmKeyInfo toKeyDirInfo = addToKeyAsDirectory(); + addKeyToTable(fromKeyInfo); + addKeyToTable(getOmKeyInfo(new Path(toKeyName, "fromKey").toString()) + .setParentObjectID(toKeyDirInfo.getObjectID()) + .build()); + + OMRequest modifiedOmRequest = doPreExecute(createRenameKeyRequest( + volumeName, bucketName, fromKeyName, toKeyName)); + OMClientResponse response = getOMKeyRenameRequest(modifiedOmRequest) + .validateAndUpdateCache(ozoneManager, 100L); + + assertEquals(OzoneManagerProtocolProtos.Status.KEY_ALREADY_EXISTS, + response.getOMResponse().getStatus()); + assertUnchangedOnFailure(); + } + + @Test + public void testValidateAndUpdateCacheWithToKeyAsExistingFile() + throws Exception { + // case-6) toKey exists and is a file, so the rename is rejected and nothing + // is mutated. + addKeyToTable(fromKeyInfo); + addKeyToTable(getOmKeyInfo(toKeyName) + .setParentObjectID(toKeyParentInfo.getObjectID()) + .build()); + + OMRequest modifiedOmRequest = doPreExecute(createRenameKeyRequest( + volumeName, bucketName, fromKeyName, toKeyName)); + OMClientResponse response = getOMKeyRenameRequest(modifiedOmRequest) + .validateAndUpdateCache(ozoneManager, 100L); + + assertEquals(OzoneManagerProtocolProtos.Status.KEY_ALREADY_EXISTS, + response.getOMResponse().getStatus()); + assertUnchangedOnFailure(); + } + + /** + * Turns toKeyName itself into a directory, so a rename to it resolves as + * case-4 instead of case-7. + */ + private OmKeyInfo addToKeyAsDirectory() throws Exception { + OmKeyInfo toKeyDirInfo = getOmKeyInfo(toKeyName) + .setParentObjectID(toKeyParentInfo.getObjectID()) + .build(); + OMRequestTestUtils.addDirKeyToDirTable(false, + OMFileRequest.getDirectoryInfo(toKeyDirInfo), volumeName, bucketName, + txnLogId, omMetadataManager); + return toKeyDirInfo; + } + + /** + * A rejected rename must leave the source in place and both parent + * directories' modification times untouched. + */ + private void assertUnchangedOnFailure() throws IOException { + assertNotNull(omMetadataManager.getKeyTable(getBucketLayout()) + .get(getDbPathKey(fromKeyParentInfo.getObjectID(), "fromKey"))); + assertEquals(fromKeyParentInfo.getModificationTime(), omMetadataManager + .getDirectoryTable().get(getDBKeyName(fromKeyParentInfo)) + .getModificationTime()); + assertEquals(toKeyParentInfo.getModificationTime(), omMetadataManager + .getDirectoryTable().get(getDBKeyName(toKeyParentInfo)) + .getModificationTime()); + } + + private String getDbPathKey(long parentObjectId, String fileName) + throws IOException { + return omMetadataManager.getOzonePathKey( + omMetadataManager.getVolumeId(volumeName), + omMetadataManager.getBucketId(volumeName, bucketName), + parentObjectId, fileName); + } + @Test public void testPreExecuteWithUnNormalizedPath() throws Exception { addKeyToTable(fromKeyInfo); From 29af3ed1978fdd39c5fc63c00cfeaa7d4b5fc706 Mon Sep 17 00:00:00 2001 From: Andrey Yarovoy Date: Fri, 25 Sep 2026 19:45:36 -0400 Subject: [PATCH 2/2] fixed the issue found by CI --- .../file/OMFileCreateRequestWithFSO.java | 17 +++++++---- .../file/TestOMFileCreateRequestWithFSO.java | 29 +++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java index bd0f4a1c57a3..460a285f51dd 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java @@ -221,6 +221,13 @@ private PreparedFileCreate prepareFileCreate(OzoneManager ozoneManager, pathInfoFSO.getLastKnownParentId(), pathInfoFSO.getLeafNodeName(), createFileRequest.getClientID()); + // Key of the leaf in the file table, for the Phase 2 re-check. Built from the parent id + // getAllMissingParentDirInfo left on pathInfoFSO, so it addresses the leaf even when intermediate + // directories were missing and this transaction creates them. dbFileKey above predates that call + // and can still name the deepest pre-existing ancestor, which is a different directory. + final String dbLeafFileKey = omMetadataManager.getOzonePathKey(volumeId, bucketId, + pathInfoFSO.getLastKnownParentId(), pathInfoFSO.getLeafNodeName()); + // Append new blocks List newLocationList = keyArgs.getKeyLocationsList() .stream().map(OmKeyLocationInfo::getFromProtobuf) @@ -231,7 +238,7 @@ private PreparedFileCreate prepareFileCreate(OzoneManager ozoneManager, newLocationList.size() * ozoneManager.getScmBlockSize() * repConfig .getRequiredNodes(); - return new PreparedFileCreate(volumeId, bucketId, dbFileKey, omFileInfo, + return new PreparedFileCreate(volumeId, bucketId, dbLeafFileKey, omFileInfo, missingParentInfos, dbOpenFileName, preAllocatedSpace); } @@ -252,7 +259,7 @@ private OMClientResponse applyFileCreate(OMMetadataManager omMetadataManager, // holds; it is a tripwire that fails safe (as checkDirectoryResult would) rather than silently // overwriting if that invariant is ever broken by a concurrent writer. if (!createFileRequest.getIsOverwrite() && OMFileRequest.getOmKeyInfoFromFileTable(false, - omMetadataManager, prepared.dbFileKey, keyName) != null) { + omMetadataManager, prepared.dbLeafFileKey, keyName) != null) { throw new OMException("File " + keyName + " already exists", OMException.ResultCodes.FILE_ALREADY_EXISTS); } @@ -299,17 +306,17 @@ private OMClientResponse applyFileCreate(OMMetadataManager omMetadataManager, private static final class PreparedFileCreate { private final long volumeId; private final long bucketId; - private final String dbFileKey; + private final String dbLeafFileKey; private final OmKeyInfo omFileInfo; private final List missingParentInfos; private final String dbOpenFileName; private final long preAllocatedSpace; - PreparedFileCreate(long volumeId, long bucketId, String dbFileKey, OmKeyInfo omFileInfo, + PreparedFileCreate(long volumeId, long bucketId, String dbLeafFileKey, OmKeyInfo omFileInfo, List missingParentInfos, String dbOpenFileName, long preAllocatedSpace) { this.volumeId = volumeId; this.bucketId = bucketId; - this.dbFileKey = dbFileKey; + this.dbLeafFileKey = dbLeafFileKey; this.omFileInfo = omFileInfo; this.missingParentInfos = missingParentInfos; this.dbOpenFileName = dbOpenFileName; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequestWithFSO.java index c04fdf7dd11e..918420111012 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequestWithFSO.java @@ -163,6 +163,35 @@ public void testValidateAndUpdateCacheWithNonRecursiveAndOverWrite() testNonRecursivePath(key, false, false, true); } + @Test + public void testCreateFileUnderMissingParentWithSameNameAtAncestor() + throws Exception { + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName, + omMetadataManager, getBucketLayout()); + final long bucketId = omMetadataManager.getBucketId(volumeName, bucketName); + + // A committed file "key1" sitting directly under the bucket. + String fileName = "key1"; + OmKeyInfo existingFile = + OMRequestTestUtils.createOmKeyInfo(volumeName, bucketName, fileName, + RatisReplicationConfig.getInstance(ONE)) + .setObjectID(bucketId + 1L) + .setParentObjectID(bucketId) + .setUpdateID(100L) + .build(); + String existingDbKey = OMRequestTestUtils.addFileToKeyTable(false, false, + fileName, existingFile, -1, 100L, omMetadataManager); + + // "d1" does not exist, so the path walk stops at the bucket: the deepest known parent of the leaf + // is the bucket itself, which is also the parent of the committed "key1". Creating "d1/key1" must + // still succeed - it is a different path, and "d1" is created by this transaction. + testNonRecursivePath("d1/" + fileName, false, true, false); + + // The file at the bucket root is untouched. + assertNotNull(omMetadataManager.getKeyTable(getBucketLayout()) + .get(existingDbKey)); + } + @Override @Test public void testCreateFileInheritParentDefaultAcls()