Skip to content

HDFS-17839. Rename skips path without valid 'DirectoryWithQuotaFeature' in FSDirRenameOp #7989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

HDFS-17839. Rename skips path without valid 'DirectoryWithQuotaFeature' in FSDirRenameOp #7989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0040fe
HDFS-17839. Rename skips path without valid 'DirectoryWithQuotaFeatur…
huangzhaobo99 Sep 21, 2025
3c60bf0
fix: snapshot scenarios & checkstyle
huangzhaobo99 Sep 21, 2025
1e9321b
fix update quota & add ut
huangzhaobo99 Sep 28, 2025
8f570c9
fix checkstyle
huangzhaobo99 Sep 29, 2025
4f46b2a
add snapshot ut
huangzhaobo99 Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...roject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirMkdirOp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ private static INodesInPath unprotectedMkdir(FSDirectory fsd, long inodeId,
timestamp);

INodesInPath iip = fsd.addLastINode(parent, dir, permission.getPermission(),
true, Optional.empty());
true, Optional.empty(), true);
if (iip != null && aclEntries != null) {
AclStorage.updateINodeAcl(dir, aclEntries, Snapshot.CURRENT_STATE_ID);
}
Expand Down
66 changes: 42 additions & 24 deletions ...oject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
*/
package org.apache.hadoop.hdfs.server.namenode;

