Skip to content

Commit c0040fe

Browse files
committed
HDFS-17839. Rename skips path without valid 'DirectoryWithQuotaFeature' in FSDirRenameOp
1 parent bcf8c35 commit c0040fe

File tree

5 files changed

+141
-31
lines changed

5 files changed

+141
-31
lines changed

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirMkdirOp.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ private static INodesInPath unprotectedMkdir(FSDirectory fsd, long inodeId,
224224
timestamp);
225225

226226
INodesInPath iip = fsd.addLastINode(parent, dir, permission.getPermission(),
227-
true, Optional.empty());
227+
true, Optional.empty(), true);
228228
if (iip != null && aclEntries != null) {
229229
AclStorage.updateINodeAcl(dir, aclEntries, Snapshot.CURRENT_STATE_ID);
230230
}

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
*/
1818
package org.apache.hadoop.hdfs.server.namenode;
1919

20-
import org.apache.commons.lang3.tuple.Pair;
20+
import org.apache.commons.lang3.tuple.Triple;
2121
import org.apache.hadoop.hdfs.protocol.HdfsConstants;
2222
import org.apache.hadoop.util.Preconditions;
2323
import org.apache.hadoop.fs.FileAlreadyExistsException;
@@ -72,14 +72,25 @@ static RenameResult renameToInt(
7272
* Verify quota for rename operation where srcInodes[srcInodes.length-1] moves
7373
* dstInodes[dstInodes.length-1]
7474
*/
75-
private static Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaForRename(
76-
FSDirectory fsd, INodesInPath src, INodesInPath dst) throws QuotaExceededException {
75+
private static Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaForRename(
76+
FSDirectory fsd, INodesInPath src, INodesInPath dst, boolean overwrite)
77+
throws QuotaExceededException {
7778
Optional<QuotaCounts> srcDelta = Optional.empty();
7879
Optional<QuotaCounts> dstDelta = Optional.empty();
7980
if (!fsd.getFSNamesystem().isImageLoaded() || fsd.shouldSkipQuotaChecks()) {
8081
// Do not check quota if edits log is still being processed
81-
return Pair.of(srcDelta, dstDelta);
82+
return Triple.of(false, srcDelta, dstDelta);
8283
}
84+
85+
// Verify srcPath and dstPath without valid 'DirectoryWithQuotaFeature'
86+
// Note: In overwrite scenarios, quota calculation is still required because:
87+
// Overwrite operations delete existing content (affecting rootDir's quota)
88+
if (FSDirectory.verifyPathWithoutValidQuotaFeature(src, src.length() - 1)
89+
&& FSDirectory.verifyPathWithoutValidQuotaFeature(dst, dst.length() - 1)
90+
&& !overwrite) {
91+
return Triple.of(false, srcDelta, dstDelta);
92+
}
93+
8394
int i = 0;
8495
while (src.getINode(i) == dst.getINode(i)) {
8596
i++;
@@ -108,7 +119,7 @@ private static Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaFor
108119
delta.subtract(counts);
109120
}
110121
FSDirectory.verifyQuota(dst, dst.length() - 1, delta, src.getINode(i - 1));
111-
return Pair.of(srcDelta, dstDelta);
122+
return Triple.of(true, srcDelta, dstDelta);
112123
}
113124

114125
/**
@@ -216,10 +227,10 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
216227
fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
217228
// Ensure dst has quota to accommodate rename
218229
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
219-
Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> countPair =
220-
verifyQuotaForRename(fsd, srcIIP, dstIIP);
230+
Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> countTriple =
231+
verifyQuotaForRename(fsd, srcIIP, dstIIP, false);
221232

222-
RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, countPair);
233+
RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, countTriple);
223234

224235
boolean added = false;
225236

@@ -436,10 +447,10 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
436447

437448
// Ensure dst has quota to accommodate rename
438449
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
439-
Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> quotaPair =
440-
verifyQuotaForRename(fsd, srcIIP, dstIIP);
450+
Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> countTriple =
451+
verifyQuotaForRename(fsd, srcIIP, dstIIP, overwrite);
441452

442-
RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, quotaPair);
453+
RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, countTriple);
443454

444455
boolean undoRemoveSrc = true;
445456
tx.removeSrc();
@@ -656,13 +667,14 @@ private static class RenameOperation {
656667
private final boolean srcChildIsReference;
657668
private final QuotaCounts oldSrcCountsInSnapshot;
658669
private final boolean sameStoragePolicy;
670+
private final boolean updateQuota;
659671
private final Optional<QuotaCounts> srcSubTreeCount;
660672
private final Optional<QuotaCounts> dstSubTreeCount;
661673
private INode srcChild;
662674
private INode oldDstChild;
663675

664676
RenameOperation(FSDirectory fsd, INodesInPath srcIIP, INodesInPath dstIIP,
665-
Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> quotaPair) {
677+
Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> quotaPair) {
666678
this.fsd = fsd;
667679
this.srcIIP = srcIIP;
668680
this.dstIIP = dstIIP;
@@ -711,9 +723,10 @@ private static class RenameOperation {
711723
} else {
712724
withCount = null;
713725
}
726+
this.updateQuota = quotaPair.getLeft();
714727
// Set quota for src and dst, ignore src is in Snapshot or is Reference
715728
this.srcSubTreeCount = withCount == null ?
716-
quotaPair.getLeft() : Optional.empty();
729+
quotaPair.getMiddle() : Optional.empty();
717730
this.dstSubTreeCount = quotaPair.getRight();
718731
}
719732

@@ -757,7 +770,9 @@ long removeSrc() throws IOException {
757770
// update the quota count if necessary
758771
Optional<QuotaCounts> countOp = sameStoragePolicy ?
759772
srcSubTreeCount : Optional.empty();
760-
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
773+
if (updateQuota) {
774+
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
775+
}
761776
srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, null);
762777
return removedNum;
763778
}
@@ -774,7 +789,9 @@ boolean removeSrc4OldRename() {
774789
// update the quota count if necessary
775790
Optional<QuotaCounts> countOp = sameStoragePolicy ?
776791
srcSubTreeCount : Optional.empty();
777-
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
792+
if (updateQuota) {
793+
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
794+
}
778795
srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, null);
779796
return true;
780797
}
@@ -785,7 +802,9 @@ long removeDst() {
785802
if (removedNum != -1) {
786803
oldDstChild = dstIIP.getLastINode();
787804
// update the quota count if necessary
788-
fsd.updateCountForDelete(oldDstChild, dstIIP, dstSubTreeCount);
805+
if (updateQuota) {
806+
fsd.updateCountForDelete(oldDstChild, dstIIP, dstSubTreeCount);
807+
}
789808
dstIIP = INodesInPath.replace(dstIIP, dstIIP.length() - 1, null);
790809
}
791810
return removedNum;
@@ -803,7 +822,7 @@ INodesInPath addSourceToDestination() {
803822
toDst = new INodeReference.DstReference(dstParent.asDirectory(),
804823
withCount, dstIIP.getLatestSnapshotId());
805824
}
806-
return fsd.addLastINodeNoQuotaCheck(dstParentIIP, toDst, srcSubTreeCount);
825+
return fsd.addLastINodeNoQuotaCheck(dstParentIIP, toDst, srcSubTreeCount, updateQuota);
807826
}
808827

809828
void updateMtimeAndLease(long timestamp) {
@@ -837,7 +856,7 @@ void restoreSource() {
837856
// the srcChild back
838857
Optional<QuotaCounts> countOp = sameStoragePolicy ?
839858
srcSubTreeCount : Optional.empty();
840-
fsd.addLastINodeNoQuotaCheck(srcParentIIP, srcChild, countOp);
859+
fsd.addLastINodeNoQuotaCheck(srcParentIIP, srcChild, countOp, updateQuota);
841860
}
842861
}
843862

@@ -847,7 +866,7 @@ void restoreDst(BlockStoragePolicySuite bsps) {
847866
if (dstParent.isWithSnapshot()) {
848867
dstParent.undoRename4DstParent(bsps, oldDstChild, dstIIP.getLatestSnapshotId());
849868
} else {
850-
fsd.addLastINodeNoQuotaCheck(dstParentIIP, oldDstChild, dstSubTreeCount);
869+
fsd.addLastINodeNoQuotaCheck(dstParentIIP, oldDstChild, dstSubTreeCount, updateQuota);
851870
}
852871
if (oldDstChild != null && oldDstChild.isReference()) {
853872
final INodeReference removedDstRef = oldDstChild.asReference();

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1196,7 +1196,7 @@ INodesInPath addINode(INodesInPath existing, INode child,
11961196
cacheName(child);
11971197
writeLock();
11981198
try {
1199-
return addLastINode(existing, child, modes, true, Optional.empty());
1199+
return addLastINode(existing, child, modes, true, Optional.empty(), true);
12001200
} finally {
12011201
writeUnlock();
12021202
}
@@ -1241,6 +1241,27 @@ static void verifyQuota(INodesInPath iip, int pos, QuotaCounts deltas,
12411241
}
12421242
}
12431243
}
1244+
1245+
/**
1246+
* Verifies that the path from the specified position to the root
1247+
* (excluding the root itself) does not contain any valid quota features.
1248+
*
1249+
* @param iip the INodesInPath containing all the ancestral INodes.
1250+
* @param pos Starting position in the path (0-based index).
1251+
* @return true if no valid quota features are found along the path (root excluded),
1252+
* false if any directory in the path has an active quota feature.
1253+
*/
1254+
static boolean verifyPathWithoutValidQuotaFeature(INodesInPath iip, int pos) {
1255+
// Does not include the root path
1256+
for (int i = Math.min(pos, iip.length()) - 1; i >= 1; i--) {
1257+
final DirectoryWithQuotaFeature q =
1258+
iip.getINode(i).asDirectory().getDirectoryWithQuotaFeature();
1259+
if (q != null) {
1260+
return false;
1261+
}
1262+
}
1263+
return true;
1264+
}
12441265

12451266
/** Verify if the inode name is legal. */
12461267
void verifyINodeName(byte[] childName) throws HadoopIllegalArgumentException {
@@ -1347,7 +1368,7 @@ private void copyINodeDefaultAcl(INode child, FsPermission modes) {
13471368
*/
13481369
@VisibleForTesting
13491370
public INodesInPath addLastINode(INodesInPath existing, INode inode,
1350-
FsPermission modes, boolean checkQuota, Optional<QuotaCounts> quotaCount)
1371+
FsPermission modes, boolean checkQuota, Optional<QuotaCounts> quotaCount, boolean updateQuota)
13511372
throws QuotaExceededException {
13521373
assert existing.getLastINode() != null &&
13531374
existing.getLastINode().isDirectory();
@@ -1379,16 +1400,23 @@ public INodesInPath addLastINode(INodesInPath existing, INode inode,
13791400
// always verify inode name
13801401
verifyINodeName(inode.getLocalNameBytes());
13811402

1382-
final QuotaCounts counts = quotaCount.orElseGet(() -> inode.
1383-
computeQuotaUsage(getBlockStoragePolicySuite(),
1384-
parent.getStoragePolicyID(), false,
1385-
Snapshot.CURRENT_STATE_ID));
1386-
updateCount(existing, pos, counts, checkQuota);
1403+
QuotaCounts counts;
1404+
if (updateQuota) {
1405+
counts = quotaCount.orElseGet(() -> inode.
1406+
computeQuotaUsage(getBlockStoragePolicySuite(),
1407+
parent.getStoragePolicyID(), false,
1408+
Snapshot.CURRENT_STATE_ID));
1409+
updateCount(existing, pos, counts, checkQuota);
1410+
}
13871411

13881412
boolean isRename = (inode.getParent() != null);
13891413
final boolean added = parent.addChild(inode, true,
13901414
existing.getLatestSnapshotId());
1391-
if (!added) {
1415+
if (!added && updateQuota) {
1416+
counts = quotaCount.orElseGet(() -> inode.
1417+
computeQuotaUsage(getBlockStoragePolicySuite(),
1418+
parent.getStoragePolicyID(), false,
1419+
Snapshot.CURRENT_STATE_ID));
13921420
updateCountNoQuotaCheck(existing, pos, counts.negation());
13931421
return null;
13941422
} else {
@@ -1401,10 +1429,10 @@ public INodesInPath addLastINode(INodesInPath existing, INode inode,
14011429
}
14021430

14031431
INodesInPath addLastINodeNoQuotaCheck(INodesInPath existing, INode i,
1404-
Optional<QuotaCounts> quotaCount) {
1432+
Optional<QuotaCounts> quotaCount, boolean updateQuota) {
14051433
try {
14061434
// All callers do not have create modes to pass.
1407-
return addLastINode(existing, i, null, false, quotaCount);
1435+
return addLastINode(existing, i, null, false, quotaCount, updateQuota);
14081436
} catch (QuotaExceededException e) {
14091437
NameNode.LOG.warn("FSDirectory.addChildNoQuotaCheck - unexpected", e);
14101438
}

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestCorrectnessOfQuotaAfterRenameOp.java

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.hadoop.fs.Options;
2222
import org.apache.hadoop.fs.Path;
2323

24+
import org.apache.hadoop.fs.QuotaUsage;
2425
import org.apache.hadoop.hdfs.DFSTestUtil;
2526
import org.apache.hadoop.hdfs.DistributedFileSystem;
2627
import org.apache.hadoop.hdfs.HdfsConfiguration;
@@ -79,24 +80,32 @@ public void testQuotaUsageWhenRenameWithSameStoragePolicy() throws Exception {
7980
DFSTestUtil.createFile(dfs, file2, fileLen, replication, 0);
8081

8182
final Path dstDir1 = new Path(testParentDir2, "dst-dir");
83+
// If dstDir1 not exist, after the rename operation,
84+
// the root dir's quota usage should remain unchanged.
85+
QuotaUsage quotaUsage1 = dfs.getQuotaUsage(new Path("/"));
8286
ContentSummary cs1 = dfs.getContentSummary(testParentDir1);
8387
// srcDir=/root/test1/src/dir
8488
// dstDir1=/root/test2/dst-dir dstDir1 not exist
8589
boolean rename = dfs.rename(srcDir, dstDir1);
8690
assertEquals(true, rename);
91+
QuotaUsage quotaUsage2 = dfs.getQuotaUsage(new Path("/"));
8792
ContentSummary cs2 = dfs.getContentSummary(testParentDir2);
93+
assertEquals(quotaUsage1, quotaUsage2);
8894
assertTrue(cs1.equals(cs2));
8995

9096

9197
final Path dstDir2 = new Path(testParentDir3, "dst-dir");
9298
assertTrue(dfs.mkdirs(dstDir2));
99+
QuotaUsage quotaUsage3 = dfs.getQuotaUsage(testParentDir2);
93100
ContentSummary cs3 = dfs.getContentSummary(testParentDir2);
94101

95102
//Src and dst must be same (all file or all dir)
96103
// dstDir1=/root/test2/dst-dir
97104
// dstDir2=/root/test3/dst-dir
98105
dfs.rename(dstDir1, dstDir2, Options.Rename.OVERWRITE);
106+
QuotaUsage quotaUsage4 = dfs.getQuotaUsage(testParentDir3);
99107
ContentSummary cs4 = dfs.getContentSummary(testParentDir3);
108+
assertEquals(quotaUsage3, quotaUsage4);
100109
assertTrue(cs3.equals(cs4));
101110
}
102111

@@ -158,4 +167,58 @@ public void testQuotaUsageWhenRenameWithDifferStoragePolicy() throws Exception {
158167
QuotaCounts subtract = dstCountsAfterRename.subtract(dstCountsBeforeRename);
159168
assertTrue(subtract.equals(srcCounts));
160169
}
170+
171+
@Test
172+
public void testRenameWithoutValidQuotaFeature() throws Exception {
173+
final int fileLen = 1024;
174+
final short replication = 3;
175+
final Path root = new Path("/testRoot");
176+
assertTrue(dfs.mkdirs(root));
177+
178+
Path testParentDir1 = new Path(root, "testDir1");
179+
assertTrue(dfs.mkdirs(testParentDir1));
180+
Path testParentDir2 = new Path(root, "testDir2");
181+
assertTrue(dfs.mkdirs(testParentDir2));
182+
Path testParentDir3 = new Path(root, "testDir3");
183+
assertTrue(dfs.mkdirs(testParentDir3));
184+
185+
final Path srcDir = new Path(testParentDir1, "src-dir");
186+
for (int i = 0; i < 2; i++) {
187+
Path file1 = new Path(srcDir, "file" + i);
188+
DFSTestUtil.createFile(dfs, file1, fileLen, replication, 0);
189+
}
190+
191+
// 1. Test rename1
192+
QuotaUsage quotaUsage1 = dfs.getQuotaUsage(new Path("/"));
193+
final Path dstDir1 = new Path(testParentDir2, "dst-dir");
194+
ContentSummary cs1 = dfs.getContentSummary(testParentDir1);
195+
196+
// srcDir=/testRoot/testDir1/src-dir
197+
// dstDir=/testRoot/testDir2/dst-dir dstDir1 not exist
198+
boolean rename = dfs.rename(srcDir, dstDir1);
199+
assertTrue(rename);
200+
ContentSummary cs2 = dfs.getContentSummary(testParentDir2);
201+
assertEquals(cs1, cs2);
202+
203+
QuotaUsage quotaUsage2 = dfs.getQuotaUsage(new Path("/"));
204+
assertEquals(quotaUsage1.getFileAndDirectoryCount(), quotaUsage2.getFileAndDirectoryCount());
205+
206+
// 2. Test rename2
207+
final Path dstDir2 = new Path(testParentDir3, "dst-dir");
208+
assertTrue(dfs.mkdirs(dstDir2));
209+
long originDstDir2Usage = dfs.getQuotaUsage(dstDir2).getFileAndDirectoryCount();
210+
// The usage for covering the root dir should not include dstDir2 usage
211+
long rootINodeCount1 =
212+
dfs.getQuotaUsage(new Path("/")).getFileAndDirectoryCount() - originDstDir2Usage;
213+
ContentSummary cs3 = dfs.getContentSummary(testParentDir2);
214+
215+
// Src and dst must be same (all file or all dir)
216+
// dstDir1=/testRoot/testDir3/dst-dir
217+
// dstDir2=/testRoot/testDir3/dst-dir
218+
dfs.rename(dstDir1, dstDir2, Options.Rename.OVERWRITE);
219+
long rootINodeCount2 = dfs.getQuotaUsage(new Path("/")).getFileAndDirectoryCount();
220+
assertEquals(rootINodeCount1, rootINodeCount2);
221+
ContentSummary cs4 = dfs.getContentSummary(testParentDir3);
222+
assertEquals(cs3, cs4);
223+
}
161224
}

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1650,7 +1650,7 @@ public void testRenameUndo_5() throws Exception {
16501650
final Path foo2 = new Path(subdir2, foo.getName());
16511651
FSDirectory fsdir2 = Mockito.spy(fsdir);
16521652
Mockito.doThrow(new NSQuotaExceededException("fake exception")).when(fsdir2)
1653-
.addLastINode(any(), any(), any(), anyBoolean(), any());
1653+
.addLastINode(any(), any(), any(), anyBoolean(), any(), anyBoolean());
16541654
Whitebox.setInternalState(fsn, "dir", fsdir2);
16551655
// rename /test/dir1/foo to /test/dir2/subdir2/foo.
16561656
// FSDirectory#verifyQuota4Rename will pass since the remaining quota is 2.

0 commit comments

Comments
 (0)