-
Notifications
You must be signed in to change notification settings - Fork 72
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
Global delay bug fix: check again credits under mutex #438
Conversation
db/write_controller.cc
Outdated
@@ -148,7 +148,13 @@ uint64_t WriteController::GetDelay(SystemClock* clock, uint64_t num_bytes) { | |||
// Refill every 1 ms | |||
const uint64_t kMicrosPerRefill = 1000; | |||
|
|||
std::lock_guard<std::mutex> lock(mu_); | |||
std::lock_guard<std::mutex> lock(metrics_mu_); |
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.
the use of credit_in_bytes_ is not right as i can see it... as you wrote above
"// This function is no longer under db mutex since credit_in_bytes_ is atomic"
in the first check you does threat it as atomic and do compare_exchange_weak meaning you do change its value not under mutex
but after you take the lock , you changed its value not using atomic operation which is problematic
the lock actually doesnt protect from you from access credit_in_bytes_...
you need to access credit_in_bytes_ with atomic functions
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.
right, we'll simply take the mutex before to include the first place where credit_in_bytes_ can change so now only 1 thread can change credit_in_bytes_ simultaneously
92b1d8a
to
9361724
Compare
while at it, rename and reorder some members
9361724
to
802c219
Compare
While at it, rename and reorder some members
While at it, rename and reorder some members
While at it, rename and reorder some members
While at it, rename and reorder some members
While at it, rename and reorder some members
i encountered a bug while running tests for the dirty memory connection to the global write controller and the bug is as follows:
during WriteController::GetDelay , we check if there are enough credits and return if there are, here:
if there arent enough credits, we proceed under the mutex but dont check again the condition above which might have changed from a different thread.
this PR adds this additional check and while at it, rename and reorder some members.