import org.apache.commons.lang3.tuple.Pair;
import org.apache.commons.lang3.tuple.Triple;
import org.apache.hadoop.hdfs.protocol.HdfsConstants;
import org.apache.hadoop.util.Preconditions;
import org.apache.hadoop.fs.FileAlreadyExistsException;
Expand Down Expand Up @@ -72,14 +72,24 @@ static RenameResult renameToInt(
* Verify quota for rename operation where srcInodes[srcInodes.length-1] moves
* dstInodes[dstInodes.length-1]
*/
private static Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaForRename(
FSDirectory fsd, INodesInPath src, INodesInPath dst) throws QuotaExceededException {
private static Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaForRename(
FSDirectory fsd, INodesInPath src, INodesInPath dst, boolean overwrite)
throws QuotaExceededException {
Comment on lines +75 to +77
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method signature change from Pair to Triple with a Boolean parameter lacks clear documentation. The Boolean return value's meaning is not obvious from the method name or parameters. Consider renaming the method or adding clear JavaDoc to explain what the Boolean represents.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

@huangzhaobo99 huangzhaobo99 Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

  /**
   * Verify quota for rename operation where srcInodes[srcInodes.length-1] moves
   * dstInodes[dstInodes.length-1]
   *
   * @param fsd       FSDirectory instance
   * @param src       Source path inodes
   * @param dst       Destination path inodes
   * @param overwrite Whether destination will be overwritten
   * @return Triple containing:
   * - Boolean: true if quota was updated, false if skipped
   * - Optional<QuotaCounts>: Source path quotaCounts delta
   * - Optional<QuotaCounts>: Destination path quotaCounts delta
   * @throws QuotaExceededException if quota limit is exceeded
   */

Optional<QuotaCounts> srcDelta = Optional.empty();
Optional<QuotaCounts> dstDelta = Optional.empty();
if (!fsd.getFSNamesystem().isImageLoaded() || fsd.shouldSkipQuotaChecks()) {
// Do not check quota if edits log is still being processed
return Pair.of(srcDelta, dstDelta);
return Triple.of(false, srcDelta, dstDelta);
}

// Verify path without valid 'DirectoryWithQuotaFeature'
// Note: In overwrite scenarios, quota calculation is still required,
// overwrite operations delete existing content, which affects the root directory's quota.
if (!overwrite && FSDirectory.verifyPathWithoutValidQuotaFeature(src)
&& FSDirectory.verifyPathWithoutValidQuotaFeature(dst)) {
return Triple.of(false, srcDelta, dstDelta);
}

int i = 0;
while (src.getINode(i) == dst.getINode(i)) {
i++;
Expand Down Expand Up @@ -108,7 +118,7 @@ private static Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaFor
delta.subtract(counts);
}
FSDirectory.verifyQuota(dst, dst.length() - 1, delta, src.getINode(i - 1));
return Pair.of(srcDelta, dstDelta);
return Triple.of(true, srcDelta, dstDelta);
}

/**
Expand Down Expand Up @@ -216,10 +226,10 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
// Ensure dst has quota to accommodate rename
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> countPair =
verifyQuotaForRename(fsd, srcIIP, dstIIP);
Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> countTriple =
verifyQuotaForRename(fsd, srcIIP, dstIIP, false);

RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, countPair);
RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, countTriple);

boolean added = false;

Expand Down Expand Up @@ -436,10 +446,10 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,

// Ensure dst has quota to accommodate rename
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> quotaPair =
verifyQuotaForRename(fsd, srcIIP, dstIIP);
Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> countTriple =
verifyQuotaForRename(fsd, srcIIP, dstIIP, overwrite);

RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, quotaPair);
RenameOperation tx = new RenameOperation(fsd, srcIIP, dstIIP, countTriple);

boolean undoRemoveSrc = true;
tx.removeSrc();
Expand Down Expand Up @@ -656,13 +666,14 @@ private static class RenameOperation {
private final boolean srcChildIsReference;
private final QuotaCounts oldSrcCountsInSnapshot;
private final boolean sameStoragePolicy;
private final boolean updateQuota;
private final Optional<QuotaCounts> srcSubTreeCount;
private final Optional<QuotaCounts> dstSubTreeCount;
private INode srcChild;
private INode oldDstChild;

RenameOperation(FSDirectory fsd, INodesInPath srcIIP, INodesInPath dstIIP,
Pair<Optional<QuotaCounts>, Optional<QuotaCounts>> quotaPair) {
Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> countTriple) {
this.fsd = fsd;
this.srcIIP = srcIIP;
this.dstIIP = dstIIP;
Expand Down Expand Up @@ -712,9 +723,10 @@ private static class RenameOperation {
withCount = null;
}
// Set quota for src and dst, ignore src is in Snapshot or is Reference
this.updateQuota = countTriple.getLeft() || withCount != null;
this.srcSubTreeCount = withCount == null ?
quotaPair.getLeft() : Optional.empty();
this.dstSubTreeCount = quotaPair.getRight();
countTriple.getMiddle() : Optional.empty();
this.dstSubTreeCount = countTriple.getRight();
}

boolean isSameStoragePolicy() {
Expand Down Expand Up @@ -755,9 +767,11 @@ long removeSrc() throws IOException {
throw new IOException(error);
} else {
// update the quota count if necessary
Optional<QuotaCounts> countOp = sameStoragePolicy ?
srcSubTreeCount : Optional.empty();
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
if (updateQuota) {
Optional<QuotaCounts> countOp = sameStoragePolicy ?
srcSubTreeCount : Optional.empty();
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
}
srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, null);
return removedNum;
}
Expand All @@ -772,9 +786,11 @@ boolean removeSrc4OldRename() {
return false;
} else {
// update the quota count if necessary
Optional<QuotaCounts> countOp = sameStoragePolicy ?
srcSubTreeCount : Optional.empty();
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
if (updateQuota) {
Optional<QuotaCounts> countOp = sameStoragePolicy ?
srcSubTreeCount : Optional.empty();
fsd.updateCountForDelete(srcChild, srcIIP, countOp);
}
srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, null);
return true;
}
Expand All @@ -785,7 +801,9 @@ long removeDst() {
if (removedNum != -1) {
oldDstChild = dstIIP.getLastINode();
// update the quota count if necessary
fsd.updateCountForDelete(oldDstChild, dstIIP, dstSubTreeCount);
if (updateQuota) {
fsd.updateCountForDelete(oldDstChild, dstIIP, dstSubTreeCount);
}
dstIIP = INodesInPath.replace(dstIIP, dstIIP.length() - 1, null);
}
return removedNum;
Expand All @@ -803,7 +821,7 @@ INodesInPath addSourceToDestination() {
toDst = new INodeReference.DstReference(dstParent.asDirectory(),
withCount, dstIIP.getLatestSnapshotId());
}
return fsd.addLastINodeNoQuotaCheck(dstParentIIP, toDst, srcSubTreeCount);
return fsd.addLastINodeNoQuotaCheck(dstParentIIP, toDst, srcSubTreeCount, updateQuota);
}

void updateMtimeAndLease(long timestamp) {
Expand Down Expand Up @@ -837,7 +855,7 @@ void restoreSource() {
// the srcChild back
Optional<QuotaCounts> countOp = sameStoragePolicy ?
srcSubTreeCount : Optional.empty();
fsd.addLastINodeNoQuotaCheck(srcParentIIP, srcChild, countOp);
fsd.addLastINodeNoQuotaCheck(srcParentIIP, srcChild, countOp, updateQuota);
}
}

Expand All @@ -847,7 +865,7 @@ void restoreDst(BlockStoragePolicySuite bsps) {
if (dstParent.isWithSnapshot()) {
dstParent.undoRename4DstParent(bsps, oldDstChild, dstIIP.getLatestSnapshotId());
} else {
fsd.addLastINodeNoQuotaCheck(dstParentIIP, oldDstChild, dstSubTreeCount);
fsd.addLastINodeNoQuotaCheck(dstParentIIP, oldDstChild, dstSubTreeCount, updateQuota);
}
if (oldDstChild != null && oldDstChild.isReference()) {
final INodeReference removedDstRef = oldDstChild.asReference();
Expand Down
46 changes: 35 additions & 11 deletions ...project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,7 @@ INodesInPath addINode(INodesInPath existing, INode child,
cacheName(child);
writeLock();
try {
return addLastINode(existing, child, modes, true, Optional.empty());
return addLastINode(existing, child, modes, true, Optional.empty(), true);
} finally {
writeUnlock();
}
Expand Down Expand Up @@ -1242,6 +1242,23 @@ static void verifyQuota(INodesInPath iip, int pos, QuotaCounts deltas,
}
}

/**
* Verify that the path from the specified position to the root
* (excluding the root itself) does not contain any valid quota feature.
*
* @param iip the INodesInPath instance containing all the INodes for the path.
* @return true if no valid quota feature, false if any directory in the path has quota feature.
*/
static boolean verifyPathWithoutValidQuotaFeature(INodesInPath iip) {
for (int i = iip.length() - 2; i >= 1; i--) {
INodeDirectory d = iip.getINode(i).asDirectory();
if (d.isWithQuota()) {
return false;
}
}
return true;
}

/** Verify if the inode name is legal. */
void verifyINodeName(byte[] childName) throws HadoopIllegalArgumentException {
if (Arrays.equals(HdfsServerConstants.DOT_SNAPSHOT_DIR_BYTES, childName)) {
Expand Down Expand Up @@ -1343,11 +1360,12 @@ private void copyINodeDefaultAcl(INode child, FsPermission modes) {
* @param inode the new INode to add
* @param modes create modes
* @param checkQuota whether to check quota
* @param updateQuota whether to update quota
* @return an INodesInPath instance containing the new INode
*/
@VisibleForTesting
public INodesInPath addLastINode(INodesInPath existing, INode inode,
FsPermission modes, boolean checkQuota, Optional<QuotaCounts> quotaCount)
public INodesInPath addLastINode(INodesInPath existing, INode inode, FsPermission modes,
boolean checkQuota, Optional<QuotaCounts> quotaCount, boolean updateQuota)
throws QuotaExceededException {
assert existing.getLastINode() != null &&
existing.getLastINode().isDirectory();
Expand Down Expand Up @@ -1379,17 +1397,23 @@ public INodesInPath addLastINode(INodesInPath existing, INode inode,
// always verify inode name
verifyINodeName(inode.getLocalNameBytes());

final QuotaCounts counts = quotaCount.orElseGet(() -> inode.
computeQuotaUsage(getBlockStoragePolicySuite(),
parent.getStoragePolicyID(), false,
Snapshot.CURRENT_STATE_ID));
updateCount(existing, pos, counts, checkQuota);
QuotaCounts counts = null;
if (updateQuota) {
counts = quotaCount.orElseGet(() -> inode.
computeQuotaUsage(getBlockStoragePolicySuite(),
parent.getStoragePolicyID(), false,
Snapshot.CURRENT_STATE_ID));
updateCount(existing, pos, counts, checkQuota);
}

boolean isRename = (inode.getParent() != null);
final boolean added = parent.addChild(inode, true,
existing.getLatestSnapshotId());
if (!added) {
updateCountNoQuotaCheck(existing, pos, counts.negation());
// When adding INode fails, if 'updateQuota' is true, rollback quota.
if (updateQuota) {
updateCountNoQuotaCheck(existing, pos, counts.negation());
}
return null;
} else {
if (!isRename) {
Expand All @@ -1401,10 +1425,10 @@ public INodesInPath addLastINode(INodesInPath existing, INode inode,
}

INodesInPath addLastINodeNoQuotaCheck(INodesInPath existing, INode i,
Optional<QuotaCounts> quotaCount) {
Optional<QuotaCounts> quotaCount, boolean updateQuota) {
try {
// All callers do not have create modes to pass.
return addLastINode(existing, i, null, false, quotaCount);
return addLastINode(existing, i, null, false, quotaCount, updateQuota);
} catch (QuotaExceededException e) {
NameNode.LOG.warn("FSDirectory.addChildNoQuotaCheck - unexpected", e);
}
Expand Down
Loading