Skip to content

HDFS-16272. Fix int overflow in computing safe length during EC block recovery #3548

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

Merged
sodonnel merged 4 commits into from
Oct 18, 2021
Merged

HDFS-16272. Fix int overflow in computing safe length during EC block recovery #3548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c54500e
Fix int overflow in computing safe length during EC block recovery
cndaimin Oct 13, 2021
06ac7c0
Fix code style
cndaimin Oct 14, 2021
30ff05f
Fix int overflow in method used in tests
cndaimin Oct 14, 2021
178404a
Fix code style
cndaimin Oct 15, 2021
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
7 changes: 3 additions & 4 deletions ...roject/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/util/StripedBlockUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,7 @@ public static long getSafeLength(ErasureCodingPolicy ecPolicy,
Arrays.sort(cpy);
// full stripe is a stripe has at least dataBlkNum full cells.
// lastFullStripeIdx is the index of the last full stripe.
int lastFullStripeIdx =
(int) (cpy[cpy.length - dataBlkNum] / cellSize);
long lastFullStripeIdx = cpy[cpy.length - dataBlkNum] / cellSize;
Copy link
Copy Markdown
Contributor

@sodonnel sodonnel Oct 13, 2021

Choose a reason for hiding this comment

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

I know this is existing code, but I'd like to understand what is happening here to review this.

This method receives an array of internal block lengths, so for 3-2 it will have 5 entries, 6-3 it will have 9 etc.

Then it sorts the lengths smallest to largest. Then it selects the one at position num_blocks - numDataUnits.

Why does it not just pick the first one, which would be the smallest, as the smallest data block in the group indicates the last full stripe.

Why is the safe length based on the full stripe, and not a potentially partial last stripe?

Copy link
Copy Markdown
Contributor

@sodonnel sodonnel Oct 13, 2021

Choose a reason for hiding this comment

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

The change you have made here makes sense to me, although I still don't fully understand how the method is used in practice. However I do worry where else a bug like this may exist.

I think there is a similar problem in offsetInBlkToOffsetInBG() in this same class. It only seems to be used in test code, but it would be good to fix it incase it is used in non-test code later.

Copy link
Copy Markdown
Contributor Author

@cndaimin cndaimin Oct 14, 2021

Choose a reason for hiding this comment

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

Many thanks for the review! @sodonnel

  1. My understanding to this why not pick the first one is that the EC background reconstruction procedure has the ability to compute the rest 2 blocks based on the 3 good blocks(take RS-3-2 policy for example).
  2. I did notice a todo in code comments of this method: "Include lastFullStripeIdx+1 stripe in safeLength, if there exists such a stripe (and it must be partial).", but I might not involve this since that work may take a while :(
  3. cellIdxInBlk * cellSize * dataBlkNum in offsetInBlkToOffsetInBG has the same problem, I am glad to fix that too.

return lastFullStripeIdx * stripeSize; // return the safeLength
// TODO: Include lastFullStripeIdx+1 stripe in safeLength, if there exists
// such a stripe (and it must be partial).
Expand All @@ -271,9 +270,9 @@ private static int lastCellSize(int size, int cellSize, int numDataBlocks,
*/
public static long offsetInBlkToOffsetInBG(int cellSize, int dataBlkNum,
long offsetInBlk, int idxInBlockGroup) {
int cellIdxInBlk = (int) (offsetInBlk / cellSize);
long cellIdxInBlk = offsetInBlk / cellSize;
return cellIdxInBlk * cellSize * dataBlkNum // n full stripes before offset
+ idxInBlockGroup * cellSize // m full cells before offset
+ (long)idxInBlockGroup * cellSize // m full cells before offset
+ offsetInBlk % cellSize; // partial cell
}

Expand Down
18 changes: 18 additions & 0 deletions ...fs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecoveryStriped.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,24 @@ public void testLeaseRecovery() throws Exception {
}
}

@Test
public void testSafeLength() {
checkSafeLength(0, 0); // Length of: 0
checkSafeLength(1024 * 1024, 6291456L); // Length of: 1 MiB
checkSafeLength(64 * 1024 * 1024, 402653184L); // Length of: 64 MiB
checkSafeLength(189729792, 1132462080L); // Length of: 189729792
checkSafeLength(256 * 1024 * 1024, 1610612736L); // Length of: 256 MiB
checkSafeLength(517399040, 3101687808L); // Length of: 517399040
checkSafeLength(1024 * 1024 * 1024, 6442450944L); // Length of: 1 GiB
}

private void checkSafeLength(int blockLength, long expectedSafeLength) {
int[] blockLengths = new int[]{blockLength, blockLength, blockLength, blockLength,
blockLength, blockLength};
long safeLength = new BlockLengths(ecPolicy, blockLengths).getSafeLength();
Assert.assertEquals(expectedSafeLength, safeLength);
}

private void runTest(int[] blockLengths, long safeLength) throws Exception {
writePartialBlocks(blockLengths);

Expand Down