kvserver: take out additional readOnlyCmdMu locks#64378
kvserver: take out additional readOnlyCmdMu locks#64378craig[bot] merged 1 commit intocockroachdb:masterfrom
readOnlyCmdMu locks#64378Conversation
|
This change is |
tbg
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @nvanbenschoten)
pkg/kv/kvserver/replica.go, line 1715 at r1 (raw file):
} r.raftMu.Lock() r.readOnlyCmdMu.Lock()
This is the order described in the "lock order" comment on Store.mu, right?
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica.go, line 1715 at r1 (raw file):
Yes, but we may not be using the correct lock order elsewhere, so good looking out. It says:
Replica.raftMu < Replica.readOnlyCmdMu < Store.mu < Replica.mu
but I believe there are places where we take out Store.mu before readOnlyCmdMu, as I wasn't aware of the store ordering here. Let me do another pass for this.
Follow-up for cockroachdb#64324 that takes out `readOnlyCmdMu` to synchronize read-only requests with replica removals in a couple of additional spots. We now take them out at all `destroyStatus.Set()` sites, even though they may not all strictly need them. Also fixes lock ordering wrt. `Store.mu` in `tryGetOrCreateReplica`. This change has already been included in backports for cockroachdb#64324. Release note: None
ed9ace4 to
a86277d
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica.go, line 1715 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Yes, but we may not be using the correct lock order elsewhere, so good looking out. It says:
Replica.raftMu < Replica.readOnlyCmdMu < Store.mu < Replica.mu
but I believe there are places where we take out Store.mu before readOnlyCmdMu, as I wasn't aware of the store ordering here. Let me do another pass for this.
Fixed lock ordering wrt. Store.mu during tryGetOrCreateReplica as well, PTAL. Couldn't see any other relevant sites. Note that this already unlocks Store.mu out of order, I'm assuming this is for good reason:
nvb
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:
|
bors r=tbg,nvanbenschoten |
|
Build succeeded: |
Follow-up for #64324 that takes out
readOnlyCmdMuto synchronizeread-only requests with replica removals in a couple of additional
spots. We now take them out at all
destroyStatus.Set()sites, eventhough they may not all strictly need them.
Also fixes lock ordering wrt.
Store.muintryGetOrCreateReplica.This change has already been included in backports for #64324.
Release note: None