Merge #64378

64378: kvserver: take out additional `readOnlyCmdMu` locks r=tbg,nvanbenschoten a=erikgrinaker

Follow-up for #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 #64324.

Release note: None

Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>

pkg/kv/kvserver/replica.go

@@ -1712,6 +1712,7 @@ func (r *Replica) maybeWatchForMergeLocked(ctx context.Context) (bool, error) {
 			}
 		}
 		r.raftMu.Lock()
+		r.readOnlyCmdMu.Lock()
 		r.mu.Lock()
 		if mergeCommitted && r.mu.destroyStatus.IsAlive() {
 			// The merge committed but the left-hand replica on this store hasn't
@@ -1726,6 +1727,7 @@ func (r *Replica) maybeWatchForMergeLocked(ctx context.Context) (bool, error) {
 		r.mu.mergeTxnID = uuid.UUID{}
 		close(mergeCompleteCh)
 		r.mu.Unlock()
+		r.readOnlyCmdMu.Unlock()
 		r.raftMu.Unlock()
 	})
 	if errors.Is(err, stop.ErrUnavailable) {

pkg/kv/kvserver/replica_application_state_machine.go

@@ -683,11 +683,13 @@ func (b *replicaAppBatch) runPreApplyTriggersAfterStagingWriteBatch(
 		// We mark the replica as destroyed so that new commands are not
 		// accepted. This destroy status will be detected after the batch
 		// commits by handleMergeResult() to finish the removal.
+		rhsRepl.readOnlyCmdMu.Lock()
 		rhsRepl.mu.Lock()
 		rhsRepl.mu.destroyStatus.Set(
 			roachpb.NewRangeNotFoundError(rhsRepl.RangeID, rhsRepl.store.StoreID()),
 			destroyReasonRemoved)
 		rhsRepl.mu.Unlock()
+		rhsRepl.readOnlyCmdMu.Unlock()
 
 		// Use math.MaxInt32 (mergedTombstoneReplicaID) as the nextReplicaID as an
 		// extra safeguard against creating new replicas of the RHS. This isn't

pkg/kv/kvserver/store_create_replica.go

@@ -159,14 +159,16 @@ func (s *Store) tryGetOrCreateReplica(
 	repl.creatingReplica = creatingReplica
 	repl.raftMu.Lock() // not unlocked
 
+	// Take out read-only lock. Not strictly necessary here, but follows the
+	// normal lock protocol for destroyStatus.Set().
+	repl.readOnlyCmdMu.Lock()
 	// Install the replica in the store's replica map. The replica is in an
 	// inconsistent state, but nobody will be accessing it while we hold its
 	// locks.
 	s.mu.Lock()
 	// Grab the internal Replica state lock to ensure nobody mucks with our
 	// replica even outside of raft processing. Have to do this after grabbing
 	// Store.mu to maintain lock ordering invariant.
-	repl.readOnlyCmdMu.Lock()
 	repl.mu.Lock()
 	repl.mu.tombstoneMinReplicaID = tombstone.NextReplicaID
 
@@ -188,8 +190,8 @@ func (s *Store) tryGetOrCreateReplica(
 	// might have snuck in and created the replica, so we retry on error.
 	if err := s.addReplicaToRangeMapLocked(repl); err != nil {
 		repl.mu.Unlock()
-		repl.readOnlyCmdMu.Unlock()
 		s.mu.Unlock()
+		repl.readOnlyCmdMu.Unlock()
 		repl.raftMu.Unlock()
 		return nil, false, errRetry
 	}
@@ -226,10 +228,10 @@ func (s *Store) tryGetOrCreateReplica(
 		// ensure nobody tries to use it.
 		repl.mu.destroyStatus.Set(errors.Wrapf(err, "%s: failed to initialize", repl), destroyReasonRemoved)
 		repl.mu.Unlock()
-		repl.readOnlyCmdMu.Unlock()
 		s.mu.Lock()
 		s.unlinkReplicaByRangeIDLocked(ctx, rangeID)
 		s.mu.Unlock()
+		repl.readOnlyCmdMu.Unlock()
 		repl.raftMu.Unlock()
 		return nil, false, err
 	}