-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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.
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
DisableManualCompaction may fail to cancel an unscheduled task #9659
Conversation
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
manual.done = true; | ||
manual.status = | ||
Status::Incomplete(Status::SubCode::kManualCompactionPaused); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a compaction has been scheduled and is currently executing, should we still wait for !manual.in_progress
?
edit: For example if RunManualCompaction()
ends while BG compaction is still running, BG compaction may attempt to modify an already-deleted ManualCompactionState
.
What if we have this just below the bg_cv_.Wait() ?
if (manual.in_progress) {
continue;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out. after looking into it, just checking manual.in_progress
might not be enough, because the BG thread might already picked the compaction task but not yet update the manual.in_progress
(as the thread-pool queue is not protected by db.mutex, I just added an unittest for such case).
How about make manual
a shared_ptr?
(other options I considered but has their problems:
env_->UnSchedule()
returns the number of tasks it cancelled, we can check that to make sure a scheduled compaction is cancelled, otherwise wait. The problem is it cannot work for multiple non-exclusive manual compactions as one manual compaction will unschedule() 2 jobs, another won't unschedule() any.- add manual job id in the Tag (hacky and may have problem if there're too many manual compactions)
- coordinate between all FG manual compaction threads to make sure all tasks are unscheduled, if not, wait. (I think it's possible, but seems more complicated than just make
manual
a shared_ptr.)
2c42088
to
627358f
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
db_->DisableManualCompaction(); | ||
|
||
compact_thread.join(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests work for me before the fix. Are they expected to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's fine on linux, even passed ASAN and TSAN...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info - tests LGTM now. Some notes below -
The first test could produce a hang without the fix if I deleted the early-return in RunManualCompaction()
based on manual compaction being paused. That early return seems to prevent us from actually scheduling background compactions to the low pri pool.
Regarding the second test, there might be something platform dependent about accessing a variable from an already-returned stack frame. I can't find an error even though the ManualCompactionState clearly went out of scope and should've been destroyed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's fine on linux, even passed ASAN and TSAN...
I have some task about enabling ASAN_OPTIONS=detect_stack_use_after_return=1 (T94055374). Can't remember what motivated that but it'd be interesting to see if it'd help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
302f192
to
ce243df
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: #9625 didn't change the unschedule condition which was waiting for the background thread to clean-up the compaction. make sure we only unschedule the task when it's scheduled. Pull Request resolved: #9659 Reviewed By: ajkr Differential Revision: D34651820 Pulled By: jay-zhuang fbshipit-source-id: 23f42081b15ec8886cd81cbf131b116e0c74dc2f
In facebook#9659, when `DisableManualCompaction()` is issued, the foreground manual compaction thread does not have to wait background compaction thread to finish. Which could be a problem that the user re-enable manual compaction with `EnableManualCompaction()`, it may re-enable the BG compaction which supposed be cancelled. This patch makes the FG compaction wait on `manual_compaction_state.done`, which either be set by BG compaction or Unschedule callback. Then when FG manual compaction thread returns, it should not have BG compaction running. So shared_ptr is no longer needed for `manual_compaction_state`. Test Plan: a StressTest and unittest
Summary: In #9659, when `DisableManualCompaction()` is issued, the foreground manual compaction thread does not have to wait background compaction thread to finish. Which could be a problem that the user re-enable manual compaction with `EnableManualCompaction()`, it may re-enable the BG compaction which supposed be cancelled. This patch makes the FG compaction wait on `manual_compaction_state.done`, which either be set by BG compaction or Unschedule callback. Then when FG manual compaction thread returns, it should not have BG compaction running. So shared_ptr is no longer needed for `manual_compaction_state`. Pull Request resolved: #9694 Test Plan: a StressTest and unittest Reviewed By: ajkr Differential Revision: D34885472 Pulled By: jay-zhuang fbshipit-source-id: e6476175b43e8c59cd49f5c09241036a0716c274
Summary: #9625 didn't change the unschedule condition which was waiting for the background thread to clean-up the compaction. make sure we only unschedule the task when it's scheduled. Pull Request resolved: #9659 Reviewed By: ajkr Differential Revision: D34651820 Pulled By: jay-zhuang fbshipit-source-id: 23f42081b15ec8886cd81cbf131b116e0c74dc2f
Summary: In #9659, when `DisableManualCompaction()` is issued, the foreground manual compaction thread does not have to wait background compaction thread to finish. Which could be a problem that the user re-enable manual compaction with `EnableManualCompaction()`, it may re-enable the BG compaction which supposed be cancelled. This patch makes the FG compaction wait on `manual_compaction_state.done`, which either be set by BG compaction or Unschedule callback. Then when FG manual compaction thread returns, it should not have BG compaction running. So shared_ptr is no longer needed for `manual_compaction_state`. Pull Request resolved: #9694 Test Plan: a StressTest and unittest Reviewed By: ajkr Differential Revision: D34885472 Pulled By: jay-zhuang fbshipit-source-id: e6476175b43e8c59cd49f5c09241036a0716c274
Summary: In #9659, when `DisableManualCompaction()` is issued, the foreground manual compaction thread does not have to wait background compaction thread to finish. Which could be a problem that the user re-enable manual compaction with `EnableManualCompaction()`, it may re-enable the BG compaction which supposed be cancelled. This patch makes the FG compaction wait on `manual_compaction_state.done`, which either be set by BG compaction or Unschedule callback. Then when FG manual compaction thread returns, it should not have BG compaction running. So shared_ptr is no longer needed for `manual_compaction_state`. Pull Request resolved: #9694 Test Plan: a StressTest and unittest Reviewed By: ajkr Differential Revision: D34885472 Pulled By: jay-zhuang fbshipit-source-id: e6476175b43e8c59cd49f5c09241036a0716c274
#9625 didn't change the unschedule condition which was waiting for the background thread to clean-up the compaction.
make sure we only unschedule the task when it's scheduled.