You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Xushaohong (via GitHub)" <gi...@apache.org> on 2023/02/27 12:20:41 UTC
[GitHub] [ozone] Xushaohong opened a new pull request, #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Xushaohong opened a new pull request, #4319:
URL: https://github.com/apache/ozone/pull/4319
## What changes were proposed in this pull request?
Found by https://issues.apache.org/jira/browse/HDDS-8031?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel
The issue is found due to the lack of lock protection.
The **addToBuffer/removeFromBuffer** may be concurrently called with **flush**, which may add KV into a closed batch and cause the JVM crash.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8036
## How was this patch tested?
Original Error test:
https://github.com/Xushaohong/ozone/actions/runs/4280636237/jobs/7452661792
POC fix test:
https://github.com/Xushaohong/ozone/actions/runs/4281772162/jobs/7455208505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4319:
URL: https://github.com/apache/ozone/pull/4319#discussion_r1119538226
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -56,13 +58,23 @@ private BatchOperation getCurrentBatchOperation() {
@Override
public <KEY, VALUE> void addToBuffer(
Table<KEY, VALUE> table, KEY key, VALUE value) throws IOException {
- table.putWithBatch(getCurrentBatchOperation(), key, value);
+ rwLock.readLock().lock();
Review Comment:
Are you sure we can use a read lock? Unless `currentBatchOperation` is thread safe, we need a write lock here.
If we change this and `removeFromBuffer` to write lock, then everything uses the write lock. We may change them to simply `synchronized`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #4319:
URL: https://github.com/apache/ozone/pull/4319#issuecomment-1447515924
@Xushaohong , It is better to add guard `init()` with the write lock since
1. it won't hurt the performance; and
2. it is easier to maintain the code (someone may call init() outside Ratis in the future.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Xushaohong commented on pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on PR #4319:
URL: https://github.com/apache/ozone/pull/4319#issuecomment-1446233833
@adoroszlai PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4319:
URL: https://github.com/apache/ozone/pull/4319#discussion_r1119574074
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -56,13 +58,23 @@ private BatchOperation getCurrentBatchOperation() {
@Override
public <KEY, VALUE> void addToBuffer(
Table<KEY, VALUE> table, KEY key, VALUE value) throws IOException {
- table.putWithBatch(getCurrentBatchOperation(), key, value);
+ rwLock.readLock().lock();
Review Comment:
@szetszwo I checked and found as below:
https://github.com/facebook/rocksdb/blob/b7e73501d82661455755b532dff8557e82cb1165/include/rocksdb/write_batch.h#L20-L23
From what I understand, the const method is the method that does not change a member variable of the class. This is exactly the case here we meet, the `flush` will change the `currentBatchOperation` while the `add/remove` will not, so I think the read lock is fine and it is good for performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4319:
URL: https://github.com/apache/ozone/pull/4319#issuecomment-1447836512
Thanks @Xushaohong for the fix, @kaijchen, @szetszwo for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4319:
URL: https://github.com/apache/ozone/pull/4319#issuecomment-1446734737
cc @tanvipenumudy @sumitagrawl can you please take a look as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4319:
URL: https://github.com/apache/ozone/pull/4319#discussion_r1119590668
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -56,13 +58,23 @@ private BatchOperation getCurrentBatchOperation() {
@Override
public <KEY, VALUE> void addToBuffer(
Table<KEY, VALUE> table, KEY key, VALUE value) throws IOException {
- table.putWithBatch(getCurrentBatchOperation(), key, value);
+ rwLock.readLock().lock();
Review Comment:
Good to know. We have add some tests to verify it in a separated JIRA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4319:
URL: https://github.com/apache/ozone/pull/4319#discussion_r1119615254
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -92,43 +104,54 @@ public void setLatestSnapshot(SnapshotInfo latestSnapshot) {
@Override
public void flush() throws IOException {
- // write latest trx info into trx table in the same batch
- Table<String, TransactionInfo> transactionInfoTable
- = metadataStore.getTransactionInfoTable();
- transactionInfoTable.putWithBatch(currentBatchOperation,
- TRANSACTION_INFO_KEY, latestTrxInfo);
-
- metadataStore.getStore().commitBatchOperation(currentBatchOperation);
- currentBatchOperation.close();
- this.latestSnapshot = latestTrxInfo.toSnapshotInfo();
- // reset batch operation
- currentBatchOperation = metadataStore.getStore().initBatchOperation();
-
- DeletedBlockLog deletedBlockLog = scm.getScmBlockManager()
- .getDeletedBlockLog();
- Preconditions.checkArgument(
- deletedBlockLog instanceof DeletedBlockLogImpl);
- ((DeletedBlockLogImpl) deletedBlockLog).onFlush();
+ rwLock.writeLock().lock();
+ try {
+ // write latest trx info into trx table in the same batch
+ Table<String, TransactionInfo> transactionInfoTable
+ = metadataStore.getTransactionInfoTable();
+ transactionInfoTable.putWithBatch(currentBatchOperation,
+ TRANSACTION_INFO_KEY, latestTrxInfo);
+
+ metadataStore.getStore().commitBatchOperation(currentBatchOperation);
+ currentBatchOperation.close();
+ this.latestSnapshot = latestTrxInfo.toSnapshotInfo();
+ // reset batch operation
+ currentBatchOperation = metadataStore.getStore().initBatchOperation();
+
+ DeletedBlockLog deletedBlockLog = scm.getScmBlockManager()
+ .getDeletedBlockLog();
+ Preconditions.checkArgument(
+ deletedBlockLog instanceof DeletedBlockLogImpl);
+ ((DeletedBlockLogImpl) deletedBlockLog).onFlush();
+ } finally {
+ rwLock.writeLock().unlock();
+ }
}
@Override
public void init() throws IOException {
metadataStore = scm.getScmMetadataStore();
Review Comment:
This assignment is idempotent, so I just left it alone. It is OK to move inside. @adoroszlai
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4319:
URL: https://github.com/apache/ozone/pull/4319#discussion_r1119654532
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -92,43 +104,54 @@ public void setLatestSnapshot(SnapshotInfo latestSnapshot) {
@Override
public void flush() throws IOException {
- // write latest trx info into trx table in the same batch
- Table<String, TransactionInfo> transactionInfoTable
- = metadataStore.getTransactionInfoTable();
- transactionInfoTable.putWithBatch(currentBatchOperation,
- TRANSACTION_INFO_KEY, latestTrxInfo);
-
- metadataStore.getStore().commitBatchOperation(currentBatchOperation);
- currentBatchOperation.close();
- this.latestSnapshot = latestTrxInfo.toSnapshotInfo();
- // reset batch operation
- currentBatchOperation = metadataStore.getStore().initBatchOperation();
-
- DeletedBlockLog deletedBlockLog = scm.getScmBlockManager()
- .getDeletedBlockLog();
- Preconditions.checkArgument(
- deletedBlockLog instanceof DeletedBlockLogImpl);
- ((DeletedBlockLogImpl) deletedBlockLog).onFlush();
+ rwLock.writeLock().lock();
+ try {
+ // write latest trx info into trx table in the same batch
+ Table<String, TransactionInfo> transactionInfoTable
+ = metadataStore.getTransactionInfoTable();
+ transactionInfoTable.putWithBatch(currentBatchOperation,
+ TRANSACTION_INFO_KEY, latestTrxInfo);
+
+ metadataStore.getStore().commitBatchOperation(currentBatchOperation);
+ currentBatchOperation.close();
+ this.latestSnapshot = latestTrxInfo.toSnapshotInfo();
+ // reset batch operation
+ currentBatchOperation = metadataStore.getStore().initBatchOperation();
+
+ DeletedBlockLog deletedBlockLog = scm.getScmBlockManager()
+ .getDeletedBlockLog();
+ Preconditions.checkArgument(
+ deletedBlockLog instanceof DeletedBlockLogImpl);
+ ((DeletedBlockLogImpl) deletedBlockLog).onFlush();
+ } finally {
+ rwLock.writeLock().unlock();
+ }
}
@Override
public void init() throws IOException {
metadataStore = scm.getScmMetadataStore();
Review Comment:
Thanks @Xushaohong for the explanation, it's OK to leave it as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4319:
URL: https://github.com/apache/ozone/pull/4319#discussion_r1119612547
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -92,43 +104,54 @@ public void setLatestSnapshot(SnapshotInfo latestSnapshot) {
@Override
public void flush() throws IOException {
- // write latest trx info into trx table in the same batch
- Table<String, TransactionInfo> transactionInfoTable
- = metadataStore.getTransactionInfoTable();
- transactionInfoTable.putWithBatch(currentBatchOperation,
- TRANSACTION_INFO_KEY, latestTrxInfo);
-
- metadataStore.getStore().commitBatchOperation(currentBatchOperation);
- currentBatchOperation.close();
- this.latestSnapshot = latestTrxInfo.toSnapshotInfo();
- // reset batch operation
- currentBatchOperation = metadataStore.getStore().initBatchOperation();
-
- DeletedBlockLog deletedBlockLog = scm.getScmBlockManager()
- .getDeletedBlockLog();
- Preconditions.checkArgument(
- deletedBlockLog instanceof DeletedBlockLogImpl);
- ((DeletedBlockLogImpl) deletedBlockLog).onFlush();
+ rwLock.writeLock().lock();
+ try {
+ // write latest trx info into trx table in the same batch
+ Table<String, TransactionInfo> transactionInfoTable
+ = metadataStore.getTransactionInfoTable();
+ transactionInfoTable.putWithBatch(currentBatchOperation,
+ TRANSACTION_INFO_KEY, latestTrxInfo);
+
+ metadataStore.getStore().commitBatchOperation(currentBatchOperation);
+ currentBatchOperation.close();
+ this.latestSnapshot = latestTrxInfo.toSnapshotInfo();
+ // reset batch operation
+ currentBatchOperation = metadataStore.getStore().initBatchOperation();
+
+ DeletedBlockLog deletedBlockLog = scm.getScmBlockManager()
+ .getDeletedBlockLog();
+ Preconditions.checkArgument(
+ deletedBlockLog instanceof DeletedBlockLogImpl);
+ ((DeletedBlockLogImpl) deletedBlockLog).onFlush();
+ } finally {
+ rwLock.writeLock().unlock();
+ }
}
@Override
public void init() throws IOException {
metadataStore = scm.getScmMetadataStore();
Review Comment:
Why is this assignment outside of the protected block?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Xushaohong commented on pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on PR #4319:
URL: https://github.com/apache/ozone/pull/4319#issuecomment-1447457449
> I think we should also guard `init()` with the write lock. It can be called from `SCMStateMachine.reinitialize`.
Hi @adoroszlai, the `init()` should be fine with no lock protection, because it is only called when the construction or when installing the snapshot(no writes coming) .
@szetszwo Could you help confirm it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #4319: HDDS-8036. Unprotected flush in SCMHADBTransactionBuffer
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4319:
URL: https://github.com/apache/ozone/pull/4319
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org